Introduce DerivationOptions#12292
Conversation
3555ae5 to
6b6529b
Compare
roberth
left a comment
There was a problem hiding this comment.
I like the idea, but I haven't reviewed thoroughly for correctness
| * format), we have the option of instead storing the options | ||
| * separately. That would be nice to separate concerns, and not make any | ||
| * environment variable names magical. | ||
| */ |
There was a problem hiding this comment.
Is it meant to be a lossless representation of part of the derivation, or is it supposed to represent part of the derivation semantically uniquely?
There was a problem hiding this comment.
Ah yes it is supposed to reflect the semantics, not the syntax.
In other words, there will not be a DerivationOptions -> Env function; instead, the ATerm logic will (eventually):
- Check that the options are implied by the rest of the
Derivation - Since the check passed, safely ignore the (eventual new)
DerivationOptions options;field of the derivation as superfluous - Serialize as today
So yes let's do these changes.
| struct Checks | ||
| { | ||
| bool ignoreSelfRefs = false; | ||
| std::optional<uint64_t> maxSize, maxClosureSize; | ||
| std::optional<Strings> allowedReferences, allowedRequisites, disallowedReferences, disallowedRequisites; | ||
| }; | ||
|
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-01-20-nix-team-meeting-minutes-209/59119/1 |
6b6529b to
5e9e7d7
Compare
5e9e7d7 to
ba2b140
Compare
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 <git@haenoe.party>
ba2b140 to
917b8b2
Compare
roberth
left a comment
There was a problem hiding this comment.
Added some more thoughts. Nice work
| * format cannot change, but in alternatives to it (like the JSON | ||
| * format), we have the option of instead storing the options | ||
| * separately. That would be nice to separate concerns, and not make any | ||
| * environment variable names magical. |
There was a problem hiding this comment.
(no action required)
Another use case is that on darwin, we've run out of environment variable space at times in the haskell builders. Removing unnecessary envs helps a bit with those scenarios too.
| JSON_IMPL(DerivationOptions); | ||
| JSON_IMPL(DerivationOptions::OutputChecks) |
There was a problem hiding this comment.
JSON_IMPL only has declarations.
Tripped me up for a sec.
No action required in this PR.
| * not needed (attributes are not passed through the environment, so | ||
| * there is no size constraint). | ||
| */ | ||
| StringSet passAsFile; |
There was a problem hiding this comment.
This could perhaps be generalized to std::map<> from environment variable name to file contents (or file creation "commands"), where the ATerm parser is responsible for applying the current semantics.
Could be done in a follow-up.
Motivation
See the Doxygen for details.
Thanks to these changes, we are able to drastically restrict how the rest of the code-base uses
ParseDerivation.Context
This is a first step towards PR #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.
CC @haenoe, who co-authored this.