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

fetchgit: add SRI hash support #79987

Draft
wants to merge 3 commits into
base: master
from
Draft

fetchgit: add SRI hash support #79987

wants to merge 3 commits into from

Conversation

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Feb 13, 2020

Motivation for this change

Sync fetchgit's hash support with fetchurl's to add support for SRI hashes (introduced in 267c8d6).

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.
, rev ? "HEAD"

, # SRI hash.
hash ? ""

This comment has been minimized.

@edolstra

edolstra Feb 13, 2020
Member

This should be called narHash for consistency with builtins.fetchX (NixOS/nix@d4df99a).

This comment has been minimized.

@jtojnar

jtojnar Feb 13, 2020
Author Contributor

Should I rename it in 267c8d6 as well?

This comment has been minimized.

@edolstra

edolstra Feb 13, 2020
Member

Yes, probably. We could also rename everything to hash, but that's ambiguous in functions that take both a file and a NAR hash...

This comment has been minimized.

@jtojnar

jtojnar Feb 13, 2020
Author Contributor

Does it make sense to call it narHash if it can also be used for outputHashMode = "flat"? If I understand the docs correctly, hash of the NAR is only used for FOD when using recursive output hash mode.

This comment has been minimized.

@jtojnar

jtojnar Feb 13, 2020
Author Contributor

functions that take both a file and a NAR hash

I am not sure what you mean.

This comment has been minimized.

@jtojnar

jtojnar Feb 13, 2020
Author Contributor

Alternately, we could use outputHash directly, as that is what gets passed to Nix in the end.

This comment has been minimized.

@jtojnar

jtojnar Jun 1, 2020
Author Contributor

Since we are using SRI hashes, using integrity attribute name like in HTML might actually fit even better.

jtojnar added 2 commits Feb 13, 2020
Including SRI hash support
@jtojnar jtojnar force-pushed the fetchgit-sri branch from a63aed3 to d1d0961 Feb 14, 2020
to sync it with fetchgit
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Mar 2, 2020

Until the attribute name issue is clarified, I cherry-picked the fetchpatch issue into #81564 since it is very annoying.

@jtojnar jtojnar marked this pull request as draft Jun 1, 2020
@worldofpeace worldofpeace mentioned this pull request Aug 17, 2020
5 of 10 tasks complete
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Aug 17, 2020

cc @edolstra

I think it's really important we decide what the attribute for #79987 (comment) is going to be. There's already some stigma on base64 encoding and sri in nixpkgs #89423 #89423 and this is awfully difficult, for example, to explain to people that they can't use hash in fetchFromGitHub correctly because if they set fetchSubmodules = true the fetcher will be fetchgit which doesn't support hash yet...

@edolstra
Copy link
Member

@edolstra edolstra commented Aug 17, 2020

narHash is probably best. hash can be misunderstood as a commit hash, and integrity doesn't say whether it's a NAR hash or a flat file hash.

@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Aug 17, 2020

@edolstra What will we use for flat files then?

And why do we need to distinguish between NAR hash and flat file hash by the checksum attribute? It could just be hidden inside the fetch functions and switched through outputHashMode as before.

@edolstra
Copy link
Member

@edolstra edolstra commented Aug 17, 2020

Flat file hashes are used in many places, e.g. fetchurl.

@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Aug 17, 2020

@edolstra I meant what attribute would we use for flat file hashes if we want to distinguish them from NAR hashes. fechurl currently uses sha256 for both and used a separate recursiveHash attribute as a switch between the two outputHashModes. So it does not say whether it's a NAR hash or a flat file hash any more that integrity would.

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.