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

rustPlatform.fetchCargoTarball: support pname+version #332975

Merged
merged 5 commits into from
Sep 7, 2024

Conversation

eclairevoyant
Copy link
Contributor

@eclairevoyant eclairevoyant commented Aug 7, 2024

Description of changes

  • rustPlatform.fetchCargoTarball: nixfmt
  • rustPlatform.fetchCargoTarball: support pname, version
  • doc/rust: prefer pname+version over name in fetchCargoTarball
  • mousai: change name -> pname+version in cargoDeps
    • simply done as a proof-of-concept, should result in 0 rebuilds
  • treewide: remove existing usages of pname+version in fetchCargoTarball, that incorrectly assumed it was supported (prevents hash mismatches)

Ths should result in exactly one rebuild (the manual).

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@eclairevoyant eclairevoyant marked this pull request as draft August 7, 2024 11:35
@eclairevoyant eclairevoyant changed the title rustPlatform.fetchCargoTarball: support pname+version [WIP] rustPlatform.fetchCargoTarball: support pname+version Aug 7, 2024
@eclairevoyant
Copy link
Contributor Author

eclairevoyant commented Aug 10, 2024

Funnily enough, it seem like there's several packages that already assumed that fetchCargoTarball could accept pname+version, resulting in hash mismatches after this PR. Therefore, to preserve this as a zero-rebuild PR, I'd have to not-pass those attrs in those cases.

Someone actually almost noticed this in python312Packages.datafusion but didn't seem to understand why.

@eclairevoyant
Copy link
Contributor Author

Okay, this now has one rebuild (the nixpkgs manual) which makes sense and I can live with.

Result of nixpkgs-review run on x86_64-linux 1

1 package built:
  • nixpkgs-manual

@eclairevoyant eclairevoyant marked this pull request as ready for review August 10, 2024 02:10
@eclairevoyant eclairevoyant changed the title [WIP] rustPlatform.fetchCargoTarball: support pname+version rustPlatform.fetchCargoTarball: support pname+version Aug 10, 2024
@eclairevoyant
Copy link
Contributor Author

Thanks for agreeing borgo

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

LGTM in principle, but I think I’d prefer updating the hashes in e878dfe145e5f71d5a5817f5244ea8002865ccc5 to dropping what is now meant to be good practice.

@eclairevoyant
Copy link
Contributor Author

eclairevoyant commented Aug 10, 2024

It's certainly possible to follow up with a treewide in another PR, but I didn't want to balloon the scope here too much, or make it harder to review or merge without conflicts. I'm sure there are many packages that don't set name at all, that we'd want to fix anyway.

@eclairevoyant eclairevoyant added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 10, 2024
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4496

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I think this looks good to merge. Barring any objections, I intend to do so in a day.

pkgs/build-support/rust/fetch-cargo-tarball/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/rust/fetch-cargo-tarball/default.nix Outdated Show resolved Hide resolved

inherit (hash_) outputHashAlgo outputHash;

impureEnvVars = lib.fetchers.proxyImpureEnvVars ++ [ "NIX_CRATES_INDEX" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Future: document this environment variable somewhere. Maybe in the Rust section of the nixpkgs-manual?

Comment on lines +93 to +111
cat >$CARGO_HOME/config.toml <<EOF
[source.crates-io]
replace-with = 'mirror'
[source.mirror]
registry = "$NIX_CRATES_INDEX"
EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs one more level of indent.

Copy link
Contributor Author

@eclairevoyant eclairevoyant Sep 5, 2024

Choose a reason for hiding this comment

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

The indentation error is present in the base branch. , but sure, I can fix it

Actually if I indent this then the contents of the toml file will be indented.
The only line that's indentable is L93 but that might look even more confusing 🫤

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, load-bearing odd indentation. Thanks Bash.

Comment on lines +76 to +96
echo
echo "ERROR: The Cargo.lock file doesn't exist"
echo
echo "Cargo.lock is needed to make sure that cargoHash/cargoSha256 doesn't change"
echo "when the registry is updated."
echo

exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

In Nix files, I do like my shell to have two-space indent. .editorconfig says four, but @emilazy and I might connive to effect a two-space-coup on shell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you hide whitespace (https://github.com/NixOS/nixpkgs/pull/332975/files?diff=unified&w=1) you'll see I didn't substantively touch these lines at all. These lines are indented in the diff, but only by the same amount that the surroundings are indented (and was done by nixfmt, not manually).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes, this is just me noticing formatting due to, well, the formatting being impacted with nixfmt.

@@ -26,7 +26,7 @@ stdenv.mkDerivation (finalAttrs: {
};

cargoDeps = rustPlatform.fetchCargoTarball {
inherit (finalAttrs) pname version src;
inherit (finalAttrs) src;
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the PR headmatter, this is removed in order to not cause hash mismatches.

Comment on lines +93 to +111
cat >$CARGO_HOME/config.toml <<EOF
[source.crates-io]
replace-with = 'mirror'
[source.mirror]
registry = "$NIX_CRATES_INDEX"
EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, load-bearing odd indentation. Thanks Bash.

This is done because the existing fetchCargoTarball does not support pname or version, resulting in a vendor tarball
with the name cargo-deps-vendor.tar.gz. Since adding pname+version support would change the name of the derivation,
and therefore its hash, we remove existing usages to avoid treewide breakage.
@philiptaron philiptaron merged commit 29cca09 into NixOS:master Sep 7, 2024
24 of 27 checks passed
@eclairevoyant eclairevoyant deleted the fetchcargotarball-pname branch September 7, 2024 14:34
@TomaSajt TomaSajt mentioned this pull request Sep 9, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants