Scrap ParsedDerivation for parts#12410
Conversation
aac9afc to
f81c89f
Compare
| }; | ||
| } | ||
|
|
||
| StringSet DerivationOptions::getRequiredSystemFeatures(const BasicDerivation & drv) const |
There was a problem hiding this comment.
Do these methods really belong to DerivationOptions, or is this just an artifact from the fact that they were previously in ParsedDerivation? Would it make sense to move them to BasicDerivation?
Edit: that is the endgoal anyway -- I forgot :D
There was a problem hiding this comment.
I don't think I care too much where these methods lie --- apologies if I said otherwise before.
| auto get_ = [&](const std::string & name) -> std::optional<StringSet> { | ||
| if (auto i = get(output, name)) { | ||
| StringSet res; | ||
| for (auto j = i->begin(); j != i->end(); ++j) { |
There was a problem hiding this comment.
We could also use getStringSet here, right?
There was a problem hiding this comment.
Good point! And it shouldn't be converted to store paths actually because that will break CA derivations using placeholders.
There was a problem hiding this comment.
Maybe a silly question, but where do we convert it to store paths right now?
There was a problem hiding this comment.
That would break CA derivations, in which is it still placeholders (and indeed for that reason the one place I did convert to store paths is a mistake)
We can, however, parse the placeholders and convert to SingleDerivedPath
There was a problem hiding this comment.
Oh the question was where. It is in when parsing the structured attrs / environment variables.
| checks.allowedRequisites = get_("allowedRequisites"); | ||
| checks.disallowedReferences = get_("disallowedReferences").value_or(StringSet{}); | ||
| checks.disallowedRequisites = get_("disallowedRequisites").value_or(StringSet{}); | ||
| ; |
src/libstore/derivation-options.hh
Outdated
| * attributes give to the builder. The set of paths in the original JSON | ||
| * is replaced with a list of `PathInfo` in JSON format. | ||
| */ | ||
| std::map<std::string, StorePathSet> exportReferencesGraph; |
There was a problem hiding this comment.
Does this field also have to be (de)serialized into JSON. If yes, then we also need to put it into the (de)serializer respectively.
There was a problem hiding this comment.
Oh yes, I just forgot about that 😓
|
🎉 All dependencies have been resolved ! |
f81c89f to
313ec2f
Compare
313ec2f to
1e1eede
Compare
This moves us towards getting rid of `ParsedDerivation` and just having `DerivationOptions`. Co-Authored-By: HaeNoe <git@haenoe.party>
Only a much smaller `StructuredAttrs` remains, the rest is is now moved to `DerivationOptions`. This gets us quite close to `std::optional<StructuredAttrs>` and `DerivationOptions` being included in `Derivation` as fields.
1e1eede to
d8be4f6
Compare
|
🎉 All dependencies have been resolved ! |
DerivationOptionsParsedDerivation for parts
|
For reference, I think this is the change that causes nix to newly reject |
|
@vcunat We can make it still work. We did another relaxation for compat already surrounding this. |
|
It's hard for me to guess how much this hits users. (including outside nixpkgs itself) |
Motivation
This is a follow up to #12292 doing more work cleaning up derivation options. Please review each commit separately.
The end goal is to have these fields on
Derivationitself:DerivationOptions optionsstd::variant<StructuredAttrs, StringMap> structuredAttrsOrEnvContext
Depends on #12292Depends on #13024Preparing the way for #10760, and the issues it addresses.
Progress on #9846
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.