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

python3Packages.cryptography: Update Cargo hash #119640

Merged
merged 1 commit into from Apr 30, 2021

Conversation

centromere
Copy link
Member

Motivation for this change

The present hash seems incorrect. Some tell me this is because Rust was recently upgraded to 1.51.

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.

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

I can confirm the hash change but I also don't know why. Two suggestions:

  1. The commit message should use the attribute name (e.g. python3Packages.cryptography instead of python-cryptography - see previous history)
  2. Optional: Would it be possible to add that email address to your GitHub account (not required but would be useful in general - then GH can link commits to you)

@centromere centromere changed the title python-cryptography: Update Cargo hash python3Packages.cryptography: Update Cargo hash Apr 16, 2021
@ofborg ofborg bot requested a review from primeos April 16, 2021 19:13
Copy link
Member

@SuperSandro2000 SuperSandro2000 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 we need to target master for this to fix cryptography.

I think we need to target master here instead of staging because the breakage is important.

@centromere
Copy link
Member Author

I think we need to target master for this to fix cryptography.

I do not understand. Aren't I targeting master already?

Screen Shot 2021-04-16 at 15 38 57

@FRidh FRidh requested a review from danieldk April 16, 2021 19:57
@SuperSandro2000
Copy link
Member

I do not understand. Aren't I targeting master already?

I updated my comment. I meant we cannot target staging here.

@@ -33,7 +33,7 @@ buildPythonPackage rec {
inherit src;
sourceRoot = "${pname}-${version}/${cargoRoot}";
name = "${pname}-${version}";
sha256 = "1wisxmq26b8ml144m2458sgcbk8jpv419j01qmffsrfy50x9i1yw";
sha256 = "1m6smky4nahwlp4hn6yzibrcxlbsw4nx162dsq48vlw8h1lgjl62";
Copy link
Contributor

@danieldk danieldk Apr 17, 2021

Choose a reason for hiding this comment

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

This should not happen on a Rust update. We should figure out the source of this change, rather than just updating hashes without understanding. I will have a look.

Copy link
Contributor

@danieldk danieldk Apr 17, 2021

Choose a reason for hiding this comment

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

This is really odd. I can still build the cargo dependencies successfully with the old hash:

❯ nix-build -A python3Packages.cryptography.cargoDeps --check
[...]
/nix/store/fri4y3fiyadjvsyxmwn15f69rbcssm51-cryptography-3.4.7-vendor.tar.gz

However, stubbing a fake hash gives the new hash (1m6smky5...). The tarballs and unpacked tar files differ. However, the contents of the unpacked tarballs are identical, as well as ls -lR output (so, doesn't seem to be any visible metadata change).

Copy link
Contributor

@danieldk danieldk Apr 17, 2021

Choose a reason for hiding this comment

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

Found it, it's permissions in the tarball:

diff -u  <(tar -ztvf 1nb0hfxj4cx19nmn2cj148d9sm0bgzwy-cryptography-3.4.7-vendor.tar.gz) <(tar ztvf fri4y3fiyadjvsyxmwn15f69rbcssm51-cryptography-3.4.7-vendor.tar.gz)      
--- /proc/self/fd/11	2021-04-17 09:01:17.537248897 +0200
+++ /proc/self/fd/12	2021-04-17 09:01:17.537248897 +0200
@@ -41,11 +41,11 @@
 -rw-r--r-- 0/0            6894 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/README.md
 drwxr-xr-x 0/0               0 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/
 -rw-r--r-- 0/0             173 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/args.rs
--rw-r--r-- 0/0            6493 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/derive.rs
+-rw-rw-r-- 0/0            6493 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/derive.rs
 -rw-r--r-- 0/0           10899 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/lib.rs
 -rw-r--r-- 0/0             992 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/parse.rs
--rw-r--r-- 0/0            1357 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/variance.rs
--rw-r--r-- 0/0            1954 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/visibility.rs
+-rw-rw-r-- 0/0            1357 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/variance.rs
+-rw-rw-r-- 0/0            1954 1970-01-01 01:00 cryptography-3.4.7-vendor.tar.gz/ghost/src/visibility.rs

This should have broken a lot of other hashes 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.

I've converted this to a draft for in the meanwhile

Copy link
Contributor

@danieldk danieldk Apr 17, 2021

Choose a reason for hiding this comment

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

I think this may be caused by: rust-lang/cargo#9131

It seems that this was fixed in 1.51:

rust-lang/cargo@75d5d8c...rust-1.51.0

So, at the very least, we should update cargoSha256s treewide. However, I wonder if we should take this opportunity to normalize permissions to avoid that this happens in the future?

Copy link
Contributor

@danieldk danieldk Apr 17, 2021

Choose a reason for hiding this comment

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

@centromere I am still wondering why building fails for you. Even though the hash is now invalid due to the cargo vendoring changes, it's a fixed-output derivation, so the vendor tarball with the old hash should still be retrieved from the binary cache. This happens on my machine if when build the vendor tarball with the old hash:

❯ nix-build -A python3Packages.cryptography.cargoDeps
this path will be fetched (8.64 MiB download, 8.83 MiB unpacked):
  /nix/store/fri4y3fiyadjvsyxmwn15f69rbcssm51-cryptography-3.4.7-vendor.tar.gz
copying path '/nix/store/fri4y3fiyadjvsyxmwn15f69rbcssm51-cryptography-3.4.7-vendor.tar.gz' from 'https://cache.nixos.org'...
/nix/store/fri4y3fiyadjvsyxmwn15f69rbcssm51-cryptography-3.4.7-vendor.tar.gz

Copy link
Member Author

Choose a reason for hiding this comment

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

I disabled the binary cache and was building all dependencies of an application from source.

@FRidh FRidh marked this pull request as draft April 17, 2021 07:05
@centromere
Copy link
Member Author

@danieldk What are the next steps for this?

@danieldk
Copy link
Contributor

danieldk commented Apr 29, 2021

@danieldk What are the next steps for this?

Sorry, I was completely sidetracked by teaching. I think the best solution would be to merge this PR to solve the issue for this derivation and create an issue that points the thread here to document the underlying problem. Someone should do a treewide check of cargoSha256/cargoHash to see if other derivations are affected. I currently only have access to my laptop, so I cannot easily do a tree-wide check/update.

@SuperSandro2000
Copy link
Member

Someone should do a treewide check of cargoSha256/cargoHash to see if other derivations are affected.

This is already a quite common issue when packaging new rust programs that people get different hashes from ofborg due to using an older rust version.

@SuperSandro2000 SuperSandro2000 marked this pull request as ready for review April 29, 2021 22:25
@SuperSandro2000 SuperSandro2000 merged commit 16093da into NixOS:master Apr 30, 2021
@centromere centromere deleted the python-cryptography branch April 30, 2021 08:52
@primeos
Copy link
Member

primeos commented Apr 30, 2021

and create an issue that points the thread here to document the underlying problem

@danieldk thanks! I've created a small issue so that this won't get lost: #121259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants