Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/mkFlake: Test suite improvements and more #183

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

bb010g
Copy link

@bb010g bb010g commented Aug 18, 2023

Split out from #137. Please read the individual commit messages.

@bb010g
Copy link
Author

bb010g commented Aug 18, 2023

Reading divnix/call-flake@5828e08, it looks like both outPath and sourceInfo would have to be inputs to outputs that don't force self in order to consistently provide proper error messages when evaluation of self errors?

@roberth
Copy link
Member

roberth commented Aug 18, 2023

You mean NixOS/nix@5d834c4

both outPath and sourceInfo would have to be inputs

I don't think we need sourceInfo, but perhaps its metadata would be nice to use. We should be careful with metadata though, because differentiating by origin can be a double edged sword. We have to make sure that inputs are interchangeable: a fork should work just as well as the original. (Not saying this is an immediate problem; just raising awareness of the requirement)

would have to be inputs to outputs that don't force self in order to consistently provide proper error messages when evaluation of self errors?

Sounds about right. I think outPath would be a very unfortunate name for such a parameter.
A similar effect could be achieved by asking the caller to say something like mkFlake { here = ./.; ... }.

Not sure if we should act on the "here" idea now. For most users it's probably better to get a change into Nix so we can improve this structurally without bothering them with an extra parameter.

Comment on lines +19 to +39
weakEvalTests.callFlake = { ... } @ flake:
let
sourceInfo = flake.sourceInfo or { };
inputs = flake.inputs or { };
outputs = flake.outputs inputs;
result = outputs;
in
result;
strongEvalTests.callFlake = { ... } @ flake:
let
sourceInfo = { outPath = "/unknown_eval-tests_flake"; } //
flake.sourceInfo or { };
inputs = flake.inputs or { };
outputs = flake.outputs (inputs // { self = result; });
result = outputs // sourceInfo // {
inherit inputs outputs sourceInfo;
_type = "flake";
};
in
assert builtins.isFunction flake.outputs;
result;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need weakEvalTests.callFlake?

Not happy about how complicated this got. Relying on laziness and overriding means that it's not immediately obvious what runs how.

Comment on lines +48 to +57
# for REPL exploration / debugging
config.allFlakeModuleArgs = args // config._module.args // specialArgs;
config.allRootModuleArgs = rootArgs // rootConfig._module.args // rootSpecialArgs;
config.transformFlakeModule = flakeModuleTransformer:
rootSpecialArgs.replaceSpecialArgs (prevSpecialArgs: prevSpecialArgs // {
inherit flakeModuleTransformer;
});
options.mySystem = lib.mkOption {
default = rootConfig.allSystems.${builtins.currentSystem};
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just set config.debug = true;?

dev is already overly complicated because it tries to work around the subflake problems. I'd like to simplify it instead of obfuscating it further.

@bb010g
Copy link
Author

bb010g commented Aug 18, 2023

I don't think we need sourceInfo, but perhaps its metadata would be nice to use. We should be careful with metadata though, because differentiating by origin can be a double edged sword. We have to make sure that inputs are interchangeable: a fork should work just as well as the original. (Not saying this is an immediate problem; just raising awareness of the requirement)

Alright. Balancing this is hard when debug information is sourced the same as normal information.

Not sure if we should act on the "here" idea now. For most users it's probably better to get a change into Nix so we can improve this structurally without bothering them with an extra parameter.

I agree on this. A manual parameter that they have to set to ./flake.nix is definitely not what this should be long-term.

Comment on lines +179 to +195
# This test case is a fun one. In the REPL, try `exhibitInfiniteRecursion.*`.
# In the case that `mkFlake` *isn't* called from a flake, `inputs.self` is
# unlikely to refer to the result of the `mkFlake` evaluation. If
# `inputs.self` isn't actually self-referential, evaluating attribute values
# of `self` is not divergent. Evaluation of `self.outPath` is useful for
# paths in documentation & error messages. However, if that evaluation occurs
# in a `builtins.addErrorContext` message forced by an erroring `self`, both
# `self` will never evaluate *and* `builtins.toString self.outPath` must
# evaluate, causing Nix to instead throw an infinite recursion error. Even
# just `inputs.self ? outPath` throws an infinite recursion error.
# (`builtins.tryEval` can only catch errors created by `builtins.throw` or
# `builtins.assert`, so evaluation is guarded with
# `exhibitingInfiniteRecursion` here to keep `runTests` from diverging.)
# In this particular case, `mkFlake` evaluates `self ? outPath` to know if the
# default module location it provides should be generic or specific. As
# explained, this evaluation is unsafe under an uncatchably divergent `self`.
# Thus, `outPath` cannot be safely sourced from `self` at the top-level.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 I can confirm that this now works as it should, since #192 or #194.

The new, informative error message
«error: error:
       … while evaluating the attribute 'strongEvalTests.nonexistentOption'

         at /home/user/h/flake-parts/dev/tests/eval-tests.nix:209:3:

          208|   # exhibiting infinite recursion, the flake is made equivalent to `empty`.
          209|   strongEvalTests.nonexistentOption = evalTests.callFlake {
             |   ^
          210|     inputs.flake-parts = evalTests.flake-parts;

       … in the left operand of the update (//) operator

         at /home/user/h/flake-parts/dev/tests/eval-tests.nix:33:24:

           32|       outputs = flake.outputs (inputs // { self = result; });
           33|       result = outputs // sourceInfo // {
             |                        ^
           34|         inherit inputs outputs sourceInfo;

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: The option `nonexistentOption' does not exist. Definition values:
       - In `/home/user/h/flake-parts/dev/tests/eval-tests.nix': null»

@roberth roberth changed the title lib/mkFlake: Document _file infinite recursion lib/mkFlake: Test suite improvements and more Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants