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 verifyCargoDeps option #67387

Merged
merged 2 commits into from Oct 1, 2019
Merged

buildRustPackage: add verifyCargoDeps option #67387

merged 2 commits into from Oct 1, 2019

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Aug 24, 2019 •
edited

When enabled, it allows to check whenever the cargoSha256 needs updating
or not. This technique depends on IFD and is not allowed inside of
nixpkgs but it's handy to have available. Eg:

let
  pkg = buildRustPackage { ... };
  srcContent = builtins.readFile "${pkg.src}/Cargo.lock";
  depsContent = builtins.readFile "${pkgs.cargoDeps}/Cargo.lock";
in
  # this will fail when the cargoSha256 gets out of sync
  assert (srcContent == depsContent);
  pkg
Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @

@zimbatm
Copy link
Member Author

zimbatm commented Aug 24, 2019

This PR needs a bit of testing. For now I am just looking for review on the idea.

@ofborg ofborg bot added the 6.topic: rust label Aug 24, 2019
@symphorien
Copy link
Member

Upstream seems to want to change the Cargo.lock format, so there could be spurious hash invalidations.

@zimbatm
Copy link
Member Author

zimbatm commented Aug 25, 2019

Good to know. I don't think this would be a major issue since the Cargo.lock is provided by upstream. So when updating the src hash, the cargoSha256 might also change, which seems reasonable to me.

This feature is mainly meant to be used outside of nixpkgs since comparing the lockfiles requires to use IFD.

@symphorien
Copy link
Member

About IFD, this assert could be checked in the builder of the derivation, right ?

@zimbatm
Copy link
Member Author

zimbatm commented Aug 25, 2019

Right, good thinking! And since the argument is passed to the main builder it will work nicely.

I will add that shortly.

@symphorien
Copy link
Member

symphorien commented Aug 25, 2019 •
edited

This is a rather nice idea, if the format of the lockfile is stable enough (notably across cargo upgrades).

@zimbatm
Copy link
Member Author

zimbatm commented Aug 25, 2019

Yeah it works quite nicely. Here is how it looks like when the cargoSha256 is out of date:

ERROR: cargoSha256 is out of date.

Cargo.lock is not the same in /nix/store/918210cgpvkhkzx5kjgysb4095dhnqlw-mdsh-0.1.4-vendor.

To fix the issue:
1. Use "1111111111111111111111111111111111111111111111111111" as the cargoSha256 value
2. Build the derivation and wait it to fail with a hash mismatch
3. Copy the 'got: sha256:' value back into the cargoSha256 field

builder for '/nix/store/vg7azjks6q3c4nk94y1br8my2h2845gd-mdsh-0.1.4.drv' failed with exit code 1
error: build of '/nix/store/vg7azjks6q3c4nk94y1br8my2h2845gd-mdsh-0.1.4.drv' failed

I am almost tempted to migrate all the packages to use this feature.

Having to update the cargoSha256 a bit more frequently is probably preferable than having them out of sync because the package maintainer forgot to update it.

@zimbatm
Copy link
Member Author

zimbatm commented Sep 27, 2019 

$ rg cargoSha256 | wc -l
179

@zimbatm
Copy link
Member Author

zimbatm commented Sep 27, 2019

Ok this PR is ready

@symphorien
Copy link
Member

Can you document the new flag in ./doc/languages-frameworks/rust.section.md ?

@Mic92 Mic92 changed the title buildRustPackage: add cargoCopyLockfile option buildRustPackage: add verifyCargoDeps option Sep 27, 2019
@Mic92
Copy link
Member

Mic92 commented Sep 27, 2019 •
edited

Can you document the new flag in ./doc/languages-frameworks/rust.section.md ?

There should be also a warning that this can potentially break the build when in cargo changes the lockfile format in future. For many users this won't be a big problem though. In nixpkgs we can coordinate such breakages when updating rustc. It will be only annoying when backporting updates...

Mic92
Mic92 approved these changes Sep 27, 2019
View reviewed changes
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Looks good to me apart from missing documentation.

@zimbatm
buildRustPackage: add verifyCargoDeps option
f8d67ec 
One issue with cargoSha256 is that it's hard to detect when it needs to
be updated or not. It's possible to upgrade a package and forget to
update cargoSha256 and run with old versions of the program or
libraries.

This commit introduces `verifyCargoDeps` which, when enabled, will check
that the Cargo.lock is not out of date in the cargoDeps by comparing it
with the package source.
@zimbatm
mdsh: use the verifyCargoDeps option
ee00d08
@zimbatm
Copy link
Member Author

zimbatm commented Sep 30, 2019

thanks for calling me out on the docs. Added those and did a quick rebase.

@Mic92 Mic92 merged commit 83a7fd2 into master Oct 1, 2019
@Mic92 Mic92 deleted the cargofetch-lock branch October 1, 2019 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 approved these changes

@symphorien symphorien Awaiting requested review from symphorien

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@zimbatm @symphorien @Mic92