Skip to content
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

buildRustPackage is broken #23282

Closed
dermetfan opened this issue Feb 28, 2017 · 27 comments
Closed

buildRustPackage is broken #23282

dermetfan opened this issue Feb 28, 2017 · 27 comments

Comments

@dermetfan
Copy link
Member

dermetfan commented Feb 28, 2017

Issue description

pkgs.rust.buildRustPackage fails every time since I upgraded to nixpkgs 17.03pre101839.53a2baa.

This is perhaps also a chance to fix #22737.

In this example project:

❯ nix-build
these derivations will be built:
  /nix/store/7z80g4d5cc4n621vl28wzd23265cgcsz-cursedlife.drv
building path(s) ‘/nix/store/j2ify6k0kfay56mhwpzasdl1w46bqk3r-cursedlife’
unpacking sources
unpacking source archive /nix/store/9r24d70n0kzxx3zrlwp8jnnkai92giqn-cursedlife
source root is cursedlife
Using cargo deps from /nix/store/hyj7wbcfirjdx8min1wxdfaizsdsy617-cursedlife-fetch
Using indexHash '-ba82b75dd6681d6f'
Using rust registry from /nix/store/p07f0q58fvszdgjfg73i73g03jaxkfx8-rustRegistry-2017-02-19-d822c58
warning: custom registry support via the `registry.index` configuration is being removed, this functionality will not work in the future
error: failed to load source for a dependency on `ncurses`

Caused by:
  Unable to update registry file:///dev/null

Caused by:
  failed to open: /tmp/nix-build-cursedlife.drv-0/deps/registry/index/-ba82b75dd6681d6f/.cargo-index-lock

To learn more, run the command again with --verbose.
builder for ‘/nix/store/7z80g4d5cc4n621vl28wzd23265cgcsz-cursedlife.drv’ failed with exit code 101
error: build of ‘/nix/store/7z80g4d5cc4n621vl28wzd23265cgcsz-cursedlife.drv’ failed

The error message (Unable to update registry file:///dev/null) indicates that the behavior described in this code comment has changed. @wizeman?

Technical details

  • System: 17.03pre101839.53a2baa (Gorilla)
  • Nix version: nix-env (Nix) 1.11.6
  • Nixpkgs version: 17.03pre101839.53a2baa
@dermetfan dermetfan changed the title buildRustPackage broken buildRustPackage is broken Feb 28, 2017
@grahamc
Copy link
Member

grahamc commented Mar 1, 2017

I'm experiencing this same problem while building the NixOS/Security tools.

@globin
Copy link
Member

globin commented Mar 1, 2017

ping @wizeman

@grahamc
Copy link
Member

grahamc commented Mar 1, 2017

This was broken here: #22987 and I'm seeing extremely suspicious behavior:

  1. With master, build NixOS/security, see it fail (note you'll need to update the depsSha.)
  2. Revert PR buildRustPackage: fix deprecated use of registry.index config key #22987
  3. Build NixOS/security, see it pass
  4. Unrevert PR buildRustPackage: fix deprecated use of registry.index config key #22987
  5. Build NixOS/security, see it doesn't need to rebuild, the previous build worked fine!

@wizeman
Copy link
Member

wizeman commented Mar 1, 2017

@grahamc: That's what is supposed to happen if, for example, the change in #22987 broke the fetching of cargo dependencies.

Since the cargo dependencies get fetched as a fixed-output derivation, even if you change how the dependencies are fetched, Nix reuses the previously-succeeding fetch and says the build succeeded. It won't try to fetch the dependencies again unless depsSha256 is also changed.

@wizeman
Copy link
Member

wizeman commented Mar 1, 2017

Perhaps the change in #22987 can be reverted temporarily until this is fixed.

@grahamc
Copy link
Member

grahamc commented Mar 1, 2017

I think I understand what you're saying, except with master, NixOS/security first fails to build saying the sha doesn't match. Then I update the sha, then it fails to build. Does this still make sense?

@wizeman
Copy link
Member

wizeman commented Mar 1, 2017

Yes, it still makes sense.

That could happen if the change in #22987 made the result of fetching the cargo dependencies incompatible with the "please-do-not-touch-the-network-just-reuse-what-we-downloaded-before" cargo fetch that happens in the postUnpack phase.

@grahamc
Copy link
Member

grahamc commented Mar 1, 2017

I get it now: I suppose it isn't recalculating what the depsha SHOULD be, but instead simply finds it already present, and skips the calculation?

@wizeman
Copy link
Member

wizeman commented Mar 1, 2017

@grahamc Yes (if I understand correctly what you're saying).

Anyway, maybe in the meantime we could just revert #22987?

Unfortunately I can't dedicate much time to these issues right now (or in the near future)...

@LnL7
Copy link
Member

LnL7 commented Mar 1, 2017

Yes indeed, I ran into a fetchurl a while back that had the sha of a different source. That will caused the derivation to use the wrong source because it was fetched before.

@anderspapitto
Copy link
Contributor

hmm. I started looking at this (I broke it in #22987), and it's not something that's super trivial to me to fix. Can we revert for now? I was just getting rid of a deprecation warning anyway.

1 similar comment
@anderspapitto
Copy link
Contributor

hmm. I started looking at this (I broke it in #22987), and it's not something that's super trivial to me to fix. Can we revert for now? I was just getting rid of a deprecation warning anyway.

@P-E-Meunier
Copy link
Contributor

I've also noticed the same for a new project with a new default.nix, this wasn't easy to debug.
So, after reading a bit about the new cargo options --frozen and --locked, I started using the code below.
I'd love to get comments!

with import <nixpkgs> {};
rec {
   hello = pkgs.stdenv.mkDerivation rec {
      name = "hello";
      src = ./hello;
      buildInputs = [ pkgs.rustc pkgs.cargo pkgs.rustPlatform.rustRegistry ];
      postUnpack = ''
      mkdir -p $sourceRoot/.cargo
      cat <<EOF > $sourceRoot/.cargo/config
[source.crates-io]
replace-with = "nix-store-rust-registry"

[source.nix-store-rust-registry]
registry = "file://${pkgs.rustPlatform.rustRegistry}"
EOF
      export SSL_CERT_FILE=${cacert}/etc/ssl/certs/ca-bundle.crt
      export CARGO_HOME=$sourceRoot/.cargo
      mkdir -p $CARGO_HOME
      cd $sourceRoot
      cargo fetch --locked --verbose
      cargo clean
      cd ..
      rm -Rf $sourceRoot/.cargo/registry/index/*/.git
      find . -exec touch --date=@$SOURCE_DATE_EPOCH {} \;
      '';

      buildPhase = ''
       export CARGO_HOME=$sourceRoot/.cargo
       cargo build --frozen --release
      '';
      installPhase = ''
         install -m 755 -d $out/bin
         install -m 755 target/release/hello $out/bin
      '';
   };
}

@Mic92
Copy link
Member

Mic92 commented Mar 3, 2017

We could actually create a cargo-nixify wrapper to generate nix expressions like bundix or go2nix do. Rust now has a source replacement so it could be implemented similar like cargo-vendor, but generating derivations instead of a vendor directory.

Advantages:

  • remove the need of global rustRegistry in nix (crate version would be fixed at generation time)
  • fix problems with git repos without a fixed commit revision
  • dependencies do not need to be fetched twice to get the depsSha256.

@dermetfan
Copy link
Member Author

dermetfan commented Mar 25, 2017

Here's what I built on top of @P-E-Meunier's script:

# buildRustPackage.nix

{ pkgs ? import <nixpkgs> {}
, stdenv ? pkgs.stdenv
, rustPlatform ? pkgs.rustPlatform
, rustRegistry ? rustPlatform.rustRegistry }:

{ name
, release ? true
, doHydra ? false
, src
, buildInputs ? []
, ... } @ args:

stdenv.mkDerivation ({
    SSL_CERT_FILE = "${pkgs.cacert}/etc/ssl/certs/ca-bundle.crt";
    RUST_BACKTRACE = 1;
    RUST_LOG = "${name}=info";

    configurePhase = ''
      # We don't need the configure phase
      # but leave the user the option
      # to specify his own.
    '';
  } // args // {
  inherit src;

  buildInputs = buildInputs ++ [
    rustPlatform.rust.rustc
    rustPlatform.rust.cargo
  ];

  postUnpack = ''
    export CARGO_HOME=$sourceRoot/.cargo
  '' + (if rustRegistry != null then ''
    mkdir -p $CARGO_HOME
    cat <<EOF > $CARGO_HOME/config
    [source.crates-io]
    replace-with = "nix-registry"

    [source.nix-registry]
    registry = "file://${rustRegistry}"
    EOF

    workdir="`pwd`"
    cd $sourceRoot
    cargo fetch --locked
    cargo clean
    cd "$workdir"

    rm -rf $CARGO_HOME/registry/index/*/.git
    find . -execdir touch --date=@$SOURCE_DATE_EPOCH {} \;
  '' else "");

  buildPhase = ''
    cargo build ${if release then "--release" else ""} --${if rustRegistry != null then "frozen" else "locked"}
  '';

  checkPhase = ''
    cargo test
  '';

  installPhase = ''
    install -m 755 -d $out/bin
    for bin in `find target/${if release then "release" else "debug"} -maxdepth 1 -type f -not -name .\*`; do
      install -m 755 $bin $out/bin/
    done
  '';

  distPhase = ''
    mkdir $out/tarballs
    tar cf $out/tarballs/${name}.tar `find . -maxdepth 1 -not -name . -not -name target -not -name .hg\* -not -name .cargo`
  '';

  hydraPhase = ''
    mkdir $out/nix-support
    for bin in `find $out/bin -type f`; do
      echo "file binary-dist $bin" >> $out/nix-support/hydra-build-products
    done
  '';

  postPhases = if doHydra then "hydraPhase" else "";
})

You can pass in your own registry so you don't have to update the rev in your local nixpkgs clone (or explicitely pass null to use the latest crates.io, which is not deterministic).

Usage example:

# default.nix

{ pkgs ? import <nixpkgs> {}
, buildRustPackage ? import ./buildRustPackage.nix { # or fetch from a repo
    inherit pkgs;
    rustRegistry = null;
  } }:

buildRustPackage {
  name = "cursedlife";

  src = ./.;

  buildInputs = with pkgs; [
    ncurses
  ];

  doCheck = true;
}

It should also be possible to adapt rust-packages.nix so that you could pass in the desired repo url and rev. That way we could choose any registry in each project's default.nix through something like

# ...
buildRustPackage = import ./buildRustPackage.nix {
  rustRegistry = pkgs.rustRegistry {
    src = pkgs.fetchFromGitHub {
      owner = "me";
      repo = "my-rust-registry";
      rev = "...";
    };
  };
}
# call buildRustPackage ...

This doesn't have a depsSha256 yet but that could probably be added. Are there other advantages to writing a bundix-like tool instead?

@NeQuissimus
Copy link
Member

Is there a PR for the above?

@P-E-Meunier
Copy link
Contributor

I've started a PR (#24991) for a replacement for the whole thing. My PR uses nix to do what cargo normally does:

  • the current buildRustPackage is calling cargo to build packages from scratch, i.e. build products are not shared between packages
  • my solution builds individual crates by calling rustc directly, and uses nix to take care of rebuilding dependencies.

As mentionned in the PR, there are a number of things that still need to be done, I wouldn't mind help.

@NeQuissimus
Copy link
Member

Ah, I was unable to find the PR. We need to it update alacritty. Unfortunately, I don't know anything about Rust :)

@ckauhaus
Copy link
Contributor

AFAIC the solution proposed by @dermetfan is not compatible with sandboxed builds. Apart from that, I really like it. How could we improve it to make it compatible?

@P-E-Meunier
Copy link
Contributor

@NeQuissimus: I'm a little slow sorry, I just found a potential source of confusion. I worked on two distinct solutions in the past:

  • The small script above in this issue page, trying to use shiny new features of cargo. Its major benefit is to reproduce exactly what cargo does: if you can compile your project locally with cargo, the package will do the same. The main drawback (quite important IMHO) is that libraries are not shared between projects: most crates on https://crates.io depend on the same few libraries such as libc, regex, ring, log, serde, byteorder, bitflags, error_chain… Some of these can be pretty long to compile. Since Rust wants to guarantee a consistent user experience, even though build products made with different versions of the compiler are incompatible, they have to recompile everything every time, at least on most platforms/linux distributions.

  • On NixOS, the situation can be largely improved by noticing that nix can do a large part of cargo's job, in a better way, and that is what my PR Introducing mkRustCrate #24991 is about. The main benefit is not only much smaller compilation times, but also reduced disk space. The difference in productivity is significant when deploying a Rust service with nixops (my new PR makes it about 100 faster to compile, a luxury that no other package manager / devops engine can provide for Rust). The downsides are (1) the packager needs to run a small program to generate a nix file automatically from the Cargo.lock file produced by cargo, and (2) my PR can compile all packages I've every tried, but that's far from all packages that can be compiled by cargo.

@ckauhaus
Copy link
Contributor

ckauhaus commented Aug 10, 2017

@NeQuissimus I worked on @dermetfan's buildRustPackage and here is what I think would do the job:

https://gist.github.com/ckauhaus/808cbea9e41d99ae63208c02f245b0e6

Disclaimer: This is a largely untested proof-of-concept.

In contrast to @P-E-Meunier's solution, I stick with the traditional approach and create a "fetch-deps" derivation specifically for each Cargo.lock. In contrast to previous solutions, I make use of the new cargo "local-registry" feature and bundle a cache of local *.crate file copies together with a index checkout.

This approach is not particularly efficient, as it unpacks $src twice, makes an additional copy of the crates.io index and, of course, begins right from the start each time the fixed-output hash changes. BUT: it works in a sandbox and works with Cargo 1.20.

I think it would be still an improvement over the current implementation of buildRustPackage especially as it is broken with 1.19 and relies on a deprecated Cargo feature (registry.index, see reverted PR #22987).

Please review, discuss, improve the code. :-)

@NeQuissimus
Copy link
Member

It looks OK to me as an interim solution. But again, I am not really a Rust person...
We could do something like this until we have #24991 in?!

@dermetfan
Copy link
Member Author

dermetfan commented Aug 10, 2017

@NeQuissimus I think we should keep both @ckauhaus solution and #24991. That way we could use mkRustCrate and fall back to buildRustPackage in case we hit an unsupported crate.

@ckauhaus
Copy link
Contributor

Ok, sounds encouraging. :-) I'll polish it a bit and test it with various Rust packages found in the source tree.

@ckauhaus
Copy link
Contributor

ckauhaus commented Aug 11, 2017

#22737 (empty deps) should also be solved on this route.

@Ekleog
Copy link
Member

Ekleog commented Nov 1, 2018

(triage) The nix-Rust ecosystem has now stabilized a bit with buildRustPackage and carnix. Is this issue still required? If so, at least the title should be changed :)

@Mic92
Copy link
Member

Mic92 commented Nov 1, 2018

I also think we have this solved now.

@Mic92 Mic92 closed this as completed Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests