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.

Sign up for GitHub

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

Jump to bottom

rust: Fix build flags #93128

Merged
merged 2 commits into from Jul 16, 2020
Merged

rust: Fix build flags #93128

merged 2 commits into from Jul 16, 2020

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Jul 14, 2020

Motivation for this change

⚠️ Caution: I only tested this against a few packages. Before merging this, we should ensure to not cause even further regressions.

This PR fixes a bug originally described in #91191: if a package has a special cargoBuildFlags-declaration, the binaries in $out aren't built with those since the artifacts in target/release get modified during checkPhase which doesn't use cargoBuildFlags.

This change copies the artifacts after buildPhase into a temporary location, so that the proper binaries get installed into $out and cargo test can resume the build-process in the checkPhase.

cc #91689
Closes #93119
Closes #91191

Things done
  • 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.

@Ma27
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 Ma27 requested review from ivan, Mic92, flokli, Flakebi and zowoq July 14, 2020 15:46
@Ma27 Ma27 requested a review from andir as a code owner July 14, 2020 15:46
@flokli flokli removed their request for review July 14, 2020 17:44
Flakebi
Flakebi approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that this fixes the rustup issue.
Some packages I tested with nixpkgs-review, all succeeded: rustup, rls, alacritty, mdbook, ripgrep, rustfmt, sd

cole-h
cole-h approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested with tokei, which also exhibited problems (#91298 (comment)) -- now it shows yaml serialization in tokei --help!

@Ma27
rustPlatform: don't install artifacts modified by checkPhase
d2694d9 
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
@zowoq
Copy link
Contributor

zowoq commented Jul 16, 2020

Sorry this turned into a mess and that I wasn't able to resolve it before it landed in master.

Thanks for fixing it @Ma27.

Hydra is quiet, might be worthwhile merging this to master so we aren't waiting for the next staging cycle?

@Ma27
Copy link
Member Author

Ma27 commented Jul 16, 2020 •
edited

Hydra is quiet, might be worthwhile merging this to master so we aren't waiting for the next staging cycle?

I don't think that it's a good idea to merge something with ~1000 rebuilds into master.

@vcunat would it be possible to merge this into staging-next to speed this up a bit as soon as this PR is approved by more folks?

@flokli
Copy link
Contributor

flokli commented Jul 16, 2020

#91090 has been approved 2 days ago - I assume staging-next is mostly waiting for rebuilds to finish.

@flokli flokli mentioned this pull request Jul 16, 2020
Closed
10 tasks
@zowoq
Copy link
Contributor

zowoq commented Jul 16, 2020

#91090 has been approved 2 days ago - I assume staging-next is mostly waiting for rebuilds to finish.

It's already in master, the PR has been recycled a couple of times. Currently staging-next is empty.

@flokli
Copy link
Contributor

flokli commented Jul 16, 2020

Uh, in that case, If staging-next is empty, and this PR causes a big rebuild, shouldn't we merge it into staging, and then staging into staging-next?

@flokli
Copy link
Contributor

flokli commented Jul 16, 2020

Let's let this cook a bit in staging, and spot breakages there.

@flokli flokli merged commit ba20bc8 into NixOS:staging Jul 16, 2020
@Ma27 Ma27 deleted the fix-rust-build-flags branch July 16, 2020 10:53
@Ma27 Ma27 mentioned this pull request Jul 16, 2020
Closed
10 tasks
@Flakebi Flakebi mentioned this pull request Jul 20, 2020
Closed
@zowoq zowoq mentioned this pull request Jul 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flakebi Flakebi Flakebi approved these changes

@cole-h cole-h cole-h approved these changes

@ivan ivan Awaiting requested review from ivan

@Mic92 Mic92 Awaiting requested review from Mic92

@zowoq zowoq Awaiting requested review from zowoq zowoq is a code owner

@andir andir Awaiting requested review from andir

Assignees
No one assigned
Labels
6.topic: rust 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Ma27 @zowoq @flokli @Flakebi @cole-h