doc/stdenv: add a note about __structuredAttrs#509970
Conversation
0cfe5d6 to
2439a7a
Compare
|
yea i found it there, but since it now affects nixpkgs/nixos i thought it would be good to add it here too. Is it okay to link to other manuals from this one, or is each manual supposed to work offline? |
I just commented something similar over on the related issue:
|
That'd be a question for the docs team, if it isn't documented in the docs READMEs. IIRC there are already a few external links in the manual, but I could be wrong. |
Yea, a quick |
2439a7a to
6be1cc3
Compare
6be1cc3 to
3e4dbd6
Compare
Mindavi
left a comment
There was a problem hiding this comment.
I'm pretty happy with this.
There was a problem hiding this comment.
I see no issues with this however @MattSturgeon might have some changes, so I would like to hear from them before merging if there is anything that is blocking this from merging.
There was a problem hiding this comment.
LGTM on the whole. I've left some (a lot of) minor and/or subjective wording suggestions.
The most significant things for me are putting too much emphasis on env (implicitly discouraging use of structured shell variables) and whether we need a similar warning about passAsFile?
passAsFile usage is fairly common in Nixpkgs, and its conflict with __structuredAttrs isn't explicitly called out by the upstream nix docs, other than:
This obviates the need for
passAsFilesince JSON files have no size restrictions, unlike process environments.
If any specific point becomes contentions, I'm happy for it to be deferred to a future PR.
|
@MattSturgeon thanks for the in depth review! I'll take a look now :)
sure, i don't see why not. it fits in nicely with the {.important} tag. |
3e4dbd6 to
cca04fa
Compare
MattSturgeon
left a comment
There was a problem hiding this comment.
I think we can merge once https://github.com/NixOS/nixpkgs/pull/509970/changes#r3089058060 and the final nits are resolved, unless anyone has further feedback.*
*(Any significant feedback my be best as a follow-up PR, to reduce contributing friction)
cca04fa to
4683c14
Compare
MattSturgeon
left a comment
There was a problem hiding this comment.
Thanks for putting this together and sticking through the review process! LGTM
I'll merge this now, if anyone has further suggestions, improvements, nits, etc I'm happy to be pinged to review follow-up PRs.
My pleasure, thanks for the quick responses! |
since NixOS/nixpkgs-vet#203, CI will fail for new top level packages that do not enable
__structuredAttrs. AFAIK there is little to no up to date documentation on this.Not exactly my area of expertise, so if somebody has a better explanation on what
__structuredAttrsdoes, feel free to drop a comment!closes #509957, NixOS/nix#14847
based on #354949
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.