Skip to content

Use fetchCargoVendor for wasm-bindgen-cli#377534

Merged
alyssais merged 3 commits intoNixOS:masterfrom
alyssais:wasm-bindgen-cli-fetchCargoVendor
Jan 30, 2025
Merged

Use fetchCargoVendor for wasm-bindgen-cli#377534
alyssais merged 3 commits intoNixOS:masterfrom
alyssais:wasm-bindgen-cli-fetchCargoVendor

Conversation

@alyssais
Copy link
Member

Exposing an overridable cargoHash parameter is problematic because it
will produce silently broken FODs if we change the hashing scheme,
which we are currently doing across the tree because Cargo 1.84.0 has
changed the output of fetchCargoTarball, meaning all hashes have been
invalidated. To avoid this happening in future, we are changing to
the fetchCargoVendor mechanism, which does not depend on
implementation details of Cargo.

The future-proof way to override a package with Cargo dependencies is
to pass a cargoDeps object. This is more verbose, and requires the
caller to have access to the src object for the package. To
compensate for this, I've introduced a buildWasmBindgenCli function
that is nicer to use than wasm-bindgen-cli.overrideAttrs, and I've
also introduced versioned attributes for wasm-bindgen-cli versions
currently in use in Nixpkgs, so that each package that wants to use a
particular version doesn't have to duplicate the src and cargoDeps
definitions for that version.

This is a bit urgent, because we need the transition to be completed by the end of the current staging-next cycle to avoid exposing broken FODs to users.

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.

@alyssais alyssais requested review from alois31 and rizary January 28, 2025 11:26
@github-actions github-actions bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Jan 28, 2025
@nix-owners nix-owners bot requested review from figsoda, winterqt and zowoq January 28, 2025 11:27
@alyssais alyssais force-pushed the wasm-bindgen-cli-fetchCargoVendor branch from 4f0e2d3 to 6b455c7 Compare January 28, 2025 11:32
@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: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jan 28, 2025
@nix-owners nix-owners bot requested review from bendlas, huantianad and pbsds January 28, 2025 11:33
@alyssais alyssais force-pushed the wasm-bindgen-cli-fetchCargoVendor branch from 6b455c7 to 8d42f8d Compare January 28, 2025 11:54
@nix-owners nix-owners bot requested a review from philiptaron January 28, 2025 11:56
@pbsds
Copy link
Member

pbsds commented Jan 28, 2025

--------- Report for 'x86_64-linux' ---------
12 packages built:
lldap pagefind teleport teleport.client teleport_15 teleport_15.client tetrio-plus tpsecore wasm-bindgen-cli wasm-bindgen-cli_0_2_92 wasm-bindgen-cli_0_2_93 wasm-bindgen-cli_0_2_95

Copy link
Contributor

@alois31 alois31 left a comment

Choose a reason for hiding this comment

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

Code looks good save for two nits. The new FOD hashes verify. I think the API is reasonable: it avoids the correctness issue of the old one, while not creating significantly more effort for users: using versions that are already in nixpkgs actually becomes easier (and there is slightly less duplication within nixpkgs too), bumps are pretty much the same effort as before (change the version and two hashes), and new out-of-tree users are not that bad either (a couple of lines more boilerplate, but with better discoverability).

Copy link
Contributor

@bendlas bendlas left a comment

Choose a reason for hiding this comment

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

lldap still builds and works. approving on behalf of that

@alyssais alyssais force-pushed the wasm-bindgen-cli-fetchCargoVendor branch from 8d42f8d to 30a2a1a Compare January 29, 2025 18:10
Exposing an overridable cargoHash parameter is problematic because it
will produce silently broken FODs if we change the hashing scheme,
which we are currently doing across the tree because Cargo 1.84.0 has
changed the output of fetchCargoTarball, meaning all hashes have been
invalidated.  To avoid this happening in future, we are changing to
the fetchCargoVendor mechanism, which does not depend on
implementation details of Cargo.

The future-proof way to override a package with Cargo dependencies is
to pass a cargoDeps object.  This is more verbose, and requires the
caller to have access to the src object for the package.  To
compensate for this, I've introduced a buildWasmBindgenCli function
that is nicer to use than wasm-bindgen-cli.overrideAttrs, and I've
also introduced versioned attributes for wasm-bindgen-cli versions
currently in use in Nixpkgs, so that each package that wants to use a
particular version doesn't have to duplicate the src and cargoDeps
definitions for that version.

The unversioned "wasm-bindgen-cli" attribute is demoted to an alias,
with a view to its eventual removal.
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.
@alyssais alyssais force-pushed the wasm-bindgen-cli-fetchCargoVendor branch from 30a2a1a to c79f97c Compare January 29, 2025 18:11
@alyssais alyssais merged commit e18b76d into NixOS:master Jan 30, 2025
25 of 27 checks passed
@alyssais alyssais deleted the wasm-bindgen-cli-fetchCargoVendor branch January 30, 2025 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants