rustPlatform.buildRustPackage: miscelaneous modernization to unify unpackPhase and cargoDeps.unpackPhase#435278
Conversation
458a7d5 to
9e47b10
Compare
9e47b10 to
7be5f9c
Compare
|
@ofborg build nnd |
7be5f9c to
8613150
Compare
|
|
@ofborg build cargo
|
|
| # Workaround the existing RUSTFLAGS specified as a list. | ||
| + interpolateString (args.RUSTFLAGS or ""); |
There was a problem hiding this comment.
Is this specification as a list already happening or an extension? The previous code didn't do that, so I suspect that this would be a bug fix, if there were already cases for it?
With this upper-case spelling, I'd argue that this should be treated as an environment variable and thus lists should not be supported anywhere.
If we want a structured way to specify them, this should be come rustFlags?
There was a problem hiding this comment.
Is this specification as a list already happening or an extension?
It is already happening.
Violated packages (via tree-wide evaluation):
- alvr
- asusctl
- cosmic-edit
- mitra
- proxmox-backup-client
The previous code didn't do that
The previous code expect it to be string-concatable via the + operator, which indicates that it should be a string. However, it is only string-concatenated in the lib.optionalAttrs block requiring buildType == debug, which is seldom triggered.
If we want a structured way to specify them, this should be come
rustFlags?
I think so. While implementing #359744, I gradually think that we should prepend goFlags/rustFlags explicitly to the compiler flags instead of relying on the GOFLAGS/RUSTFLAGS environment variables. Let's implement such interface change in a separate PR.
8613150 to
34c2358
Compare
wolfgangwalther
left a comment
There was a problem hiding this comment.
Diff LGTM, didn't test. I don't know enough about the Rust / Cargo ecosystem in Nixpkgs to say too much beyond simply reading the code.
ShamrockLee
left a comment
There was a problem hiding this comment.
We no longer have to consider backporting to Nixpkgs 25.05.
I'll drop some overly conservative backport compatibility code.
34c2358 to
de2a3da
Compare
|
Fails eval in the queue! |
de2a3da to
a6e0d52
Compare
|
Let's rebase and try again. |
a6e0d52 to
7f229fb
Compare
|
I see. I forgot to handle the |
7f229fb to
cdb722f
Compare
|
I got a new failing build and bisected to this PR, more specifically to this diff: diff --git a/pkgs/build-support/rust/build-rust-package/default.nix b/pkgs/build-support/rust/build-rust-package/default.nix
index 577463a0f689..bc2372278283 100644
--- a/pkgs/build-support/rust/build-rust-package/default.nix
+++ b/pkgs/build-support/rust/build-rust-package/default.nix
@@ -33,6 +33,7 @@ lib.extendMkDerivation {
"cargoUpdateHook"
"cargoLock"
"useFetchCargoVendor"
+ "RUSTFLAGS"
];
extendDrvArgs =
@@ -89,6 +90,10 @@ lib.extendMkDerivation {
true;
{
+ env = {
+ RUSTFLAGS = interpolateString (args.RUSTFLAGS or "");
+ };
+
cargoDeps =
if cargoVendorDir != null then
nullThe package sets CARGO_TARGET_X86_64_PC_WINDOWS_GNU_RUSTFLAGS=-L /nix/store/kchvqpqwlnh0442x75bj4y7hrghv9sh9-mingw_w64-pthreads-x86_64-w64-mingw32-13.0.0/lib And after the change it results in: CARGO_TARGET_X86_64_PC_WINDOWS_GNU_RUSTFLAGS=-L /nix/store/kchvqpqwlnh0442x75bj4y7hrghv9sh9-mingw_w64-pthreads-x86_64-w64-mingw32-13.0.0/lib
RUSTFLAGS= This causes the first variable to be ignored and generates this error: |
Caused by NixOS/nixpkgs#435278 RUSTFLAGS is now always set and overrides whatever we specify in .cargo/config.toml TODO: Keep an eye on NixOS/nixpkgs#464707 NixOS/nixpkgs#464992 and possibly revert or adjust to the new status quo in nixpkgs.
RUST_LOG) viaenvcargoDepsHook(a hook for tweaks before Rust vendor fromcargoDepsorcargoVendorDirgets unpacked) insidecargoSetupPostUnpackHookinstead of prepending topostUnpack.postUnpackof the main derivation andcargoDepsin PR rustPlatform.buildRustPackage: fix cargoDeps inherited attribute overriding #435239Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.