Skip to content

uiua,uiua-unstable: useFetchCargoVendor#376519

Merged
cafkafk merged 1 commit intoNixOS:masterfrom
alyssais:uiua-fetchCargoVendor
Jan 25, 2025
Merged

uiua,uiua-unstable: useFetchCargoVendor#376519
cafkafk merged 1 commit intoNixOS:masterfrom
alyssais:uiua-fetchCargoVendor

Conversation

@alyssais
Copy link
Member

Cargo 1.84.0 seems to have changed the output format of cargo vendor again, once again invalidating fetchCargoTarball FOD hashes. It's time to fix this once and for all, switching across the board to fetchCargoVendor, which is not dependent on cargo vendor's output format.

It is definitely the same hash for both.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Cargo 1.84.0 seems to have changed the output format of cargo vendor
again, once again invalidating fetchCargoTarball FOD hashes.  It's
time to fix this once and for all, switching across the board to
fetchCargoVendor, which is not dependent on cargo vendor's output
format.

It /is/ definitely the same hash for both.
@alyssais alyssais requested a review from emilazy January 24, 2025 21:37
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 24, 2025
@nix-owners nix-owners bot requested review from Defelo, TomaSajt and cafkafk January 24, 2025 21:45
@Defelo
Copy link
Member

Defelo commented Jan 25, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 376519


x86_64-linux

✅ 1 package built:
  • uiua (uiua-unstable)

@Defelo
Copy link
Member

Defelo commented Jan 25, 2025

For some reason fetching the crates using fetchCargoVendor was very slow for me, so building the uiua-0.14.1-vendor-staging derivation took more than eight minutes. Compared to cargo vendor which finished in just a few seconds.

@cafkafk
Copy link
Member

cafkafk commented Jan 25, 2025

For some reason fetching the crates using fetchCargoVendor was very slow for me, so building the uiua-0.14.1-vendor-staging derivation took more than eight minutes. Compared to cargo vendor which finished in just a few seconds.

For me it only took 45s on a ~200mbps wifi connection.

┃       ├─ ✔ uiua-0.14.1-vendor-staging ⏱ 45s
┃    ┌─ ✔ uiua-0.14.1-vendor ⏱ 14s

@cafkafk
Copy link
Member

cafkafk commented Jan 25, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 376519


x86_64-linux

✅ 1 package built:
  • uiua (uiua-unstable)

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems correct for uiua and uiua-unstable.

I worry about @Defelo's report of slow performance, I think it may be worth investigating. Seeing as you've made a bunch of PRs changing this for other packages as well, we'll probably quickly learn if this is a general problem from those other reviews.

@alyssais
Copy link
Member Author

I have also seen useFetchCargoVendor being slower. However, we are under time pressure here. Either we break FODs (violating a Nix invariant and thus having a very long tail of people discovering their FODs are broken far into the future when they happen to build them uncached), or we delay updating Rust. Both of these would be significantly more disruptive than the FOD build being slower. I'd be thrilled if somebody looked into why it was slower, but my focus is on getting the conversion done before the start of the next staging cycle so that we don't have to revert the next update. 2000 packages have already been converted on staging, but there is lots more work still to do to avoid being stuck unable to update Rust indefinitely.

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also seen useFetchCargoVendor being slower. However, we are under time pressure here. Either we break FODs (violating a Nix invariant and thus having a very long tail of people discovering their FODs are broken far into the future when they happen to build them uncached), or we delay updating Rust.

We agree that it would technically be possible to update the hashes as part of the rust 1.84 update right, instead of switching to useFetchCargoVendor, at least technically?

I totally see that doing that would be another 1.80 and may be out of scope for this rust bump (and a lot of work in general every time they break it), and shouldn't be blocking to solve this, but long-term I'd prefer to avoid that cost, and assuming useFetchCargoVender is likely just a bunch of nix that will never really be as fast, it would be nice to have options to avoid this in the future, since it clearly incurs a significant performance cost.

Considering ~2000 packages so far, if the increase is just 45 seconds that's 25 hours added for building all those packages in aggregate if done in series... That's a lot, even if this is a very bad first approximation of the real cost.

Anyways thanks for the clarification, here's a LGTM

@cafkafk cafkafk merged commit 25a14e6 into NixOS:master Jan 25, 2025
23 of 25 checks passed
@alyssais
Copy link
Member Author

We agree that it would technically be possible to update the hashes as part of the rust 1.84 update right, instead of switching to useFetchCargoVendor, at least technically?

It would be possible, but it would have other bad consequences. All out of tree derivations using the old mechanism would all be silently broken in ways that could take months to notice. To avoid this, we need to deprecate it, and to do that, we need to stop using it in Nixpkgs.

I totally see that doing that would be another 1.80 and may be out of scope for this rust bump (and a lot of work in general every time they break it), and shouldn't be blocking to solve this, but long-term I'd prefer to avoid that cost, and assuming useFetchCargoVender is likely just a bunch of nix that will never really be as fast, it would be nice to have options to avoid this in the future, since it clearly incurs a significant performance cost.

Considering ~2000 packages so far, if the increase is just 45 seconds that's 25 hours added for building all those packages in aggregate if done in series... That's a lot, even if this is a very bad first approximation of the real cost.

It's Python code. I don't see why it couldn't be fast. Right now I don't even know that it's doing parallel downloads.

@TomaSajt WDYT?

@alyssais alyssais deleted the uiua-fetchCargoVendor branch January 25, 2025 11:36
@TomaSajt
Copy link
Contributor

It's Python code. I don't see why it couldn't be fast. Right now I don't even know that it's doing parallel downloads.

@TomaSajt WDYT?

There was a PR where I limited the multi-threading to 5 parallel jobs. #356539

IIRC, it wasn't strictly required, since the main issue was with concurrently fetching git dependencies. I just wanted to make sure we didn't go overboard with parallelism.

We could increase it back to the maximum of mp.cpu_count()), though do note that I am NOT very experienced with parallelism in general, so there might be a more robust solution than this.

# run tarball download jobs in parallel, with at most 5 concurrent download jobs
with mp.Pool(min(5, mp.cpu_count())) as pool:
if len(registry_packages) != 0:
(out_dir / "tarballs").mkdir()
tarball_args_gen = ((pkg, out_dir) for pkg in registry_packages)
pool.starmap(download_tarball, tarball_args_gen)

@cafkafk
Copy link
Member

cafkafk commented Jan 25, 2025

We could increase it back to the maximum of mp.cpu_count()), though do note that I am NOT very experienced with parallelism in general, so there might be a more robust solution than this.

I assume the best way to set this is with a max of NIX_BUILD_CORES, and an unbounded minimum. May also be worth investigating the starmap_async function, since I'm assuming this is mostly constrained by net I/O.

For what it's worth, I'm by no means a python dev, and I'm not familiar with the multiprocessing part of the standard library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants