-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
fix inputDerivation in case of __structuredAttrs #469652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
It would also be nice if this could get back-ported to 25.11. Since 25.11, Which makes this bug much more likely to happen. (or at least, that's why we ran into it in |
|
Dupliate of #463882 ? |
|
Ah good call. I prefer my solution, but I would take the rename and test from the other. @infinisil What do you think? |
|
Let's get the tests from that other PR (and add more of them), too. |
35d1721 to
b52e0f0
Compare
|
@Ericson2314 This time I intentionally reintroduced the typo temporarily to make sure the tests I added would catch it. |
Co-authored-by: infinisil <[email protected]>
27c269f to
ddec463
Compare
|
Is it normal for CI to be stuck in queue for a whole day? I don't mind necessarily, I just want to make sure there's nothing I should be doing to push this forward. |
Setting allowedReferences to null seems to only work as a fluke. It doesn't work with outputChecks, and I couldn't get it to work at all when declaring my own derivation manually (I'm honestly still unsure why it works at all as-is in inputDerivation) In any case, Rather than setting allowedReferences etc to values that mimic the default behaviour, we can remove those attributes to get the default behaviour.
Co-authored-by: infinisil <[email protected]> !fixup every test needs a meta field? !fixup refactor inputDerivation tests !fixup fix tests
ddec463 to
97c3645
Compare
|
Okay, I tested this locally using And they pass with this PR as it currently is. |
I don't mind as long as it works, thanks for the additional tests, I'll close mine! |
|
Successfully created backport PR for |
package.inputDerivationis broken whenpackagesets__structuredAttrs = true;. You can test this withThere is an attempt to account for this in the implementation of
inputDerivation, but it's broken. The issue is that it assumes settingallowedRefrencesandallowedRequisitestonullwill be like not setting them at all, which is false.It does seem to work as top-level attributes (
though I'm honestly not sure why this works, and I couldn't get it to work in my own derivations1), but inoutputChecks.out.allowedReferences, it definitely doesn't work.In any case, Rather than setting
allowedReferences,disallowedReferences,allowedRequisitesanddisallowedRequisitesto values that we hope mimic the default behaviour, we can remove those attributes to get the default behaviour.Related issues:
allowed* = nullproperly in structuredAttrs nix#14750Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.
Footnotes
It works currently because
__ignoreNulls = true;is added. However, this only seems to work for top-level attributes, notoutputChecks.out.foo. I still think getting rid of the attributes entirely is the right way forward. ↩