-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci(gh-actions): Add initial CI pipeline config #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have absolutely no experience or knowledge from the CI yml so no insights on that.
Definitely wanted to have the CI though, it's maybe the most important thing that is missing from OldDDerivations.
1c9671d
to
bee016e
Compare
b2bc1f3
to
7fcf165
Compare
348e0ff
to
6782619
Compare
I suspect you're trying to swallow more in one go than you want to. For example, I don't think it's a good idea to include LDC bootstrap update to 1.32.1 here. Instead, I'd just get the CI pipeline going, merge this, and then do those other tasks in separate direct commits or pull requests. |
6782619
to
32880ba
Compare
722ed48
to
d984817
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(reviewed a single commit)
Looks good in general, although I have no experience about CI and thus am not really qualified to stamp this.
d984817
to
f3bebaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(reviewing a single commit, "Define the package for more platforms")
All good here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Reviewing commit "Include dmd package only if the hostPlatform is x86")
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(reviewing "Remove nixpkgs fallback")
I suspect that better not. Flakes still are not the default, not everybody is necessarily using them.
If this was the package itself, I'd agree regardless: the user should declare from the terminal what channel is to be used and if he/she forgets, better tell. However, my understanding is that shell.nix is supposed to be an easy-to-use, quick-and-dirty convenience tool rather than the scalable generic build description Nix files in general are. Thus an easy-to-use default makes sense in them even if it breaks the purity ideals we usually follow in Nix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(reviewing commit "Use pkgs instance resulting from applying the flake overlay")
I'd think this makes no difference, with the new code working like the old one, but I have a feeling I'm missing something - if so, LGTM.
ca61a25
to
e5384d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Reviewing "Fix the dmd runnable_cxx parts of the checkPhase by using GCC11")
There is a commit made later on in DMD that changes the compatibility to 12 instead, so this will probably break with newer DMD versions. Hence I prefer my solution in #4 where I disable this test for versions where that change isn't yet included.
Of course we could pick GCC based on version, but IMO such complexity is an overkill to just enable one test for old versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Reviewing "Expose {dmd,ldc}-binary packages")
I'm on fence about this. Exposing binary packages, in itself, is an idea I'm receptive to. However, this is conflating binary package with the bootstrap package. Yes, the bootstrap packages are binary packages, but the term "binary" doesn't indicate in any way what version you get, so I'd expect that:
- I can pick the version myself if I provide hashes, which would imply the
binary.nix
files instead. - If there is a default, it'd be the same as the source-built version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Reviewing "Remove flake-utils
")
I frankly don't know what flake-utils
and flake-parts
are about, so really I'm unqualified to do this review but I see nothing suspicious here.
It's getting late so I have to review the rest another day. |
I've been living in an alternative universe where flakes are not only stable but the only way to use Nix 😄
As mentioned above, I did this because I was bitten by using packages from a stale nix channel. I suggest you try out the |
Before this change, the To understand the what's going on in the flake.nix outputs I suggest you check the docs of the Flake Parts library that we're using. Specifically, we're using its Easy Overlay feature, where it creates an overlay function out of the flake packages outputs and then it applies this overlay to nixpkgs and passes it to us as the This has the effect of making the pkgs defined in this repo directly accessible in its dev shell. |
Yes, you are correct that |
Agreed. I plan to redo the flake output structure completely to allow the user to pick any binary or source version of the compiler packages and to allow the user manually specify hashes (e.g. I want dmd from this PR branch). For now the flake output structure is a placeholder just to show what works. |
Thanks for the review so far! I myself am traveling this past week (without a computer nearby), so that's why I've been so slow. Although there are many things to improve, I feel that it's better for me to go ahead and merge this PR, so I can proceed with rebasing and merging yours. I'd appreciate if you could finish reviewing this PR and if you find any issue that need addressing (even pure questions) file issue(s) for them, so they're not forgotten. |
Good point. Since I and anyone else who wants to develop this should figure out flakes anyway, it's reasonable to demand developers have flakes enabled.
It's enough that the packages themselves - dmd, ldc and dub - can be built without flakes. Our update scripts and the CI using them is okay.
Sounds worth checking! I personally have been still using channels, trying to avoid that problem by simply avoiding impurity. I don't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing "Set CC env variable for both the build and the check phase".
Looks okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing "config(flake.nix): Add Cachix binary cache settings"
These prebuilds still go through hash checks, no? If so, the more binary mirror sites the merrier!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing "Allow skipping missing binary packages".
Makes sense. It's not the sort of thing that would cause bigger harm down the road if some binaries didn't get laoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing "Upgrade to 1.32.1"
If you were updating DMD bootstrapper, I'd protest since it'd be likely to break older DMD versions I haven't yet ported. But this is LDC, where we have no older versions to break, so no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing "Add D-based implementation of fetch_binary script".
I'd prefer this file had @safe
attributes added, but for a script it makes no big difference. Nothing worth blocking here.
? "1.34.0" | ||
: "2.105.0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to keep these around long, since they will be continuosly getting out of date. But I'm pretty sure you're planning on replacing them with something that reads the newest version we support from a flake, or something like that.
: "2.105.0"; | ||
|
||
if (help.helpWanted) | ||
defaultGetoptPrinter("Some information about the program.", help.options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Help texts not written yet, sorry."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing "Disable failing macos tests".
#4 is removing cppa anyway for versions before 2.103.0 (and I think 2.102.2 is the newest one we currently support), so that file can probably be removed from here. Most likely it'll work on Darwing too from 2.103 on, and if not we can add that back.
Otherwise ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing "Upgrade to 1.34.0"
Same comment as with "Update to 1.32.1", in other words, ok! Plus FreeBSD hash is there again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing "Allow ldc on macos to fail"
That you have to write two tables as opposed to one to turn "experimental" on for one complier/platform combination feels a bit kludgy for me, but this is a small configuration file so probably not worth it to hunt for a more scalable solution. Looks ok.
Every commit is reviewed unless I missed something. |
No description provided.