buildRustPackage: fix cargoBuildFlags #91689

Flakebi · 2020-06-27T20:44:01Z

Motivation for this change

When features were supplied in cargoBuildFlags, the binaries were built
with these features enabled. Unless checking was disabled, cargo test
was executed without these build flags, meaning the binaries were
rebuilt and overwritten without the specified features.

Fix this bug by supplying the build flags also in the check phase.

Fixes #91191.

Things done

I tested rustup with this patch and it is fixed.

I also started a nixpkgs-review but it tries to rebuild all packages depending on buildRustPackage, so I regularly run out of ram.
The last result was: [228/288 built (8 failed)]

pkgs/development/tools/rust/rustup/default.nix

B4dM4n · 2020-06-28T16:46:45Z

Another solution might be to use installCheckPhase instead of the current checkPhase for tests in buildRustPackage.

This way cargo test can be run with any checkFlags / buildType without interfering with the desired buildPhase output.

Mic92 · 2020-06-28T19:26:06Z

Another solution might be to use installCheckPhase instead of the current checkPhase for tests in buildRustPackage.

This way cargo test can be run with any checkFlags / buildType without interfering with the desired buildPhase output.

That sounds like a better solution to me actually. There might be many tests suite that could start to break once we start adding custom build flags.

When features were supplied in cargoBuildFlags, the binaries were built with these features enabled. Unless checking was disabled, `cargo test` was executed without these build flags, meaning the binaries were rebuilt and overwritten without the specified features. Fix this bug by running tests after the installation phase.

Flakebi · 2020-06-29T19:41:45Z

Good idea, I tested rustup with nixpkgs-review and it works.

Hoverbear · 2020-06-29T21:55:21Z

Also bumped into this today!

zowoq · 2020-07-02T01:44:00Z

~~What do you think about merging this to master with the rustc version bump so this is fixed quickly and the number of rebuilds are more worthwhile? Hydra seems to be quiet at the moment.~~ rustc version bump is now in staging-next.

8573 · 2020-07-03T02:37:05Z

pkgs/build-support/rust/default.nix

@@ -199,7 +199,7 @@ stdenv.mkDerivation (args // {
      -executable ! \( -regex ".*\.\(so.[0-9.]+\|so\|a\|dylib\)" \))
  '';

-  checkPhase = args.checkPhase or (let
+  installCheckPhase = args.checkPhase or (let


If checkPhase is changed to installCheckPhase, should doCheck—

nixpkgs/pkgs/build-support/rust/default.nix

Line 213 in deb7815

doCheck = args.doCheck or true;

—be changed to doInstallCheck? By my reading of it, section 6.5.9 of the Nixpkgs manual seems to say that installCheckPhase won't be run unless doInstallCheck = true is set, as section 6.5.6 seems to say for checkPhase and doCheck.

Yes, with this it seems we have correct flags but we aren't running the checks.

However running the checks results in:

Running cargo test --release --target x86_64-apple-darwin --frozen -- Blocking waiting for file lock on build directory

I don't have more time to look at this right now, I can come back to it tomorrow.

I won't bother reverting this as having correct flags without checks seems preferable over the status quo.

This seems to break the rls build on master:

checking for references to /build/ in /nix/store/fa4hvsckbjq80dhck6lxmwgx3waxjck1-rls-1.44.1... running install tests /build/rustc-1.44.1-src/src/tools/rls /build/rustc-1.44.1-src Running cargo test --release --target x86_64-unknown-linux-gnu --frozen -- Blocking waiting for file lock on build directory

It is wrong. It should be at least installCheckPhase = args.installCheckPhase or ....
Now it overrides the original installCheckPhase and also breaks #91359 (infinite Blocking waiting for file lock).

I'm trying to fix the original problem in a different way atm to get this patch reverted btw.

Thanks for trying to fix this in a better way @Ma27.
Sorry for breaking packages, I should have tested more with nixpkgs-review.

Ma27 · 2020-07-13T21:57:51Z

Strong 👎 on this.

This breaks several packages and is fairly confusing since we mix up two distinct phases during a derivation's build without even updating the docs on this! Rather than modifying the build process for an entire ecosystem to get a single package fixed, we should discuss how to deal with it. A window of 5 days is too close for that IMHO.

Since we broke at least two more packages (rls, ripgrep-all and probably more) to fix a runtime bug of a package whose core-functionality worked fine, I'd prefer to revert this.

Unless checking was disabled, cargo test
was executed without these build flags, meaning the binaries were
rebuilt and overwritten without the specified features.

I can think of three (unverified) ways to get this fixed:

We can set build-flags during checkPhase as well to make sure that unused features don't get tested.
We can copy the results from buildPhase to a different location and use that for installPhase.

We can leave out the rm here (just a wild guess though):

nixpkgs/pkgs/build-support/rust/default.nix

Lines 222 to 228 in deb7815

    
               # rename the output dir to a architecture independent one 
        
               mapfile -t targets < <(find "$NIX_BUILD_TOP" -type d | grep '${releaseDir}$') 
        
               for target in "''${targets[@]}"; do 
        
                 rm -rf "$target/../../${buildType}" 
        
                 ln -srf "$target" "$target/../../" 
        
               done 
        
               mkdir -p $out/bin $out/lib

oxalica · 2020-07-14T13:06:20Z

pkgs/build-support/rust/default.nix

@@ -199,7 +199,7 @@ stdenv.mkDerivation (args // {
      -executable ! \( -regex ".*\.\(so.[0-9.]+\|so\|a\|dylib\)" \))
  '';

-  checkPhase = args.checkPhase or (let
+  installCheckPhase = args.checkPhase or (let
    argstr = "${stdenv.lib.optionalString (checkType == "release") "--release"} --target ${rustTarget} --frozen";
  in ''
    ${stdenv.lib.optionalString (buildAndTestSubdir != null) "pushd ${buildAndTestSubdir}"}


The next line runs preCheck hook in installCheckPhase, which is quite a weird behavior.
So does postCheck.

This reverts commit deb7815. Mixing up two distinct phases of a derivation's build is not a good idea. See also NixOS#91689 (comment).

While the artifacts from `buildPhase` should be used for testing as well, it should be avoided that those are modified during `checkPhase`. This can happen if a package is built e.g. with special `cargoBuildFlags` that don't apply to the `checkPhase`. In that case, a binary would be installed into `$out` without those flags since `checkPhase` overrides the binary in the `target`-directory. This patch copies the state of `target/release` into a temporary location at the end of the `buildPhase` and installs the results from that temporary directory into `$out` while `checkPhase` can continue using the configured build-dir. cc NixOS#91689 Closes NixOS#93119 Closes NixOS#91191

Flakebi requested a review from andir as a code owner June 27, 2020 20:44

Flakebi mentioned this pull request Jun 27, 2020

rustup tries to update itself and fails with error: Read-only file system #91191

Closed

ofborg bot added the 6.topic: rust label Jun 27, 2020

ofborg bot requested a review from Mic92 June 27, 2020 20:51

ofborg bot added 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 labels Jun 27, 2020

Mic92 mentioned this pull request Jun 27, 2020

tokei: 11.2.1 -> 12.0.3 #91298

Closed

10 tasks

Mic92 reviewed Jun 27, 2020

View reviewed changes

pkgs/development/tools/rust/rustup/default.nix Show resolved Hide resolved

Flakebi force-pushed the cargo-options branch from 77e4d87 to 226b405 Compare June 29, 2020 19:38

ofborg bot requested a review from Mic92 June 29, 2020 19:46

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 501-1000 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 and removed 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 labels Jun 29, 2020

zowoq mentioned this pull request Jul 1, 2020

ripgrep: PCRE2 is not available in this build of ripgrep #91928

Closed

veprbl linked an issue Jul 1, 2020 that may be closed by this pull request

ripgrep: PCRE2 is not available in this build of ripgrep #91928

Closed

zowoq changed the base branch from master to staging-next July 2, 2020 22:53

zowoq approved these changes Jul 2, 2020

View reviewed changes

zowoq merged commit deb7815 into NixOS:staging-next Jul 2, 2020

8573 reviewed Jul 3, 2020

View reviewed changes

oxalica reviewed Jul 14, 2020

View reviewed changes

oxalica mentioned this pull request Jul 14, 2020

buildRustPackage: broken installCheckPhase and ignore custom installCheckPhase #93119

Closed

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Jul 14, 2020

Revert "buildRustPackage: fix cargoBuildFlags"

7713fba

This reverts commit deb7815. Mixing up two distinct phases of a derivation's build is not a good idea. See also NixOS#91689 (comment).

Ma27 mentioned this pull request Jul 14, 2020

rust: Fix build flags #93128

Merged

10 tasks

Flakebi deleted the cargo-options branch July 14, 2020 19:52

symphorien mentioned this pull request Jul 20, 2020

RLS hangs during install #93499

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buildRustPackage: fix cargoBuildFlags #91689

buildRustPackage: fix cargoBuildFlags #91689

Flakebi commented Jun 27, 2020

B4dM4n commented Jun 28, 2020

Mic92 commented Jun 28, 2020 •

edited

Flakebi commented Jun 29, 2020

Hoverbear commented Jun 29, 2020

zowoq commented Jul 2, 2020 •

edited

8573 Jul 3, 2020

zowoq Jul 3, 2020

flokli Jul 13, 2020

oxalica Jul 14, 2020

Ma27 Jul 14, 2020

Flakebi Jul 14, 2020

Ma27 commented Jul 13, 2020

oxalica Jul 14, 2020

buildRustPackage: fix cargoBuildFlags #91689

buildRustPackage: fix cargoBuildFlags #91689

Conversation

Flakebi commented Jun 27, 2020

Motivation for this change

Things done

B4dM4n commented Jun 28, 2020

Mic92 commented Jun 28, 2020 • edited

Flakebi commented Jun 29, 2020

Hoverbear commented Jun 29, 2020

zowoq commented Jul 2, 2020 • edited

8573 Jul 3, 2020

Choose a reason for hiding this comment

zowoq Jul 3, 2020

Choose a reason for hiding this comment

flokli Jul 13, 2020

Choose a reason for hiding this comment

oxalica Jul 14, 2020

Choose a reason for hiding this comment

Ma27 Jul 14, 2020

Choose a reason for hiding this comment

Flakebi Jul 14, 2020

Choose a reason for hiding this comment

Ma27 commented Jul 13, 2020

oxalica Jul 14, 2020

Choose a reason for hiding this comment

Mic92 commented Jun 28, 2020 •

edited

zowoq commented Jul 2, 2020 •

edited