Clean up Nix Flake & make it easier to customize

[RossSmyth]

Hello. It has been a while since #11528. I am better at Nix, so I decided to take a crack at cleaning the flake up.

My main issues with it is that it was not setup to be very customizable. Also the Cachix cache is nearly useless since it is compiled with target-cpu=native, so the cache will nearly always be missed. So I refactored it and added some comments.

1. I am unconvinced it needs all the source filtering.
2. By default everything uses the MSRV compiler as that is a safe default.
3. Cleaned up a few vestigial things that my guess is were left from earlier revisions

I did preserve the behavior that the devshell will compile with target-cpu=native and other flags. But now the Cachix cache is actually possible to hit.

You can see an example of it in use in my NixOS config here: https://github.com/RossSmyth/nixos/blob/338fce6a5b7c3c5fe9c1790929bae40ce1494006/home-manager/helix.nix#L10-L29

The main issue is that the previous weird setup may be relied one by some people. So this will break them.

Commits can be squashed when merged.

[the-mikedavis]

I was considering trying to remove the dependency on crane and only use rust-overlay directly (for example like nil does) but I'm not sure if that's an easy change. I haven't taken advantage of the checks part of the flake so I think it's fine to drop that part.

Removing target-cpu=native from the package build but keeping it in the shell sounds good.

Should we also consider switching to the opt profile for flake builds? (See #12453 for context - opt is what we use for releases)

[RossSmyth]

I was considering trying to remove the dependency on crane and only use rust-overlay directly

Yeah I considered that but also didn't think it was a big deal. I don't think it would be very hard to move to that. I can take a look later to see. It should be even easier to customize with only rust-overlay.

I haven't taken advantage of the checks part of the flake so I think it's fine to drop that part.

It's not a maintenance burden so there's not really a reason to remove it imo.

Should we also consider switching to the opt profile for flake builds?

Ah, I did not look at this. Sure, makes sense. FYI fat LTO is sort of a placebo effect now days since it doesn't get much attention in LLVM anymore. The main thing it does is make your builds take longer. Thin is generally pretty close or faster (at runtime) in many cases since Google and other folks put money into thin LTO (for chrome and such).

[RossSmyth]

Ah, I whipped up the overlay-only version today during lunch. It has the problem of customization being pretty obtuse atm.
https://github.com/RossSmyth/helix/blob/PureOver/flake.nix

See:
https://discourse.nixos.org/t/how-do-you-override-the-commit-rev-used-by-a-rust-package/47698
NixOS/nixpkgs#194475
NixOS/nixpkgs#234651
NixOS/nixpkgs#288430
NixOS/nixpkgs#354999

Basically this line:
https://github.com/RossSmyth/helix/blob/93bf32a8e82dd6e19e2407b7cc967929cc4807a9/flake.nix#L106-L110
Doesn't actually do anything useful.

[RossSmyth]

Moving to the Rust-Overlay only version would still be a net win though because Cachix would work. And it wouldn't be much different from the status-quo wrt customization.

[RossSmyth]

@the-mikedavis I found a solution.

Here's my NixOS config with customizations:
https://github.com/RossSmyth/nixos/blob/338fce6a5b7c3c5fe9c1790929bae40ce1494006/home-manager/helix.nix#L10-L29

[the-mikedavis]

On this branch I'm seeing an error when creating the dev shell:

$ nix --version
nix (Nix) 2.24.11
$ nix develop
evaluation warning: rustPlatform.rust.rustc is deprecated. Use rustc instead.
evaluation warning: rustPlatform.rust.cargo is deprecated. Use cargo instead.
error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:34:12:
           33|
           34|   strict = derivationStrict drvAttrs;
             |            ^
           35|

       … while evaluating derivation 'nix-shell'
         whose name attribute is located at /nix/store/sdzpqjwx7pdx6lsq6llyfqqf7hspp83c-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildInputs' of derivation 'nix-shell'
         at /nix/store/sdzpqjwx7pdx6lsq6llyfqqf7hspp83c-source/pkgs/stdenv/generic/make-derivation.nix:383:7:
          382|       depsHostHost                = elemAt (elemAt dependencies 1) 0;
          383|       buildInputs                 = elemAt (elemAt dependencies 1) 1;
             |       ^
          384|       depsTargetTarget            = elemAt (elemAt dependencies 2) 0;

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: cannot coerce a set to a string: { bindgenHook = «thunk»; buildRustPackage = «thunk»; cargoBuildHook = «thunk»; cargoCheckHook = «thunk»; cargoInstallHook = «thunk»; cargoNextestHook = «thunk»; cargoSetupHook = «thunk»; fetchCargoTarball = «thunk»; importCargoLock = «thunk»; maturinBuildHook = «thunk»; «3 attributes elided» }

The package build with nix build works however

[RossSmyth]

Oh yeah, I did not test that.

[the-mikedavis]

Should we use rustc and cargo from the attrs passed to mkHelix instead? I'm seeing a deprecation warning on accessing these

[RossSmyth]

No as they are not spliced in properly if you do that. The alternative is to change the callPackage invocations to be something like

pkgs.callPackage mkHelix {
    rustPlatform = msrvPlatform;
    rustc = msrvToolchain.rustc; 
    cargo = msrvToolchain.cargo;
};

[the-mikedavis]

For the package build (not the shell) we don't need to follow the MSRV, it's ok to build with latest stable. We do roughly the same in CI - the build workflow for PRs checks with the MSRV and then the release workflow uses whatever is the latest stable

[RossSmyth]

This is not for Helix, but rather for me (or anyone else who overrides the toolchain) so that when I override the toolchain that the correct Cargo and Rustc are used for the entire build.

[RossSmyth]

Hmm there's something weird with the fromRustupToolchainFile API. The check fails to build, and it was what was wrong with the devShell

[RossSmyth]

Ok with some help from oxalica I found my typo and it should work fine now.

[RossSmyth]

An optional thing that could be done (and I do this in my repos at work) is move the mkHelix function to the default.nix file.
See: RossSmyth@bb2141c

[RossSmyth]

It also looks like finalAttrs for buildRustPackage was merged, so the TODOs can be resolved.
RossSmyth@4edfad0

[the-mikedavis]

Looks great, thanks for working on this! The flake is much simpler now and I like having the actual derivation in default.nix (plus it looks very similar to what I'd expect to see for a package in nixpkgs)

[RossSmyth]

Thanks!

[RossSmyth]

For reference, this is what my overrides look like with the merged changes:
https://github.com/RossSmyth/nixos/blob/1d1bfacc61941809180ed80cbf54bf2d15bde83a/home-manager/helix.nix#L10-L24

[RossSmyth]

I may clean up the grammar file at some point but it's not as high of priority to me.

[the-mikedavis]

Eventually we will replace the hx -g fetch and hx -g update way that we manage grammars now with a single repository clone (using some stuff from https://github.com/helix-editor/tree-house). For that change we will probably need to rewrite or maybe discard grammars.nix

[ulic-youthlic]

Hi guys, I have a question about the new nix drv. In the past, i can use packages.<system>.helix.wrapper <config> function to customize the behavior of grammar.nix. For example, i can pass grammarOverlays in <config> to the wrapper to modify the tree-sitter grammars. But now, i cannot find a api for doing this. Is there a recommended way?

[the-mikedavis]

Hmm I'm not sure we expose anything for this now - I don't think it's possible to pass through grammarOverlays to grammars.nix anymore after the refactor.

[ulic-youthlic]

yes, i know that. i have already build my own helix-runtime for add grammar and tree-sitter libs.

[the-mikedavis]

It would be nice to have that functionality. I'm not very handy with Nix so I'm not sure the best way to add it back. But I'd be interested in a PR that makes it possible again