Skip to content

fetchurl: fix downloadToTemp & hashedMirrors#445592

Merged
philiptaron merged 1 commit intoNixOS:masterfrom
tweag:balsoft/fetchurl-no-skipPostFetch
Sep 29, 2025
Merged

fetchurl: fix downloadToTemp & hashedMirrors#445592
philiptaron merged 1 commit intoNixOS:masterfrom
tweag:balsoft/fetchurl-no-skipPostFetch

Conversation

@balsoft
Copy link
Member

@balsoft balsoft commented Sep 23, 2025 •
edited
Loading

We must copy the file from $downloadedFile to $out when we're
skipping postFetch; otherwise hashedMirrors won't work with
downloadToTemp.

The provided test demonstrates a case which fails before this commit.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) labels Sep 23, 2025
@nix-owners nix-owners bot requested a review from philiptaron September 23, 2025 17:48
@nixpkgs-ci nixpkgs-ci bot added 9.needs: reviewer This PR currently has no reviewers requested and needs attention. and removed 9.needs: reviewer This PR currently has no reviewers requested and needs attention. labels Sep 23, 2025
philiptaron
philiptaron requested changes Sep 23, 2025
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's make the derivation exit 1 in this case. It's a programming error to specify this combo, as it will always fail; we shouldn't try to make specifying dontFetch and downloadToTemp together work.

@balsoft
Copy link
Member Author

balsoft commented Sep 26, 2025

Let's make the derivation exit 1 in this case. It's a programming error to specify this combo, as it will always fail; we shouldn't try to make specifying dontFetch and downloadToTemp together work.

The user only specifies downloadToTemp; skipPostFetch is an internal variable that is set to true when using hashedMirrors. I think what we should do instead is disable downloadToTemp when trying hashedMirrors.

@balsoft balsoft force-pushed the balsoft/fetchurl-no-skipPostFetch branch from 7330afb to 4a201df Compare September 26, 2025 09:33
@balsoft balsoft requested a review from philiptaron September 26, 2025 09:33
@infinisil infinisil force-pushed the balsoft/fetchurl-no-skipPostFetch branch from 4a201df to 98df5dd Compare September 29, 2025 21:29
infinisil
infinisil approved these changes Sep 29, 2025
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

This was kind of hard to understand, so I improved the code and commit message, I hope this helps:

fetchurl: fix hashedMirrors with downloadToTemp

fetchurl supports postFetch, which must not run when fetching from hashedMirrors, because they provide pre-built derivation outputs.

fetchurl also supports downloadToTemp, which modifies behavior relating to postFetch.

Before this commit, a call like

fetchurl {
  url = "...";
  downloadToTemp = true;
  postFetch = ''
    mv "$downloadedFile" "$out"
  '';
}

would work in normal scenarios, but fail when fetching from hashedMirrors, due to how internally downloadToTemp was not tied to postFetch as closely as it should've been.
This resulted in an error like

builder [...] failed to produce output path

This commit fixes this, making hashedMirror fetching work even for derivations that have downloadToTemp enabled.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 29, 2025
@balsoft
Copy link
Member Author

balsoft commented Sep 29, 2025

@infinisil treefmt fail

@balsoft @infinisil
fetchurl: fix hashedMirrors with downloadToTemp
27a36ae 
fetchurl supports postFetch, which must not run when fetching from
hashedMirrors, because they provide pre-built derivation outputs.

fetchurl also supports downloadToTemp, which modifies behavior relating
to postFetch.

Before this commit, a call like

    fetchurl {
      url = "...";
      downloadToTemp = true;
      postFetch = ''
        mv "$downloadedFile" "$out"
      '';
    }

would work in normal scenarios, but fail when fetching from
hashedMirrors, due to how internally downloadToTemp was not tied to
postFetch as closely as it should've been. This resulted in an error
like

    builder [...] failed to produce output path

This commit fixes this, making hashedMirror fetching work even for
derivations that have downloadToTemp enabled.

Co-Authored-By: Silvan Mosberger <[email protected]>
@infinisil infinisil force-pushed the balsoft/fetchurl-no-skipPostFetch branch from 98df5dd to 27a36ae Compare September 29, 2025 21:47
balsoft
balsoft commented Sep 29, 2025
View reviewed changes
Copy link
Member Author

@balsoft balsoft left a comment

Choose a reason for hiding this comment

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

Otherwise looks better than my version

@philiptaron
Copy link
Contributor

philiptaron commented Sep 29, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 445592
Commit: 27a36aeb9cbb1bbd6c1df76a5c8f19039cc4247a

x86_64-linux

✅ 48 packages built:
  • tests.fetchDebianPatch.libPackage
  • tests.fetchDebianPatch.simple
  • tests.fetchFirefoxAddon.simple (tests.fetchFirefoxAddon.overridden-source)
  • tests.fetchFromBitbucket.withEncodedWhitespace
  • tests.fetchFromBitbucket.withoutWhitespace
  • tests.fetchNextcloudApp.simple-sha256
  • tests.fetchNextcloudApp.simple-sha512
  • tests.fetchPypiLegacy.fetchSimple
  • tests.fetchgit.dumb-http-signed-tag
  • tests.fetchgit.fetchTags
  • tests.fetchgit.leave-git
  • tests.fetchgit.prefetch-git-no-add-path
  • tests.fetchgit.rootDir
  • tests.fetchgit.simple
  • tests.fetchgit.sparseCheckout
  • tests.fetchgit.sparseCheckoutNonConeMode
  • tests.fetchgit.submodule-deep
  • tests.fetchgit.submodule-leave-git
  • tests.fetchgit.submodule-leave-git-deep
  • tests.fetchgit.submodule-simple
  • tests.fetchgit.withGitConfig
  • tests.fetchpatch.decode
  • tests.fetchpatch.fileWithApostrophe
  • tests.fetchpatch.fileWithSpace
  • tests.fetchpatch.full
  • tests.fetchpatch.relative
  • tests.fetchpatch.simple
  • tests.fetchpatch2.decode
  • tests.fetchpatch2.fileWithApostrophe
  • tests.fetchpatch2.fileWithSpace
  • tests.fetchpatch2.full
  • tests.fetchpatch2.relative
  • tests.fetchpatch2.simple
  • tests.fetchtorrent.http-link (tests.fetchtorrent.http-link-transmission)
  • tests.fetchtorrent.http-link-rqbit (tests.fetchtorrent.http-link-rqbit-flattened)
  • tests.fetchtorrent.http-link-rqbit-unflattened
  • tests.fetchtorrent.magnet-link (tests.fetchtorrent.magnet-link-transmission)
  • tests.fetchtorrent.magnet-link-rqbit (tests.fetchtorrent.magnet-link-rqbit-flattened)
  • tests.fetchtorrent.magnet-link-rqbit-unflattened
  • tests.fetchurl.header
  • tests.fetchurl.no-skipPostFetch
  • tests.fetchzip.hiddenDir
  • tests.fetchzip.postFetch
  • tests.fetchzip.simple
  • tests.haskell.cabalSdist.assumptionLocalHasDirectReference
  • tests.haskell.cabalSdist.localHasNoDirectReference
  • tests.testers.runCommand.bork
  • tests.testers.runCommand.dns-resolution

philiptaron
philiptaron approved these changes Sep 29, 2025
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I like Silvan's change a lot.

@philiptaron philiptaron added this pull request to the merge queue Sep 29, 2025
Merged via the queue into NixOS:master with commit 386f535 Sep 29, 2025
27 of 31 checks passed
@infinisil infinisil deleted the balsoft/fetchurl-no-skipPostFetch branch September 30, 2025 22:52
@infinisil infinisil mentioned this pull request Oct 6, 2025
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philiptaron philiptaron philiptaron approved these changes

@infinisil infinisil infinisil approved these changes

Assignees

No one assigned

Labels

6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@balsoft @philiptaron @infinisil