cargo: fix cross #196333
cargo: fix cross #196333
Conversation
alyssais
added
6.topic: rust
6.topic: cross-compilation
ofborg
bot
added
10.rebuild-darwin-stdenv
ofborg
bot
requested review from
AndersonTorres,
globin,
cstrahan,
zaninime,
retrry and
madjar
| # but it does support checking these idiosyncratic PKG_CONFIG_${TRIPLE} | ||
| # environment variables. | ||
| # [1]: https://github.com/rust-lang/pkg-config-rs/issues/53 | ||
| export PKG_CONFIG_@rustTarget@=@targetPrefix@@baseBinName@ |
There was a problem hiding this comment.
I can't really approve of this rust-specific thing being added in all derivations that use pkg-config, even non-rust ones. I also don't disapprove, but for a quick merge you could move this change out of this PR and let me review only the cargo stuff (with that PKG_CONFIG_ variable being set just like in rustc).
There was a problem hiding this comment.
What if it was in the cargo setuphook? Than as soon as you add rustc to a build, you would get it.
There was a problem hiding this comment.
I think the pkg-config wrapper is the right place for this. It's the only place that makes sense that is shared between the rustc build, and all the builds that use Cargo.
Technically, it doesn't have anything to do with pkg-config (since it comes from the pkg-config crate), but it doesn't have anything to do with Cargo either. It's also extremely similar to PKG_CONFIG_FOR_BUILD, defined on the line above, so it makes sense to set both very similar environment variables in one place.
There was a problem hiding this comment.
I agree it doesn't go here.
We can have a separate setup hook that Rust. buildRustCrate, buildRustPackage, etc. all use.
There was a problem hiding this comment.
rust-lang/pkg-config-rs#139 Also, I opened an issue in the relevant package build.rs file use, so we can nip this in the bud.
There was a problem hiding this comment.
It's a small amount of code, but changing it is mass rebuild. So I think about the "mass rebuild" costs of these sorts of things too.
The rebuild could be indirect from changing the toRustTarget stuff too.
There was a problem hiding this comment.
Also think how if we do two setup hooks it's easy to combine later, but if we do one setup hook it's hard to split later.
There was a problem hiding this comment.
That would be an extremely rare change to make, especially for the limited set of architectures we actually build for. We do full rebuilds every couple of weeks anyway, so I don't think it's at all worth optimising for avoidance of mass-rebuilds in infrastructural stuff like this. Indeed, avoiding mass rebuilds is how we end up with unnecessary conditionals making maintenance more complicated forever, because it avoided mass rebuilds once. And Rust is so widely used that any change to it is going to cause huge rebuilds anyway, regardless of whether it affects pkg-config.
There was a problem hiding this comment.
And having a separate setup hook just for this (cargoPkgConfigSetupHook?) would mean broken cross packages forever, because since it would only be required for cross, nobody would ever remember to apply it.
There was a problem hiding this comment.
If we did this, would we want to put this same logic sort of in bintools-wrapper and cc-wrapper too?
There was a problem hiding this comment.
The first time I looked at this, it was not obvious that PKG_CONFIG_FOR_BUILD is set directly above the newly added code, and that it is autoconf/meson specific.
I now think this might be fine, but would give @Ericson2314 the last word (to change their mind or explain why this is still a bad idea)
|
@Ericson2314 and @alyssais, what would be a good way to move this forward? I do kinda agree that it feels wrong to modify the pkg-config wrapper, if a hook were to be added, could that be added to buildRustPackage or injected when cargo or rust is added? Then there might be less of a need to remember to add it. |
I asked John to expand on his most recent comment on Matrix, and AIUI we're still waiting for him to find the time to talk it through with me. |
By moving this from the rustc derivation, the necessary environment variables will also be set for other Rust packages that need them, like cargo.
It's not possible to run installCheckPhase when cross-compiling, so we have to tack this on to postInstall instead.
This fixes cross-compilation.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/unable-to-build-pkgsstatic-gcc/27795/6 |
alyssais
added
the
2.status: merge conflict
wegank
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
wegank
added
the
2.status: stale
Description of changes
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes