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: fix cargoBuildFlags #91689

Merged
merged 1 commit into from Jul 2, 2020
Merged

Conversation

@Flakebi
Copy link
Member

Flakebi commented Jun 27, 2020

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)]

  • nushell fails 40/41 tests, same with and without this patch

  • other errors are build errors (svgbob, sit, polkadot, racerd-unstable, gleam) or failing dependencies (ycmd, vimplugin-YouCompleteMe), which is unchanged with this patch

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)

  • 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 nixpkgs-review --run "nixpkgs-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)

  • Ensured that relevant documentation is up to date

  • Fits CONTRIBUTING.md.

@B4dM4n
Copy link
Contributor

B4dM4n commented Jun 28, 2020

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
Copy link
Contributor

Mic92 commented Jun 28, 2020

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 Flakebi force-pushed the Flakebi:cargo-options branch from 77e4d87 to 226b405 Jun 29, 2020
@Flakebi
Copy link
Member Author

Flakebi commented Jun 29, 2020

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

@Hoverbear
Copy link

Hoverbear commented Jun 29, 2020

Also bumped into this today!

@zowoq
Copy link
Contributor

zowoq commented Jul 2, 2020

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.

@zowoq zowoq changed the base branch from master to staging-next Jul 2, 2020
@zowoq
zowoq approved these changes Jul 2, 2020
@zowoq zowoq merged commit deb7815 into NixOS:staging-next Jul 2, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="226b405"; rev="226b405b3f1e792fb38e79390366b0b3eab9dca0"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="226b405"; rev="226b405b3f1e792fb38e79390366b0b3eab9dca0"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="226b405"; rev="226b405b3f1e792fb38e79390366b0b3eab9dca0"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="226b405"; rev="226b405b3f1e792fb38e79390366b0b3eab9dca0"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="226b405"; rev="226b405b3f1e792fb38e79390366b0b3eab9dca0"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="226b405"; rev="226b405b3f1e792fb38e79390366b0b3eab9dca0"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="226b405"; rev="226b405b3f1e792fb38e79390366b0b3eab9dca0"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@@ -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

This comment has been minimized.

@8573

8573 Jul 3, 2020 Contributor

If checkPhase is changed to installCheckPhase, should doCheck

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.

This comment has been minimized.

@zowoq

zowoq Jul 3, 2020 Contributor

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 comment has been minimized.

@flokli

flokli Jul 13, 2020 Contributor

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

This comment has been minimized.

@oxalica

oxalica Jul 14, 2020 Contributor

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).

This comment has been minimized.

@Ma27

Ma27 Jul 14, 2020 Member

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

This comment has been minimized.

@Flakebi

Flakebi Jul 14, 2020 Author Member

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

@Ma27
Copy link
Member

Ma27 commented Jul 13, 2020

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):
    # 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
@@ -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}"}

This comment has been minimized.

@oxalica

oxalica Jul 14, 2020 Contributor

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

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Jul 14, 2020
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 added a commit to Ma27/nixpkgs that referenced this pull request Jul 14, 2020
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
@Ma27 Ma27 mentioned this pull request Jul 14, 2020
3 of 10 tasks complete
@Flakebi Flakebi deleted the Flakebi:cargo-options branch Jul 14, 2020
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Jul 15, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.