Skip to content

Parse allowed* = null properly in structuredAttrs #14750

Closed
Ericson2314 wants to merge 3 commits intomasterfrom
structured-attrs-null
Closed

Parse allowed* = null properly in structuredAttrs #14750
Ericson2314 wants to merge 3 commits intomasterfrom
structured-attrs-null

Conversation

@Ericson2314
Copy link
Member

Motivation

This is the explicit way to indicate that there is no allow list.

Context

Should help with failures in #14688 CC @Radvendii


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Ericson2314 Ericson2314 added the backport 2.31-maintenance Automatically creates a PR against the branch label Dec 9, 2025
@Ericson2314 Ericson2314 requested a review from edolstra as a code owner December 9, 2025 06:32
@Ericson2314 Ericson2314 added the backport 2.32-maintenance Automatically creates a PR against the branch label Dec 9, 2025
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Dec 9, 2025
@xokdvium
Copy link
Contributor

xokdvium commented Dec 9, 2025

This seems like a breaking change? `null' was never allowed.

storePaths.insert(parseSingleDerivedPath(s));
ret.insert_or_assign(key, std::move(storePaths));
if (auto * e = get(parsed->structuredAttrs, "exportReferencesGraph")) {
for (auto & [key, storePathsJson] : getObject(*e)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail if it's not an object. That's not what the code above used to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK sure we can put that back, this is the only breaking change of that nature?

@xokdvium
Copy link
Contributor

xokdvium commented Dec 9, 2025

This isn't a regression. I don't see why this needs to change.

@Radvendii
Copy link
Contributor

Looking at this discussion, I'm realising this should really be fixed in nixpkgs not nix. I was reluctant to come to that conclusion because

  1. 25.11 is already out, so fixing this in nixpkgs doesn't solve the problem for us.
  2. I still don't understand why what's in Nixpkgs works when __structuredAttrs = false, so I thought maybe I was missing something / holding it wrong.

But I think the bottom line is that nixpkgs should strip out allowedReferences in inputDerivation (and probably any time it's set to null)

With regard to (1), I will see if this can be backported, but I don't think it's a recent regression, so we might have to think of something else.

@Radvendii
Copy link
Contributor

Radvendii commented Dec 9, 2025

To be clear, this might be a worthwhile change to make to Nix as well, but it is a breaking change, so we should discuss it more thoroughly.

Thanks for looking into it @Ericson2314 💜

@Ericson2314
Copy link
Member Author

@Radvendii to be clear, I think there is still a good chance that I am fixing an accidental breaking change from a much older version of Nix.

@xokdvium
Copy link
Contributor

xokdvium commented Dec 9, 2025

much older version of Nix.

When did this first break? I think I've tested 2.4 and it also disallowed nulls.

@xokdvium
Copy link
Contributor

xokdvium commented Dec 9, 2025

I've checked the code and the issue has existed since 2.3 at the very least, so we are better off testing current behavior and being done with this. I don't see how changing the semantics of the store layer isn't a rat's nest. We simply must preserve it at all costs when it comes to subtle behavioural differences.

@Ericson2314
Copy link
Member Author

Yeah sounds good. We can devise some sort opt-in at a later point if we want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 2.31-maintenance Automatically creates a PR against the branch backport 2.32-maintenance Automatically creates a PR against the branch with-tests Issues related to testing. PRs with tests have some priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants