oxalica / rust-overlay

Pure and reproducible nix overlay of binary distributed rust toolchains
MIT License
930 stars 53 forks source link

Overriding nixpkgs' rust/cargo to rust-overlay's toolchain causes evaluation failures #153

Open pacak opened 9 months ago

pacak commented 9 months ago

At some point nixpkgs made a decision to include cargo-auditable wrapper of some sort which ends up passing attribute auditable to rust-overlay, failure looks like this:

       error: function 'anonymous lambda' called with unexpected argument 'auditable'                                                                                                                                                          

       at /nix/store/xq090frv0r8j7091kby7q1rjc2fnjsbr-source/rust-overlay.nix:294:24:                                                                                                                                                          

          293|     mkProfile = name: profileComponents:                                                                                                                                                                                        
          294|       makeOverridable ({ extensions, targets, targetExtensions }:                                                                                                                                                               
             |                        ^                                                                                                                                                                                                        
          295|         mkAggregated {                                                                                                                                                                                                          

I was able to work around the problem by passing auditable = false; to every instance of buildRustPackage, but having to specify it all over the place is mildly annoying. I suspect it should be possible to specify it globally for all the things I'm compiling but my nix fu is limited.

I don't really care about auditable functionality, but it would be great to be able to use rust-overlay without having to deal with it.

oxalica commented 8 months ago

At some point nixpkgs made a decision to include cargo-auditable wrapper of some sort which ends up passing attribute auditable to rust-overlay, failure looks like this:

Could you provide the repro code? I tried to override a random package to use the toolchain from rust-overlay, and it compiles without any issue. Seems there's a auditable-<name> wrapper handles the auditable stuff correctly.

with import <nixpkgs> { overlays = [ (import /path/to/rust-overlay) ]; };
pkgs.cargo-asm.override {
  rustPlatform = makeRustPlatform {
    cargo = rust-bin.stable.latest.minimal;
    rustc = rust-bin.stable.latest.minimal;
  };
}
Fuuzetsu commented 8 months ago

I have made https://github.com/Fuuzetsu/rust-overlay-auditable-repro as a repro.

You can comment/uncomment the auditable = false part to so it build or fail.

I have experimented a little and found that it Just Works™ if I remove this bit:

                cargo = rustChannel;
                rustPlatform = pkgs.makeRustPlatform {
                  rustc = rustChannel;
                  cargo = rustChannel;
                };

In our original code we have a larger block like so:

rustToolchain = pkgs:
    let
      rustChannel = (pkgs.rust-bin.fromRustupToolchainFile ../rust-toolchain).override {
        extensions = [
          "clippy"
          "rust-analysis"
          "rust-docs"
          "rust-src"
          "rustfmt"
        ];
      };
    in
    {
      rustc = rustChannel;
      cargo = rustChannel;
      rust-fmt = rustChannel;
      rust-std = rustChannel;
      clippy = rustChannel;
      rustPlatform = pkgs.makeRustPlatform {
        rustc = rustChannel;
        cargo = rustChannel;
      };
    };

I don't completely remember why it was done this way, maybe it's no longer necessary and we should just be keeping the extensions bit as-is...

oxalica commented 8 months ago

Overriding nixpkgs' root rust and/or cargo should be avoided. I think many of nixpkgs' infrastructures rely on some internal attributes of compilers and build systems. I suggest to construct your own rustPlatform only for building or overriding specific packages, not overriding the global one.

            rustToolchain = pkgs:
              let
                rustChannel = (pkgs.rust-bin.fromRustupToolchain { channel = "1.76.0"; });
              in
              {
                # Commenting out the below makes the auditable thing go away
                cargo = rustChannel;
                rustPlatform = pkgs.makeRustPlatform {
                  rustc = rustChannel;
                  cargo = rustChannel;
                };
              };

            rust = import nixpkgs {
              inherit system; overlays = [
              (import rust-overlay)
              (self: _: rustToolchain self)
            ];
          };
Fuuzetsu commented 8 months ago

Overriding nixpkgs' root rust and/or cargo should be avoided. I think many of nixpkgs' infrastructures rely on some internal attributes of compilers and build systems. I suggest to construct your own rustPlatform only for building or overriding specific packages, not overriding the global one.

That's a bit sad because one needs to very carefully override everything rust-based explicitly or end up with 2 versions of rust in the dependency tree... "Use specific rust version for nixpkgs" doesn't seem like it should be that wild of a use-case, right? Maybe we're in the minority...

Fuuzetsu commented 2 months ago

After we updated nixpkgs recently, we start seeing this error again. We followed the suggestion to stop overriding the toolchain for all of nixpkgs and it's all OK for now. So for us at least it's less priority than it was...

I still think it should probably be possible to do this but if it's not supported, maybe just adding some info about why in README is good enough.