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

Jump to bottom

buildRustPackage: add cargoHash for SRI hashes of vendored deps #103118

Merged
merged 2 commits into from Dec 31, 2020
Merged

buildRustPackage: add cargoHash for SRI hashes of vendored deps #103118

merged 2 commits into from Dec 31, 2020

Conversation

danieldk
Copy link
Contributor

@danieldk danieldk commented Nov 8, 2020

Motivation for this change

buildRustCrate currently accepts cargoSha256 as a hash for vendored
dependencies. This change adds cargoHash which accepts SRI hashes,
setting outputHashAlgo to null.

The hash mismatch message still uses cargoSha256 as an example,
which it probably should until we completely switch to SRI hashes.

This PR also changes the broot derivation to use cargoHash as an
example.

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.

@danieldk danieldk requested a review from bhipple November 8, 2020 08:55
bhipple
bhipple reviewed Nov 8, 2020
View reviewed changes
pkgs/build-support/rust/default.nix
, cargoHash ? ""

# Legacy hash
, cargoSha256 ? ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a simple migration to a new var name / style without any real functional difference? Would be worth updating the section of the manual with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that SRI hashes are already used in nixpkgs but I am not sure if we endorse them yet. I am also not sure whether with stable Nix will actually generate them for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to work:

❯ nix --version
nix (Nix) 2.3.8
❯ nix-build -A broot
these derivations will be built:
  /nix/store/w86388xb4h1250wzyyzli61ji7gh6y8x-broot-1.0.4-vendor.tar.gz.drv
  /nix/store/zswggbvzvi858czcqr7cvblzk84zk58k-broot-1.0.4.drv
building '/nix/store/w86388xb4h1250wzyyzli61ji7gh6y8x-broot-1.0.4-vendor.tar.gz.drv'...
error: --- Error --- nix-daemon
hash mismatch in fixed-output derivation '/nix/store/w86388xb4h1250wzyyzli61ji7gh6y8x-broot-1.0.4-vendor.tar.gz.drv':
  specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
     got:    sha256-ikx2jlSg+kzhZce3C2+7sKgMokJ24dmHpw8d+eSIe7E=
error: error: --- Error --- nix-daemon
1 dependencies of derivation '/nix/store/zswggbvzvi858czcqr7cvblzk84zk58k-broot-1.0.4.drv' failed to build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bhipple any objections against merging this? The hash attribute of fetchurl is not documented yet either, but this change makes things consistent with fetchurl. (It's kinda weird to place SRI hashes in cargoSha256 as well ;) .)

Copy link
Member

Choose a reason for hiding this comment

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

Need documentation update.

Copy link
Member

Choose a reason for hiding this comment

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

I know that SRI hashes are already used in nixpkgs but I am not sure if we endorse them yet. I am also not sure whether with stable Nix will actually generate them for you.

It won't generate it but you can convert it and it will understand it.

@danieldk
Copy link
Contributor Author

I propose that we just add cargoHash as a supported attribute first and update the documentation when SRI hashes become endorsed as the default for nixpkgs.

@andir any opinion on this?

@andir
Copy link
Member

andir commented Nov 12, 2020

The commit message is mixing buildRustCrate and buildRustPackage. I am not sure what the relation is. I usually do only care about buildRustCrate as I am not really happy with our FOD abuse.

@danieldk danieldk force-pushed the build-rust-package-sri branch 2 times, most recently from 5d7733a to dcbe434 Compare November 13, 2020 10:33
@danieldk
Copy link
Contributor Author

The commit message is mixing buildRustCrate and buildRustPackage.

Whoops, fixed!

Mic92
Mic92 reviewed Dec 3, 2020
View reviewed changes
pkgs/build-support/rust/default.nix
patches = cargoPatches;
sha256 = cargoSha256;
} // depsExtraArgs)
else null;

# If we have a cargoSha256 fixed-output derivation, validate it at build time
# against the src fixed-output derivation to check consistency.
validateCargoDeps = cargoSha256 != "unset";
validateCargoDeps = !(cargoHash == "" && cargoSha256 == "");
Copy link
Member

@Mic92 Mic92 Dec 3, 2020 •
edited

Choose a reason for hiding this comment

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

Is there any problem with keeping the value unset here for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess not, but I wanted to use the same conventions as fetchurl in fetchCargoTarball.

Mic92
Mic92 reviewed Dec 3, 2020
View reviewed changes
pkgs/build-support/rust/default.nix
, cargoSha256 ? "unset"

# SRI hash
, cargoHash ? ""
Copy link
Member

Choose a reason for hiding this comment

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

This flag needs to be documented in doc/languages-frameworks/rust.section.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a commit to add documentation for cargoHash.

@danieldk danieldk requested a review from Mic92 December 9, 2020 19:30
@danieldk
buildRustPackage: add cargoHash for SRI hashes of vendored deps
67a10c8 
`buildRustPackage` currently accepts `cargoSha256` as a hash for
vendored dependencies. This change adds `cargoHash` which accepts SRI
hashes, setting `outputHashAlgo` to `null`.

The hash mismatch message still uses `cargoSha256` as an example,
which it probably should until we completely switch to SRI hashes.
@danieldk
docs/rust: describe cargoHash
b6728fa
@danieldk
Copy link
Contributor Author

@Mic92 does this look good to you with the added documentation?

@Mic92 Mic92 merged commit 675d754 into NixOS:master Dec 31, 2020
@Mic92
Copy link
Member

Mic92 commented Dec 31, 2020

Thanks!

@danieldk danieldk mentioned this pull request Jan 2, 2021
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhipple bhipple bhipple left review comments

@andir andir Awaiting requested review from andir

@zowoq zowoq Awaiting requested review from zowoq zowoq is a code owner

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees
No one assigned
Labels
6.topic: rust 8.has: documentation 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@danieldk @andir @Mic92 @bhipple