fetchurl, fetchgit: use __structuredAttrs = true and pass curlOptsList and sparseCheckout as lists#464475
Conversation
nixpkgs-ci
bot
added
10.rebuild-linux: 11-100
ShamrockLee
force-pushed
the
fetchurl-structuredAttrs
branch
3 times, most recently
from
13e79a4 to
82311b3
Compare
|
|
|
The new test |
nixpkgs-ci
bot
requested review from
Artturin,
Ericson2314,
RossComputerGuy,
emilazy,
philiptaron,
reckenrode and
toonn
ShamrockLee
requested review from
infinisil
and removed request for
Artturin,
Ericson2314,
RossComputerGuy,
emilazy,
philiptaron,
reckenrode and
toonn
nixpkgs-ci
bot
added
12.approvals: 3+
|
It seems this broke my fetcher. I bisected it and cd13136 is the first bad commit. fetchFromGitLab {
domain = "mydomain";
private = true;
varPrefix = "MYPREFIX";
# to show curl SSL trust anchors:
curlOptsList = [ "--verbose" ];
# usual attrs
owner = "owner";
repo = "repo";
rev = "v${version}";
hash = lib.fakeHash;
}
With/after cd13136, it fails with: |
|
Specifying the certificate in the src = fetchurl {
# ...
curlOptsList = [
"--cacert"
"${cacert}/etc/ssl/certs/ca-bundle.crt"
];
}; |
If a fix isn't immediately obvious then yes, we should revert for now. If we think we'll have a fix today, then I'm happy to hold off on reverting. I suspect the certificate flags may be being defined somewhere in a way that assumes Personally I'd like to see a regression test added for "empty hash" FODs, so that we can know this is fixed and feel more confident about it not breaking again in the future. |
|
With declare SSL_CERT_FILE='/nix/store/m65dckzfgr1lr90g5v6jx0iynnz38nsk-nss-cacert-3.115/etc/ssl/certs/ca-bundle.crt'I suspect without Adding: # Export SSL_CERT_FILE so curl can see it
export SSL_CERT_FILE="$SSL_CERT_FILE"to builder.sh seems to fix the issue. EDIT: #470503 |
|
Sorry for my loose ends. Packages with AFAIK, we don't view How about we merge and backport @MattSturgeon's workaround (PR #470503) when backporting this PR, while leaving the |
I don't understand why to split this. #470504 seems like the correct fix to me, so we should just do that everywhere? (I only looked at this from very, very, very far away - but handling environment variables correctly for structuredAttrs should be straight-forward) |
|
#470504 is definitely the better fix. I'm not an authority on this, but I assume we don't consider the If you want to be extra safe, it could be mentioned in 26.05 release notes, and in the 25.11 backport it could be implemented as: env.SSL_CERT_FILE = finalAttrs.SSL_CERT_FILE;Or use my workaround. But either of those is probably overkill. |
Ah, yes! That's a good point.
IIRC,
Having a unified interface for future backports seems to be more important than preserving all local workarounds, especially since we don't guarentee such workarounds to be stable. |
|
Sorry, this is way too advanced for me, I can only describe the symptoms I see from afar: |
is now incorrect variable expansion, as curlOpts is a bash array when __structuredAttrs is enabled.
It should be: curlOpts+=(--header @./private-token)EDIT: same for which should be: curlOpts+=(--netrc-file "$PWD"/netrc) |
PR NixOS#464475 enables __structuredAttrs and makes curlOpts a bash array. Consequently, it must be extended as such to be effective. See also NixOS#464475 (comment)
|
The change cause |
|
Why? We explicitly warn against |
|
Did I misinterpret: - $curlOpts
+ ${curlOpts[*]}? I assumed that meant that shell code should treat |
That means it "can be" an array. We only warn (not throw) against it if users specify it as a list. |
7 participants
While preserving existing interface, this PR enables
fetchurlto take__struncuredAttrs. If specified astrue, thecurlOptsListwill be passed tostdenvNoCC.mkDerivationas a list without stringification, facilitating its overriding with<pkg>.overrideAttrs.To make
fetchurl/builder.shget the attributes when__structuredAttrsistrue, it now sources$NIX_ATTRS_SH_FILEif presented.As
fetchFromGitHubswitches betweenfetchurlandfetchgitwith the samepostFetch, this PR also converts thefetchgit-constructed FOD to use__structuredAttrs = trueand passessparseCheckoutas a list. This way, out-of-tree projects that need to support multiple Nixpkgs releases can detect this series of changes based on__structuredAttrs.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.