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

fetchCargoTarball: backport to 20.03 #79981

Closed

Conversation

@bhipple
Copy link
Contributor

@bhipple bhipple commented Feb 13, 2020

Cherry-picked from PR #78501, minus the README.md migration/design doc, which I'm deleting.

This causes no functional change, but does recompile all Rust applications. It's
necessary to get the new fetcher on the 20.03 branch in order to backport
updated Rust packages in the future.

See #79975 for details.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@Mic92
Mic92 approved these changes Feb 13, 2020
Copy link
Contributor

@Mic92 Mic92 left a comment

Agreed. We should have that.

@mmahut
Copy link
Member

@mmahut mmahut commented Feb 13, 2020

Looks good to me.

@misuzu
misuzu approved these changes Feb 13, 2020
Copy link
Member

@worldofpeace worldofpeace left a comment

Out of my league here as far as knowledge of proceedings, but it seems fine to me.
I'll defer merge to those who can do such in an informed manner.

Copy link
Member

@Ma27 Ma27 left a comment

Before we backport this to a release branch, can you please check out if/why this patch breaks custom build commands for crates in the buildPhase? (Example: https://hydra.nixos.org/build/112753750, already fixed in ce84052 by using the new fetcher).

@bhipple
Copy link
Contributor Author

@bhipple bhipple commented Feb 15, 2020

Ugh, thanks for the catch (and fix on spotifyd). The systemic fix unfortunately requires another rebuild-the-world for all the legacy Rust fetches, so I sent the PR with the fix to staging.

Fortunately, it's the legacy implementation that's broken, and the new one that works, so a "workaround" is to do the upgrade work . . . details here: #79975 (comment)

Is there a way to filer Hydra on master to show all failing Rust packages?

@ofborg ofborg bot requested a review from misuzu Feb 15, 2020
@FRidh
Copy link
Member

@FRidh FRidh commented Feb 16, 2020

Please use cherry-pick -x.

bhipple added 2 commits Jan 12, 2020
…e hash dir

This has several advantages:

1. It takes up less space on disk in-between builds in the nix store.
2. It uses less space in the binary cache for vendor derivation packages.
3. It uses less network traffic downloading from the binary cache.
4. It plays nicely with hashed mirrors like tarballs.nixos.org, which only
   substitute --flat hashes on single files (not recursive directory hashes).
5. It's consistent with how simple `fetchurl` src derivations work.
6. 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`. Likewise, in
   the store it's harder to get confused with something that is just there as a
   build-time dependency vs. a runtime dependency, since the build-time
   src dependencies are tarred up.

Disadvantages are:
1. It takes slightly longer to untar at the start of a build.

As currently implemented, this attaches the compacted vendor.tar.gz feature as a
rider on `verifyCargoDeps`, since both of them are relatively newly implemented
behavior that change the `cargoSha256`.

If this PR is accepted, I will push forward the remaining rust packages with a
series of treewide PRs to update the `cargoSha256`s.

(cherry picked from commit 2115a20)
Cherry-picked from PR #80153

(cherry picked from commit 4f8921b)
@bhipple bhipple force-pushed the bhipple:feature/fetchCargoTarball-backport branch from 4f8921b to faa46f4 Feb 16, 2020
@bhipple
Copy link
Contributor Author

@bhipple bhipple commented Feb 16, 2020

Aha, I was wondering how you got the nice (cherry-picked from ...) metadata; noted for the future!

Reset to origin/staging-20.03, then re-cherry-picked the two commits with -x.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Feb 16, 2020

did you want to cherry-pick the upgrade fixes as part of this?

@bhipple
Copy link
Contributor Author

@bhipple bhipple commented Feb 16, 2020

The second commit includes the bugfix at an infra level, which means the application bugfix PRs are unnecessary to backport. For example, this was red on master, but is now green, after the second commit was merged to master:

#80219
https://hydra.nixos.org/job/nixpkgs/trunk/xidlehook.x86_64-linux

This means we don't need to backport all of the individual application changes, except when there's some real change that's actually needed (like bumping a version, security fix, etc.).

Aside from this PR, the only other change that I'd like to pre-emptively backport is this one, which changes the default cargo fetcher implementation to match master, so that the cargoSha256 values are (out of the box) equivalent between the staging branch and master branch:
#79978

As with all of these, the only purpose of backporting is to make it easier for maintainers to cherry-pick Rust applications, since otherwise they'd have to re-compute the cargoSha256.

@bhipple
Copy link
Contributor Author

@bhipple bhipple commented Feb 25, 2020

Per suggestion, let's wait until #80262 makes its way through staging and over to master, which fixes custom patches for Cargo.lock files.

As it currently stands, the implementation works fine for anything that has a Cargo.lock file out of the gate, but there are ~20 packages in Nix that use custom crate mutations outside of the src attribute. These work fine on the legacy fetcher, but they need #80262 before they can be upgraded cleanly to the newer one.

@bhipple bhipple closed this Feb 25, 2020
@bhipple bhipple added this to Done in Staging (stable) via automation Feb 25, 2020
@drewrisinger
Copy link
Contributor

@drewrisinger drewrisinger commented Jul 21, 2020

@bhipple, any plans to renew this backport? I have a package that I'd like to be able to build on both release-20.03 and nixpkgs-unstable (for my nur-packages repo), but I understand from this and #79975 that isn't possible. However, it should be possible if this is merged, correct?

@bhipple
Copy link
Contributor Author

@bhipple bhipple commented Jul 23, 2020

With 20.09 not that far away I'd say no. This is a no-op functionally to the end-user, but it's a rebuild-the-rust-world and a ton of sha changes in the release branch, as well as an API change mid-stable release for those consuming the nix exprs directly.

I'd do something like this as a work around:

cargoSha256 = if <condition> then X else Y;

where <condition> is something like switching on system.stateVersion == 20.03.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jul 23, 2020

Rather than system.stateVersion (which is a NixOS option and therefore rather hard to actually use in NUR) I'd recommend to use lib.trivial.release instead. Apart from that I fully agree with @bhipple.

@drewrisinger
Copy link
Contributor

@drewrisinger drewrisinger commented Jul 23, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.