fetchurl: disallow specifying both `sha256` and `hash` #182953

vcunat · 2022-07-26T10:48:50Z

A full check would be more complicated to write -
and more importantly - probably also more expensive.

Motivation: eval-time catch for errors like in commit 8198636.

A full check would be more complicated to write - and more importantly - probably also more expensive. Motivation: eval-time catch for errors like in commit 8198636.

vcunat · 2022-07-26T11:15:46Z

So, OfBorg doesn't show removed packages, but if you run the rebuild-amount script in reverse, you can see which packages get thrown by this. It's more than I expected, hundreds just from home-assistant.

I believe this needs community feedback, around disallowing this, using src.override, etc.

vcunat · 2022-07-26T11:17:58Z

Other unresolved packages:

ansible_2_12
ansible
hadoop2
hadoop_3_2
hadoop_3_3
hadoop3
hadoop
nixos-install-tools
oci-cli
pig
powerdns-admin
python310Packages.dask-yarn
python310Packages.skein
python39Packages.dask-yarn
python39Packages.skein
spark_3_1
spark_3_2
spark3
spark
tests.nixos-functions.nixos-test
tests.testers.nixosTest-example
tests.trivial-builders.references

Let's stop using src.override. I see no advantage.

mweinelt · 2022-07-26T11:22:30Z

Completed the changes to home-assistant to get rid of these kinds of overrides.

vcunat · 2022-07-26T11:31:40Z

/cc @SuperSandro2000 as the other author of src.override around what I changed today.

SuperSandro2000 · 2022-07-26T11:56:06Z

pkgs/build-support/fetchurl/default.nix

+    # 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


Suggested change

# 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 != "" && sha1 != "" && sha256 != "" && sha512 != "" then throw "multiple hashes passed to fetchurl" else

might as well. Also what about we drop sha1 like md5?

That's not how you check for existance of more than one hash though. And the proposed solution is at least pareto optimal.

❯ rg "sha256 =" -l | wc -l 19006 ❯ rg "sha512 =" -l | wc -l 155 ❯ rg "sha1 =" -l | wc -l 55

You suggest wrong conditions. (uh, comment race, GitHub didn't update the thread for me)

That will only fail on all of them present, so it would have to be a hash & sha1 || hash & sha256 || hash & sha512 || sha1 & sha256.... or some kind of (count-non-empty-in [hash sha1 sha256 sha512]) > 1

Before outright dropping the support I'd go through some time when it just prints an annoying warning. And I haven't checked the current usage in nixpkgs and tools. Anyway, that's way bigger than this PR and there's an issue already: #77238

piegamesde · 2022-07-27T12:31:59Z

Instead of backing off overriding sources, could we maybe fix the override function so that it produces the correct behavior? For example, setting sha256 in the override also removes hash. Alternatively, it could enforce that the same attribute as in the original is used.

vcunat · 2022-07-27T14:23:36Z

AFAIK it's general .override function. That has no knowledge about inter-dependencies between attributes of the function. EDIT: though I suppose we could redefine the fetcher's .override to do such things, but I don't know...

vcunat · 2022-07-27T14:26:42Z

Also, what if the original derivation switches the fetchers. We commonly use many: fetchurl, fetchFromGitHub, fetchPypi... so after switching your override won't make really sense anymore. Overall I couldn't find motivation why to use .src.override, but perhaps you do so that would be nice to hear in this thread.

mweinelt · 2022-07-27T14:34:26Z

Alternatively we could converge on a single hash field, part of what is proposed in NixOS/rfcs#131.

vcunat · 2022-07-27T16:20:00Z

Possibly, but that sounds orders of magnitude more complex and far way, in comparison to this PR. EDIT: I'm not trying to claim that something like this PR will be a perfect final solution.

jtojnar · 2022-07-27T22:04:23Z

AFAIK it's general .override function. That has no knowledge about inter-dependencies between attributes of the function. EDIT: though I suppose we could redefine the fetcher's .override to do such things, but I don't know...

Yeah, we could have mkFetcherOverridable and use it on fetchurl:

https://github.com/NixOS/nixpkgs/blob/18692f7718b8568f1738a334397db887e27e26ae/pkgs/top-level/all-packages.nix#L728

Also, what if the original derivation switches the fetchers. We commonly use many: fetchurl, fetchFromGitHub, fetchPypi... so after switching your override won't make really sense anymore. Overall I couldn't find motivation why to use .src.override, but perhaps you do so that would be nice to hear in this thread.

That’s just the pain we have to deal with when using any overrides. We are hoping such changes are relatively rare.

The point is that with #119942, the best case scenario allows you to just do hello.overrideAttrs (attrs: { version = "1.0.0"; src = attrs.src.override { hash = "xxx"; }; }) rather than rebuilding the src completely from scratch.

mweinelt · 2022-08-23T15:07:23Z

It would be simpler to just move pkgs/development/python-modules/ansible/core.nix to hash instead.

pkgs/tools/admin/oci-cli/default.nix

vcunat · 2022-08-23T15:13:19Z

Well, perhaps. This seemed simple and foolproof. I just wanted to get some OK solution so this PR can get merged without dropping packages. EDIT: meaning that later improvements are welcome – and people will get the throw so it should just "get maintained by itself" in future.

I now checked eval against master and found no dropped packages anymore.

SuperSandro2000 · 2022-08-24T18:16:25Z

Not really a fan of the last 3 commits but I also don't know a better solution than just using hash all the time.

fetchurl: disallow specifying both sha256 and hash

cb704bf

A full check would be more complicated to write - and more importantly - probably also more expensive. Motivation: eval-time catch for errors like in commit 8198636.

vcunat added the 9.needs: community feedback label Jul 26, 2022

github-actions bot added the 6.topic: fetch label Jul 26, 2022

ofborg bot added 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jul 26, 2022

vcunat requested a review from mweinelt as a code owner July 26, 2022 11:14

home-assistant: don't pass two different hashes to fetchPypi

ccf609a

Let's stop using src.override. I see no advantage.

mweinelt force-pushed the p/fetchurl-multihash branch from 9c066c0 to ccf609a Compare July 26, 2022 11:21

SuperSandro2000 reviewed Jul 26, 2022

View reviewed changes

tts: don't override librosa fetcher, replace it

28eee98

powerdns-admin: avoid duplicate hash in src.override

500d1a6

mweinelt reviewed Aug 23, 2022

View reviewed changes

pkgs/tools/admin/oci-cli/default.nix Show resolved Hide resolved

vcunat added 2 commits August 23, 2022 17:10

oci-cli: avoid duplicate hash in src.override

97afcab

ansible: avoid duplicate hash in src.override

ad8d5e0

vcunat force-pushed the p/fetchurl-multihash branch from ffe156f to ad8d5e0 Compare August 23, 2022 15:11

vcunat merged commit 0e304ff into NixOS:master Aug 24, 2022

vcunat deleted the p/fetchurl-multihash branch August 24, 2022 15:12

samuela mentioned this pull request Aug 27, 2022

cudaPackages.cudnn is currently failing to evaluate #188447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fetchurl: disallow specifying both `sha256` and `hash` #182953

fetchurl: disallow specifying both `sha256` and `hash` #182953

vcunat commented Jul 26, 2022

vcunat commented Jul 26, 2022 •

edited

Loading

vcunat commented Jul 26, 2022

mweinelt commented Jul 26, 2022

vcunat commented Jul 26, 2022

SuperSandro2000 Jul 26, 2022 •

edited

Loading

mweinelt Jul 26, 2022 •

edited

Loading

vcunat Jul 26, 2022 •

edited

Loading

viraptor Jul 26, 2022 •

edited

Loading

vcunat Jul 26, 2022

piegamesde commented Jul 27, 2022

vcunat commented Jul 27, 2022 •

edited

Loading

vcunat commented Jul 27, 2022 •

edited

Loading

mweinelt commented Jul 27, 2022

vcunat commented Jul 27, 2022 •

edited

Loading

jtojnar commented Jul 27, 2022

mweinelt commented Aug 23, 2022

vcunat commented Aug 23, 2022 •

edited

Loading

SuperSandro2000 commented Aug 24, 2022

		# 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

	# 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 != "" && sha1 != "" && sha256 != "" && sha512 != "" then throw "multiple hashes passed to fetchurl" else

fetchurl: disallow specifying both sha256 and hash #182953

fetchurl: disallow specifying both sha256 and hash #182953

Conversation

vcunat commented Jul 26, 2022

vcunat commented Jul 26, 2022 • edited Loading

vcunat commented Jul 26, 2022

mweinelt commented Jul 26, 2022

vcunat commented Jul 26, 2022

SuperSandro2000 Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

mweinelt Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

vcunat Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

viraptor Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

vcunat Jul 26, 2022

Choose a reason for hiding this comment

piegamesde commented Jul 27, 2022

vcunat commented Jul 27, 2022 • edited Loading

vcunat commented Jul 27, 2022 • edited Loading

mweinelt commented Jul 27, 2022

vcunat commented Jul 27, 2022 • edited Loading

jtojnar commented Jul 27, 2022

mweinelt commented Aug 23, 2022

vcunat commented Aug 23, 2022 • edited Loading

SuperSandro2000 commented Aug 24, 2022

fetchurl: disallow specifying both `sha256` and `hash` #182953

fetchurl: disallow specifying both `sha256` and `hash` #182953

vcunat commented Jul 26, 2022 •

edited

Loading

SuperSandro2000 Jul 26, 2022 •

edited

Loading

mweinelt Jul 26, 2022 •

edited

Loading

vcunat Jul 26, 2022 •

edited

Loading

viraptor Jul 26, 2022 •

edited

Loading

vcunat commented Jul 27, 2022 •

edited

Loading

vcunat commented Jul 27, 2022 •

edited

Loading

vcunat commented Jul 27, 2022 •

edited

Loading

vcunat commented Aug 23, 2022 •

edited

Loading