search

MercuryTechnologies / nix-your-shell

A `nix` and `nix-shell` wrapper for shells other than `bash`
MIT License
85 stars 12 forks source link

Housekeeping #40

Closed spikespaz closed 9 months ago

spikespaz commented 9 months ago

Any of these commits may be dropped or rebased should you disapprove.

I recommend reviewing this PR by individual commit because of the diff-noise produced by the formatter (last commit).

  1. Add meta.mainProgram (and other meta attributes) to the package. https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/af3c6a8820135c0897e77b7cc6666e99c78a3630
  2. Appease nil lint about unused function argument flake-compat, replace with .... https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/079602288895d5a1ebb7bbdd670cad1611235a98
  3. Remove usage of rec from packages output, prefer referencing through self for package aliases. https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/30ebc4953280dcced19fdab8f1ad94a688cfd3bf
  4. Remove system from import nixpkgs arguments, prefer localSystem. https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/82a5d248c3fe0cd1bf06666af2324f7d5b8ba12d
  5. Remove flake-utils. https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/61d2ab423753e4ebc799cd39ba7a4b80cf9bc817
  6. Introduce nix-systems to replace functionality of flake-utils. https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/898524afa011e8eb2b6f431406a9afbf8c8e9b2d
    • This makes systems overrideable through usage of nix-your-shell.inputs.systems.
  7. Adopt nixfmt as the flake's formatter. https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/d47c7a1ee367fb82805d44fc1a92e94459f97bed
    • This is perhaps somewhat controversial. There was discussion in the issues of nixfmt with members of the NixOS organization about "blessing" this as the canon formatter. Unfortunately, contrary to the style preferred by many individual flake projects, they decided not to go through with it and instead are "waiting" for nixpkgs-fmt to be "ironed out".
    • I chose this one because:
      1. I like it.
      2. It matches the style you were already using, mostly.
    • I had to change the sed script and the LOAD-BEARING COMMENT to make it shorter, so that nixfmt doesn't wrap the line. Since sed can't do multiline matching replacement easily, this was the solution that resisted me the least. I recommend taking a look at its replacement, and advising me further if need-be. No matter the flake's formatter, making this mechanism less fragine would be a good idea.

I will also look into making another PR down the line (once I figure it out for myself) about simplifying the version.yaml workflow. This makes me uneasy, having a sed script target a comment. Ideally, that version should be based on the flake's revision, however I do understand it is necessary to be the way it is because of publishing to crates.io.

9999years commented 9 months ago

Hi Spike! Thanks for this contribution!

Add meta attributes

Looks good, perhaps we should add a description and maintainers to match the nixpkgs derivation:

https://github.com/NixOS/nixpkgs/blob/0987c80f48b63cfa0e9a12aafd99474079495ae5/pkgs/shells/nix-your-shell/default.nix#L19-L24

Appease nil

Mixed feelings about this one. On one hand, this lint does bug me. On the other hand, I like the idea of Nix checking that all my inputs are being bound to names, and ... undoes that. Probably fine though, no major objections.

Remove rec

This is good, I'd probably forgotten about the self input when I was writing this.

Prefer localSystem to system

TIL.

Remove flake-utils

Sure. I'm not convinced this is a huge improvement, and I think we'll have to revamp all this anyways once Flakes have a better story around systems in general. I was watching the NixCon 2023 talks the other day and everyone wants Flakes to be configurable, but I imagine it'll be years of bikeshedding before anything lands.

Introduce nix-systems

This is the change I like the least. As stated above, I'm not convinced this is a viable or useful configuration mechanism. I'd almost rather generate packages for every system in lib.systems.doubles or export a function to build the package for an arbitrary system. (In fact, this is what the overlay is for!) And nix-your-shell is both packaged in nixpkgs and easy to package (the default pkgs.buildRustPackage works fine with no additional dependencies or configuration), which I suspect is the easiest option for consumers of this package on odd architectures. I'm open to discussion on this, though.

Adopt nixfmt

I've actually been formatting the Nix code with Alejandra. We should set that as the formatter and add a check to ensure that the Nix code is formatted correctly.

Removing sed / LOAD-BEARING COMMENT

TBH I think the load-bearing comment is Fine, but I understand the desire to replace it. It might be worth rearchitecting the flake to use Crane, which can read the version number from the Cargo.toml directly. (I've done this for ghcid-ng and am pretty happy with the way it works. It also caches dependency compilation between builds, though nix-your-shell has a small-enough dependency footprint that I doubt it matters much.)

spikespaz commented 9 months ago
  1. Fixed the checks workflow (rebased) (missing argument in call to eachSystem)
  2. Replaced nixfmt with alejandra https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/f75461cba78469b64f6e4f3da3f3cabcb3731f90
    • Still using the flake for the formatter instead of the one on Nixpkgs. I did this because following upstream may prove helpful to the developer, should you find an error in the formatting. If you disagree with this, or desire the "stable" version from Nixpkgs, leave a review on that commit and I will replace the flake with the Nixpkgs package.
    • Retained the change to rename LOAD-BEARING COMMENT to something shorter with a more explicit explanation.
spikespaz commented 9 months ago
  1. Instead of adding another dependency, I have just opted to read package.version from Cargo.toml. I think this is a decent middleground. https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/c23cc5fa0a94b0201918c8e65e998785e8d56efa
  2. Add meta.maintainers. https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/88f4c747f3ad9da4931c2166801a35be969e1ea2
    • If you want to keep c23cc5f, this commit (88f4c74) should be squashed/fixup'd into the former.

Here we go. I was going to leave it alone and just accept that people like Alejandra, but it does cause problems. The reason I dislike Alejandra is because of what I just dealt with:

Alejandra causes merge conflicts, because it tends to change a lot of code for even small changes.

It is also not strict enough (like rustfmt is) and might leave some lines alone (particularly braces on different indentation levels) which is part of the problem I describe below.

I tried to squash flake: package: add meta.maintainers into flake: package: add meta attributes, but gave up because Alejandra changes the code so much between minor changes. This is more obvious in flake: package: read version from Cargo.toml, where a let block is introduced; adding that code caused Alejandra to change the indentation significantly, resulting in a lot of diff-noise. Nixfmt does not have this problem. When formatting with nixfmt or even nixpkgs-fmt, the diff-noise is kept to a minimum. I strongly recommend reverting flake: formatter: adopt alejandra.

spikespaz commented 9 months ago

As for nix-systems:

I'm not convinced this is a viable or useful configuration mechanism.

Imagine a flake only wants to export packages for platforms it officially supports. Such as, x86_64-linux and aarch64-linux. The package maintainer (you or me) may only have an x86_64-linux system, and then trust that aarch64-linux is similar enough to also provide support for it.

Now, a user comes along using aarch64-darwin, and since it's similar enough (and works with Home Manager), they want to try their hand at getting it to work.

All they have to do in their flake is this:

inputs.nix-your-shell.inputs.systems.follows = "systems";

Where they have their own systems list set up like this:

inputs.systems.url = "path:./flake.systems.nix";

That file might contain:

[ "x86_64-linux" "armv7l-linux" "aarch64-darwin" ]

You don't have to edit the flake to add those two platforms, they don't have to fork it, everyone is happy. They know it isn't officially supported, because you didn't export that as your default systems list.

For me, a practical example would be adding armv7l-linux. I might want to use this over SSH for my FDM printer's Raspberry Pi (which I have a branch for in my dotfiles, with a nixosConfigurations attribute for it). I could use the mechanism described above, using *.follows.*.

flake-utils provides something similar, but this is by far simpler than including that whole dependency. Now we just have a list.

generate packages for every system in lib.systems.doubles

I'd like to point that out as a bad idea... You should want to be precise here, not just brute-force a solution to the problem.

export a function to build the package for an arbitrary system

You don't need to do that, because the user can simply override inputs.nix-your-shell.inputs.systems. Adding a function as a flake output, or even adding it to lib, is unnecessarily complicated and would definitely be abnormal.

(In fact, this is what the overlay is for!)

No, that is not what overlays are for. Overlays simply allow you to merge packages or attributes into the top-level pkgs object, and that grants you the ability to build the package using latest packages in whatever Nixpkgs branch you want.

Someone might want to stay with the packages output. Your package is built, and you know it builds, with the packages locked by flake.lock. You should keep it that way, because using the overlay means I accept responsibility when things break. The same thing goes for inputs.nix-your-shell.inputs.nixpkgs.follows, which I do not recommend people do unless they have a specific reason. I know I'm not the only one either, that can introduce the same problems I described previously for overlays. Functions in nixpkgs.lib might change slightly, or dependency packages might change, or any combination of other issues; it is best to keep packages and overlays explicitly separate because the serve different purposes.

All of that said, I notice a mistake with the way I did this. I should not be using default-linux, but rather default, because I think that you do support MacOS and MacOS ARM64. I will update that.

To appease you however, I will not use github:nix-systems/default, but instead add a flake.systems.nix and import that path so that you can customize in the future. This also removes a flake dependency.

This does have a downside: the nix-systems org will keep default updated with Nixpkgs, similar to flake-utils.lib.eachDefaultSystem.

My recommendation is using github:nix-systems/default.

New commit: https://github.com/MercuryTechnologies/nix-your-shell/pull/40/commits/ce46d1a05c285c8e5aa44bc5e30293f9999cda4d

spikespaz commented 9 months ago

You might also want --all-systems in the check command in .github/workflows/ci.yaml. Do you want me to do that? Might be nice to test cross-compilation too.

spikespaz commented 9 months ago

I think that adding all of the packages as checks is not necessary, because nix flake check automatically checks packages.

Edit:

The Nix manual says it checks that the packages are derivations, but it doesn't say anything about running checkPhase.

What it does do is try to build the packages make sure certain outputs are derivations, which will, of course, run the phase if doCheck is true, which it is by default.

I found your issue: https://github.com/NixOS/nix/issues/8882

It doesn't say anything about building packages, but: image

It appears to me that with the way checks is set up, the package's checkPhase does not fail if the Rust code is unformatted. The echo messages at the end of the check commands was causing it not to work.

spikespaz commented 9 months ago

flake: package: fix issues with checkPhase

  1. Move custom check commands to preCheck instead of postCheck, because source code checks should be run before cargo test (which is what the defualt checkPhase for buildRustPackage does).
  2. Remove cargo fmt --check because the derivation should still build regardless of formatting.
  3. Add cargo check --frozen because the package should not be used if this does not pass, in addition to the existing Clippy command.
  4. Remove the && echo parts of the check commands, because they cause the check to always succeed. There will already be output if these commands fail, which is what we actually care about as opposed to if they were successful.

flake: checks: add alejandra and cargo fmt checks

I have added a formatting check, as you requested. Took me a while to figure out how to do it, but now I know for my own projects in the future.

9999years commented 9 months ago

I'll have to check out nixfmt again, I'm curious to see how it avoids changing indentation levels!

spikespaz commented 9 months ago

Could you please re-run the CI?

9999years commented 9 months ago

I'm seeing error: cannot fetch input 'path:./flake.systems.nix' because it uses a relative path, corresponding to https://github.com/NixOS/nix/issues/3978... I guess we might have to use nix-systems to get around this?

spikespaz commented 9 months ago

Changed to use github:nix-systems/default. I didn't realize a relative path wouldn't work, that's strange to me. It's recommended in the readme even.

spikespaz commented 9 months ago

Thank you! Pleasure contributing.