fetchurl: builder.sh: handle urls as a Bash array (#471851)

pkgs/build-support/fetchurl/builder.sh

@@ -124,10 +124,10 @@ tryHashedMirrors() {
 # URL list may contain ?. No glob expansion for that, please
 set -o noglob
 
-urls2=
+resolvedUrls=()
 for url in "${urls[@]}"; do
     if test "${url:0:9}" != "mirror://"; then
-        urls2="$urls2 $url"
+        resolvedUrls+=("$url")
     else
         url2="${url:9}"; echo "${url2/\// }" > split; read site fileName < split
         #varName="mirror_$site"
@@ -142,18 +142,17 @@ for url in "${urls[@]}"; do
             if test -n "${!varName}"; then mirrors="${!varName}"; fi
 
             for url3 in $mirrors; do
-                urls2="$urls2 $url3$fileName";
+                resolvedUrls+=("$url3$fileName");
             done
         fi
     fi
 done
-urls="$urls2"
 
 # Restore globbing settings
 set +o noglob
 
 if test -n "$showURLs"; then
-    echo "$urls" > $out
+    echo "${resolvedUrls[*]}" > $out
     exit 0
 fi
 
@@ -165,7 +164,7 @@ fi
 set -o noglob
 
 success=
-for url in $urls; do
+for url in "${resolvedUrls[@]}"; do
     if [ -z "$postFetch" ]; then
        case "$url" in
            https://github.com/*/archive/*)

pkgs/build-support/fetchurl/default.nix

@@ -35,6 +35,40 @@ let
   # "gnu", etc.).
   sites = builtins.attrNames mirrors;
 
+  /**
+    Resolve a URL against the available mirrors.
+
+    If the input is a `"mirror://"` URL, it is normalized.
+    Otherwise, the URL is returned unmodified in a singleton list.
+
+    Mirror URLs should be formatted as:
+    ```
+    mirror://{mirror_name}/{path}
+    ```
+
+    The specified `mirror_name` must correspond to an entry in `pkgs/build-support/fetchurl/mirrors.nix`, otherwise an error is thrown.
+
+    # Inputs
+
+    `url` (String)
+    : A (possibly `"mirror://"`) URL to resolve.
+
+    # Output
+
+    A list of resolved URLs.
+  */
+  resolveUrl =
+    url:
+    let
+      mirrorSplit = lib.match "mirror://([[:alpha:]]+)/(.+)" url;
+      mirrorName = lib.head mirrorSplit;
+      mirrorList = mirrors."${mirrorName}" or (throw "unknown mirror:// site ${mirrorName}");
+    in
+    if mirrorSplit == null || mirrorName == null then
+      [ url ]
+    else
+      map (mirror: mirror + lib.elemAt mirrorSplit 1) mirrorList;
+
   impureEnvVars =
     lib.fetchers.proxyImpureEnvVars
     ++ [
@@ -223,20 +257,7 @@ lib.extendMkDerivation {
       finalHashHasColon = lib.hasInfix ":" finalAttrs.hash;
       finalHashColonMatch = lib.match "([^:]+)[:](.*)" finalAttrs.hash;
 
-      resolvedUrl =
-        let
-          mirrorSplit = lib.match "mirror://([[:alpha:]]+)/(.+)" url;
-          mirrorName = lib.head mirrorSplit;
-          mirrorList =
-            if lib.hasAttr mirrorName mirrors then
-              mirrors."${mirrorName}"
-            else
-              throw "unknown mirror:// site ${mirrorName}";
-        in
-        if mirrorSplit == null || mirrorName == null then
-          url
-        else
-          "${lib.head mirrorList}${lib.elemAt mirrorSplit 1}";
+      resolvedUrl = lib.head (resolveUrl url);
     in
 
     derivationArgs
@@ -343,3 +364,6 @@ lib.extendMkDerivation {
   # No ellipsis
   inheritFunctionArgs = false;
 }
+// {
+  inherit resolveUrl;
+}

pkgs/build-support/fetchurl/tests.nix

@@ -3,9 +3,11 @@
   testers,
   fetchurl,
   writeShellScriptBin,
+  writeText,
   jq,
   moreutils,
   emptyFile,
+  hello,
   ...
 }:
 let
@@ -138,4 +140,50 @@ in
     # have to fallback to fetching the previously-built derivation from
     # tarballs.nixos.org, which provides pre-built derivation outputs.
   };
+
+  showURLs-urls-mirrors = testers.invalidateFetcherByDrvHash fetchurl (finalAttrs: {
+    name = "test-fetchurl-showURLs-urls-mirrors";
+    showURLs = true;
+    urls = [
+      "http://broken"
+    ]
+    ++ hello.src.urls;
+    hash =
+      let
+        hashAlgo = lib.head (lib.splitString "-" lib.fakeHash);
+      in
+      hashAlgo
+      + ":"
+      + builtins.hashString hashAlgo (
+        lib.concatStringsSep " " (lib.concatMap fetchurl.resolveUrl finalAttrs.urls) + "\n"
+      );
+  });
+
+  urls-simple = testers.invalidateFetcherByDrvHash fetchurl {
+    name = "test-fetchurl-urls-simple";
+    urls = [
+      "http://broken"
+      hello.src.resolvedUrl
+    ];
+    hash = hello.src.outputHash;
+  };
+
+  urls-mirrors = testers.invalidateFetcherByDrvHash fetchurl rec {
+    name = "test-fetchurl-urls-simple";
+    urls = [
+      "http://broken"
+    ]
+    ++ hello.src.urls;
+    hash = hello.src.outputHash;
+    postFetch = hello.postFetch or "" + ''
+      if ! diff -u ${
+        builtins.toFile "urls-resolved-by-eval" (
+          lib.concatStringsSep "\n" (lib.concatMap fetchurl.resolveUrl urls) + "\n"
+        )
+      } <(printf '%s\n' "''${resolvedUrls[@]}"); then
+        echo "ERROR: fetchurl: build-time-resolved URLs \`urls' differ from the evaluation-resolved URLs." >&2
+        exit 1
+      fi
+    '';
+  };
 }

pkgs/build-support/testers/default.nix

@@ -132,18 +132,36 @@
   invalidateFetcherByDrvHash =
     f: args:
     let
-      drvPath = (f args).drvPath;
+      optionalFix = if lib.isFunction args then lib.id else lib.fix;
+      unsalted = f args;
+      drvPath = unsalted.drvPath;
       # It's safe to discard the context, because we don't access the path.
       salt = builtins.unsafeDiscardStringContext (lib.substring 0 12 (baseNameOf drvPath));
+      saltName = name: "${name}-salted-${salt}";
+      getSaltedNames =
+        args:
+        if args.pname or null != null then
+          { pname = saltName args.pname; }
+        else
+          { name = saltName args.name or "source"; };
       # New derivation incorporating the original drv hash in the name
-      salted = f (args // { name = "${args.name or "source"}-salted-${salt}"; });
-      # Make sure we did change the derivation. If the fetcher ignores `name`,
+      saltedByArgs = f (optionalFix (lib.extends (lib.toExtension getSaltedNames) (lib.toFunction args)));
+      saltedByOverrideAttrs = unsalted.overrideAttrs (previousAttrs: getSaltedNames previousAttrs);
+      saltedByOverrideAttrsForced = unsalted.overrideAttrs (previousAttrs: {
+        name = saltName unsalted.name;
+      });
+      # Make sure we did change the derivation.
+      # If the fetcher ignores `pname` and `name` and provide a broken `overrideAttrs`,
       # `invalidateFetcherByDrvHash` doesn't work.
       checked =
-        if salted.drvPath == drvPath then
-          throw "invalidateFetcherByDrvHash: Adding the derivation hash to the fixed-output derivation name had no effect. Make sure the fetcher's name argument ends up in the derivation name. Otherwise, the fetcher will not be re-run when its implementation changes. This is important for testing."
+        if saltedByArgs.drvPath != drvPath then
+          saltedByArgs
+        else if saltedByOverrideAttrs.drvPath != drvPath then
+          saltedByOverrideAttrs
+        else if saltedByOverrideAttrsForced.drvPath != drvPath then
+          saltedByOverrideAttrsForced
         else
-          salted;
+          throw "invalidateFetcherByDrvHash: Neither adding pname/name to the fetcher args nor overriding with overrideAttrs change the result drvPath.";
     in
     checked;