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

Allow composing overrides #4004

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Allow composing overrides #4004

wants to merge 7 commits into from

Conversation

mkenigs
Copy link
Contributor

@mkenigs mkenigs commented Sep 11, 2020

Allow multi-level overridees, overriding with higher level overrides
when appropriate.

See tests for the following motivating cases:

  • multi-level overrides: A(override B.C.D=D2) -> B -> C -> D
  • overriding an override: A (override B.C.D) -> B (override C.D)-> C -> D
  • overrides are merged: A (override B.C.D.E) -> B (override C.D)-> C -> D -> E

The prior implementation used a single overrides data structure
accessible throughout recursive calls to computeLocks. Instead, only
pass each recursive call exactly the overrides it needs. This better
protects individual recursive calls from modifying the overrides
"global" to the recursion.

This change depends on allowing no FlakeRef for FlakeInput.
That allows treating "nodes" in the override graph as empty if they
don't have a FlakeRef, which allows combining overrides set by different
flakes

@edolstra
Copy link
Member

Allow flake inputs to have overrides, but override them with higher level overrides when appropriate.

It's not entirely clear to me what this means since flake inputs can already have overrides. What is a higher level override?

@mkenigs
Copy link
Contributor Author

mkenigs commented Sep 17, 2020

Flakes can have overrides, but as far as I can tell overrides for flake inputs are not currently working. A trivial example of A -> B (override C.D) -> C -> D doesn't currently apply the override.

Overriding with higher level overrides means using A's override rather than B's in an example like this:
A (override B.C.D) -> B (override C.D)-> C -> D.

Guess I should update the commit to explain that more clearly?

@edolstra
Copy link
Member

I'm not sure I understand the notation "A -> B (override C.D) -> C -> D". What does that mean exactly?

@mkenigs
Copy link
Contributor Author

mkenigs commented Sep 17, 2020

Sorry flake A has input flake B etc. Like this: https://gist.github.com/mkenigs/7c07c53bfab2825fa9f9b03adcd59b79

@mkenigs
Copy link
Contributor Author

mkenigs commented Sep 22, 2020

Does that make sense now/seem like an improvement?

@mkenigs
Copy link
Contributor Author

mkenigs commented Oct 5, 2020

@edolstra anything I can cleanup or explain for this? Or should I close it?

@edolstra edolstra self-assigned this Oct 6, 2020
@edolstra
Copy link
Member

edolstra commented Oct 6, 2020

No, please keep it open, I'll try to look at it soon.

Copy link
Member

@siraben siraben left a comment

Choose a reason for hiding this comment

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

LGTM, the gist motivates it well since I would expect that overrides composed analogously to overlays.

@fare
Copy link

fare commented Mar 9, 2021

Yeah, flakes are a great place where you'd want multiple inheritance, as implemented in pop.nix from #114449

@fare
Copy link

fare commented Mar 14, 2021

Are you reinventing multiple-inheritance, but without confronting the fact openly?

@mkenigs
Copy link
Contributor Author

mkenigs commented Mar 14, 2021

Not that I'm aware of, I think I'd need you to explain that a little more though. I'm just viewing flakes as functions, but the top level flake can decide the inputs of lower level functions. So if I define a bunch of flakes:
C=C(D)
B can override the default argument to C with D2:
B=B(C(D2))
A can override the default and overriden argument to C:
A=A(B(C(D3)))

@kira-bruneau
Copy link

kira-bruneau commented Jun 3, 2021

I just ran into this problem when having three levels of flakes A -> B -> C where I want C to follow an input from B and not the input from A that has the same name.

The current behaviour is pretty confusing, so I'd love to see this merged! assuming follows uses the same implementation as overrides: #3602 (comment)

@nwg
Copy link

nwg commented Jul 14, 2021

I just ran into this problem with three levels of flakes, too. Mine was where the third flake followed one of its own inputs. It ended up trying to resolve with the second flake as the root instead of the first, which was actually fine for building the second flake (b) but broke when I moved onto the first, outer flake (a). It resulted in a "follows a nonexistent input 'b/c/some/thing'" error. It was supposed to look at 'a/b/c/some/thing'. I reported it here and might merge it with this bug but can't quite tell if it's the same exact bug because they talk about circular refs and stuff and you don't need to introduce circular refs to trigger this one.

@stale
Copy link

stale bot commented Apr 16, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Apr 16, 2022
Copy link
Member

@edolstra edolstra left a comment

Choose a reason for hiding this comment

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

Idea looks good, the tests describe the behavior that we want from Nix. I didn't look very closely at the C++ side since it's probably a bit outdated at the moment.

tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated

# B, C, and C2 same as previous test
# A + B + C = 3
[[ $(nix eval $flakeA#foo --recreate-lock-file) = 3 ]]
Copy link
Member

Choose a reason for hiding this comment

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

Should this --recreate-lock-file be necessary? Ideally Nix detects that the override in flakeA has been removed and updates the lock file accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped a bunch of them but the rest are necessary for tests to pass. I haven't looked into why

tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
src/libexpr/flake/flake.cc Outdated Show resolved Hide resolved
src/libexpr/flake/flake.cc Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member

Converted to draft because pending changes from review.

@mkenigs
Copy link
Contributor Author

mkenigs commented Jun 14, 2023

Thanks for the review @edolstra! I'll rebase this when I get a chance, but it might take me a while to find time to brush up on the codepath it's changing

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-06-12-nix-team-meeting-minutes-62/29315/1

@tomberek
Copy link
Contributor

Does this also fix #8325 ?

Copy link
Contributor

@aakropotkin aakropotkin left a comment

Choose a reason for hiding this comment

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

<3 Thanks @mkenigs

I ran out of steam writing more suggestions, but you get the idea.

tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
tests/flakes.sh Outdated Show resolved Hide resolved
mkenigs and others added 4 commits August 18, 2023 16:21
This allows treating "nodes" in the override graph as empty if they
don't have a FlakeRef, which allows combining overrides set by different
flakes
Allow multi-level overridees, overriding with higher level overrides
when appropriate.

See tests for the following motivating cases:
- multi-level overrides: A(override B.C.D=D2) -> B -> C -> D
- overriding an override: A (override B.C.D) -> B (override C.D)-> C -> D
- overrides are merged: A (override B.C.D.E) -> B (override C.D)-> C -> D -> E

The prior implementation used a single overrides data structure
accessible throughout recursive calls to computeLocks. Instead, only
pass each recursive call exactly the overrides it needs. This better
protects individual recursive calls from modifying the overrides
"global" to the recursion.
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Aug 18, 2023
@@ -146,5 +146,5 @@ EOF

git -C $flakeFollowsA add flake.nix

nix flake lock $flakeFollowsA 2>&1 | grep "warning: input 'B' has an override for a non-existent input 'invalid'"
nix flake lock $flakeFollowsA 2>&1 | grep "warning: input 'B' has a follows for a non-existent input 'invalid'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it's helpful to have this distinction or not - can revert if not

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should make this distinction but if we do want to keep it this way I suggest changing the message to input 'B' follows a non-existent input

Copy link
Contributor Author

@mkenigs mkenigs Aug 18, 2023

Choose a reason for hiding this comment

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

Agreed!

Copy link
Contributor Author

@mkenigs mkenigs Aug 19, 2023

Choose a reason for hiding this comment

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

Fixed

@mkenigs mkenigs marked this pull request as ready for review August 18, 2023 22:54
@mkenigs
Copy link
Contributor Author

mkenigs commented Aug 18, 2023

I ran out of steam writing more suggestions, but you get the idea.

Pushed a bunch of quotes but let me know if I missed any

@mkenigs
Copy link
Contributor Author

mkenigs commented Aug 18, 2023

Does this also fix #8325 ?

I think it may - that's part of the reason for 973eb60. Unless there's a difference between follows and overrides, that's tested in this test case https://github.com/NixOS/nix/pull/4004/files#diff-4682489d70e9a85daa7f2a8a11cc25dd59a602399cd6e0b96b52eaa610e2cf8bR119

@mkenigs mkenigs requested a review from edolstra August 18, 2023 23:01
@mkenigs
Copy link
Contributor Author

mkenigs commented Aug 18, 2023

Idea looks good, the tests describe the behavior that we want from Nix. I didn't look very closely at the C++ side since it's probably a bit outdated at the moment.

Updated all the C++ and is ready for re-review!

@mkenigs
Copy link
Contributor Author

mkenigs commented Nov 8, 2023

@edolstra @Ericson2314 is there still any interest in this?

@roberth roberth added this to the Flakes milestone Dec 23, 2023
@mkenigs
Copy link
Contributor Author

mkenigs commented Jan 23, 2024

I am happy to resolve merge conflicts if anyone ever wants to take a look at this

@edolstra edolstra removed their assignment Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flakes with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.