fetchpatch breaks the generic rewriter

[alois31]

If a patch is retrieved using fetchpatch, there is a second hash in the file, which triggers the multiple hash check of the generic version rewriter (example: https://r.ryantm.com/log/vlc/2024-06-09.log). Consequently, the package will not be updated. This is pretty surprising behaviour.

[rhendric]

I think this is intentional. If multiple hashes need updating, there isn't a way to determine which new hash goes to which location. nix-update is looser about this (just grabs the first hash and hopes for the best); it behooves the tool used by the automated process to be stricter.

PRs welcome if there's a sufficiently robust thing that we could do differently.

[jtojnar]

update-source-version uses the old hash from src attribute (by default):

https://github.com/NixOS/nixpkgs/blob/7987842fec1cd1f81cf481bcfdd00b8809831381/pkgs/common-updater/scripts/update-source-version#L138

[rhendric]

The problem isn't getting the old hash of src; the problem is that in the general case, updating version might affect things other than src. If your package has a primary src and a secondary bundle of data from a URL that depends on version, then two things could happen:

• We could zero out all hashes in the file, and get two reports from Nix that hashes don't match, and then which new hash do we put in which zeroed-out position?
• We could zero out only the src hash, but then the package would be left with a hash in the secondary position that matches an old version of the secondary data and so, depending on whether the old version is in the store, builders would either get an error or silently proceed with the previous version's data—and that's quite bad!

[jtojnar]

We can zero out src first, and cargoDeps only after we update the src hash. That way, we will only have one zero hash to replace at a time.

To prevent unknown secondary source attributes not being updated, the multiple hashes check could be updated to ignore known source fields.

[alois31]

It is clear that the check is there for a reason, otherwise I would have posted a pull request removing instead of an issue. After giving it some thought, I came up with the following solution:

1. Invalidate everything that looks like a hash; crucially, don't just zero them out, but replace them by unique placeholders (for example successive numbers)
2. Run the nix build, remember the hash mismatches it reported, and fix them
3. Repeat 2. until there are no hash mismatches any more
4. Succeed iff all hash mismatches have been fixed

Unless I'm missing something, this should fix the fetchpatch issue, while also subsuming the functionality of the Rust, Go and NPM rewriters, fixing some potential false negatives too (like hash = { … }.${…}, like in NixOS/nixpkgs#318181), while not creating significant risk for false negatives.

Other (less ideal, but still preferable to the status quo) ideas:

1. Add more heuristics (like ignoring fetchpatch, or disabling the multiple-hash check in by-name, where the "multiple packages in one file" pattern that prompted this check is disallowed)
2. Introduce a way to explicitly annotate hashes to be skipped by nixpkgs-update (for example, using a special comment)
3. Explicitly document this behavior here: https://nix-community.github.io/nixpkgs-update/nixpkgs-maintainer-faq/#no-update

[rhendric]

Don't bad things happen if a fake hash other than zeroes is used? ISTR reading something about how that's not good practice.

Other than that, again, PRs welcome if you want to implement something that complicated. But I'd be more supportive of an in-tree solution; I'm of the opinion that the updating logic nixpkgs-update does if there is no update script ought to be simple, and in particular not better than the functionality available to update scripts (packages should not be effectively penalized for having update scripts). The thing this project does well is determining what packages need updating and doing it in batch; making a generic everything-updater is more of a nix-update thing, and choosing nix-update-script or gitUpdater or something else for an update script is a per-package decision. I'm inclined to see the fallback rewriting that nixpkgs-update does as almost a legacy feature at this point.

[alois31]

Don't bad things happen if a fake hash other than zeroes is used? ISTR reading something about how that's not good practice.

Other than treating an empty hash as zero, I don't think Nix treats the zero hash specially. Of course you shouldn't choose a fake hash that collides with something that's already in your store (for example, not changing the hash at all).

Other than that, again, PRs welcome if you want to implement something that complicated. But I'd be more supportive of an in-tree solution; I'm of the opinion that the updating logic nixpkgs-update does if there is no update script ought to be simple, and in particular not better than the functionality available to update scripts (packages should not be effectively penalized for having update scripts).

This sounds quite reasonable. I think it would still be good to add some note about this to the documentation, particularly given how common fetchpatch usage is.

The thing this project does well is determining what packages need updating and doing it in batch; making a generic everything-updater is more of a nix-update thing, and choosing nix-update-script or gitUpdater or something else for an update script is a per-package decision. I'm inclined to see the fallback rewriting that nixpkgs-update does as almost a legacy feature at this point.

Obtaining the latest version can be quite involved, particularly for projects that supply their releases as tarballs. Of course you could duplicate the logic of nixpkgs-update, but this does not sound like a good idea, and repology probably won't like the bulk requests that causes. It might be a good idea if nixpkgs-update could communicate the target version (which it already knows anyway) to the update script.

[rhendric]

Obtaining the latest version can be quite involved, particularly for projects that supply their releases as tarballs.

Yeah, though note that in the specific case of VLC there is a Git repository we could read tags from. You could probably use

passthru.updateScript = genericUpdater {
  versionLister = "${common-updater-scripts}/bin/list-git-tags --url=https://code.videolan.org/videolan/vlc";
  ignoredVersions = "[-]";
};

if the common updater infrastructure could cope with the hash situation.

It might be a good idea if nixpkgs-update could communicate the target version (which it already knows anyway) to the update script.

I think that's a good idea! We could set an environment variable, or extend the updateScript interface to include a version hint and use that.