-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
perlPackages: Switch to SRI hashes, add hash
support to bootstrap fetchurl, bump minimal nix version
#187884
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
5ba7ae6
to
e5758f2
Compare
57f23b5
to
d7b3439
Compare
hash
support to bootstrap fetchurl, bump minimal nix version
@@ -94,6 +94,8 @@ Available as [services.patroni](options.html#opt-services.patroni.enable). | |||
|
|||
## Backward Incompatibilities {#sec-release-22.11-incompatibilities} | |||
|
|||
- nixpkgs now requires nix 2.3 or newer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- nixpkgs now requires nix 2.3 or newer. | |
- Nixpkgs now requires Nix 2.3 or newer. |
Yes I'm being anal about capitalisation :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't know that Nixpkgs and Nix are to be capitalized. Learned something along the way ;)
Nix 2.3 is already several years old so bumping that should be fine IMO. How are the hashes for the Perl packages obtained? There is a RFC about the usage of SRI hashes in Nixpkgs NixOS/rfcs#131. There was a discussion recently about the usage of those hashes in the Python packages set. On PyPI they are shown in a different base. Using SRI hashes means you cannot directly compare hashes and this was considered a disadvantage. What is your view on that in case of Perl packages? |
@FRidh there are no hashes shown on metacpan as far as I can tell, although they can be obtained using the API. Also, the Perl package set has not been updated in a long time, but the tool that @stigtsp is devloping to do that in the future uses SRI hashes as well. Edit: Sorry @stig, I pinged the wrong stig :( |
inherit system sha256 name; | ||
inherit system hash sha256 name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/NixOS/nix/blob/master/src/libexpr/fetchurl.nix is not checking if both are set. We should assert that here.
8142077
to
a583734
Compare
, name ? baseNameOf (toString url) | ||
}: | ||
|
||
# assert only one hash is set | ||
assert hash != "" -> sha256 == ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for allowing fetchurl
without any hashes? I figure that to preserve the old behaviour, the code should also assert that at least one hash is set (and use null
as default value, to distinguish from an intentionally empty value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last time I tried ofborg failed evaluation because of this, although I'm not able to reproduce this locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I was too tired yesterday and forgot how boolean logic works :/ Pushed the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the other fetchurl has hash ? ""
, so I should drop my comment about null
! All good for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it's not possible to set the hash to "" and TOFU
It works in fetchurl/default
nixpkgs/pkgs/build-support/fetchurl/default.nix
Lines 123 to 134 in e04579e
hash_ = | |
# Many other combinations don't make sense, but this is the most common one: | |
if hash != "" && sha256 != "" then throw "multiple hashes passed to fetchurl" else | |
if hash != "" then { outputHashAlgo = null; outputHash = hash; } | |
else if md5 != "" then throw "fetchurl does not support md5 anymore, please use sha256 or sha512" | |
else if (outputHash != "" && outputHashAlgo != "") then { inherit outputHashAlgo outputHash; } | |
else if sha512 != "" then { outputHashAlgo = "sha512"; outputHash = sha512; } | |
else if sha256 != "" then { outputHashAlgo = "sha256"; outputHash = sha256; } | |
else if sha1 != "" then { outputHashAlgo = "sha1"; outputHash = sha1; } | |
else if cacert != null then { outputHashAlgo = "sha256"; outputHash = ""; } | |
else throw "fetchurl requires a hash for fixed-output derivation: ${lib.concatStringsSep ", " urls_}"; |
I agree that the format of the hashes probably don't matter that much, since they decode to the same bytes.
Since some issues have been highlighted recently with verifying CHECKSUMS files generated by CPAN/PAUSE, using the MetaCPAN API seems to be the best source of these hashes. Thanks again @dasJ for working on |
, name ? baseNameOf (toString url) | ||
}: | ||
|
||
# assert only one hash is set | ||
assert hash != "" -> sha256 == ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the other fetchurl has hash ? ""
, so I should drop my comment about null
! All good for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Huge) diff LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preConfigure in perlPackages.DBFile
was inadvertently modified (can't comment inline since the diff is too huge)
@@ -6826,13 +6826,13 @@ let
src = fetchurl {
url = "mirror://cpan/authors/id/P/PM/PMQS/DB_File-1.855.tar.gz";
- sha256 = "0q599h7g4jkzks5dxf1zifx9k7l9vif26r2dlgkzxkg6bfif5zyr";
+ hash = "sha256-2f/iolvmzf7no01kI1zciZ6Zuos/uN6Knn9K8g5MqWA=";
};
preConfigure = ''
cat > config.in <<EOF
PREFIX = size_t
- HASH = u_int32_t
+ hash = u_int32_t
LIB = ${pkgs.db.out}/lib
INCLUDE = ${pkgs.db.dev}/include
EOF
The future is now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The future LGTM!
Don't worry about it! It's not the first time someone does that 😄 |
The future is now!
Description of changes
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes