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

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

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.
Ma27
Ma27 approved these changes Jun 1, 2020
zimbatm
zimbatm approved these changes Jun 1, 2020
@vcunat
Copy link
Member

@vcunat 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

FRidh
FRidh approved these changes Jun 1, 2020
@@ -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

@LnL7 LnL7 Jun 1, 2020

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

/cc @adisbladis

srhb
srhb approved these changes Jun 2, 2020
@zimbatm zimbatm merged commit 9f15e95 into NixOS:master Jun 2, 2020
14 checks passed
@LnL7 LnL7 deleted the remove-sri-hashes branch Jun 2, 2020
@Ma27 Ma27 mentioned this pull request Jun 2, 2020
10 tasks
@rycee
Copy link
Member

@rycee 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 Ma27 commented Jun 2, 2020

IMHO yes.

@LnL7
Copy link
Member Author

@LnL7 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
10 tasks
@edolstra
Copy link
Member

@edolstra 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 edolstra commented Jun 4, 2020

@vcunat
Copy link
Member

@vcunat vcunat commented Jun 7, 2020

🤔 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
10 tasks
@bhipple
Copy link
Contributor

@bhipple 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 LnL7 commented Jun 14, 2020

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

@bhipple
Copy link
Contributor

@bhipple 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
10 tasks
@jonringer jonringer mentioned this pull request Aug 24, 2020
10 tasks
@jonringer jonringer mentioned this pull request Sep 18, 2020
9 tasks
Gabriel439 pushed a commit to awakesecurity/nixpkgs that referenced this issue May 26, 2021
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.
Gabriel439 pushed a commit to awakesecurity/nixpkgs that referenced this issue May 26, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment