fetchurl: builder.sh: handle urls as a Bash array#471851
Merged
MattSturgeon merged 10 commits intoNixOS:masterfrom Dec 21, 2025
Merged
fetchurl: builder.sh: handle urls as a Bash array#471851MattSturgeon merged 10 commits intoNixOS:masterfrom
urls as a Bash array#471851MattSturgeon merged 10 commits intoNixOS:masterfrom
Conversation
13 tasks
nixpkgs-ci
bot
added
10.rebuild-linux: 11-100
Contributor
MattSturgeon
left a comment
MattSturgeon
left a comment
There was a problem hiding this comment.
Initial read through the diff, without cloning or testing.
Co-authored-by: Matt Sturgeon <matt@sturgeon.me.uk>
ShamrockLee
force-pushed
the
fetchurl-structuredAttrs-urls-fix
branch
from
5c2f3f6 to
13af71d
Compare
Contributor
Author
|
MattSturgeon
approved these changes
Dec 18, 2025
Contributor
MattSturgeon
left a comment
MattSturgeon
left a comment
There was a problem hiding this comment.
LGTM. The test failures seem to be darwin-specific network issues? I wonder if sandbox = off would resolve them?
nixpkgs-ci
bot
added
the
12.approvals: 1
MattSturgeon
force-pushed
the
fetchurl-structuredAttrs-urls-fix
branch
from
ca58959 to
65a6b72
Compare
Contributor
Author
I guess it is transient. GitHub seems to suffer from connection instability again. Quite some of my PRs suffer from CI check failures due to failed connection. |
Contributor
|
I'll do x86_64-darwin builds with and without sandbox, to satisfy my curiosity:
But either way, I think we should merge this promptly as it's a correctness fix for a core part of nixpkgs. If any issues are pointed out post-merge they can be dealt with in follow-up PRs. |
Contributor
Author
|
My |
Contributor
Author
Wait a minute. I found a syntax error in the seldom-checked |
Contributor
|
ShamrockLee
force-pushed
the
fetchurl-structuredAttrs-urls-fix
branch
from
65a6b72 to
a3ea395
Compare
Contributor
Author
|
@SuperSandro2000, are you available for a final look? |
MattSturgeon
force-pushed
the
fetchurl-structuredAttrs-urls-fix
branch
from
2a8dade to
20e0811
Compare
Comment on lines
+150
to
+152
| urls="$urls2" | ||
| urls=("${urlsResolved[@]}") | ||
|
|
||
| unset urlsResolved |
Contributor
There was a problem hiding this comment.
Let's just do unset urls instead of all the "redefine the same name" thing. Let's use urlsResolved further down.
Contributor
Author
There was a problem hiding this comment.
This would further change how postFetch would access the URL list. Should we add a release note entry about this fix?
I haven't found any fetchurl's postFetch referencing "$urls", but a close example is fetchelpa referencing $url, the URL of the successful download.
nixpkgs/pkgs/applications/editors/emacs/elisp-packages/fetchelpa.nix
Lines 18 to 22 in 4f0dadb
Contributor
Author
There was a problem hiding this comment.
We never state that the $urls will always be handled the same way in fetchurl.builder across Nixpkgs revision, and developers of downstream projects should be able to handle this.
Applied.
wolfgangwalther
approved these changes
Dec 18, 2025
Contributor
|
Diff LGTM except the nit above; didn't test. |
nixpkgs-ci
bot
added
12.approvals: 2
nixpkgs-ci
bot
added
12.approvals: 3+
Contributor
Author
|
ShamrockLee
force-pushed
the
fetchurl-structuredAttrs-urls-fix
branch
from
20e0811 to
cfdd4a3
Compare
nixpkgs-ci
bot
added
the
6.topic: testing
Clean up leftover for commit cd13136 ("fetchurl: use __structuredAttrs = true and pass curlOptsList directly") Continue the work of commit 23236b3 ("fetchurl: fix handling of fallback URLs"), addressing a Bash array re-assignment quirk: when assigning a Bash array variable as if it were a plain variable, the value goes to the first element, and the rest of the array stays the same. ```console $ foo=(a b) $ declare -p foo declare -a foo=([0]="a" [1]="b") $ foo="c d" $ declare -p foo declare -a foo=([0]="c d" [1]="b") ``` Don't rewrite the `${urls[@]}` with resolved URLs, but hold them with `${resolvedUrls[@]}` instead. Co-authored-by: Matt Sturgeon <matt@sturgeon.me.uk> Co-authored-by: Wolfgang Walther <walther@technowledgy.de>
…re name arguments
ShamrockLee
force-pushed
the
fetchurl-structuredAttrs-urls-fix
branch
from
3a1ee2a to
0b27145
Compare
Contributor
Author
|
I just pushed the following changes:
|
13 tasks
MattSturgeon
approved these changes
Dec 21, 2025
Contributor
MattSturgeon
left a comment
MattSturgeon
left a comment
There was a problem hiding this comment.
Still LGTM. Let's get this merged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clean up leftover for PR #464475.
Revert and re-implementContinue the work of PR #470643 to address a Bash variable reassignment quirk:PR #470643 probably does make the mirror fetching. Even if it doesn't clean up the "mirror://" pseudo-URL, it does add the resolved URLs which
curlwill use to download the file. We revert and re-implement in the same PR to avoid re-introducing the breakage PR #470643 has fixed.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.