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 building in debug mode #63666

Closed
wants to merge 6 commits into from
Closed

rust: fix building in debug mode #63666

wants to merge 6 commits into from

Conversation

arcnmx
Copy link
Member

@arcnmx arcnmx commented Jun 22, 2019 •
edited

Motivation for this change

#56540 broke a few things regarding rust builds and did not have the intended effect:

  • bdd3c3f intended to support debug builds, but passes an invalid --debug flag to cargo build
  • 60761e6 intended to make sense of cargo target directories, but breaks cargo.
    1. it deletes $CARGO_TARGET_DIR/debug, and symlinking/moving $CARGO_TARGET_DIR/x86_64-unknown-linux-gnu/debug to it, assuming they're redundant?
    2. this is an incorrect change, as cargo seems to produce and use both directories for different purposes? Each directory has its own lock file, which cargo attempts to use. When both lock files are symlinked to be the same thing, cargo will deadlock itself.
    3. this prevents cargo test from working, the only reason it happens to work now is because this step mangles the release directory, while the default checkPhase builds tests in debug mode. Changing buildType to debug or using cargo test --release instead will expose the deadlock.
    4. buildRustPackage provides releaseDir for this, anything that hard-codes target/release are messy hm...
      • messageformat=json stuff could maybe be used to find binary paths for packaging purposes instead? This is a a rather annoying and long-standing limitation of cargo.

cc @illegalprime @Ericson2314

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 nix-review --run "nix-review wip"
    • will try this soon, it might take a while..?
  • 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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested review from dywedir, Mic92 and marsam June 22, 2019 19:59
@Mic92
Copy link
Member

Mic92 commented Jun 23, 2019 •
edited

Could you also mention $releaseDir in doc/languages-frameworks/rust.section.md?
Maybe we should also mention this in the NixOS changelog for out-of-tree rust packages because people need to upgrade.

@arcnmx
Copy link
Member Author

arcnmx commented Jun 23, 2019

Could you also mention $releaseDir in doc/languages-frameworks/rust.section.md?

Seems appropriate yeah.

Maybe we should also mention this in the NixOS changelog for out-of-tree rust packages because people need to upgrade.

Hmm... If you want backward-compatibility we could set CARGO_TARGET_DIR so cargo doesn't build in $sourceRoot/target by default and then just ln -s $releaseDir target/$buildType for derivations that rely on that path?

@FRidh
Copy link
Member

FRidh commented Jul 17, 2019

Status?

@arcnmx
Copy link
Member Author

arcnmx commented Jul 17, 2019

Oh, was looking for feedback on whether CARGO_TARGET_DIR was a better idea.

To itemize the concerns I'd like feedback on more specifically before proceeding:

  • CARGO_TARGET_DIR is currently unset (and thus roughly defaults to $sourceRoot/target). Is setting it acceptable? This seems less likely to break existing derivations than moving $releaseDir.
  • Re: Documenting/using $releaseDir as the way to find build outputs rather than hardcoding target/release - any sense in bikeshedding this into a less vague name while we're at it? There's nothing cargo/rusty about it and the build may not even be release mode!
  • symlink $releaseDir to $sourceRoot/target/release to preserve compatibility with existing derivations seems like a good idea y/n?
  • Should CARGO_TARGET_DIR and $releaseDir be relative to cwd (presumed to be $sourceRoot) or an absolute path? I'm not too familiar with stdenv conventions regarding this.

@AtkinsChang AtkinsChang mentioned this pull request Jul 25, 2019
Closed
10 tasks
@Mic92
Copy link
Member

Mic92 commented Jul 26, 2019

CARGO_TARGET_DIR sounds better then relying on releaseDir which is nixpkgs specific.

@arcnmx
rust: work around target/release expectations
020b061 
Out-of-tree derivations may rely on copying binaries from target/release
as part of a custom installPhase, so move CARGO_TARGET_DIR and put a
symlink in place just in case.
@arcnmx
buildRustPackage: cc-rs env vars
ecc1a14 
Global cross variables are available for all phases of a build and are
nicer for nix-shell workflows.
@arcnmx
Copy link
Member Author

arcnmx commented Jul 28, 2019

CARGO_TARGET_DIR sounds better then relying on releaseDir which is nixpkgs specific

So the thing is we'll presumably need something nixpkgs specific anyway, since the point of $releaseDir is to point to where to get outputs; $CARGO_TARGET_DIR on the other hand only points to the root under which there's still $CARGO_TARGET_DIR/${hostPlatform.config}/$buildType (also hostPlatform.config is subtly incorrect on many platforms) so... some variations:

  1. Set up CARGO_TARGET_DIR to be absolute and provide releaseDir as relative to it: $CARGO_TARGET_DIR/$releaseDir.
  2. Set up CARGO_TARGET_DIR to be absolute but provide releaseDir as relative to pwd/$sourceRoot, so unless you've chdir'd you can just refer to $releaseDir
  3. Set up both as absolute paths.

(I'm not going to suggest relative CARGO_TARGET_DIR because cargo is inconsistent and setting it to anything disables its usual heuristics regarding where to put the path; "target" is not the same as it being unset, and things just get messy if derivations chdir...)

I'm still pretty indecisive about the details but just pushed the following:

  • CARGO_TARGET_DIR is set via .cargo/config because it seems like a more noncommittal way to go right now, though I don't really have any problem setting it to an absolute path and exposing it to derivations.
  • releaseDir is relative to pwd (expected to roughly be sourceRoot)
  • target/release is symlinked to $releaseDir, though all uses of it should be removed from nixpkgs so this is just for backwards compatibility
  • set up the cargo and cc-rs env vars for the whole derivation, if we're going to cause rust rebuilds may as well fix the cross compiling environment up a bit.

@arcnmx arcnmx requested a review from LnL7 as a code owner July 28, 2019 07:15
@ofborg ofborg bot requested a review from wizeman July 28, 2019 07:31
@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@SuperSandro2000 SuperSandro2000 marked this pull request as draft November 28, 2020 02:02
@stale
Copy link

stale bot commented Jun 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2021
@arcnmx
Copy link
Member Author

arcnmx commented Feb 12, 2022

Enough has changed in the rust build helpers that this is mostly irrelevant now, closing.

@arcnmx arcnmx closed this Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dywedir dywedir Awaiting requested review from dywedir

@Mic92 Mic92 Awaiting requested review from Mic92

@marsam marsam Awaiting requested review from marsam

@LnL7 LnL7 Awaiting requested review from LnL7

@wizeman wizeman Awaiting requested review from wizeman

@zowoq zowoq Awaiting requested review from zowoq zowoq will be requested when the pull request is marked ready for review zowoq is a code owner

@winterqt winterqt Awaiting requested review from winterqt winterqt will be requested when the pull request is marked ready for review winterqt is a code owner

@figsoda figsoda Awaiting requested review from figsoda figsoda will be requested when the pull request is marked ready for review figsoda is a code owner

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: work-in-progress 6.topic: rust 8.has: documentation 10.rebuild-darwin: 101-500 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
4 participants
@arcnmx @Mic92 @FRidh @ryantm