-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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.
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
fetchcargo: use flat tar.gz file for vendored src instead of recursive hash dir #78501
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
# Updated fetchCargo behavior | ||
|
||
Changes to the `fetchcargo.nix` behavior that cause changes to the `cargoSha256` | ||
are somewhat disruptive, so historically we've added conditionals to provide | ||
backwards compatibility. We've now accumulated enough of these that it makes | ||
sense to do a clean sweep updating hashes, and delete the conditionals in the | ||
fetcher to simplify maintenance and implementation complexity. These | ||
conditionals are: | ||
|
||
1. When cargo vendors dependencies, it generates a config. Previously, we were | ||
hard-coding our own config, but this fails if there are git dependencies. We | ||
have conditional logic to sometimes copy the vendored cargo config in, and | ||
sometimes not. | ||
|
||
2. When a user updates the src package, they may forget to update the | ||
`cargoSha256`. We have an opt-in conditional flag to add the `Cargo.lock` | ||
into the vendor dir for inspection and compare at build-time, but it defaults | ||
to false. | ||
|
||
3. We were previously vendoring into a directory with a recursive hash, but | ||
would like to vendor into a compressed tar.gz file instead, for the reasons | ||
specified in the git commit message adding this feature. | ||
|
||
|
||
## Migration plan | ||
|
||
1. (DONE in this PR) Implement `fetchCargoTarball` as a separate, clean fetcher | ||
implementation along-side `fetchcargo`. Rename `verifyCargoDeps` (default | ||
false) to `legacyCargoFetcher` (default true), which switches the fetcher | ||
implementation used. Replace `verifyCargoDeps = true;` with | ||
`legacyCargoFetcher = false;` in Rust applications. | ||
|
||
2. Send a treewide Rust PR that sets `legacyCargoFetcher = true;` in all Rust | ||
applications not using this (which is ~200 of them), with a note to | ||
maintainers to delete if updating the package. Change the default in | ||
`buildRustPackage` to false. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the actual implementation is relatively straightforward and the advantages are clear, but I could foresee this causing git merge conflict hell for maintainers. As implemented here and discussed I think this provides the easiest/clearest way for maintainers to resolve cherry-pick conflicts on stable backports, but if anyone has any better guidance LMK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A script that accomplishes this is attached at the bottom. I have run it on this implementation and verified that it does not cause any rebuilds or change any hashes. Since it touches 200+ files and causes no rebuilds, my current plan is to let the implementation filter its way through staging -> staging-next -> master, then send the treewide PR to master to reduce merge conflict burden on the maintainers, as mentioned above. At that point, all of the Rust applications will be explicitly opting into the legacy Cargo fetcher, with a comment to maintainers to delete the attr line on the next update. #!/usr/bin/env zsh
set -euo pipefail
cd ~/src/nixpkgs || exit 1
sed -i 's|^, legacyCargoFetcher.*|, legacyCargoFetcher ? false|' pkgs/build-support/rust/default.nix
for f in $(rg -l 'cargoSha256 = "' **/*.nix); do
if rg -q 'legacyCargoFetcher = false;' $f; then
continue
fi
sed -i '/cargoSha256 = "/i # Please delete this line on next update\n legacyCargoFetcher = true;\n' $f
done |
||
|
||
3. Go through all Rust src packages deleting the `legacyCargoFetcher = false;` | ||
line and re-computing the `cargoSha256`, merging as we go. | ||
|
||
4. Delete the `fetchcargo.nix` implementation entirely and also remove: | ||
- All overrides in application-level packages | ||
- The `fetchcargo-default-config.toml` and conditionals around using it when | ||
no `$CARGO_CONFIG` exists | ||
- This README.md file |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
{ stdenv, cacert, git, rust, cargo, rustc, fetchcargo, buildPackages, windows }: | ||
{ stdenv, cacert, git, rust, cargo, rustc, fetchcargo, fetchCargoTarball, buildPackages, windows }: | ||
|
||
{ name ? "${args.pname}-${args.version}" | ||
, cargoSha256 ? "unset" | ||
|
@@ -14,34 +14,41 @@ | |
, cargoUpdateHook ? "" | ||
, cargoDepsHook ? "" | ||
, cargoBuildFlags ? [] | ||
, # Set to true to verify if the cargo dependencies are up to date. | ||
# This will change the value of cargoSha256. | ||
verifyCargoDeps ? false | ||
# Please set to true on any Rust package updates. Once all packages set this | ||
# to true, we will delete and make it the default. For details, see the Rust | ||
# section on the manual and ./README.md. | ||
, legacyCargoFetcher ? true | ||
, buildType ? "release" | ||
, meta ? {} | ||
, target ? null | ||
|
||
, cargoVendorDir ? null | ||
, ... } @ args: | ||
|
||
assert cargoVendorDir == null -> cargoSha256 != "unset"; | ||
assert buildType == "release" || buildType == "debug"; | ||
|
||
let | ||
|
||
cargoFetcher = if legacyCargoFetcher | ||
then fetchcargo | ||
else fetchCargoTarball; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we're making some light changes in the |
||
|
||
cargoDeps = if cargoVendorDir == null | ||
then fetchcargo { | ||
then cargoFetcher { | ||
inherit name src srcs sourceRoot unpackPhase cargoUpdateHook; | ||
copyLockfile = verifyCargoDeps; | ||
patches = cargoPatches; | ||
sha256 = cargoSha256; | ||
} | ||
else null; | ||
|
||
# If we're using the modern fetcher that always preserves the original Cargo.lock | ||
# and have vendored deps, check them against the src attr for consistency. | ||
validateCargoDeps = cargoSha256 != "unset" && !legacyCargoFetcher; | ||
|
||
setupVendorDir = if cargoVendorDir == null | ||
then '' | ||
unpackFile "$cargoDeps" | ||
cargoDepsCopy=$(stripHash $(basename $cargoDeps)) | ||
chmod -R +w "$cargoDepsCopy" | ||
cargoDepsCopy=$(stripHash $cargoDeps) | ||
'' | ||
else '' | ||
cargoDepsCopy="$sourceRoot/${cargoVendorDir}" | ||
|
@@ -54,9 +61,14 @@ let | |
ccForHost="${stdenv.cc}/bin/${stdenv.cc.targetPrefix}cc"; | ||
cxxForHost="${stdenv.cc}/bin/${stdenv.cc.targetPrefix}c++"; | ||
releaseDir = "target/${rustTarget}/${buildType}"; | ||
|
||
# Fetcher implementation choice should not be part of the hash in final | ||
# derivation; only the cargoSha256 input matters. | ||
filteredArgs = builtins.removeAttrs args [ "legacyCargoFetcher" ]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also added this, which ensures that the default value of |
||
|
||
in | ||
|
||
stdenv.mkDerivation (args // { | ||
stdenv.mkDerivation (filteredArgs // { | ||
inherit cargoDeps; | ||
|
||
patchRegistryDeps = ./patch-registry-deps; | ||
|
@@ -95,14 +107,13 @@ stdenv.mkDerivation (args // { | |
''} | ||
EOF | ||
|
||
unset cargoDepsCopy | ||
export RUST_LOG=${logLevel} | ||
'' + stdenv.lib.optionalString verifyCargoDeps '' | ||
if ! diff source/Cargo.lock $cargoDeps/Cargo.lock ; then | ||
'' + stdenv.lib.optionalString validateCargoDeps '' | ||
if ! diff source/Cargo.lock $cargoDepsCopy/Cargo.lock ; then | ||
echo | ||
echo "ERROR: cargoSha256 is out of date" | ||
echo | ||
echo "Cargo.lock is not the same in $cargoDeps" | ||
echo "Cargo.lock is not the same in $cargoDepsCopy" | ||
echo | ||
echo "To fix the issue:" | ||
echo '1. Use "1111111111111111111111111111111111111111111111111111" as the cargoSha256 value' | ||
|
@@ -112,6 +123,8 @@ stdenv.mkDerivation (args // { | |
|
||
exit 1 | ||
fi | ||
'' + '' | ||
unset cargoDepsCopy | ||
'' + (args.postUnpack or ""); | ||
|
||
configurePhase = args.configurePhase or '' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
{ stdenv, cacert, git, cargo, python3 }: | ||
let cargo-vendor-normalise = stdenv.mkDerivation { | ||
name = "cargo-vendor-normalise"; | ||
src = ./cargo-vendor-normalise.py; | ||
nativeBuildInputs = [ python3.pkgs.wrapPython ]; | ||
dontUnpack = true; | ||
installPhase = "install -D $src $out/bin/cargo-vendor-normalise"; | ||
pythonPath = [ python3.pkgs.toml ]; | ||
postFixup = "wrapPythonPrograms"; | ||
doInstallCheck = true; | ||
installCheckPhase = '' | ||
# check that ./fetchcargo-default-config.toml is a fix point | ||
reference=${./fetchcargo-default-config.toml} | ||
< $reference $out/bin/cargo-vendor-normalise > test; | ||
cmp test $reference | ||
''; | ||
preferLocalBuild = true; | ||
}; | ||
in | ||
{ name ? "cargo-deps" | ||
, src ? null | ||
, srcs ? [] | ||
, patches ? [] | ||
, sourceRoot | ||
, sha256 | ||
, cargoUpdateHook ? "" | ||
, ... | ||
} @ args: | ||
stdenv.mkDerivation ({ | ||
name = "${name}-vendor.tar.gz"; | ||
nativeBuildInputs = [ cacert git cargo-vendor-normalise cargo ]; | ||
|
||
phases = "unpackPhase patchPhase buildPhase installPhase"; | ||
|
||
buildPhase = '' | ||
# Ensure deterministic Cargo vendor builds | ||
export SOURCE_DATE_EPOCH=1 | ||
|
||
if [[ ! -f Cargo.lock ]]; then | ||
echo | ||
echo "ERROR: The Cargo.lock file doesn't exist" | ||
echo | ||
echo "Cargo.lock is needed to make sure that cargoSha256 doesn't change" | ||
echo "when the registry is updated." | ||
echo | ||
|
||
exit 1 | ||
fi | ||
|
||
# Keep the original around for copyLockfile | ||
cp Cargo.lock Cargo.lock.orig | ||
|
||
export CARGO_HOME=$(mktemp -d cargo-home.XXX) | ||
CARGO_CONFIG=$(mktemp cargo-config.XXXX) | ||
|
||
${cargoUpdateHook} | ||
|
||
cargo vendor $name | cargo-vendor-normalise > $CARGO_CONFIG | ||
|
||
# Add the Cargo.lock to allow hash invalidation | ||
cp Cargo.lock.orig $name/Cargo.lock | ||
|
||
# Packages with git dependencies generate non-default cargo configs, so | ||
# always install it rather than trying to write a standard default template. | ||
install -D $CARGO_CONFIG $name/.cargo/config; | ||
''; | ||
|
||
# Build a reproducible tar, per instructions at https://reproducible-builds.org/docs/archives/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've run this through a bunch of times and verified that it is indeed reproducible. |
||
installPhase = '' | ||
tar --owner=0 --group=0 --numeric-owner --format=gnu \ | ||
--sort=name --mtime="@$SOURCE_DATE_EPOCH" \ | ||
-czf $out $name | ||
''; | ||
|
||
outputHashAlgo = "sha256"; | ||
outputHash = sha256; | ||
|
||
impureEnvVars = stdenv.lib.fetchers.proxyImpureEnvVars; | ||
} // (builtins.removeAttrs args [ | ||
"name" "sha256" "cargoUpdateHook" | ||
])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the hash on every package that has
verifyCargoDeps = true;
in this PR, and verified that other rust packages with this set to false are not impacted, e.g., changing 1 character on the hash forripgrep
and re-building it produces an error that shows the old hash.I have a script to do the rest of them and set it to true everywhere, but figured I'd open the PR first for feedback about whether others agree this is a good idea in general first. If so, once it's merged I can send the treewide follow-up PR that changes all the hashes and sets the attribute to true, then once that's merged I'll send another that removes the boolean.