Skip to content
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

Rust Application Migration to fetchCargoTarball Implementation #79975

Closed
bhipple opened this issue Feb 13, 2020 · 26 comments · Fixed by #82901
Closed

Rust Application Migration to fetchCargoTarball Implementation #79975

bhipple opened this issue Feb 13, 2020 · 26 comments · Fixed by #82901

Comments

@bhipple
Copy link
Contributor

bhipple commented Feb 13, 2020

What

We are making general infrastructure improvements to the cargo fetcher used to
produce the vendor package in Rust applications. This should not materially
change behavior, but will change the cargoSha256, so this is being rolled out
carefully to avoid disruption and merge conflicts.

Motivation

Currently fetchcargo will pull all of the cargo vendor deps into a
attr.cargoDeps package with a recursive sha256 hash. We are migrating to a new
implementation that will compress the cargoDeps into a tar.gz archive that gets
unpacked at the start of the build. This has been implemented and merged in
#78501

The new implementation has several advantages:

  1. It takes up significantly less space on disk in-between builds in the nix store.

  2. It plays nicely with hashed mirrors like tarballs.nixos.org, which only
    substitute --flat hashes on single files (not recursive directory hashes).
    This is important for internal reproducibility and air-gapped builders.

  3. It's significantly faster with distributed builders, because in that scenario
    the actual full src package is copied back and forth (unlike with a binary cache,
    where it's compressed). This can be very slow.

  4. It's consistent with how simple fetchurl src derivations work.

  5. It provides a stronger abstraction between input src-package and output
    package, e.g., it's harder to accidentally depend on the src derivation at
    runtime by referencing something like ${src}/etc/index.html. Moreover, in the
    store it's harder to get confused with something that is just there as a
    build-time dependency directory vs. an actual runtime package directory,
    since the build-time src dependencies are flat tarballs.

  6. It ensures that if a patchPhase makes modifications to the src, the
    build/install must reference the patched sources in the build directory,
    rather than the original input src package. This prevents issues like fzf: fix patch for vim plugin; enable tests; avoid direct $src layout dependency #79575

Additional Improvements

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 users update 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, which catches
    this mistake; but it defaults to false.

We will bring both of these features in as defaults in the new
fetchCargoTarball implementation.

Migration Plan and Current Status

  1. (DONE in fetchcargo: use flat tar.gz file for vendored src instead of recursive hash dir #78501) 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. (DONE in treewide: change fetchCargoTarball default to opt-out #79978) 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.

  3. (ABANDONED) Backport both (1) and (2) to 20.03 so that other Rust backports can be done without issue. Note: per discussion below, we're not going to do this, and instead will just have maintainers re-compute the cargoSha256 when backporting Rust applications to 20.03. This will give it time to incubate and then sweep for 20.09.

  4. (DONE) Go through all Rust src packages deleting the legacyCargoFetcher = false;
    line and re-computing the cargoSha256, merging as we go.

  5. (DONE in rust: remove legacy cargo fetcher #82901) Delete the fetchcargo.nix implementation entirely and all conditionals around it. Update the manual accordingly.

bhipple added a commit to bhipple/nixpkgs that referenced this issue Feb 13, 2020
The readme was nice to discuss in the implementation PR, but now that this is
merged it's better to have an issue that can be linked against in PRs and
doesn't require further merges to update status.

Ported with a status update in NixOS#79975
bhipple added a commit to bhipple/nixpkgs that referenced this issue Feb 14, 2020
Changes the default fetcher in the Rust Platform to be the newer
`fetchCargoTarball`, and changes every application using the current default to
instead opt out.

This commit does not change any hashes or cause any rebuilds. Once integrated,
we will start deleting the opt-outs and recomputing hashes.

See NixOS#79975 for details.
jonringer pushed a commit that referenced this issue Feb 14, 2020
Changes the default fetcher in the Rust Platform to be the newer
`fetchCargoTarball`, and changes every application using the current default to
instead opt out.

This commit does not change any hashes or cause any rebuilds. Once integrated,
we will start deleting the opt-outs and recomputing hashes.

See #79975 for details.
@jonringer jonringer mentioned this issue Feb 14, 2020
10 tasks
@bhipple
Copy link
Contributor Author

bhipple commented Feb 15, 2020

This introduced a bug in the legacy fetcher, as found by @Ma27 : previously, it had a step where it would explicitly run chmod -R +w on the vendor directory. I spot-checked ~20 out of the ~220 Rust applications and couldn't find one where this line did anything, and it's done by default in the newer fetcher's unpackFile hook, so I removed the line as a cleanup of "dead" code.

Unfortunately, some Rust applications will run a custom build hook in cargo that writes to the vendor directory; these packages will fail without one of the following two fixes:

  1. Upgrade to the new fetcher, which is now the default behavior unless the package opts-out. This is preferred since we want to do this anyways for all packages.

  2. Leave it broken on master and wait for the next staging -> master cycle that includes a systemic fix for the legacy cargo fetcher, at which point it will evaluate again. (See rust: Fix for legacy fetch cargo #80153)

@bhipple bhipple mentioned this issue Feb 15, 2020
10 tasks
bhipple added a commit to bhipple/nixpkgs that referenced this issue Feb 15, 2020
Includes some bugfixes/cleanups to the scripts and packaging, a run of the
updater, a bump of the version, an upgrade to the newer cargo fetcher in NixOS#79975,
and gets the web assembly portion to compile successfully.

Fixes NixOS#75863
bhipple added a commit to bhipple/nixpkgs that referenced this issue Feb 15, 2020
As mentioned in NixOS#79975, the default on `legacyCargoFetcher` if left unspecified
is now `false`.
bhipple added a commit to bhipple/nixpkgs that referenced this issue Feb 15, 2020
See inline comment and NixOS#79975 for details.
bhipple added a commit to bhipple/nixpkgs that referenced this issue Feb 15, 2020
Resolves NixOS#80117 by using the newer fetcher; see NixOS#79975 for details.

Would also be fixed by NixOS#80153 eventually, but we want to upgrade Rust packages
either way, so might as well start with the broken ones!
bhipple added a commit to bhipple/nixpkgs that referenced this issue Feb 15, 2020
Infra upgrade as part of NixOS#79975; no functional change expected.
jonringer pushed a commit that referenced this issue Feb 15, 2020
Resolves #80117 by using the newer fetcher; see #79975 for details.

Would also be fixed by #80153 eventually, but we want to upgrade Rust packages
either way, so might as well start with the broken ones!
@bhipple
Copy link
Contributor Author

bhipple commented Feb 15, 2020

For anyone who's looking at bumping a package, no need to recompute the cargoSha256 by hand; just use this script:

#!/usr/bin/env zsh
set -euo pipefail

# Helper script for migrating a NixPkgs Rust pkg to the new cargoSha256
# verification. Run from the root of a NixPkgs git checkout.
if [ -z "$1" ]; then
    echo "USAGE: $0 <attribute>"
    echo "EXAMPLE: $0 ripgrep"
    exit 1
fi

ATTR=$1
FNAME=$(EDITOR=ls nix edit -f . $ATTR)

section() {
    echo "********************************************************************************"
    echo "$ATTR: $1"
    echo "********************************************************************************"
}

main() {
    sed -i '/.*Delete this on next update.*/,/^$/d' $FNAME

    section "Nuking cargoSha256 reference for $FNAME, then rebuilding"
    sed -i 's|cargoSha256.*|cargoSha256 = "0000000000000000000000000000000000000000000000000000";|' $FNAME;
    nix-build -A $ATTR 2>&1 | tee /tmp/nix-rust-logfile-$ATTR || true
    actual=$(grep 'got:.*sha256:.*' /tmp/nix-rust-logfile-$ATTR | cut -d':' -f3-)
    echo "Build of $ATTR determined that cargoSha256 should be $actual"
    sed -i "s|cargoSha256.*|cargoSha256 = \"$actual\";|" $FNAME

    section "Rebuilding with updated hash, expecting a pass:"
    nix-build -A $ATTR || exit 1

    section "Finished successfully!"
}

main

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/steps-towards-even-more-pr-automation/5634/11

bhipple added a commit to bhipple/nixpkgs that referenced this issue Mar 14, 2020
The attribute `verifyCargoDeps` is no longer defined in the rustPlatform helper;
it is now the default and always on as part of the improvements in NixOS#79975
alyssais pushed a commit that referenced this issue Mar 14, 2020
The attribute `verifyCargoDeps` is no longer defined in the rustPlatform helper;
it is now the default and always on as part of the improvements in #79975
bhipple added a commit to bhipple/nixpkgs that referenced this issue Mar 15, 2020
Infra upgrade as part of NixOS#79975; no functional change expected.
@bhipple
Copy link
Contributor Author

bhipple commented Mar 17, 2020

There are just three uses of the legacy fetcher left:

If someone could help with reviewing/merging, that'd be great. Once those three are gone, we can proceed with the deletion of the legacy infra and complete this ticket!

bhipple added a commit to bhipple/nixpkgs that referenced this issue Mar 19, 2020
We have now migrated every single Rust package in NixPkgs! This deletes the
legacy fetcher, which is now unused.

Resolves NixOS#79975
jonringer pushed a commit that referenced this issue Mar 19, 2020
We have now migrated every single Rust package in NixPkgs! This deletes the
legacy fetcher, which is now unused.

Resolves #79975
paumr added a commit to paumr/nixpkgs that referenced this issue Apr 25, 2020
The cargo hash differed from the cherry-picked one due to changes to
fetchCargoTarball on the master branch NixOS#79975

On the master this happened here:
eb11fea
1f03a34

This should not effect the actual build result.
stigok pushed a commit to stigok/nixpkgs that referenced this issue Jun 12, 2020
The cargo hash differed from the cherry-picked one due to changes to
fetchCargoTarball on the master branch NixOS#79975

On the master this happened here:
eb11fea
1f03a34

This should not effect the actual build result.
@flokli flokli mentioned this issue Jul 23, 2020
8 tasks
@syberant syberant mentioned this issue Aug 15, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.