lib/modules: improve errors for options/config-mixups#131205
lib/modules: improve errors for options/config-mixups#131205lheckemann merged 6 commits intoNixOS:masterfrom
options/config-mixups#131205Conversation
Ma27
changed the title
options/config-mixups
ofborg
bot
added
10.rebuild-darwin: 0
lheckemann
left a comment
lheckemann
left a comment
There was a problem hiding this comment.
I like it! Some suggestions here.
|
I already tried to add this in both #98477 and #98761, but @Profpatsch didn't want it |
My main problem is that it's relatively easy to (accidentally) write a configuration which results in a I don't have much of a problem to implement |
|
@Ma27 This isn't what @Profpatsch has been arguing in above threads. He argues that things like recursion limiting should be a separate function that first transforms the input, then passes it to |
|
Whoops, sorry, thanks for pointing this out! I guess we have (more or less) the same opinion on this, so I'd say, let's wait for @Profpatsch to chime in and see if we can agree on something mergeable :) |
|
@infinisil @Profpatsch any updates here regarding this discussion? :) |
|
I’m pretty sure this should be a separate function indeed. nix is lazy for a reason, we can separate these things logically and then compose them together without incurring extra costs. |
|
OK, thanks for the feedback :) |
When having e.g. recursive attr-set, it cannot be printed which is
solved by Nix itself like this:
$ nix-instantiate --eval -E 'let a.b = 1; a.c = a; in builtins.trace a 1'
trace: { b = 1; c = <CYCLE>; }
1
However, `generators.toPretty` tries to evaluate something until it's
done which can result in a spurious `stack-overflow`-error:
$ nix-instantiate --eval -E 'with import <nixpkgs/lib>; generators.toPretty { } (mkOption { type = types.str; })'
error: stack overflow (possible infinite recursion)
Those attr-sets are in fact rather common, one example is shown above, a
`types.<type>`-declaration is such an example. By adding an optional
`depthLimit`-argument, `toPretty` will stop evaluating as soon as the
limit is reached:
$ nix-instantiate --eval -E 'with import ./Projects/nixpkgs-update-int/lib; generators.toPretty { depthLimit = 2; } (mkOption { type = types.str; })' |xargs -0 echo -e
"{
_type = \"option\";
type = {
_type = \"option-type\";
check = <function>;
deprecationMessage = null;
description = \"string\";
emptyValue = { };
functor = {
binOp = <unevaluated>;
name = <unevaluated>;
payload = <unevaluated>;
type = <unevaluated>;
wrapped = <unevaluated>;
};
getSubModules = null;
getSubOptions = <function>;
merge = <function>;
name = \"str\";
nestedTypes = { };
substSubModules = <function>;
typeMerge = <function>;
};
}"
Optionally, it's also possible to let `toPretty` throw an error if the
limit is exceeded.
When having a bogus declaration such as
{ lib, ... }:
{
foo.bar = mkOption {
type = types.str;
};
}
the evaluation will terminate with a not-so helpful
error: stack overflow (possible infinite recursion)
To make sure a useful error is still provided, I added a `depthLimit` of
`10` which should be perfectly sufficient to `toPretty` when it's used
in an error-case for `showDefs`.
The message I originally implemented here was to catch a mixup of
`config' and `options' in a `types.submodule'[1]. However it looks
rather weird for a wrongly declared top-level option.
So I decided to throw
error: The option `foo' does not exist. Definition values:
- In `<unknown-file>':
{
bar = {
_type = "option";
type = {
_type = "option-type";
...
It seems as you're trying to declare an option by placing it into `config' rather than `options'!
for an expression like
with import ./lib;
evalModules {
modules = [
{
foo.bar = mkOption {
type = types.str;
};
}
];
}
[1] fa30c9a
As suggested in NixOS#131205. Now it's possible to pretty-print a value with `lib.generators` like this: with lib.generators; toPretty { } (withRecursion { depthLimit = 10; } /* arbitrarily complex value */) Also, this can be used for any other pretty-printer now if needed.
Ma27
force-pushed
the
showdefs-overflow
branch
from
fe52892 to
5773ae9
Compare
|
I added a function called |
|
cc @Profpatsch would you mind re-reviewing this in case you currently have time&energy for that? :) |
|
cc @Profpatsch any chance to get another review? :) |
|
cc @infinisil @Profpatsch is there anything else I can do to get this merged? :) |
lheckemann
left a comment
lheckemann
left a comment
There was a problem hiding this comment.
Text nits, but I'm all for merging if these are resolved.
lib/modules: Test optionless module errors from #131205
4 participants
Motivation for this change
I realized that the error-message I implemented in fa30c9a isn't very helpful if
configandoptionsisn't mixed up in a submodule, but in a "top-level" declaration. While working on that, I also added adepthLimit-parameter togenerators.toPrettyto avoid weirdstack overflow-errors.The details can be found in the commit-messages.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)