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.

Sign up for GitHub

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: allow patches to fix Cargo.lock #44967

Merged
merged 1 commit into from Aug 13, 2018
Merged

buildRustPackage: allow patches to fix Cargo.lock #44967

merged 1 commit into from Aug 13, 2018

Conversation

Ekleog
Copy link
Member

@Ekleog Ekleog commented Aug 13, 2018

Motivation for this change

Sometime maintainers forget to update their Cargo.lock before pushing a release.

This change makes it possible for the packager to use a patch updating the Cargo.lock.

cc @Mic92 @LnL7

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@Ekleog Ekleog mentioned this pull request Aug 13, 2018
Merged
9 tasks
@Mic92 Mic92 merged commit 3dc78e6 into NixOS:master Aug 13, 2018
@Mic92
Copy link
Member

Mic92 commented Aug 13, 2018

Mhm. Hopefully this did not break any existing checksums for rust packages that used patches.

@Mic92
Copy link
Member

Mic92 commented Aug 13, 2018

Sorry I had to revert this again: #44967. We first need to fix checksums:

$ ag -A400 'buildRustPackage' | grep patches
doc/languages-frameworks/rust.section.md:191-patches the derivation:
doc/languages-frameworks/rust.section.md:256-  `patches`, `postPatch`, `preConfigure` (in the case of a Rust crate,
pkgs/development/tools/rust/racer/default.nix:22-  patches = [
pkgs/development/tools/rust/rustup/default.nix:26-  patches = lib.optionals stdenv.isLinux [
pkgs/development/tools/git-series/default.nix:37-    description = "A tool to help with formatting git patches for review on mailing lists";
pkgs/development/compilers/rust/cargo.nix:13-  inherit version src patches;
pkgs/tools/networking/gnirehtet/default.nix:29-  patches = [
pkgs/tools/misc/parallel-rust/default.nix:16-  patches = [ ./fix_cargo_lock_version.patch ];
pkgs/applications/altcoins/parity/parity.nix:19-  inherit cargoSha256 patches;

It is also a bit tricky, if dynamic patches are used because then the fixed output derivation breaks:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/rust/racer/default.nix#L23

Since this is quite non-obivious should a patch phase for the Cargo.lock file called differently?

Mic92 added a commit that referenced this pull request Aug 13, 2018
@Mic92
Revert "buildRustPackage: allow patches to fix Cargo.lock"
cfff3eb 
This reverts commit b6e881a.

We need to fix checksums for this pull request first.

Also see #44967
@Ekleog Ekleog deleted the rust-patch branch August 13, 2018 12:55
@Ekleog
Copy link
Member Author

Ekleog commented Aug 13, 2018

Oh :/ I had tested on a random Rust package, didn't think of testing on a patch-having Cargo package, sorry! Will open a new PR with a different name for the patch attribute :)

(BTW, it's kind of weird ofborg displayed 0 rebuilds for Darwin and Linux… it looks like it missed racer, among others?)

@Ekleog Ekleog mentioned this pull request Aug 13, 2018
Merged
9 tasks
@Mic92
Copy link
Member

Mic92 commented Aug 13, 2018 •
edited

The rebuilds are zero, because the source packages are fixed input derivations (derivations with a checksum). To check if the checksum of a fixed input derivations stays the same one also has to change it to an invalid one, because nix will not rebuild it otherwise.

@Ekleog
Copy link
Member Author

Ekleog commented Aug 13, 2018

Oh good point! For #44981 I've just checked this with rustracer, it looks like it's OK :) (sorry for the delay, it looks like my internet connection didn't want to download the registry without failing…)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
6.topic: rust 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Ekleog @Mic92 @GrahamcOfBorg