Fix nested flake input overrides #6621

Kha · 2022-06-06T20:49:34Z

Currently, B in inputs.A.inputs.B.inputs.C... is interpreted as registry flake, discarding any of its configuration in A, leading to an unexpected "cannot find flake 'flake:B' in the flake registries". This PR implements the expected behavior:

input overrides (those nested inside inputs) are not defaulted to registry flakes.
Input overrides do not outright replace the corresponding input of the nested flake, but are recursively merged into it, meaning e.g. inputs.B.url set in A is preserved by the above config.

Fixes #5790

Kha · 2022-06-06T20:58:01Z

src/libexpr/flake/flake.cc

                        // Respect the “flakeness” of the input even if we
                        // override it
                        i->second.isFlake = input2.isFlake;


Note that this workaround could be replaced by a "proper" merge as now done with the other attributes below if isFlake had a value for "not specified", e.g. was changed to std::optional<bool>. But wanting to override the flakeness of an input does seem like an exotic use case.

tomberek · 2022-06-15T17:21:01Z

Works as expected. There are a few changes in-flight to follows/overrides and the lazy-trees branch with which I'd wonder if there is any interaction or conflict.

thufschmitt

Thanks for that, and sorry for the delay (I'm always a bit reluctant to touch that logic 😛).

Looks good overall, just a few suggestions, mostly around the tests

thufschmitt · 2022-07-05T08:28:57Z

src/libexpr/flake/flake.cc

@@ -90,11 +90,11 @@ static void expectType(EvalState & state, ValueType type,

 static std::map<FlakeId, FlakeInput> parseFlakeInputs(
    EvalState & state, Value * value, const PosIdx pos,
-    const std::optional<Path> & baseDir, InputPath lockRootPath);
+    const std::optional<Path> & baseDir, InputPath lockRootPath, bool isOverride);


Mind making this a proper enum (enum structeven) rather than an opaque bool?

I can do that, but looking at the code again, it seemed more elegant to just pass the recursion depth given that isOverride corresponds to depth > 0. Does it look sensible?

thufschmitt · 2022-07-05T09:10:39Z

tests/flakes.sh

+
+# Test nested flake overrides
+
+cat <<EOF > $flakeFollowsD/flake.nix
+{ outputs = _: {}; }
+EOF
+cat <<EOF > $flakeFollowsC/flake.nix
+{
+  inputs.D.url = "path:nosuchflake";
+  outputs = _: {};
+}
+EOF
+cat <<EOF > $flakeFollowsB/flake.nix
+{
+  inputs.C.url = "path:../flakeC";
+  outputs = _: {};
+}
+EOF
+cat <<EOF > $flakeFollowsA/flake.nix
+{
+  inputs.B.url = "path:./flakeB";
+  inputs.D.url = "path:./flakeD";
+  inputs.B.inputs.C.inputs.D.follows = "D";
+  outputs = _: {};
+}
+EOF
+
+nix flake lock $flakeFollowsA


Can you move that to its own test file? The flakes.sh test is already infuriatingly slow (and totally sequential) and I wouldn't to expand it further

Moved to the new follows.sh

thufschmitt · 2022-07-05T09:36:48Z

tests/flakes.sh

+EOF
+cat <<EOF > $flakeFollowsB/flake.nix
+{
+  inputs.C.url = "path:../flakeC";


That input doesn't seem right (flakeC is a subdir of flakeB). It's probably safer to use absolute paths (via the $flakeFollows* variables everywhere here

thufschmitt · 2022-07-05T09:54:03Z

tests/flakes.sh

+}
+EOF
+
+nix flake lock $flakeFollowsA


I get the idea of the test (the flake lock can only succeed if the override is properly taken into account), but it would be nice to have a more explicit check, if only to make the intent clearer. Maybe something like

nix flake metadata $flakeFollowsA --json | jq --exit-status '.locks.nodes.C.inputs.D == [ "D" ]'

done in the same style as in the other tests in the file

Kha · 2022-08-28T09:50:44Z

I noticed and fixed a related bug, see new commit above. I added it to this PR to avoid merge conflicts in the test file, but happy to relocate.

Kha · 2022-08-28T09:52:17Z

tests/flakes/follow-paths.sh

+EOF
+
+# bug was not triggered without recreating the lockfile
+nix flake lock $flakeFollowsA --recreate-lock-file


It feels a bit concerning that one gets an entirely different lockfile without this flag, but that's probably a different problem

thufschmitt

Sorry for not coming back to this earlier. I'm only 95% confident that this isn't braking something (because I still couldn't wrap my head around the flake locking code), but since we're just at the beginning of a new release cycle, let's merge and revert if needs be. Thanks!

This reverts commit c530cda, reversing changes made to 4adcdff.

Revert "Merge pull request #6621 from Kha/nested-follows"

This reverts commit c530cda, reversing changes made to 4adcdff.

Fix nested flake input overrides

Revert "Merge pull request NixOS#6621 from Kha/nested-follows"

This reverts commit a8b3d77. This undoes the revert of PR#6621, which allows nested `follows`, i.e. { inputs = { foo.url = "github:bar/foo"; foo.inputs.bar.inputs.nixpkgs = "nixpkgs"; }; } does the expected thing now. This is useful to avoid the 1000 instances of nixpkgs problem without having each flake in the dependency tree to expose all of its transitive dependencies for modification. This was in fact part of Nix before and the C++ changes applied w/o conflicts. However, it got reverted then because people didn't want to merge lazy-trees against it which was supposed to be merged soon back in October 2022. Fixes: https://git.lix.systems/lix-project/lix/issues/201 Change-Id: I5ddef914135b695717b2ef88862d57ced5e7aa3c

…follows""" into main

Kha commented Jun 6, 2022

View reviewed changes

tomberek requested a review from thufschmitt June 28, 2022 16:05

thufschmitt suggested changes Jul 5, 2022

View reviewed changes

Kha mentioned this pull request Jul 12, 2022

flakes: throw an error if follows-declaration for an input is invalid #6663

Merged

Kha force-pushed the nested-follows branch from 62c60df to c043a27 Compare July 17, 2022 21:00

mupdt mentioned this pull request Jul 19, 2022

nix copy --all from local binary cache fails #6730

Closed

Fix nested flake input overrides

2b9d381

Kha force-pushed the nested-follows branch from c043a27 to 5015506 Compare August 28, 2022 09:47

Fix overlapping flake follows

6f65c11

Kha force-pushed the nested-follows branch from 5015506 to 6f65c11 Compare August 28, 2022 09:50

Kha commented Aug 28, 2022

View reviewed changes

thufschmitt approved these changes Sep 1, 2022

View reviewed changes

thufschmitt merged commit c530cda into NixOS:master Sep 1, 2022

edolstra added a commit to edolstra/nix that referenced this pull request Sep 1, 2022

Revert "Merge pull request NixOS#6621 from Kha/nested-follows"

a8b3d77

This reverts commit c530cda, reversing changes made to 4adcdff.

edolstra added a commit that referenced this pull request Sep 1, 2022

Merge pull request #6983 from edolstra/revert-6621

4823067

Revert "Merge pull request #6621 from Kha/nested-follows"

Kha mentioned this pull request Oct 15, 2022

Don’t override flake ref if no url or type provided #7176

Closed

deemp mentioned this pull request Nov 15, 2022

Transitive inputs "not in registry" #5790

Open

Minion3665 pushed a commit to Minion3665/nix that referenced this pull request Feb 23, 2023

Revert "Merge pull request NixOS#6621 from Kha/nested-follows"

89f3425

This reverts commit c530cda, reversing changes made to 4adcdff.

Minion3665 pushed a commit to Minion3665/nix that referenced this pull request Feb 23, 2023

Merge pull request NixOS#6621 from Kha/nested-follows

c861a9a

Fix nested flake input overrides

Minion3665 pushed a commit to Minion3665/nix that referenced this pull request Feb 23, 2023

Merge pull request NixOS#6983 from edolstra/revert-6621

370eca1

Revert "Merge pull request NixOS#6621 from Kha/nested-follows"

Kha mentioned this pull request May 12, 2023

Allow overriding the follows of a transitive input without overriding its flakeref #8325

Open

tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024

Merge "Revert "Revert "Merge pull request NixOS#6621 from Kha/nested-…

799d013

…follows""" into main

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nested flake input overrides #6621

Fix nested flake input overrides #6621

Kha commented Jun 6, 2022

Kha Jun 6, 2022

tomberek commented Jun 15, 2022

thufschmitt left a comment

thufschmitt Jul 5, 2022

Kha Jul 17, 2022

thufschmitt Jul 5, 2022

Kha Jul 17, 2022

thufschmitt Jul 5, 2022

Kha Jul 17, 2022

thufschmitt Jul 5, 2022

Kha Jul 17, 2022

Kha commented Aug 28, 2022

Kha Aug 28, 2022

thufschmitt left a comment

Fix nested flake input overrides #6621

Fix nested flake input overrides #6621

Conversation

Kha commented Jun 6, 2022

Choose a reason for hiding this comment

tomberek commented Jun 15, 2022

thufschmitt left a 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

Kha commented Aug 28, 2022

Choose a reason for hiding this comment

thufschmitt left a comment

Choose a reason for hiding this comment