Skip to content

fix pkgsMusl.cargo via cargoSetupHook#198311

Merged
yu-re-ka merged 2 commits intoNixOS:stagingfrom
yu-re-ka:musl-cargo
Oct 30, 2022
Merged

fix pkgsMusl.cargo via cargoSetupHook#198311
yu-re-ka merged 2 commits intoNixOS:stagingfrom
yu-re-ka:musl-cargo

Conversation

@yu-re-ka
Copy link
Contributor

@yu-re-ka yu-re-ka commented Oct 28, 2022

Description of changes

This is a variant of #190796, which removes the need for patching rustc by always applying the unstable host-config flag in cargoSetupHook.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@yu-re-ka yu-re-ka requested review from LnL7, Mic92 and zowoq as code owners October 28, 2022 17:06
@github-actions github-actions bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Oct 28, 2022
@ofborg ofborg bot requested a review from retrry October 28, 2022 17:21
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Oct 28, 2022
@yu-re-ka yu-re-ka changed the base branch from master to staging October 28, 2022 17:47
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to specify the triple here, or can we just do [host]?

Copy link
Contributor Author

@yu-re-ka yu-re-ka Oct 28, 2022

Choose a reason for hiding this comment

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

I think both work, do you prefer just [host]?

Copy link
Member

Choose a reason for hiding this comment

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

I'd have a mild preference for the former, just because it means we don't have to worry about what happens if e.g. using a custom build spec. But I don't think it's too important, and would understand if you wanted it to be consistent with what we were already doing for target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Might make sense to rename this, and probably expand the comment a bit to explain what's going on? You could probably just adapt what I wrote in the other PR if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, please let me know if the comment is ok

@symphorien
Copy link
Member

are -Z flags allowed without RUSTC_BOOTSTRAP=1 ?

@yu-re-ka
Copy link
Contributor Author

are -Z flags allowed without RUSTC_BOOTSTRAP=1 ?

The way we set it, in .cargo/config, it works. I tested that fd builds, which does not have RUSTC_BOOTSTRAP=1.

@yu-re-ka yu-re-ka mentioned this pull request Oct 29, 2022
13 tasks
@yu-re-ka yu-re-ka requested a review from alyssais October 30, 2022 08:21

[unstable]
host-config = true
target-applies-to-host = true
Copy link
Member

Choose a reason for hiding this comment

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

I'm very puzzled as to why you set target-applies-to-host to true: the doc says

It requires the -Zhost-config and -Ztarget-applies-to-host command-line options to be set, and that target-applies-to-host = false is set in the Cargo configuration file.

https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#host-config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is two things here:

  • Is the target-applies-to-host unstable feature enabled?
  • If yes, this unlocks a new target-applies-to-host option

Also see, in the same section:

Setting -Zhost-config changes the default for target-applies-to-host to false from true.

Copy link
Member

Choose a reason for hiding this comment

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

ok I understand. Then the change looks sensible to me, and if things build with it then go for it.

Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

I like this better than my approach, and in my testing, all the builds my version fixed are also fixed by this change.

It's still an open question what to do for rustc on Musl when used outside of Cargo, but that's orthogonal to this change. For Cargo this is the way to go.

@yu-re-ka yu-re-ka merged commit 7852c61 into NixOS:staging Oct 30, 2022
@alyssais alyssais mentioned this pull request Oct 30, 2022
13 tasks
@yu-re-ka yu-re-ka deleted the musl-cargo branch October 31, 2022 02:01
@yu-re-ka yu-re-ka mentioned this pull request Oct 31, 2022
13 tasks
@lf-
Copy link
Member

lf- commented Nov 24, 2022

This PR seems to have regressed packages being able to set target.*.rustflags inside a .cargo/config file. I'm investigating sapling being broken on darwin (see #201798), and that setting is not getting through to the compiler. Since the setup hook sets host.rustflags, it seems that any .cargo/config file inside projects will have its target.aarch64-apple-darwin.rustflags setting ignored. This is the resulting config:

[nix-shell:/tmp/sapling/source/eden/scm]$ RUSTC_BOOTSTRAP=1 cargo config -Z unstable-options get --show-origin
-snip-
source.vendored-sources.directory = "cargo-vendor-dir" # /private/tmp/sapling/.cargo/config
target.aarch64-apple-darwin.linker = "/nix/store/v069wkyb6y6qw2bhpzag5kka19j7clgg-clang-wrapper-11.1.0/
bin/cc" # /private/tmp/sapling/.cargo/config
target.aarch64-apple-darwin.rustflags = [
    "-C", # /private/tmp/sapling/source/eden/scm/.cargo/config
    "link-args=-Wl,-undefined,dynamic_lookup", # /private/tmp/sapling/source/eden/scm/.cargo/config
    "-C", # /private/tmp/sapling/.cargo/config
    "target-feature=-crt-static", # /private/tmp/sapling/.cargo/config
]
target.arm64-apple-darwin.rustflags = [
    "-C", # /private/tmp/sapling/source/eden/scm/.cargo/config
    "link-args=-Wl,-undefined,dynamic_lookup", # /private/tmp/sapling/source/eden/scm/.cargo/config
]
target.i686-apple-darwin.rustflags = [
    "-C", # /private/tmp/sapling/source/eden/scm/.cargo/config
    "link-args=-Wl,-undefined,dynamic_lookup", # /private/tmp/sapling/source/eden/scm/.cargo/config
]
target.x86_64-apple-darwin.rustflags = [
    "-C", # /private/tmp/sapling/source/eden/scm/.cargo/config
    "link-args=-Wl,-undefined,dynamic_lookup", # /private/tmp/sapling/source/eden/scm/.cargo/config
]
unstable.host-config = true # /private/tmp/sapling/.cargo/config
unstable.target-applies-to-host = true # /private/tmp/sapling/.cargo/config

@lf-
Copy link
Member

lf- commented Nov 24, 2022

Ah, it's actually specifically the unstable.* options that cause this. Nevertheless, this causes some breakage of project cargo config files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants