buildRustPackage: factor out cargoCheckHook and cargoInstallHook#113193
buildRustPackage: factor out cargoCheckHook and cargoInstallHook#113193Mic92 merged 6 commits intoNixOS:stagingfrom
Conversation
ofborg
bot
added
6.topic: rust
ofborg
bot
added
10.rebuild-darwin: 501+
wasmer will always be a special children eh package. Thank you for your effort! |
There was a problem hiding this comment.
Should we add that to the changelog?
There was a problem hiding this comment.
I agree. There was another API change (removal of the target argument of buildRustPackage). I can do a PR for the release notes after this one that summarizes all the changes.
API change: `cargoParallelTestThreads` suggests that this attribute sets the number of threads used during tests, while it is actually a boolean option (use 1 thread or NIX_BUILD_CORES threads). In the hook, this is replaced by a more canonical name `dontUseCargoParallelTests`.
danieldk
force-pushed
the
cargo-install-hook
branch
from
3aac9c8 to
8cc697d
Compare
preBuild is now run before changing to buildAndTestSubdir, so use full path to tests/client.rs in preBuild.
danieldk
force-pushed
the
cargo-install-hook
branch
from
8cc697d to
2376921
Compare
|
I am doing one last test building firefox and some rust stuff. |
| threads=1 | ||
| fi | ||
|
|
||
| argstr="--${cargoBuildType} --target @rustTargetPlatformSpec@ --frozen"; |
There was a problem hiding this comment.
Seems like you changed:
${lib.optionalString (checkType == "release") "--release"}->--${cargoBuildType}
But there are crates out there that on checkType, like cargo-deb. There may be others too.
There was a problem hiding this comment.
This is an oversight. We should pass checkType as cargoCheckType and then use that in the hook.
There was a problem hiding this comment.
I don't know how many packages are out there using that, but I added this to work around issues with packages where cargo test only works with debug, so this shouldn't be dropped entirely IMHO.
There was a problem hiding this comment.
I agree, it's an oversight. I'll do a PR that fixes this.
The `checkType` attribute does not apply after NixOS#113193, and since we're changing tests anyway, let's run them in the release mode to avoid the need for `checkType`. This fixes the otherwise broken build.
7 participants
Motivation for this change
Converts the last parts of
buildRustPackageto hooks. There is two APIs changes:cargoParallelTestThreadssuggests that this attribute sets the number of threads used during tests, while it is actually a boolean option (use 1 thread or NIX_BUILD_CORES threads). In the hook, this is replaced by a more canonical namedontUseCargoParallelTests.preBuild/postBuildare run outsidepushd/popdinbuildPhase, to make it possible to hookify the binary/library collection.Wrt. (2) I went over all derivations that use
buildRustPackageandpreBuild/postBuild. I could only find a problematicpreBuildinwasmer, which is fixed in this PR.I also checked
cargoBuildFlagsin all derivations that use them and fixed feature quotations indiesel-cliandwasmer.Related issues:
Follow-up to:
CC @FRidh @Mic92
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)