PetarKirov / dlang.nix

Nix expressions for building D compilers
MIT License
6 stars 2 forks source link

ci(gh-actions): Add initial CI pipeline config #3

Closed PetarKirov closed 11 months ago

dukc commented 1 year ago

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.

PetarKirov commented 11 months ago

I have marked ldc on macos job in the CI matrix as allowed to fail as I want to merge this and #4 as soon as possible (it's been long enough...). I just tried it on a local m1 MacBook and when LDC builds itself it segfaults on both aarch64-darwin and x86_64-darwin, so it's not just the CI being flaky here (I initially suspected it was running out of memory, as this was a specific root cause I had with a different project tested with GH actions).

PetarKirov commented 11 months ago

@dukc I've just sent you a GitHub collaboration invite to this repo. You should see it in either your email or GitHub notifications.

dukc commented 11 months ago

@dukc I've just sent you a GitHub collaboration invite to this repo. You should see it in either your email or GitHub notifications.

Alraedy accepted - thanks very much!

PetarKirov commented 11 months ago

@dukc Welcome aboard! :wave:

PetarKirov commented 11 months ago

[..] so if you commit this separately either of us will have to rebase :-).

I'm used to being the resident git surgent at work :smile: so I was planning on doing the rebase of #4 for you ;)

dukc commented 11 months ago

It's getting late so I have to review the rest another day.

PetarKirov commented 11 months ago

(reviewing "Remove nixpkgs fallback")

I suspect that better not. Flakes still are not the default, not everybody is necessarily using them.

I've been living in an alternative universe where flakes are not only stable but the only way to use Nix :smile: Joke aside (well not really a joke, we've been using them in every "production" project at work for the past 2+ years) and so far we have no reason to consider going back to the pre-flake nix channel world; their "experimental" nature doesn't bother us in the slightest), I have two points:

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.

As mentioned above, I did this because I was bitten by using packages from a stale nix channel. I suggest you try out the nix develop + direnv combo - it works great!

PetarKirov commented 11 months ago

(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.

Before this change, the pkgs instance passed to the perSystem function for us by Flake Parts was an instance of nixpkgs without any overlays applied, so when using dmd, ldc, etc. in shell.nix they would be the exact same version as in nixpkgs upstream.

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 final parameter in perSystem.

This has the effect of making the pkgs defined in this repo directly accessible in its dev shell.

PetarKirov commented 11 months ago

(reviewing commit "Split packages list based on the platform")

This change in itself is good.

However, an urelated notice: for me it seems this default.nix is relying on flakes being enabled. Is this a good idea?

Yes, you are correct that pkgs/default.nix is usable only through flake.nix and this is intentional, as this repo so far is currently designed with flake-only usage in mind. That said, this is not a blocker for non-flake usage, as we could easily support this scenario via https://github.com/edolstra/flake-compat, which allows non-flake users to use all everything exposed by the flake through a top-level default.nix file (not to be confused with pkgs/default.nix), so we wouldn't need to make changes to structure.

PetarKirov commented 11 months ago

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 [..]

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.

PetarKirov commented 11 months ago

It's getting late so I have to review the rest another day.

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.

dukc commented 11 months ago
* This change is only about `shell.nix` which useful only for developers working on this project, so this change is not user-facing. I did it because by mistake I entered the shell with `nix-shell` rather than `nix develop` and I got wildly different package versions, so this change is a precaution to ensure the dev environment is reproducible.

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.

* Since I'm using Nix in a flake-only mode, for now I consider this project flake-only as well. This doesn't mean that I'm against non-flake usage, but it's simply an aspect that I haven't tested and I would have add additional CI configuration to guarantee it works. For now the CI only tests the flake interface. If non-flake usage is important to you, I suggest we dedicate a separate PR where we would specific tests and document the usage.

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.

As mentioned above, I did this because I was bitten by using packages from a stale nix channel. I suggest you try out the nix develop + direnv combo - it works great!

Sounds worth checking!

I personally have been still using channels, trying to avoid that problem by simply avoiding impurity. I don't import <anything> in the .nix files, instead I list dependencies just like in NixPkgs and build like nix-build -E 'with import <pick-your-channel> {}; callPackage ./the-package.nix {inherit (darwin.apple_sdk.frameworks) Foundation;}' I think this already works reasonably well, but there must be a reason why flakes were developed so probably they're even better.

dukc commented 11 months ago

Every commit is reviewed unless I missed something.