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

Remove sri hashes #89308

Merged
merged 2 commits into from
Jun 2, 2020
Merged

Remove sri hashes #89308

merged 2 commits into from
Jun 2, 2020
+60 −60

Conversation

LnL7
Copy link
Member

@LnL7 LnL7 commented Jun 1, 2020

Motivation for this change

These break compatibility with nix 2.0, see #89275. Alternative would be to bump minvers.nix, but some of this might also be relevant for 20.03 and 19.09.

Things done

Validated using nix-env -qa --out-path -f. with new hash types disabled.

  • 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.

Sorry, something went wrong.

LnL7 added 2 commits June 1, 2020 15:24
@LnL7
treewide: replace SRI hashes

Verified

This commit was signed with the committer’s verified signature.
renovate-bot Mend Renovate
GPG key ID: 81C63AE1C224182B
Verified
Learn about vigilant mode
b7ddbd5
@LnL7
poetry2nix: translate hashes to sha256
8d705d9
@LnL7 LnL7 force-pushed the remove-sri-hashes branch from 2aad9d5 to 8d705d9 Compare June 1, 2020 13:24
@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 6.topic: python 8.has: documentation This PR adds or changes documentation labels Jun 1, 2020
@ofborg ofborg bot requested review from zimbatm, brainrake and steshaw June 1, 2020 13:32
@vcunat
Copy link
Member

vcunat commented Jun 1, 2020

Lowest release with SRI support is 2.2 (Jan 2019), if I look right, which is not that long. Perhaps minver.nix should contain some hint about the lag we'll maintain here, e.g. it might say that we want at least two years margin.

Another issue is how to maintain the state; my suggestion: NixOS/ofborg#513

LnL7
LnL7 commented Jun 1, 2020
View reviewed changes
pkgs/development/tools/poetry2nix/poetry2nix/lib.nix
@@ -146,7 +146,7 @@ let
else
pkgs.fetchurl {
url = predictURLFromPypi { inherit pname file hash kind; };
inherit hash;
sha256 = builtins.elemAt (builtins.match "sha256:(.*)" hash) 0; # nix 2.0 backwards compatibility.
Copy link
Member Author

Choose a reason for hiding this comment

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

If we go this route, poetry2nix should probably be updated to handle this properly.

/cc @adisbladis

@zimbatm zimbatm merged commit 9f15e95 into NixOS:master Jun 2, 2020
@LnL7 LnL7 deleted the remove-sri-hashes branch June 2, 2020 16:25
@Ma27 Ma27 mentioned this pull request Jun 2, 2020
Merged
10 tasks
@rycee
Copy link
Member

rycee commented Jun 2, 2020

Would it be worth also removing these from the release-20.03 branch as well?

@Ma27
Copy link
Member

Ma27 commented Jun 2, 2020

IMHO yes.

@LnL7
Copy link
Member Author

LnL7 commented Jun 2, 2020

Yeah, but I'm working a followup for base64 hashes first.

@LnL7 LnL7 mentioned this pull request Jun 3, 2020
Merged
10 tasks
@LnL7 LnL7 mentioned this pull request Jun 3, 2020
Closed
@edolstra
Copy link
Member

edolstra commented Jun 4, 2020

@vcunat I disagree. I think in the past our rule was it should be possible to evaluate Nixpkgs using the Nix version from the previous NixOS release. So using SRI hashes is fine.

@edolstra
Copy link
Member

edolstra commented Jun 4, 2020

#89476

@teto teto mentioned this pull request Jun 5, 2020
Merged
10 tasks
@vcunat
Copy link
Member

vcunat commented Jun 7, 2020 •
edited
Loading

🤔 oh, in terms of NixOS release support cycles it is a relatively long time now. Last default nix < 2.2 was in 18.09, which has been unsupported for over a year now.

@bhipple bhipple mentioned this pull request Jun 14, 2020
Merged
10 tasks
@bhipple
Copy link
Contributor

bhipple commented Jun 14, 2020

What's the final takeaway here? Should we allow SRI hashes in master now? Base64? And to clarify, the right format is this, right?

hash = "sha256-rJbpoPjNMlw4diWjwNQ/DPo3rltvISU4kuRqBbvlBZ0=";

@LnL7
Copy link
Member Author

LnL7 commented Jun 14, 2020

Master requires nix 2.2 now so using SRI hashes there is fine.

@bhipple
Copy link
Contributor

bhipple commented Jun 14, 2020

Sounds good. Should we revert your PR to get rid of base64 hashes? I actually quite like them, as aside from being shorter they're also an external standard, unlike our homebrew base32 encoding scheme.

@worldofpeace worldofpeace mentioned this pull request Aug 17, 2020
Merged
10 tasks
Gabriella439 added a commit to awakesecurity/nixpkgs that referenced this pull request May 26, 2021
@Gabriella439
Remove sri hashes
60d0b3f 
The motivation for this change is to support Nix 2.1.3.  In theory
Nixpkgs 20.03 should have supported Nix 2.1.3 (according to
`./lib/minver.nix`, but in practice it did not:

NixOS#89275

However, this was fixed in:

NixOS#89308

… and this change manually backports that to the `awake-20.03` branch.  The
above change could not be cherry-picked due to merge conflicts with frequently
changing hashes.
@Gabriella439 Gabriella439 mentioned this pull request May 26, 2021
Merged
Gabriella439 added a commit to awakesecurity/nixpkgs that referenced this pull request May 26, 2021
@Gabriella439
Remove sri hashes (#12)
bd52f0e 
The motivation for this change is to support Nix 2.1.3.  In theory
Nixpkgs 20.03 should have supported Nix 2.1.3 (according to
`./lib/minver.nix`, but in practice it did not:

NixOS#89275

However, this was fixed in:

NixOS#89308

… and this change manually backports that to the `awake-20.03` branch.  The
above change could not be cherry-picked due to merge conflicts with frequently
changing hashes.
@symphorien symphorien mentioned this pull request Jul 11, 2021
Merged
11 tasks
@Artturin Artturin mentioned this pull request Nov 27, 2022
Closed
13 tasks
Artturin added a commit to Artturin/nixpkgs that referenced this pull request Dec 10, 2022
@Artturin
Bump minver.nix to 2.8
e075e71 
source for the quote is at NixOS#89308 (comment)
edolstra> ... I think in the past our rule was it should be possible to evaluate Nixpkgs using the Nix version from the previous NixOS release...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimbatm zimbatm

@srhb srhb

@FRidh FRidh

@Ma27 Ma27

@cdepillabout cdepillabout

@infinisil infinisil

@jonringer jonringer

@thoughtpolice thoughtpolice

@brainrake brainrake

@steshaw steshaw

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 6.topic: python 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@LnL7 @vcunat @rycee @Ma27 @edolstra @bhipple @zimbatm @srhb @FRidh