Store DerivationOption inside Derivation, put in JSON format#10760
Store DerivationOption inside Derivation, put in JSON format#10760haenoe wants to merge 1 commit intoNixOS:masterfrom
DerivationOption inside Derivation, put in JSON format#10760Conversation
src/libstore/derivations.hh
Outdated
| StringPairs env; | ||
| std::string name; | ||
|
|
||
| DerivationOptions options; |
There was a problem hiding this comment.
It seems that having this field in BasicDerivation "denormalizes" it, since information like the value of __impureEnvVars is now stored in both drv.env and drv.options.impureEnvVars, which could get out of sync and would therefore be error-prone to use. Wouldn't it be better to have methods like BasicDerivation::{get,set}ImpureEnvVars()?
There was a problem hiding this comment.
@edolstra the goal is, via the JSON format, to allow that stuff to not exist in the env. Then only the legacy decoding results looks like denormalization — but actually I prefer the perspective that there is no denormalization and rather the legacy format simply lacks the expressive power to set one without setting the other.
Does that make sense? It is some subtle philosophical perspective-shifting.
There was a problem hiding this comment.
Philisophical doesn't do it justice. It's anticipating a design improvement.
There was a problem hiding this comment.
Yeah philosophical before the new feature (better JSON format as input), practical with it.
Ericson2314
force-pushed
the
derivation-untangle
branch
from
2b0ffc1 to
5d08553
Compare
fricklerhandwerk
added
contributor-experience
haenoe
force-pushed
the
derivation-untangle
branch
from
b06017d to
5fab050
Compare
github-actions
bot
added
the
with-tests
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This is a first step towards PR NixOS#10760, and the issues it addresses. See the Doxygen for details. Thanks to these changes, we are able to drastically restrict how the rest of the code-base uses `ParseDerivation`. Co-Authored-By: HaeNoe <[email protected]>
This is a first step towards PR NixOS#10760, and the issues it addresses. See the Doxygen for details. Thanks to these changes, we are able to drastically restrict how the rest of the code-base uses `ParseDerivation`. Co-Authored-By: HaeNoe <[email protected]>
This is a first step towards PR NixOS#10760, and the issues it addresses. See the Doxygen for details. Thanks to these changes, we are able to drastically restrict how the rest of the code-base uses `ParseDerivation`. Co-Authored-By: HaeNoe <[email protected]>
This is a first step towards PR NixOS#10760, and the issues it addresses. See the Doxygen for details. Thanks to these changes, we are able to drastically restrict how the rest of the code-base uses `ParseDerivation`. Co-Authored-By: HaeNoe <[email protected]>
This is a first step towards PR NixOS#10760, and the issues it addresses. See the Doxygen for details. Thanks to these changes, we are able to drastically restrict how the rest of the code-base uses `ParseDerivation`. Co-Authored-By: HaeNoe <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
This is a first step towards PR NixOS#10760, and the issues it addresses. See the Doxygen for details. Thanks to these changes, we are able to drastically restrict how the rest of the code-base uses `ParseDerivation`. Co-Authored-By: HaeNoe <[email protected]>
This is a first step towards PR NixOS#10760, and the issues it addresses. See the Doxygen for details. Thanks to these changes, we are able to drastically restrict how the rest of the code-base uses `ParseDerivation`. Co-Authored-By: HaeNoe <[email protected]>
|
OK I rebased this, and it now depends on #13263. Some unit tests fail in a subtle way --- the rendered JSON is equal, but the value are not. Need to sit in GDB and debug those equality constraints. |
|
🎉 All dependencies have been resolved ! |
Ericson2314
changed the title
DerivationOption instead Derivation, put in JSON format
| "additionalSandboxProfile": "", | ||
| "allowLocalNetworking": false, | ||
| "allowSubstitutes": true, | ||
| "impureEnvVars": [], | ||
| "impureHostDeps": [], | ||
| "noChroot": false, | ||
| "outputChecks": { | ||
| "forAllOutputs": { | ||
| "allowedReferences": null, | ||
| "allowedRequisites": null, | ||
| "disallowedReferences": [], | ||
| "disallowedRequisites": [], | ||
| "ignoreSelfRefs": true | ||
| } | ||
| }, | ||
| "passAsFile": [], | ||
| "preferLocalBuild": false, | ||
| "requiredSystemFeatures": [], | ||
| "unsafeDiscardReferences": {} |
There was a problem hiding this comment.
So I would, contra our usual JSON policies, not want unused options in here. This is because I want to think of options as an "extensible record", where various Nix implementations are e.g. free to do their own custom options under their own prefix, and whenever an unknown option is encountered, as sensible warning is given.
Ericson2314
force-pushed
the
derivation-untangle
branch
2 times, most recently
from
7e04b86 to
f54ef07
Compare
Ericson2314
force-pushed
the
derivation-untangle
branch
from
f54ef07 to
ce6baa2
Compare
Ericson2314
force-pushed
the
derivation-untangle
branch
from
ce6baa2 to
b9d9f66
Compare
Ericson2314
force-pushed
the
derivation-untangle
branch
from
b9d9f66 to
dabd6f5
Compare
|
Note that the code touched in #14306 will need to be updated once again with this |
Ericson2314
force-pushed
the
derivation-untangle
branch
from
dabd6f5 to
27e1d6c
Compare
Ericson2314
force-pushed
the
derivation-untangle
branch
7 times, most recently
from
b0f0f0a to
c6c954c
Compare
Ericson2314
force-pushed
the
derivation-untangle
branch
from
c6c954c to
a9fdd53
Compare
Ericson2314
force-pushed
the
derivation-untangle
branch
from
a9fdd53 to
d3b338b
Compare
|
🎉 All dependencies have been resolved ! |
This opens the door to derivations that *directly* specify their options, rather than "stealing" environment variables to do so. The A-Term derivation format used on disk didn't change --- we cannot do that for existing derivations, so those derivations will continue to not support separate options. But having a more flexible JSON format opens the door to extending the on-disk format in others, like directly using JSON, using CBOR, etc. Co-authored-by: HaeNoe <[email protected]> Co-authored-by: Jonathan Gibbons <[email protected]>
Ericson2314
force-pushed
the
derivation-untangle
branch
from
d3b338b to
87c110e
Compare
Ericson2314
changed the title
DerivationOption instead Derivation, put in JSON formatDerivationOption inside Derivation, put in JSON format
6 participants
Motivation
This opens the door to derivations that directly specify their options, rather than "stealing" environment variables to do so.
The A-Term derivation format used on disk didn't change --- we cannot do that for existing derivations, so those derivations will continue to not support separate options. But having a more flexible JSON format opens the door to extending the on-disk format in others, like directly using JSON, using CBOR, etc.
Context
Fixes #9846
Progress on #9866 (regarding the "magic values"
envfield that are used as options)Depends on #13263
Depends on #14506
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.