Rust build-support: fixing a compilation error in some crates #50452

P-E-Meunier · 2018-11-16T12:15:33Z

BuildRustCrate fails on some crates (such as proc-macro2), because some environment variables were not set. This fixes it.

Additionally, an error message related to the install script is fixed.

Motivation for this change

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

…s proc-macro2)

dtzWill · 2018-11-16T14:20:58Z

What's the motivation for NUM_JOBS=1? Does that impact parallel building?

andir · 2018-11-16T15:18:24Z

This is the definition of NUM_CPUS:

* `NUM_JOBS` - the parallelism specified as the top-level parallelism. This can
               be useful to pass a `-j` parameter to a system like `make`. Note
               that care should be taken when interpreting this environment
               variable. For historical purposes this is still provided but
               recent versions of Cargo, for example, do not need to run `make
               -j` as it'll automatically happen. Cargo implements its own
               [jobserver] and will allow build scripts to inherit this
               information, so programs compatible with GNU make jobservers will
               already have appropriately configured parallelism.

And the RUSTC case:

* `RUSTC`, `RUSTDOC` - the compiler and documentation generator that Cargo has
                       resolved to use, passed to the build script so it might
                       use it as well.

So I think the addition is warranted. We might as well go for the full list of them as it can be found in the cargo search at src/doc/src/reference/environment-variables.md.

Should we also add test cases so that we can ensure we are properly compiling something that actually depends on those?

andir

See the inline comments and #50452 (comment).

andir · 2018-11-16T15:21:11Z

pkgs/build-support/rust/build-rust-crate/configure-crate.nix

@@ -71,6 +71,8 @@ in ''
  export CARGO_PKG_VERSION_MAJOR=${builtins.elemAt version 0}
  export CARGO_PKG_VERSION_MINOR=${builtins.elemAt version 1}
  export CARGO_PKG_VERSION_PATCH=${builtins.elemAt version 2}
+  export NUM_JOBS=1
+  export RUSTC="rustc"


Shouldn't we set this to the absolute path maybe? I am not sure what the exactly expectation is in this case.

I don't think it matters, as I believe this is meant to be argv[0] when rustc is called, and since we call rustc directly everywhere else, I believe this is what is expected in most build scripts.
Since we have rustc in the builtInputs, rustc is in the path, I think we're fine.

I remember rustc to do a realpath on /proc/self/exe at some point, when trying to cheat with our rustup patch.

andir · 2018-11-16T15:22:29Z

pkgs/build-support/rust/build-rust-crate/configure-crate.nix

@@ -71,6 +71,8 @@ in ''
  export CARGO_PKG_VERSION_MAJOR=${builtins.elemAt version 0}
  export CARGO_PKG_VERSION_MINOR=${builtins.elemAt version 1}
  export CARGO_PKG_VERSION_PATCH=${builtins.elemAt version 2}
+  export NUM_JOBS=1


According to the docs this should not be required in the future. Does this imply that newer code will just ignore it an use the proper jobserver implementation?

I don't know, it's still in the docs. I was just trying to include as many environment variables from the docs as possible.

P-E-Meunier · 2018-11-16T17:02:00Z

What's the motivation for NUM_JOBS=1? Does that impact parallel building?

Rustc is already called with -j1 by buildRustCrate, so this environment variable will not change anything.
When used with cargo, this environment variable means cargo will compile multiple modules of the same crate in parallel. Nix adopts a different strategy, by parallelising only at the crate level (doing otherwise doesn't work).

dtzWill · 2018-11-16T17:12:54Z

FWIW I'm not against it and don't know much about rust building, so don't really have an opinion either way :). Just noticed and was curious (and to confirm / be clear about such a reason, whatever it may be).

…

On Fri, 16 Nov 2018 09:02:53 -0800, Pierre-Etienne Meunier ***@***.***> wrote: P-E-Meunier commented on this pull request. > @@ -71,6 +71,8 @@ in '' export CARGO_PKG_VERSION_MAJOR=${builtins.elemAt version 0} export CARGO_PKG_VERSION_MINOR=${builtins.elemAt version 1} export CARGO_PKG_VERSION_PATCH=${builtins.elemAt version 2} + export NUM_JOBS=1 I don't know, it's still in the docs. I was just trying to include as many environment variables from the docs as possible. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #50452 (comment) part: text/html

P-E-Meunier · 2018-11-16T17:14:31Z

So I think the addition is warranted. We might as well go for the full list of them as it can be found in the cargo search at src/doc/src/reference/environment-variables.md.

Right.

Should we also add test cases so that we can ensure we are properly compiling something that actually depends on those?

I don't know how to add test cases in Nix, but I'd be happy to learn.

thoughtpolice · 2018-11-18T00:16:46Z

I just ran into this failure myself when using proc_macro2 as well; here was my patch, after I reverse-engineered the failure back to RUSTC:

diff --git a/pkgs/build-support/rust/build-rust-crate/configure-crate.nix b/pkgs/build-support/rust/build-rust-crate/configure-crate.nix
index 37fef2abd77..0be77fb4c0a 100644
--- a/pkgs/build-support/rust/build-rust-crate/configure-crate.nix
+++ b/pkgs/build-support/rust/build-rust-crate/configure-crate.nix
@@ -1,4 +1,4 @@
-{ lib, stdenv, echo_build_heading, noisily, makeDeps }:
+{ lib, stdenv, which, echo_build_heading, noisily, makeDeps }:
 { build, buildDependencies, colors, completeBuildDeps, completeDeps, crateAuthors, crateFeatures, crateName, crateVersion, extraLinkFlags, libName, libPath, release, target_os, verbose, workspace_member }:
 let version_ = lib.splitString "-" crateVersion;
     versionPre = if lib.tail version_ == [] then "" else builtins.elemAt version_ 1;
@@ -61,6 +61,8 @@ in ''
   export CARGO_CFG_TARGET_POINTER_WIDTH=${toString stdenv.hostPlatform.parsed.cpu.bits}
   export CARGO_CFG_TARGET_VENDOR=${stdenv.hostPlatform.parsed.vendor.name}

+  export RUSTC=$(${which}/bin/which rustc)
+  export RUSTDOC=$(${which}/bin/which rustdoc)
   export CARGO_MANIFEST_DIR="."
   export DEBUG="${toString (!release)}"
   export OPT_LEVEL="${toString optLevel}"
diff --git a/pkgs/build-support/rust/build-rust-crate/default.nix b/pkgs/build-support/rust/build-rust-crate/default.nix
index ec11472bbae..ac790b0b62f 100644
--- a/pkgs/build-support/rust/build-rust-crate/default.nix
+++ b/pkgs/build-support/rust/build-rust-crate/default.nix
@@ -4,7 +4,7 @@
 # This can be useful for deploying packages with NixOps, and to share
 # binary dependencies between projects.

-{ lib, stdenv, defaultCrateOverrides, fetchCrate, rustc }:
+{ lib, stdenv, which, defaultCrateOverrides, fetchCrate, rustc }:

 let
     # This doesn't appear to be officially documented anywhere yet.
@@ -53,7 +53,7 @@ let
       }
     '';

-    configureCrate = import ./configure-crate.nix { inherit lib stdenv echo_build_heading noisily makeDeps; };
+    configureCrate = import ./configure-crate.nix { inherit lib stdenv which echo_build_heading noisily makeDeps; };
     buildCrate = import ./build-crate.nix { inherit lib stdenv echo_build_heading noisily makeDeps; };
     installCrate = import ./install-crate.nix;

i.e. I set the absolute path.

I think not setting the absolute path is also just fine.

In general a test case would be nice, though, but I don't think we should hold this up for that? proc-macro2 is used quite widely through some indirect dependencies, so I think this is generally OK, and overall this change is relatively minor. I'd like to see this land soon (so I can eliminate some in-tree patches)

andir · 2018-11-18T00:23:51Z

@P-E-Meunier The tests are currently in pkgs/build-support/rust/build-rust-crate/test.

Other then that we can go ahead and merge it.

grahamc · 2018-12-01T02:43:44Z

Looks like a compilation error in some crates might include thread 'main' panicked at 'Failed to get rustc version', libcore/option.rs:1000:5 in libc.

Ralith · 2018-12-01T02:45:37Z

Shouldn't this also set RUSTDOC?

P-E-Meunier · 2018-12-01T11:18:12Z

I'm currently tweaking this to try and compile Rust to WASM. This would be very cool, but doesn't work yet. When called with Cargo, Rustc generates perfect WASM. When called with Nix, Rustc does generate WASM, but it is not usable.

grahamc · 2018-12-01T19:47:36Z

This patch-set as of yesterday works nicely, and I've used this patch set in ofborg. We should probably merge it, and do wasm dev in a different patch set?

…

On Sat, Dec 1, 2018 at 6:18 AM Pierre-Etienne Meunier < ***@***.***> wrote: I'm currently tweaking this to try and compile Rust to WASM. This would be very cool, but doesn't work yet. When called with Cargo, Rustc generates perfect WASM. When called with Nix, Rustc does generate WASM, but it is not usable. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#50452 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAErrJs8d5so8hx6WTxxl7Mo7pzbaWLLks5u0mV7gaJpZM4YmJeS> .

thoughtpolice · 2018-12-01T20:19:09Z

I think this patch should work nicely and honestly I'm willing to go ahead with this, without a test, since it's been waiting for a little bit.

I think RUSTDOC should also be set, too, as it was in my original patch (otherwise this will just happen again)

P-E-Meunier · 2018-12-02T10:19:52Z

No problem! Since I've used this commit to deploy stuff in production, I sort of messed up my branches locally to make this feasible, and merging would spare me a morning of rebasing, so yes please!

See NixOS#50452 (comment) Signed-off-by: Austin Seipp <aseipp@pobox.com>

See #50452 (comment) Signed-off-by: Austin Seipp <aseipp@pobox.com>

Bofore this change, NUM_JOBS was set to 1. Some crates for building C/C++ code (e.g. the cc and cmake crates), rely on this variable to set the number of jobs. As a consequence, we were compiling embedded libraries serially. Change this to NIX_BUILD_CORES to permit parallel builds. Prior discussion: NixOS#50452 (comment)

Rust build-support: fixing a compilation error in some crates (such a…

f1de24f

…s proc-macro2)

GrahamcOfBorg added 6.topic: rust 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Nov 16, 2018

Mic92 requested a review from andir November 16, 2018 13:44

andir reviewed Nov 16, 2018

View reviewed changes

thoughtpolice mentioned this pull request Nov 18, 2018

Consider failing loudly when RUSTC is unset in build.rs dtolnay/proc-macro2#153

Closed

Pierre-Étienne Meunier added 2 commits November 22, 2018 11:40

Fixing "include"

0e8332c

Carnix 0.9.2

3083fa2

grahamc merged commit fc459de into NixOS:master Dec 3, 2018

thoughtpolice added a commit to thoughtpolice/nixpkgs that referenced this pull request Dec 5, 2018

buildRustCreate: export RUSTDOC during cargo config

6c5279b

See NixOS#50452 (comment) Signed-off-by: Austin Seipp <aseipp@pobox.com>

thoughtpolice mentioned this pull request Dec 5, 2018

buildRustCreate: export RUSTDOC during cargo config #51536

Merged

10 tasks

thoughtpolice added a commit that referenced this pull request Dec 5, 2018

buildRustCreate: export RUSTDOC during cargo config

3aa9091

See #50452 (comment) Signed-off-by: Austin Seipp <aseipp@pobox.com>

danieldk mentioned this pull request Dec 3, 2020

buildRustCrate: set NUM_JOBS to NIX_BUILD_CORES #105787

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust build-support: fixing a compilation error in some crates #50452

Rust build-support: fixing a compilation error in some crates #50452

P-E-Meunier commented Nov 16, 2018

dtzWill commented Nov 16, 2018

andir commented Nov 16, 2018 •

edited

andir left a comment

andir Nov 16, 2018

P-E-Meunier Nov 16, 2018

Mic92 Nov 19, 2018

andir Nov 16, 2018

P-E-Meunier Nov 16, 2018

P-E-Meunier commented Nov 16, 2018

dtzWill commented Nov 16, 2018 via email

P-E-Meunier commented Nov 16, 2018

thoughtpolice commented Nov 18, 2018

andir commented Nov 18, 2018

grahamc commented Dec 1, 2018

Ralith commented Dec 1, 2018

P-E-Meunier commented Dec 1, 2018

grahamc commented Dec 1, 2018 via email

thoughtpolice commented Dec 1, 2018

P-E-Meunier commented Dec 2, 2018

Rust build-support: fixing a compilation error in some crates #50452

Rust build-support: fixing a compilation error in some crates #50452

Conversation

P-E-Meunier commented Nov 16, 2018

Motivation for this change

Things done

dtzWill commented Nov 16, 2018

andir commented Nov 16, 2018 • edited

andir left a comment

Choose a reason for hiding this comment

andir Nov 16, 2018

Choose a reason for hiding this comment

P-E-Meunier Nov 16, 2018

Choose a reason for hiding this comment

Mic92 Nov 19, 2018

Choose a reason for hiding this comment

andir Nov 16, 2018

Choose a reason for hiding this comment

P-E-Meunier Nov 16, 2018

Choose a reason for hiding this comment

P-E-Meunier commented Nov 16, 2018

dtzWill commented Nov 16, 2018 via email

P-E-Meunier commented Nov 16, 2018

thoughtpolice commented Nov 18, 2018

andir commented Nov 18, 2018

grahamc commented Dec 1, 2018

Ralith commented Dec 1, 2018

P-E-Meunier commented Dec 1, 2018

grahamc commented Dec 1, 2018 via email

thoughtpolice commented Dec 1, 2018

P-E-Meunier commented Dec 2, 2018

andir commented Nov 16, 2018 •

edited