Conversation
KiaraGrouwstra
requested review from
KiaraGrouwstra,
fricklerhandwerk and
roberth
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
Please forgive me for a superficial first review and coming up with proofreading stuff, but maybe there's already something useful in here.
Can't say particularly much about the broad design yet but I hope I've sprinkled a bit of useful info in there.
I'd like to do a more in depth review later.
nixos/modules/contracts/default.nix
Outdated
There was a problem hiding this comment.
Does this get loaded into NixOS itself (_class = "nixos";), or evalModules, or anywhere?
There was a problem hiding this comment.
I was assuming adding it to module-list.nix is enough?
nixos/modules/contracts/default.nix
Outdated
There was a problem hiding this comment.
Not sure yet what "create an option" means here. Probably not declare an option?
There was a problem hiding this comment.
Indeed create is vague, declare seems more appropriate: one would declare an option with the type config.contracts.<name>.consumer since that is of type optionType. Do you see another word?
There was a problem hiding this comment.
thought: Neither of the backup contracts covers any scheduling of backups or integration with file system snapshots. Maybe that's ok for now, or should be covered by something else anyway.
There was a problem hiding this comment.
Contracts are for communication between the consumer and provider, without the end user intervention. I see the file backup contract here only useful for the consumer to tell what files should be backed up or not.
On the other hand, I see scheduling as the end user communicating with the provider directly. I could see a need for a common interface for scheduling though. So maybe a separate contract for that?
Now I could be wrong and there is a valid use case for the consumer having a say in the scheduling, I just did not cross it yet.
There was a problem hiding this comment.
Correct, the module attrset must not be strict in (require evaluation of) config or even options.
Maybe imports could be useful here? Not sure I see "a hacky import".
There was a problem hiding this comment.
I just assumed using import this way was more hacky or at least less favorable than relying on config.
ibizaman
force-pushed
the
contracts-rfc
branch
5 times, most recently
from
19d98a5 to
806da47
Compare
| ./config/xdg/sounds.nix | ||
| ./config/xdg/terminal-exec.nix | ||
| ./config/zram.nix | ||
| ./contracts/default.nix |
There was a problem hiding this comment.
for the record, despite this line the error i get building the manual ((cd nixos/; nix-build release.nix -A manual.x86_64-linux)) is:
error:
… while calling the 'deepSeq' builtin
at /home/kiara/code/nixpkgs/lib/customisation.nix:478:35:
477| in
478| if drv == null then null else deepSeq drv' drv';
| ^
479|
… while evaluating the attribute 'outPath'
at /home/kiara/code/nixpkgs/lib/customisation.nix:467:13:
466| value = commonAttrs // {
467| outPath = output.outPath;
| ^
468| drvPath = output.drvPath;
(stack trace truncated; use '--show-trace' to show the full trace)
error: attribute 'contracts' missing
at /home/kiara/code/nixpkgs/nixos/modules/services/web-apps/stash.nix:435:16:
434| jwtSecretKeyFile = mkOption {
435| type = config.contracts.secret.consumer;
| ^
436| description = "Path to file containing a secret used to sign JWT tokens.";
not sure how this happened.
There was a problem hiding this comment.
Indeed I get the same. I guess only the options attribute is populated at this point, not the config?
There was a problem hiding this comment.
hm, config seems introspected at plenty other tests tho?
There was a problem hiding this comment.
Not in the type field as far as I grepped.
There was a problem hiding this comment.
that explanation seems to add up, given after commenting those lines the error seems to change:
details
~/code/nixpkgs> nix-build nixos/release.nix -A manual.x86_64-linux
these 3 derivations will be built:
/nix/store/vinkj8blr6mrsy2k5z8lk0n6i11l3vjj-lazy-options.json.drv
/nix/store/qzykfx31izdzq9c2inyvpm48xxm07x1n-options.json.drv
/nix/store/29xq1kdbafpar7v70zwag14l73cigwa8-nixos-manual-html.drv
these 12 paths will be fetched (14.00 MiB download, 49.57 MiB unpacked):
/nix/store/a8s3qf5ydqbpqcshg1dgga9lag3xgbbp-brotli-1.1.0
/nix/store/lnab2vg7mcwddh63rimw1vh82nplzsxj-brotli-1.1.0-dev
/nix/store/7bcdsr7fykhji6dy8gq8jzd8qjswf13v-documentation-highlighter
/nix/store/my02p81al90i4anmaz79ad98a397zrri-nixos-render-docs-0.0
/nix/store/8d4pij8ir718s90cj8yrhhrxd5pc81bw-nixos-test-driver-docstrings
/nix/store/fgx655d67ry55jw7dddmb49avywqkvyb-options.json
/nix/store/lk0mhvqjffjx087p8wlwf71agdkz71ha-options.json
/nix/store/xiw440bqysi2p7dr375nca54in97b0vi-options.json
/nix/store/58dmrdsib24m3468d9n5s7g9g2gcgqmg-python3.13-markdown-it-py-3.0.0
/nix/store/kiyxdnmd5j4b5i124imq8jcgh4zgcrwq-python3.13-mdit-py-plugins-0.4.2
/nix/store/aa0shvk4589im4w2wsq5qxiws381q9k8-python3.13-mdurl-0.1.2
/nix/store/7i6x1w92r7njk1fvmq2fwhz7fjsd26pa-source
copying path '/nix/store/7bcdsr7fykhji6dy8gq8jzd8qjswf13v-documentation-highlighter' from 'https://cache.nixos.org'...
copying path '/nix/store/8d4pij8ir718s90cj8yrhhrxd5pc81bw-nixos-test-driver-docstrings' from 'https://cache.nixos.org'...
copying path '/nix/store/fgx655d67ry55jw7dddmb49avywqkvyb-options.json' from 'https://cache.nixos.org'...
copying path '/nix/store/lk0mhvqjffjx087p8wlwf71agdkz71ha-options.json' from 'https://cache.nixos.org'...
copying path '/nix/store/xiw440bqysi2p7dr375nca54in97b0vi-options.json' from 'https://cache.nixos.org'...
copying path '/nix/store/7i6x1w92r7njk1fvmq2fwhz7fjsd26pa-source' from 'https://cache.nixos.org'...
copying path '/nix/store/aa0shvk4589im4w2wsq5qxiws381q9k8-python3.13-mdurl-0.1.2' from 'https://cache.nixos.org'...
copying path '/nix/store/a8s3qf5ydqbpqcshg1dgga9lag3xgbbp-brotli-1.1.0' from 'https://cache.nixos.org'...
building '/nix/store/vinkj8blr6mrsy2k5z8lk0n6i11l3vjj-lazy-options.json.drv'...
copying path '/nix/store/58dmrdsib24m3468d9n5s7g9g2gcgqmg-python3.13-markdown-it-py-3.0.0' from 'https://cache.nixos.org'...
copying path '/nix/store/lnab2vg7mcwddh63rimw1vh82nplzsxj-brotli-1.1.0-dev' from 'https://cache.nixos.org'...
copying path '/nix/store/kiyxdnmd5j4b5i124imq8jcgh4zgcrwq-python3.13-mdit-py-plugins-0.4.2' from 'https://cache.nixos.org'...
error:
… from call site
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/eval-cacheable-options.nix:1:1:
1| {
| ^
2| libPath,
… while calling anonymous lambda
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/eval-cacheable-options.nix:1:1:
1| {
| ^
2| libPath,
… while evaluating the attribute 'optionsNix'
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/doc/manual/default.nix:158:36:
157| rec {
158| inherit (optionsDoc) optionsJSON optionsNix optionsDocBook;
| ^
159|
… while evaluating the attribute 'optionsNix'
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/make-options-doc/default.nix:180:11:
179| rec {
180| inherit optionsNix;
| ^
181|
… while calling the 'listToAttrs' builtin
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/make-options-doc/default.nix:167:16:
166|
167| optionsNix = builtins.listToAttrs (
| ^
168| map (o: {
… while calling the 'map' builtin
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/make-options-doc/default.nix:168:5:
167| optionsNix = builtins.listToAttrs (
168| map (o: {
| ^
169| name = o.name;
… from call site
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/make-options-doc/default.nix:120:17:
119| filteredOpts = lib.filter (opt: opt.visible && !opt.internal) transformedOpts;
120| optionsList = lib.flip map filteredOpts (
| ^
121| opt:
… while calling 'flip'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/trivial.nix:306:11:
305| flip =
306| f: a: b:
| ^
307| f b a;
… while calling the 'map' builtin
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/trivial.nix:307:5:
306| f: a: b:
307| f b a;
| ^
308|
… while calling the 'filter' builtin
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/make-options-doc/default.nix:119:18:
118| transformedOpts = map transformOptions rawOpts;
119| filteredOpts = lib.filter (opt: opt.visible && !opt.internal) transformedOpts;
| ^
120| optionsList = lib.flip map filteredOpts (
… while calling the 'map' builtin
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/make-options-doc/default.nix:118:21:
117| rawOpts = lib.optionAttrSetToDocList options;
118| transformedOpts = map transformOptions rawOpts;
| ^
119| filteredOpts = lib.filter (opt: opt.visible && !opt.internal) transformedOpts;
… from call site
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/lib/make-options-doc/default.nix:117:13:
116| let
117| rawOpts = lib.optionAttrSetToDocList options;
| ^
118| transformedOpts = map transformOptions rawOpts;
… while calling 'optionAttrSetToDocList''
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/options.nix:570:8:
569| optionAttrSetToDocList' =
570| _: options:
| ^
571| concatMap (
… while calling the 'concatMap' builtin
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/options.nix:571:5:
570| _: options:
571| concatMap (
| ^
572| opt:
… from call site
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/options.nix:609:8:
608| [ docOption ] ++ optionals subOptionsVisible subOptions
609| ) (collect isOption options);
| ^
610|
… while calling 'collect'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:866:11:
865| collect =
866| pred: attrs:
| ^
867| if pred attrs then
… while calling the 'concatMap' builtin
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:870:7:
869| else if isAttrs attrs then
870| concatMap (collect pred) (attrValues attrs)
| ^
871| else
… while calling 'collect'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:866:11:
865| collect =
866| pred: attrs:
| ^
867| if pred attrs then
… while calling the 'concatMap' builtin
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:870:7:
869| else if isAttrs attrs then
870| concatMap (collect pred) (attrValues attrs)
| ^
871| else
… while calling 'collect'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:866:11:
865| collect =
866| pred: attrs:
| ^
867| if pred attrs then
… while calling the 'concatMap' builtin
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:870:7:
869| else if isAttrs attrs then
870| concatMap (collect pred) (attrValues attrs)
| ^
871| else
… while calling 'collect'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:866:11:
865| collect =
866| pred: attrs:
| ^
867| if pred attrs then
… while evaluating a branch condition
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:867:5:
866| pred: attrs:
867| if pred attrs then
| ^
868| [ attrs ]
… from call site
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/attrsets.nix:867:8:
866| pred: attrs:
867| if pred attrs then
| ^
868| [ attrs ]
… while calling 'isType'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/types.nix:103:20:
102| outer_types = rec {
103| isType = type: x: (x._type or "") == type;
| ^
104|
… from call site
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/types.nix:103:24:
102| outer_types = rec {
103| isType = type: x: (x._type or "") == type;
| ^
104|
… while calling anonymous lambda
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:854:37:
853|
854| matchedOptions = mapAttrs (n: v: v.matchedOptions) resultsByName;
| ^
855|
… while evaluating the attribute 'matchedOptions'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:820:13:
819| {
820| matchedOptions = evalOptionValue loc opt defns';
| ^
821| unmatchedDefns = [ ];
… from call site
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:820:30:
819| {
820| matchedOptions = evalOptionValue loc opt defns';
| ^
821| unmatchedDefns = [ ];
… while calling 'evalOptionValue'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:1052:15:
1051| evalOptionValue =
1052| loc: opt: defs:
| ^
1053| let
… in the left operand of the update (//) operator
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:1090:5:
1089| warnDeprecation opt
1090| // {
| ^
1091| value = addErrorContext "while evaluating the option `${showOption loc}':" value;
… from call site
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:1085:9:
1084| warnDeprecation =
1085| warnIf (opt.type.deprecationMessage != null)
| ^
1086| "The type `types.${opt.type.name}' of option `${showOption loc}' defined in ${showFiles opt.declarations} is deprecated. ${opt.type.deprecationMessage}";
… while calling 'warnIf'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/trivial.nix:825:18:
824| */
825| warnIf = cond: msg: if cond then warn msg else x: x;
| ^
826|
… while evaluating a branch condition
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/trivial.nix:825:23:
824| */
825| warnIf = cond: msg: if cond then warn msg else x: x;
| ^
826|
… from call site
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:817:19:
816| let
817| opt = fixupOptionType loc (mergeOptionDecls loc decls);
| ^
818| in
… while calling 'fixupOptionType'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:1296:10:
1295| fixupOptionType =
1296| loc: opt:
| ^
1297| if opt.type.getSubModules or null == null then
… while evaluating a branch condition
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:1297:5:
1296| loc: opt:
1297| if opt.type.getSubModules or null == null then
| ^
1298| opt // { type = opt.type or types.unspecified; }
… from call site
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:817:40:
816| let
817| opt = fixupOptionType loc (mergeOptionDecls loc decls);
| ^
818| in
… while calling 'mergeOptionDecls'
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:940:10:
939| mergeOptionDecls =
940| loc: opts:
| ^
941| foldl'
… while calling the 'foldl'' builtin
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:941:5:
940| loc: opts:
941| foldl'
| ^
942| (
… while calling anonymous lambda
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:943:14:
942| (
943| res: opt:
| ^
944| let
… in the right operand of the update (//) operator
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:1001:11:
1000| opt.options
1001| // res
| ^
1002| // {
… in the right operand of the update (//) operator
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:1002:11:
1001| // res
1002| // {
| ^
1003| declarations = res.declarations ++ [ opt._file ];
… in the right operand of the update (//) operator
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:1023:11:
1022| }
1023| // typeSet
| ^
1024| )
… while evaluating a branch condition
at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:978:20:
977| "The option `${showOption loc}' in `${opt._file}' is already declared in ${showFiles res.declarations}."
978| else if opt.options.type ? functor.wrappedDeprecationMessage then
| ^
979| { type = addDeprecatedWrapped opt.options.type; }
… while evaluating the attribute 'options.type'
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/modules/services/web-apps/nextcloud.nix:1024:7:
1023| fileBackup = lib.mkOption {
1024| type = config.contracts.fileBackup.consumer;
| ^
1025| };
error: attribute 'contracts' missing
at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/modules/services/web-apps/nextcloud.nix:1024:14:
1023| fileBackup = lib.mkOption {
1024| type = config.contracts.fileBackup.consumer;
| ^
1025| };
Cacheable portion of option doc build failed.
Usually this means that an option attribute that ends up in documentation (eg `default` or `description`) depends on the restricted module arguments `config` or `pkgs`.
Rebuild your configuration with `--show-trace` to find the offending location. Remove the references to restricted arguments (eg by escaping their antiquotations or adding a `defaultText`) or disable the sandboxed build for the failing module by setting `meta.buildDocsInSandbox = false`.
error: builder for '/nix/store/vinkj8blr6mrsy2k5z8lk0n6i11l3vjj-lazy-options.json.drv' failed with exit code 1;
last 25 log lines:
> … while evaluating a branch condition
> at /nix/store/xq9gm26vql7v1d7alaq4q20fvr5rgaw7-lib/modules.nix:978:20:
> 977| "The option `${showOption loc}' in `${opt._file}' is already declared in ${showFiles res.declarations}."
> 978| else if opt.options.type ? functor.wrappedDeprecationMessage then
> | ^
> 979| { type = addDeprecatedWrapped opt.options.type; }
>
> … while evaluating the attribute 'options.type'
> at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/modules/services/web-apps/nextcloud.nix:1024:7:
> 1023| fileBackup = lib.mkOption {
> 1024| type = config.contracts.fileBackup.consumer;
> | ^
> 1025| };
>
> error: attribute 'contracts' missing
> at /nix/store/349j6vlcdmf2a2xv0vz4mq7yb6wix7ql-nixos/modules/services/web-apps/nextcloud.nix:1024:14:
> 1023| fileBackup = lib.mkOption {
> 1024| type = config.contracts.fileBackup.consumer;
> | ^
> 1025| };
> Cacheable portion of option doc build failed.
> Usually this means that an option attribute that ends up in documentation (eg `default` or `description`) depends on the restricted module arguments `config` or `pkgs`.
>
> Rebuild your configuration with `--show-trace` to find the offending location. Remove the references to restricted arguments (eg by escaping their antiquotations or adding a `defaultText`) or disable the sandboxed build for the failing module by setting `meta.buildDocsInSandbox = false`.
>
For full logs, run 'nix log /nix/store/vinkj8blr6mrsy2k5z8lk0n6i11l3vjj-lazy-options.json.drv'.
error: 1 dependencies of derivation '/nix/store/qzykfx31izdzq9c2inyvpm48xxm07x1n-options.json.drv' failed to build
error: 1 dependencies of derivation '/nix/store/29xq1kdbafpar7v70zwag14l73cigwa8-nixos-manual-html.drv' failed to build
There was a problem hiding this comment.
Also the usual escape hatch of adding defaultText does not work here. There should be an equivalent defaultType 😅
There was a problem hiding this comment.
Btw I’m not opposed at all to try to fix something in the eval modules code but I’d love some guidance as of at least is it the correct thing to do. That’s also why I published the RFC as-is, I just didn’t know what to do.
Same for the other issues like the one where the documentation does not build.
There was a problem hiding this comment.
Normally config is available, but NixOS also uses split evaluation for docs, making that... complicated.
Maybe some meta.buildInSandbox = false can help?
nixpkgs/nixos/modules/misc/meta.nix
Lines 69 to 82 in 160ee19
nixpkgs-ci
bot
added
the
2.status: merge conflict
| passwordFile = toString (pkgs.writeText "password" "password"); | ||
| initialize = true; | ||
|
|
||
| fileBackup.consumer = config.services.nextcloud.fileBackup; |
There was a problem hiding this comment.
on this dual link, my hunch would be to maybe instead have nixos/modules/services/web-apps/nextcloud.nix add config.contracts.fileBackup.consumer.provider.consumer = cfg.fileBackup;.
this yielded an infinite recursion tho, that seems to say this would make the consumer's type depend on the value of the consumer itself.
… while evaluating the option `nodes.nextcloud.contracts.fileBackup.consumer':
138| default = provider.config.consumer.input or null;
| ^
at /home/kiara/code/nixpkgs/nixos/modules/contracts/default.nix:132:45:
132| type = lib.types.nullOr interface.config.consumer;
| ^
… while evaluating the option `nodes.nextcloud.contracts.fileBackup.consumer':
at /home/kiara/code/nixpkgs/nixos/modules/services/web-apps/nextcloud.nix:1567:57:
1567| contracts.fileBackup.consumer.provider.consumer = cfg.fileBackup;
| ^
… while evaluating the attribute 'options.type'
at /home/kiara/code/nixpkgs/nixos/modules/services/web-apps/nextcloud.nix:1024:7:
1024| type = config.contracts.fileBackup.consumer;
| ^
...
error: infinite recursion encountered
making it `config.contracts.fileBackup.provider.consumer = cfg.fileBackup;` instead similarly yielded an infinite recursion, that i think says the provider's type would depend on the provider's own value.
at /home/kiara/code/nixpkgs/nixos/modules/services/backup/restic.nix:307:39:
307| type = lib.types.nullOr config.contracts.fileBackup.provider;
| ^
… while evaluating the option `nodes.nextcloud.contracts.fileBackup.provider':
at /home/kiara/code/nixpkgs/nixos/modules/contracts/default.nix:98:31:
98| default = consumer.config.provider.output;
| ^
at /home/kiara/code/nixpkgs/nixos/modules/contracts/default.nix:90:28:
90| type = interface.config.provider;
| ^
so hm, looks like i've mostly reproduced your infinite recursion issues so far.
There was a problem hiding this comment.
Unfortunately, this has two downsides:
- It requires one more line in each provider definition. This would be okay if there wasn't the following downside.
- There's no way to write side effects. This means the provider can only write to its own output, which misses the whole point of having contracts in the first place.
so, @fricklerhandwerk was right on needing to type effects, huh.
There was a problem hiding this comment.
to be fair, the primary effect relevant here so far (back-ups, secrets, SSL) would seem to be 'in addition to having input/output, will write to these file paths'?
like i'm not sure that'd already cover further contracts such as reverse proxying, but maybe it'd help scope things for the example in this PR.
There was a problem hiding this comment.
will write to these file paths
It's more general, it's setting some option anywhere in the config tree.
There was a problem hiding this comment.
i just realized module-interfaces didn't actually define an interfaces.*.provider.consumer option, so i guess that was how they got away without needing dual linking.
in this implementation that's contracts.*.provider.consumer, so for the case of nextcloud's restic file back-ups, my (initial) infinite recursion (that is, using config.contracts.fileBackup.consumer.provider.consumer = cfg.fileBackup;) seems to have gotten tripped up over its use in typing the consumer (here: nextcloud)'s fileBackup, i.e. its mentioned line:
at nixpkgs/nixos/modules/services/web-apps/nextcloud.nix:1024:7:
1024| type = config.contracts.fileBackup.consumer;
| ^
... which does then actually seem to have involved a value getting propagated up so as to type its parent, causing the mentioned recursion.
my next step would be to actually think if there's a way to address that (or if we're at the limit of what's technically / logically possible).
There was a problem hiding this comment.
the provider.consumer attribute seems used to propagate input from the consumer to the provider.
module-interfaces had instead had the provider as a function of input, but over here that approach left questions on how to generate configuration outside of the interface yet contingent on input, as alluded to by @ibizaman:
It's more general, it's setting some option anywhere in the config tree.
on ideas for mitigation to the mentioned .consumer issues, i thought of:
- juggling the types to prevent flowing upward
- narrow down / filter the links to just the bits needed to prevent overlap
- type not the whole but the needed part to prevent overlap
- getting rid of
.consumerto get closer tomodule-interfacesagain, given that was able to do without the dual link. - move up
mkIfto include what I had moved toother? - possibly namespaced such as to make it feel less concerned about recursion.
i've now made an attempt to explore that second route (arbitrarily trying to route further options thru an other attribute yielded from the provider function distinct from its output). that ran into infinite recursion errors as well tho - perhaps someone better-versed in the module system might be able to shed light on whether that could be remedied.
the idea of taking stuff through other otherwise seems to work.
There was a problem hiding this comment.
fyi, detnix just saw a fix merged related to infinite recursion, tho it's not clear to me if that has relevance to upstream nix as well. (the code there and the upstream code look somewhat different to me, so not sure it applies.)
edit: this does not seem a magical fix
There was a problem hiding this comment.
trying more:
`config = config.services.nextcloud.fileBackup.other;` makes `config` depend on itself
… while evaluating the attribute 'drvPath'
at /home/kiara/code/nixpkgs/lib/customisation.nix:418:7:
417| // {
418| drvPath =
| ^
419| assert condition;
...
… from call site
at /home/kiara/code/nixpkgs/nixos/modules/services/web-apps/nextcloud.nix:1024:5:
1023| # cfg.fileBackup.other
1024| config.services.nextcloud.fileBackup.other
| ^
1025|
… while evaluating the module argument `config' in "/home/kiara/code/nixpkgs/nixos/modules/services/web-apps/nextcloud.nix":
… while evaluating the attribute 'config'
at /home/kiara/code/nixpkgs/lib/modules.nix:257:21:
256| options
257| config
| ^
258| specialArgs
error: infinite recursion encountered
at /home/kiara/code/nixpkgs/lib/derivations.nix:159:9:
158| outputName
159| drvPath
| ^
160| name
if i split it out so .other is extracted in a qualified manner (rather than directly into config), i instead get:
provider `services.nextcloud.fileBackup.provider`'s `.other` thru the consumer depends on the provider which has `other`... infinite recursion
… from call site
at /home/kiara/code/nixpkgs/nixos/modules/services/web-apps/nextcloud.nix:1591:44:
1590| # services.restic = mkIf cfg.enable cfg.fileBackup.other.services.restic;
1591| services.restic = if cfg.enable then cfg.fileBackup.other.services.restic else { };
| ^
1592| }
...
… from call site
at /home/kiara/code/nixpkgs/nixos/modules/contracts/default.nix:116:30:
115| # default = consumer.config.provider.other;
116| default = (consumer.config.provider consumer.config.input).other;
| ^
117| };
...
… while evaluating the option `nodes.nextcloud.services.nextcloud.fileBackup.provider':
...
… from call site
at /home/kiara/code/nixpkgs/nixos/tests/nextcloud/default.nix:70:37:
69| };
70| fileBackup.provider = config.services.restic.backups.nextcloud.fileBackup;
| ^
71| };
...
… while evaluating the option `nodes.nextcloud.services.restic.backups':
(8 duplicate frames omitted)
… while calling the 'zipAttrsWith' builtin
at /home/kiara/code/nixpkgs/lib/modules.nix:765:30:
764| # extract the definitions for each loc
765| rawDefinitionsByName = zipAttrsWith (n: v: v) (
| ^
766| map (
… while calling the 'map' builtin
at /home/kiara/code/nixpkgs/lib/modules.nix:766:9:
765| rawDefinitionsByName = zipAttrsWith (n: v: v) (
766| map (
| ^
767| module:
… in the condition of the assert statement
at /home/kiara/code/nixpkgs/lib/modules.nix:733:9:
732| checkedConfigs =
733| assert all (
| ^
734| c:
… while calling the 'all' builtin
at /home/kiara/code/nixpkgs/lib/modules.nix:733:16:
732| checkedConfigs =
733| assert all (
| ^
734| c:
error: infinite recursion encountered
narrowing the types down further seemed to not help there either.
i'm starting less confident this is easy / possible. so maybe i should stop pretending i could figure this out.
|
Let me just second @roberth that this should absolutely leverage Modular Services (#428084). Modular services removes us from "singleton" hell making the interface / instance distinction much clearer. (In contrast, if you only have a single "PostgreSQL", for example, it is much harder to learn the difference between "the PostgresSQL module interface" (contract) an "a PostgresSQL instance", because all there is the "the PostgresSQL instance".) |
nyabinary
added
the
1.severity: significant
How would this work even, wouldn't this change a lot of the RFC as its implemented right now? |
|
@Ericson2314 That's a good remark but TBH I'm not sure it should apply here. I don't see how having multiple instances of the same contract would be something we want. I feel (it's really a feeling) that we would actually want a singleton behavior for contracts. I suppose your worry is about having multiple similar instances of a same contract? A reverse proxy or secret contract would have different names, I assume we would agree on that, and thus the modular service implementation would apply for variations of a same contract, or maybe multiple versions. I still got the feeling in this case we would want to name the contracts with unique names, like |
| services.stash.passwordFile.input = mkIf (cfg.passwordFile != null) { | ||
| owner = cfg.user; | ||
| group = cfg.group; | ||
| mode = "0400"; | ||
| }; |
There was a problem hiding this comment.
I think services.stash.passwordFile.input = mkIf (cfg.passwordFile != null) .. would never have the intended consequence of setting services.stash.passwordFile.input only if services.stash.passwordFile.provider is set.
This very definition mandates cfg.passwordFile != null since it includes at least { input = «thunk»; }. I think I managed to illustrate that with the following reproducer:
❯ nix-instantiate --json --eval --expr '
let inherit (import <nixpkgs> {}) lib; in
(lib.evalModules {
modules = [
({ config, ... }:{
options.foo = lib.mkOption {
type = lib.types.nullOr (lib.types.submodule ({
options.bar = lib.mkOption {
type = lib.types.str;
};
}));
default = null;
};
config.foo.bar = lib.mkIf (lib.traceVal config.foo != null) "qux";
})
];
}).config.foo.bar
'
trace: { bar = «thunk»; }
"qux"Observe that config.foo.bar evaluates to "qux" even though that is supposedly only meant to happen if foo is actually defined.
Am I on to something here?
There was a problem hiding this comment.
so basically, the config.foo != null check here is nonsensical in that the assignment config.foo.bar = lib.mkIf ..., being syntactic sugar for config.foo = { bar = lib.mkIf ...; }, already invalidates the check, rendering it always true?
(i haven't so much considered the author's intent to think of fixes yet.)
There was a problem hiding this comment.
i wondered if the check could instead look at other clues like the user-specified path, tho i imagine that might give weird recursion.
alternatively, if we cannot conditionally initialize these, we could instead consider the impact of this, i.e. whether external null-checks could be re-written to verify initialization of the secret (e.g. do we have output).
|
i feel like the dual link may be a necessary evil. |
|
I'm still grasping at straws a bit here, but if the design is stuck in a local optimum with the dual linking, maybe it's worth exploring potential larger changes to the design? Maybe the contract instances could be more prominent, reifying them as "Writing to all instances" is a recipe for infinite recursion if you try with Then, somewhat separately, the provider service can generate its own configurations off of the actual instance values. Since that approach largely automates the provider part from a user perspective, that seems like it might get rid of the dual link problem, but I can't really tell if I've overlooked something else at this point. |
|
I've also given the whole subject a thought, and concluded the whole thing could be a lot simpler, very much along the lines of what @roberth proposed. What's really to standardise is each namespace and type of interface, not the mechanism. Because the mechanism is actually just global module options, and what we haven't formally settled is whether and how exactly certain behaviors should be communicated via e.g. So maybe this is not required to be an RFC at all, and we could just start with a concrete use case, such as @Lassulus did with secrets? |
|
There may well be value in forcefully standardizing the solution by means of generic logic when it's shown to work well. Regarding my suggestion, Un-nesting it might be nice, but I don't know if that's actually feasible without something like
(So don't do that or save it for a refactor when everything else works) |
|
i made an attempt at a simple hard-coded variant of @roberth's suggestion edit: got it to work now |
|
I agree the intent is superior to the mechanism itself. A working mechanism that I'm using in my project is based on functions that create option types.
It's not super pretty because as you can see I needed to set defaults and defaultTexts everywhere to be able to be build the documentation. But at least it works! And also, I tend to think the UX is more important than the messiness of the code. It seems we're moving towards making this just work and refining later. I like that. I like indeed Concerning the dual linking, I've come to terms with it. At least, the other experience I have with it is using sops-nix with for example, this alone won't work: {
myoption = config.sops.secrets.mysecret.path;
}You need to define the secret, even with just an empty attrset: {
sops.secrets.mysecret = {};
} |
|
let's get branches up and see what sticks, yeah :) (for that purpose, not having the |
|
i fixed the module part now - |
|
okay, i think i can explain what i did now.
while that might not amount to a proper solution (and in fact, i hadn't really solidified a new provider structure or contracting flow), it may be of interest to take these tricks into account to use when appropriate.
|
|
by the way, while maybe getting reviews has gotten harder in general, once we're comfortable settling for our answers to general technical questions (e.g. bulk configuring, cross-node use, common denominator questions, contract interactions/hierarchies), we could split the PR up into individual contracts (with their corresponding providers/consumers/testing) - hopefully that would help reduce the cognitive load on reviewers so we could explicitly invite those with more experience in such specific fields to help with feedback on the UX of individual contracts.
could you maybe comment a bit on design considerations in how you designed the interface at selfhostblocks vis-a-vis the implementation in this PR so far, so we understand better the direction you would prefer here? |
|
on selfhostblocks vs PR implementations, i see the PR uses the module system, whereas selfhostblocks looks implemented using plain nix. on the aforementioned common denominator / features question, i think we could work from a concrete example.
now let's say we have a provider requiring at least the former, so would also have enough info using a request taking the enhanced form. conversely, if the provider wanted at least the enhanced form, could we get our contracts to reject this (despite for docs purposes the selfhostblocks code specifying dummy defaults everywhere)? if we can tackle such type variance, then i imagine features/extensibility should work out. |
Although I agree with you in principle, I don’t know how much we would need to extend an existing contract. Because this difference is just to generate a contract, after that step it’s back in the module system anyway.
There are two different aspects here. For default value specifically, it’s because I use values coming from Concerning needing to thread the
Without actually testing it I’d say yes. We should just need to make the provider a freeform submodule which ignores extraneous arguments.
Similarly without testing, if the provider requests the extended form, the module system would error out saying an option was not defined. It wouldn’t be a great error message though. (btw I’m working on rebasing this PR then on the PR to use the system from SHB, to compare apples to apples) |
if we can get branches side by side for such a comparison, that sounds great ✌️
cool, that's sounding somewhat future-proof.
hm interesting. the default values feel a bit awkward for me still, in the sense it would sound like they'd affect the meaning (setting defaults if we wanted for an option to be mandatory)? i feel like i'm misunderstanding, not quite sure what to think there still. maybe the core of my confusion is: if nixpkgs has had option documentation at nixos-search, does code for documenting contracts look different from nixpkgs' modules, and if so, do we know what would cause such difference in documentation demand there, if not use of a distinct documentation library?
right, that makes sense. (i think with config we also have the |
|
for expanded context over the present PR, here's my sense of what stuff in selfhostblocks'
my take-away from this is that from what we have so far, i think areas not requiring the round-trip perhaps could already get their own PRs to e.g. add in while that would render what's left here both more homogeneous while also maybe leaving less sample implementations here so far, for reviewing purposes such scoping could be a step forward: this could perhaps prevent trip-ups such as my (if not also @fricklerhandwerk's) alternative implementation attempts that turned out not to generalize to the canonical use-case here. (documentation/testing might get a bit easier too for types, given their Footnotes |
ibizaman
force-pushed
the
contracts-rfc
branch
from
806da47 to
396329a
Compare
- build manual: `(cd nixos/; nix-build release.nix -A manual.x86_64-linux)`
nixpkgs-ci
bot
removed
the
2.status: merge conflict
This commit makes the manual impossible to build. I understand the reason but do not know how to fix it.
Tests succeed: - nix-build -A nixosTests.contracts-fileBackup-restic - nix-build -A nixosTests.restic
Tests succeed: - nix-build -A nixosTests.contracts-fileBackup-restic - nix-build -A nixosTests.contracts-streamingBackup-restic - nix-build -A nixosTests.restic
ibizaman
force-pushed
the
contracts-rfc
branch
from
396329a to
45ecb73
Compare
|
I just rebased on latest master and applied the treefmt linter. And also updated the restic streaming backup implementation by using the new command restic option. Nothing significant but it’s cool to see that new option. |
|
FYI I created a new draft PR with an alternative implementation which is IMO better than the first. It has less weird quirks at least. I added a comparison in the description of the PR. #485453 |
7 participants
The following draft PR is probably better than this one. I added a comparison in the description of the PR. #485453
Draft PR showing implementation of the Contracts RFC-189 as well as 3 contracts example. From this PR, only the
nixos/modules/contracts/default.nixwould be merged as part of the RFC. The rest would happen later.It is intended to be reviewed after reading the RFC document and also commit by commit.
(cd nixos/; nix-build release.nix -A manual.x86_64-linux)(does not work yet)nix-build -A nixosTests.contracts-fileBackup-resticnix-build -A nixosTests.contracts-secret-hardcodedSecretnix-build -A nixosTests.contracts-streamingBackup-resticnix-build -A nixosTests.resticnix-build -A nixosTests.stashOpen questions:
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.