Skip to content

fetchurl: Add hook for rewriting/filtering URLs#410186

Merged
philiptaron merged 1 commit intoNixOS:masterfrom
tweag:jherland/add-rewriteURL-hook
Jun 5, 2025
Merged

fetchurl: Add hook for rewriting/filtering URLs#410186
philiptaron merged 1 commit intoNixOS:masterfrom
tweag:jherland/add-rewriteURL-hook

Conversation

@jherland
Copy link
Contributor

@jherland jherland commented May 23, 2025

This allows on-the-fly rewriting of URLs before they are passed from fetchurl (or fetchurlBoot) to curl.

The intended use is to allow inserting company-internal mirrors, or working around company firewalls and similar network restrictions, without having to extensively patch across all of nixpkgs. Instead, users can pass a function in their nixpkgs config that performs the necessary URL rewrites.

We first tried solving this with a shell hook called from inside fetchurl's builder.sh, as that would apply rewrites after the mirror:// scheme has been resolved, however, this was not feasible to also apply to fetchurlBoot, hence we instead settled on a Nix function called before the builder is invoked. If we still want/need the rewrite to happen after resolving mirror:// URLs, then we probably need to move the mirroring logic out of the builder.sh and rewrite it in nix.

This is related in spirit to #409010, but does not directly depend on it.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • Nixpkgs 25.11 Release Notes (or backporting 24.11 and 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 24.11 and 25.05 NixOS Release notes)
    • Added a release notes entry about the new config.rewriteURL attribute
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: stdenv Standard environment labels May 23, 2025
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label May 23, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels May 23, 2025
toonn
toonn reviewed May 23, 2025
View reviewed changes
@jherland jherland force-pushed the jherland/add-rewriteURL-hook branch 2 times, most recently from cf0413c to 2802c3c Compare June 2, 2025 14:24
philiptaron
philiptaron approved these changes Jun 2, 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.

I'm amenable to this feature.

I do wonder if it's not going to be thorough enough for the stated usecase. Will you actually need to reach into the derivation and mutate (say) the environment variables also? I suspect you will, in order to set environment variables needed by proxies and the like.

If that's the case, you're going to be looking for a heavier hammer than this. We have those hammers! We can make that work!

@jherland jherland force-pushed the jherland/add-rewriteURL-hook branch from 2802c3c to 7e5cde7 Compare June 3, 2025 15:22
@jherland @balsoft
fetchurl: Add hook for rewriting/filtering URLs
abda866 
This allows on-the-fly rewriting of URLs before they are passed from
fetchurl (or fetchurlBoot) to curl.

The intended use is to allow inserting company-internal mirrors, or
working around company firewalls and similar network restrictions,
without having to extensively patch across all of nixpkgs. Instead,
users can pass a function in their nixpkgs that performs the necessary
URL rewrites.

Co-authored-by: Alexander Bantyev <[email protected]>
@jherland jherland force-pushed the jherland/add-rewriteURL-hook branch from 7e5cde7 to abda866 Compare June 4, 2025 10:29
@jherland
Copy link
Contributor Author

jherland commented Jun 4, 2025

@philiptaron writes:

I do wonder if it's not going to be thorough enough for the stated usecase. Will you actually need to reach into the derivation and mutate (say) the environment variables also? I suspect you will, in order to set environment variables needed by proxies and the like.

For proxies and similar we should already be able to pass those in via impureEnvVars, shouldn't we?

If that's the case, you're going to be looking for a heavier hammer than this. We have those hammers! We can make that work!

For now, we're simply looking to do bulk overrides of URLs onto company-internal mirrors, and to do so without causing world rebuilds, or otherwise hacking each individual derivation. What are the hammers you're referring to, and how would those help us here?

@philiptaron
Copy link
Contributor

philiptaron commented Jun 4, 2025

The hammers I'm thinking of equate to an overlay replacing fetchurl.

@philiptaron
Copy link
Contributor

philiptaron commented Jun 4, 2025

I move to merge unless objections arise in the next day or so.

@balsoft
Copy link
Member

balsoft commented Jun 5, 2025

The hammers I'm thinking of equate to an overlay replacing fetchurl.

I'd like to note that that wouldn't work for fetchurlBoot, while this change transparently works for both fetchurl and fetchurlBoot.

@philiptaron philiptaron merged commit 950afd1 into NixOS:master Jun 5, 2025
16 of 18 checks passed
@github-project-automation github-project-automation bot moved this to Done in Stdenv Jun 5, 2025
@trofi
Copy link
Contributor

trofi commented Jun 7, 2025

Bisect says abda866 fetchurl: Add hook for rewriting/filtering URLs broke the eval of minimal-bootstrap.bash on master as:

$ nix-instantiate -A minimal-bootstrap.bash --argstr system i686-linux
error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:37:12:
           36|
           37|   strict = derivationStrict drvAttrs;
             |            ^
           38|

       … while evaluating derivation 'bash-5.2.15'
         whose name attribute is located at pkgs/os-specific/linux/minimal-bootstrap/bash/2.nix:85:19

       … while evaluating attribute 'PATH' of derivation 'bash-5.2.15'
         at pkgs/os-specific/linux/minimal-bootstrap/bash/2.nix:108:11:
          107|           SHELL = "${bash_2_05}/bin/bash";
          108|           PATH = lib.makeBinPath (
             |           ^
          109|             (env.nativeBuildInputs or [ ])

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: function 'anonymous lambda' called without required argument 'rewriteURL'
       at /tmp/nixpkgs/pkgs/build-support/fetchurl/boot.nix:5:1:
            4|
            5| {
             | ^
            6|   rewriteURL,

@trofi
Copy link
Contributor

trofi commented Jun 7, 2025

Proposed the fix as:

@philiptaron
Copy link
Contributor

philiptaron commented Jun 7, 2025

Thanks, merged. Sorry for the breakage.

@rhelmot rhelmot mentioned this pull request Jun 9, 2025
Merged
13 tasks
This was referenced Jul 23, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toonn toonn toonn left review comments

@philiptaron philiptaron philiptaron approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@emilazy emilazy Awaiting requested review from emilazy

@reckenrode reckenrode Awaiting requested review from reckenrode

@Artturin Artturin Awaiting requested review from Artturin

@RossComputerGuy RossComputerGuy Awaiting requested review from RossComputerGuy

Assignees

No one assigned

Labels

6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: stdenv Standard environment 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

Stdenv
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jherland @philiptaron @balsoft @trofi @toonn