Flakes: provide robust access to `outPath` through new `meta` argument. #8908

roberth · 2023-09-03T16:19:56Z

Motivation

Original goal: This solves an issue of unnecessary evaluation strictness encountered by flake frameworks. Errors like this are terrible because they obscure the underlying solvable user error.
Side catch:
- Make inputs.self declaration invalid (at least until we have self fetching configuration)
- Add reflection primops to optimally manage the migration really well

When reporting an error, it is beneficial to include the location of the flake that causes it. For this, a flake framework needs to access outPath, but outPath is only available in self.

This PR adds a new outputs argument besides the inputs. We already had self; now we also have meta.

I've chosen meta for a couple of reasons:

Adding a "pseudo input" is a breaking change for flakes that don't have an ellipsis in the outputs function. It would be nice to be able to avoid this problem in the future, and meta provides such an extension point.
Naming flake.outPath is hard. I'd like to have a better name for it, such as flakeDir, but creates a more questionable scope. Even if we find a nice and consistent name for it, we'll want to put it in meta anyway.

The problem can be illustrated by the new test:

{
  outputs = args:
    # imagine some framework logic in between here
    throw "can't evaluate self, because outputs can't return any attribute names, but I know I can be identified as ${toString args.meta.extraAttributes.outPath}/flake.nix}";
}

The ability to refer to args.meta.extraAttributes.outPath is crucial for conveying what's the origin of the error, especially when the error occurs within some input.

An flake name would also be very helpful, and it could be added to meta once we agree that a flake can declare a name for itself (in a "non-binding" way like we have for derivations). Again something that I'd like to scope out because it requries more discussion that's not quite necessary for making substantial progress on the problem.

Context

Unblocks infinite recursion when self assigned to option hercules-ci/flake-parts#185
Not aware of concrete issues in other frameworks, but for anything more complex than some mapAttrs, errors will tend to have locations from the framework rather than the flake that uses it. They should also add the location of their calling flake to their error messages where possible.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
documentation in the internal API docs
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

…Path on inputs

Atry · 2023-09-03T18:20:00Z

It seems a backward incompatible change. Could it be an on-demand attribute like nix flake registry names?

src/libexpr/flake/call-flake.nix

Atry

Do you want to make it backward-compatible?

roberth · 2023-09-03T18:27:14Z

It seems a backward incompatible change.

Only for outputs argument lists without ellipses, right? I would consider such argument lists to be an antipattern, but I guess we could try something with builtins.functionArgs to make it a bit more lenient in that specific case. That does come at the cost of being a slightly more surprising interface in the final design, so I'd want to get rid of the compatibility logic before stabilizing flakes. (And at that point I wonder if the effort is worth it)

roberth · 2023-09-03T18:28:43Z

Didn't run all the test during development. My bad.

Looks like some C++ needs to ignore meta like it ignores self.
Also I think we should check that inputs.self and inputs.meta don't exist in the input declarations.

Atry · 2023-09-03T18:33:41Z

Since we already have some on-demand inputs from registry, I think it makes sense to make meta also on-demand.

This way, flake frameworks have access to the flake location even when an error causes `self` not to evaluate.

... and do not add `meta` to the lock when formal is present, like we do for `self`.

sternenseemann · 2023-09-04T18:47:23Z

src/libexpr/primops.cc

+    }
+}
+
+static RegisterPrimOp primop_functionStrict({


This should be functionHasFormals or whatever. It is not immediately obvious to the casual user what a “strict function” is supposed to be (and it is a bit ambiguous as well, I'd say). A function with formals is immediately syntactically obvious, so there is no confusion what this builtin would check for.

Additionally, not all functions that are strict in their argument have formals, as the documentation also admits, e.g.:

foo: builtins.seq foo (/* body … */)

I couldn't care less about the syntax of a function, which is why I've named these primops after the semantics.

By calling it has formals we can't go back and add a syntax to make functions with formals lazy. What matters is that the function definition is trivially strict, so that's what the name reflects. Should it be functionTriviallyStrict?

If this function is too contentious, I might remove it because it turned out that this one wasn't necessary for the use case I'm trying to solve. I think it's good to have, but low impact, so I don't want to waste time on it.

not all functions that are strict in their argument have formals

This is for improving error messages in a few cases, and nothing more.

Well the semantics are fundamentally not introspectable, so the naming is very deceptive.

we can't go back and add a syntax to make functions with formals lazy

It wouldn't really matter, the introspection would still do its job as advertised—there would be a breaking change in the semantics of the language, but that would be a problem regardless of the existence of that builtin.

sternenseemann · 2023-09-04T18:48:24Z

src/libexpr/primops.cc

+    .fun = prim_functionStrict,
+});
+
+static void prim_functionOpen(EvalState & state, const PosIdx pos, Value * * args, Value & v)


Open is new terminology that has not appeared so far, as far as I am aware. I'd stick to something with ellipsis which is also terminology used by builtins.toXML.

src/libexpr/primops.cc

sternenseemann · 2023-09-04T18:50:37Z

src/libexpr/primops.cc

+    .fun = prim_functionOpen,
+});
+
+static void prim_functionBindsAllAttrs(EvalState & state, const PosIdx pos, Value * * args, Value & v)


What is this needed for? I don't really see an use case for this right away.

It's used in this PR to report a warning when a flake's outputs function is invoked. This PR adds an attribute, which causes somewhat of a problem when the function is of the form

args@{ self }:

The lack of ellipsis means that we'd have to remove the attribute for compatibility, while args@ exposes that workaround, which would be a source of confusion. Detecting this situation and reporting a warning means that we get a reasonable upgrade story for this feature.

Why not just check for an ellipsis (functionOpen)? Whether the user also binds the whole attribute set, is not really interesting for that.

If it has ellipsis, but no binding for the whole attrset, there's no conflict and the backcompat logic can comply with the old interface and it doesn't have to warn about anything.

I don't want it to warn about something that isn't a problem, because too many warnings lead people to ignore them.

Right, but why not just warn if it lacks an ellipsis. The full attribute set binding should be irrelevant.

The full attribute set binding should be irrelevant.

The ambiguity of #8908 (comment) does not occur if the full attrset binding is missing. So to keep warnings to a minimum, I want to know whether the full attrset binding exists or not.

I can agree that it should but unfortunately it would require me to warn in cases where it doesn't matter.

These are relevant for determining compatibility properties of functions. - Can I add an attribute? Is it open for extension? - If I omit an attribute that I'd be expected to add, does that lead to a conflict? Such a conflict arises when the function is closed and also binds all attributes using `@` syntax. It also answers a related question: is the function defined using the strict syntax? This is useful for explaining the misconception that changing a plain lambda to a strict lambda is a no-op refactor. The fact that it makes the function strict is overlooked and the error message, infinite recursion sends users into panic mode. With the new functionStrict primop we can write functions that catch the mistake before we enter it, which could be very helpful.

Inputs is not quite an appropriate name because of `self` and `meta`, which aren't inputs.

This doesn't suggest that two is the total count, and I think it's a nice way to phrase it, because the functions are related.

... and fix an incorrect use of `grep -v`

roberth · 2023-09-04T22:51:22Z

I'm considering the following simplification.
Rather than having three value (bool + null) return values, I could make the interface a bit simpler but also less powerful.

isTriviallyClosed

simple lambda: false
formals + ellipsis: false
formals, no allipsis: true

isTriviallyStrict

formals: true
simple lambda: false

bindsAllAttrs

simple lambda: true
@: true
no @: false

I believe this still covers the needs for the warning, and it simplifies the code a little.

Advantages

Simpler interface avoiding null - usable directly in if
Reflection can be a double edged sword. Exposing less may be better

Disadvantages

May need to revisit the topic later, to create more functions to cover the info lost by this simplification

I'm leaning towards doing it this way.

nixos-discourse · 2023-09-19T09:56:20Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-09-18-nix-team-meeting-minutes-87/33194/1

roberth · 2024-03-22T12:11:14Z

Other meta attributes could include the raw top level attributes, such as nixConfig and inputs.

All 4 top-level flake attributes should be exposed #10210

tests/flakes/inputs: Add root case and test outPath vs sourceInfo.out…

dfe4fa9

…Path on inputs

roberth requested review from fricklerhandwerk and edolstra as code owners September 3, 2023 16:19

github-actions bot added documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Sep 3, 2023

roberth added flakes error-messages Confusing messages and better diagnostics and removed documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Sep 3, 2023

github-actions bot added documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Sep 3, 2023

This was referenced Sep 3, 2023

infinite recursion when self assigned to option hercules-ci/flake-parts#185

Closed

Add moduleLocation to mkFlake argument hercules-ci/flake-parts#158

Merged

Atry approved these changes Sep 3, 2023

View reviewed changes

Atry reviewed Sep 3, 2023

View reviewed changes

src/libexpr/flake/call-flake.nix Show resolved Hide resolved

Atry suggested changes Sep 3, 2023

View reviewed changes

roberth marked this pull request as draft September 3, 2023 18:28

roberth force-pushed the flakes-robust-outPath-access-through-meta-argument branch 2 times, most recently from 62c33c0 to 733537d Compare September 4, 2023 15:11

roberth marked this pull request as ready for review September 4, 2023 15:11

roberth added 2 commits September 4, 2023 17:40

flakes: Add robust meta argument to outputs arguments

2d235db

This way, flake frameworks have access to the flake location even when an error causes `self` not to evaluate.

flakes: Reserve input names self and meta

d10bc47

... and do not add `meta` to the lock when formal is present, like we do for `self`.

roberth force-pushed the flakes-robust-outPath-access-through-meta-argument branch from 733537d to e3b90dd Compare September 4, 2023 15:40

sternenseemann reviewed Sep 4, 2023

View reviewed changes

roberth force-pushed the flakes-robust-outPath-access-through-meta-argument branch from e3b90dd to 7b81118 Compare September 4, 2023 22:30

roberth added 5 commits September 5, 2023 00:49

flakes: Add compatibility for most outputs functions that don't accept

e9e1897

tests: Rename outputs arguments to args instead of inputs

2cf4b75

Inputs is not quite an appropriate name because of `self` and `meta`, which aren't inputs.

rl-next: Two functions -> A pair of functions

ab6dbf3

This doesn't suggest that two is the total count, and I think it's a nice way to phrase it, because the functions are related.

refactor: Introduce isRoot in call-flake.nix

1f739ff

flakes: Don't warn about meta and ellipsis except for root flake

72675eb

... and fix an incorrect use of `grep -v`

roberth force-pushed the flakes-robust-outPath-access-through-meta-argument branch from 7b81118 to 72675eb Compare September 4, 2023 22:49

roberth mentioned this pull request Sep 11, 2023

Add functionInfo builtin. #8961

Open

roberth mentioned this pull request Mar 22, 2024

All 4 top-level flake attributes should be exposed #10210

Open

roberth mentioned this pull request Mar 28, 2024

Extend function reflection #10350

Open

roberth mentioned this pull request May 16, 2024

docs: recommend setting moduleLocation by default hercules-ci/flake-parts#221

Open

fricklerhandwerk removed their request for review June 4, 2024 19:54

ShamrockLee mentioned this pull request Jul 26, 2024

lib.extendMkDerivation, lib.adaptMkDerivation: init NixOS/nixpkgs#234651

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flakes: provide robust access to `outPath` through new `meta` argument. #8908

Flakes: provide robust access to `outPath` through new `meta` argument. #8908

roberth commented Sep 3, 2023 •

edited

Loading

Atry commented Sep 3, 2023 •

edited

Loading

Atry left a comment

roberth commented Sep 3, 2023

roberth commented Sep 3, 2023 •

edited

Loading

Atry commented Sep 3, 2023

sternenseemann Sep 4, 2023

roberth Sep 4, 2023

sternenseemann Sep 9, 2023

sternenseemann Sep 4, 2023

sternenseemann Sep 4, 2023

roberth Sep 4, 2023

sternenseemann Sep 9, 2023

roberth Sep 14, 2023

sternenseemann Sep 19, 2023

roberth Oct 3, 2023

roberth commented Sep 4, 2023

nixos-discourse commented Sep 19, 2023

roberth commented Mar 22, 2024

Flakes: provide robust access to outPath through new meta argument. #8908

Are you sure you want to change the base?

Flakes: provide robust access to outPath through new meta argument. #8908

Conversation

roberth commented Sep 3, 2023 • edited Loading

Motivation

Context

Checklist for maintainers

Priorities

Atry commented Sep 3, 2023 • edited Loading

Atry left a comment

Choose a reason for hiding this comment

roberth commented Sep 3, 2023

roberth commented Sep 3, 2023 • edited Loading

Atry commented Sep 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth commented Sep 4, 2023

nixos-discourse commented Sep 19, 2023

roberth commented Mar 22, 2024

Flakes: provide robust access to `outPath` through new `meta` argument. #8908

Flakes: provide robust access to `outPath` through new `meta` argument. #8908

roberth commented Sep 3, 2023 •

edited

Loading

Atry commented Sep 3, 2023 •

edited

Loading

roberth commented Sep 3, 2023 •

edited

Loading