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

[RFC 0134] Carve out a store-only Nix #134

Merged
merged 19 commits into from
Feb 20, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Sep 15, 2022

@Ericson2314 Ericson2314 changed the title Carve out a store-only Nix [RFC 0134] Carve out a store-only Nix Sep 15, 2022
Copy link
Member

@lheckemann lheckemann left a comment

Choose a reason for hiding this comment

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

I like the idea and hope we can get this going!

rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
There are also many possible ways set up build farms.
Our current central dispatcher, many remote-builder agents model, point-to-point protocol model is also just point in a much larger design space.

The "derivation language" and store *interface* however, seems to me at least to be a very natural design.
Copy link
Member

Choose a reason for hiding this comment

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

By derivation language, you mean the ATerm-based language that drv files are written in?

Copy link
Member Author

@Ericson2314 Ericson2314 Sep 15, 2022

Choose a reason for hiding this comment

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

The abstract syntax of that. Basically "a derivation is an execve system call arguments (prog, args, env vars), input black box data, and input other drvs' outputs". I think that right there is a very natural design.

Copy link
Contributor

Choose a reason for hiding this comment

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

That terms needs a more formal introduction at some point, I think. I wrote about it in this blog post, if you care about a more detailed explanation.

rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
Meaning was reversed!
Comment on lines +108 to +109
Below, they want to experiment with the standardized containerization technologies that already exist for new ways of sandboxing and distributing builds with less bespoke Nix-specific code.
They also want to apply the layering paradigm *within* go-nix, fostering even more modularity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do you get all of this info about go-nix (e.g. what they want to experiment with, and where)? I can't find it in the links you mention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Taking on https://matrix.to/#/!YUnRYAzgytLSZbBhbx:hackint.org more information should be published soon after which I'll update the RFC to link it.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://code.tvl.fyi/about/tvix/docs/components.md I'll update this once I can get permalink.

1. Making sure Nix as a whole continues to make sense
2. Make sure layers make sense in isolation not just in the context of the way they are currently used.

## Standardization across projects
Copy link

@arcuru arcuru Sep 15, 2022

Choose a reason for hiding this comment

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

Have you reached out to Guix yet? When do you think it would be valuable to get their input?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have! But I think for this RFC the ball is purely in our court. This RFC is not about making technical decisions that would impact Guix, but about ratifying the idea of a store layer that is usable in arbitrary ways including the Nix language and Scheme via Guix.

I'll continue talking to them, and I encourage anyone else that wants to reach and say hi too — let's build more bridges between our communities — but if we don't do something like this I don't see why they should take us very seriously about store layer interop.

```
nix show-derivation flake#bar
```
will still work.
Copy link
Member

Choose a reason for hiding this comment

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

Should implementation of this be migrated to a model where no executable links libstore and libexpr at once, and the CLI executable invokes the store-only backend (possibly for multiple stores as in case of nix copy)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I lean against that, but regardless I would defer such questions until after this RFC and this first step. If we can be sure full Nix stays the same in the short run, that makes this a lot less risky — hardly any risk at all, I think.

@AndersonTorres
Copy link
Member

In a preliminar look, this can affect #132 somehow.


Guix is more diverged from Nix than Tvix + go-nix, and thus hints more at the end breadth of the design space yet to be explored.

The store layer is the same, but the layers above, instead of being a implementing of the Nix language, is a completely different design with Guile Scheme.
Copy link

Choose a reason for hiding this comment

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

I heard that Guix started out using a store format which was compatible with Nix's libstore (presumably so they could use the Nix tools to bootstrap their development process) but that their store is no longer perfectly compatible.

If this is true, does anybody know what motivated them to diverge? Or did they simply not adopt some forward-incompatible change that Nix made to its store format?


### Guix

Guix is more diverged from Nix than Tvix + go-nix, and thus hints more at the end breadth of the design space yet to be explored.
Copy link

Choose a reason for hiding this comment

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

TVL has taught me an excellent software engineering trick: if you want to ensure a clean separation between two parts of your codebase, write the two parts in two different languages whose communities see each other as competitors.

Absolutely brilliant; I tip my hat to these folks for figuring that out. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

And still keep both in a monorepo, that is.

rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
Ericson2314 and others added 2 commits September 16, 2022 16:30
Thanks!

Co-authored-by: Tor Bjornrud <bjornrud@users.noreply.github.com>
Thanks!

Co-authored-by: Adam Joseph <54836058+amjoseph-nixpkgs@users.noreply.github.com>
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

I had a look at two previous drafts in the past months, and like the idea and the detailed elaboration very much.

If that RFC is to be accepted, I would really like to have it at most 2/3 of the length just to save future readers the time to work through it. This is of course only about writing style, but more brevity and focus would have helped me evaluate the proposal.

PS: Suggestion for title: Nix store as separate executable.

rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
Most of this is in Flakes, which just has enormous surface area, but things like floating content-addressed derivations and other RFCs add complexity to the core store layer too.

If we view Nix as one monolithic whole, it will grow too complex and unwieldy, and we will be unable to manage it as we the complexity bogs us down.
However, if we embrace layering we can "divide and conquer" the project, and manage that complexity.
Copy link
Contributor

Choose a reason for hiding this comment

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

"layering" does not really capture what this is about.

Suggested change
However, if we embrace layering we can "divide and conquer" the project, and manage that complexity.
However, if we define Nix's internal architecture more clearly, we can "divide and conquer" the project, and manage that complexity.

However, if we embrace layering we can "divide and conquer" the project, and manage that complexity.
This will ensure the continued sustainability of Nix.

We currently embrace layering somewhat as an implementation detail, but only as an implementation detail.
Copy link
Contributor

Choose a reason for hiding this comment

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

The layered architecture of Nix is not very prominent.

Copy link
Member Author

@Ericson2314 Ericson2314 Sep 21, 2022

Choose a reason for hiding this comment

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

Suggested change
We currently embrace layering somewhat as an implementation detail, but only as an implementation detail.
We currently care about layering enough to partition the implementation into separate `libnix*` libraries, but this is only an implementation detail.

how is this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. (Although I would prefer to phrase it as separation of concerns instead of layering, but that may be me having Heard- and read "layering" way too often.)

rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved

To be clear, this is *not* to say we should abandon the idea of Nix as a whole.
There can still be governance of Nix as a whole; this team, and similar hypothetical, say, Flakes, Nix language, or User Experience teams would ultimately need to report to.
The goal is not to overreact, but strike a balance between:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The goal is not to overreact, but strike a balance between:
The goal is to strike a balance between:

rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
rfcs/0134-nix-store-layer.md Outdated Show resolved Hide resolved
@thufschmitt
Copy link
Member

I'm sympathetic with the idea, with a few caveats:

  • As stated, the motivations feel a bit off to me. I fully agree with the complexity part (though that's mostly an internal implementation problem), but the part about the pluralism not so much:
    • The store protocol is a very bad one to build on because It's very painful to implement and debug, and way too rigid for several implementations to cohabit peacefully. In particular I suspect that the Guix protocol is now irreconcilably incompatible with the Nix one.
    • Being able to mix and match different client and daemons doesn't strictly require the code to be splitted, as long as there's a well-defined protocol. Which is anyways the case (I said well defined, not well documented mind you) because of Nix being a distributed system. And there's no clear explanation of why just building the store part could be beneficial.
  • I might be convinced otherwise, but a priori distributing a store-only Nix with its own store-only documentation seems like a fairly bad idea – unless that's explicitly stated as a reference document pointed at people who want to try nasty things in their basement. There's already enough important concepts for people to grasp without having to care about the fact that they are running a client-server thing
  • The testing part is in my opinion the biggest gain and would deserve more than just a mention (although that also is mostly geared towards the internal implementation rather than the external interface)

But again, I agree with the broad idea, even if only from an implementation point of view

Thanks so much!!!

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@tomberek
Copy link
Contributor

tomberek commented Sep 21, 2022

  • I might be convinced otherwise, but a priori distributing a store-only Nix with its own store-only documentation seems like a fairly bad idea – unless that's explicitly stated as a reference document pointed at people who want to try nasty things in their basement. There's already enough important concepts for people to grasp without having to care about the fact that they are running a client-server thing

My interpretation of this is that is should be possible to distribute such a thing, not that it will be prominent or even recommended. Allow building a store-only version of Nix. and It should be possible to build a store-only manual without... seem to be deliberately intending that the possibility to create such a limited binary is the important part - and the ability to do so is itself the proof that the store layer is independent. It is also an artifact with a much smaller surface for testing, fuzzing, documentation, and potential re-use in unexpected places.

Thanks @fricklerhandwerk, just tweaked a few things from your suggestions to make these.
@Ericson2314
Copy link
Member Author

@tomberek Agree completely. I'll try to make that clearer in the RFC proper, but suggestions on how to do so welcome!

@thufschmitt I very much agree the current protocol is bad. What I like is the abstract store interface, like the C++ class or its "Platonic form", not the way we communicate between processes today.

I think some rephrasing I can do to emphasis this and in fact use the things you are saying as motivation:

  • If we always distribute the sole client and server, then it is easy to have a "closed world assumption" where the protocol being bad doesn't matter because both end are Nix itself, and they already understand each other, and there isn't any reason to improve the protocol.

  • if we distribute just the store side, we are very strongly implying "bring your own frontend" is a kosher idea, and this shatters the "closed world assumption" above, thereby justifying making a much better protocol.

You are right that all the end-user benefits do not inherently depend on this; using and "overkill" binary indeed doesn't prevent use-cases. But the combination of concrete implementation-side benefits with more nebulous more more far reaching "messaging" benefits user-side feels good to me. If you want to see the implementation-side concrete benefits like testing giving more weight in the motivation I am happy to do that.

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Sep 22, 2022

Maybe the following is a slight tangent, so excuse the noise: What the success of such RFCs (where @Ericson2314 shows a prototype and has a track record of following up and implementing the whole thing) seem to boil down to is the commitment and capacity of maintainers to review and merge the proposed changes, which will be nontrivial, over a longer period of time. So the question to @thufschmitt and ultimately @edolstra is if they care enough to provide the necessary resources to support such an endeavour once it's conceptually matured enough to be accepted as RFC.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Sep 22, 2022

Yes that is true. In this case the draft PR is very much incomplete (a lot of things crash), but I think getting it "pre-approved" and then I go finish it is going to feel better than other ones where I am farther along but blocked on approval.

Also, it is still important for the community to way on on whether the thing in question sounds beneficial to them. Than can influence those decisions (Nix maintainers know the costs best, community can speak to benefits as felt by the most people).

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Feb 21, 2023
Do this as opposed to associating it with each `DerivedPath` produced by
the installable. (Installables can produce multiple). `getExtraPathInfo`
is a new method which gets instead.

Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are moved
to `InstallableValue`.

To my pleasant surprise, the extra path infos associated with each
`DerivedPath` of a given installable were all the same, so this was an
easy refactor with no behavior change.

I did this because NixOS/rfcs#134 ; with these
things moved to `InstallableValue`, the base `Installable` no longer
depends on libexpr! This is a major step towards that.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 6, 2023
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.

However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.

This isn't so clear to use.

What this commit does is make those command classes like the others, with richer run functions.

Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:

- `prepare` and `load` are gone entirely! One command just hands just
  hands off to the next.

- We can use `ref` instead of `std::shared_ptr`. The former must be
  initialized (so it is like Rust's `Box` rather than `Option<Box>`,
  This expresses the invariant that the installable are in fact
  initialized much better.

  This is possible because since we just have local variables not
  fields, we can stop worrying about the not-yet-initialized case.

- Fewer lines of code! (Finally I have a large refactor that makes the
  number go down not up...)

- `nix repl` is now implemented in a clearer way.

The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.

To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.

This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.

Finally do note that I stopped overloading the `run` functions. The
reason was we are liable to get `-Woverloaded-virtual` warnings from
this. Yes, there is a workaround in putting `using
BaseClass::virtual_method;` next to overrides, but since I made the
`run`-delegation chain cheaper that would have required *far* more
`using` statements. I figured it was less obnoxious to just slightly
vary the names than force all the commands to do that.

Helps with NixOS/rfcs#134
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 7, 2023
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.

However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.

This isn't so clear to use.

What this commit does is make those command classes like the others, with richer run functions.

Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:

- `prepare` and `load` are gone entirely! One command just hands just
  hands off to the next.

- We can use `ref` instead of `std::shared_ptr`. The former must be
  initialized (so it is like Rust's `Box` rather than `Option<Box>`,
  This expresses the invariant that the installable are in fact
  initialized much better.

  This is possible because since we just have local variables not
  fields, we can stop worrying about the not-yet-initialized case.

- Fewer lines of code! (Finally I have a large refactor that makes the
  number go down not up...)

- `nix repl` is now implemented in a clearer way.

The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.

To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.

This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.

Finally do note that I stopped overloading the `run` functions. The
reason was we are liable to get `-Woverloaded-virtual` warnings from
this. Yes, there is a workaround in putting `using
BaseClass::virtual_method;` next to overrides, but since I made the
`run`-delegation chain cheaper that would have required *far* more
`using` statements. I figured it was less obnoxious to just slightly
vary the names than force all the commands to do that.

Helps with NixOS/rfcs#134
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 7, 2023
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.

However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.

This isn't so clear to use.

What this commit does is make those command classes like the others, with richer run functions.

Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:

- `prepare` and `load` are gone entirely! One command just hands just
  hands off to the next.

- `useDefaultInstallables` because `defaultInstallables`. This takes
  over "prepare" for the one case that needs it, it provides enough
  flexiblity to handle `nix repl`'s idiosyncratic migration.

- We can use `ref` instead of `std::shared_ptr`. The former must be
  initialized (so it is like Rust's `Box` rather than `Option<Box>`,
  This expresses the invariant that the installable are in fact
  initialized much better.

  This is possible because since we just have local variables not
  fields, we can stop worrying about the not-yet-initialized case.

- Fewer lines of code! (Finally I have a large refactor that makes the
  number go down not up...)

- `nix repl` is now implemented in a clearer way.

The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.

To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.

This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.

Finally do note that I stopped overloading the `run` functions. The
reason was we are liable to get `-Woverloaded-virtual` warnings from
this. Yes, there is a workaround in putting `using
BaseClass::virtual_method;` next to overrides, but since I made the
`run`-delegation chain cheaper that would have required *far* more
`using` statements. I figured it was less obnoxious to just slightly
vary the names than force all the commands to do that.

Helps with NixOS/rfcs#134
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 8, 2023
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.

However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.

This isn't so clear to use.

What this commit does is make those command classes like the others, with richer run functions.

Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:

- `prepare` and `load` are gone entirely! One command just hands just
  hands off to the next.

- `useDefaultInstallables` because `defaultInstallables`. This takes
  over `prepare` for the one case that needs it, and provides enough
  flexiblity to handle `nix repl`'s idiosyncratic migration.

- We can use `ref` instead of `std::shared_ptr`. The former must be
  initialized (so it is like Rust's `Box` rather than `Option<Box>`,
  This expresses the invariant that the installable are in fact
  initialized much better.

  This is possible because since we just have local variables not
  fields, we can stop worrying about the not-yet-initialized case.

- Fewer lines of code! (Finally I have a large refactor that makes the
  number go down not up...)

- `nix repl` is now implemented in a clearer way.

The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.

To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.

This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.

Finally do note that I stopped overloading the `run` functions. The
reason was we are liable to get `-Woverloaded-virtual` warnings from
this. Yes, there is a workaround in putting `using
BaseClass::virtual_method;` next to overrides, but since I made the
`run`-delegation chain cheaper that would have required *far* more
`using` statements. I figured it was less obnoxious to just slightly
vary the names than force all the commands to do that.

Helps with NixOS/rfcs#134
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 9, 2023
These methods would previously fail on the other `Installable`s, so
moving them to this class is more correct as to where they actually
work.

Additionally, a `InstallableValueCommand` is created to make it easier
(or rather no worse than before) to write commands that just work on
`InstallableValue`s.

Besides being a cleanup to avoid failing default methods, this gets us
closer to NixOS/rfcs#134.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 10, 2023
Do this as opposed to associating it with each `DerivedPath` produced by
the installable. (Installables can produce multiple). `getExtraPathInfo`
is a new method which gets instead.

Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are moved
to `InstallableValue`.

To my pleasant surprise, the extra path infos associated with each
`DerivedPath` of a given installable were all the same, so this was an
easy refactor with no behavior change.

I did this because NixOS/rfcs#134 ; with these
things moved to `InstallableValue`, the base `Installable` no longer
depends on libexpr! This is a major step towards that.
@Ericson2314 Ericson2314 restored the nix-store-layer branch March 10, 2023 16:54
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 15, 2023
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.

However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.

This isn't so clear to use.

What this commit does is make those command classes like the others, with richer run functions.

Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:

- `prepare` and `load` are gone entirely! One command just hands just
  hands off to the next.

- `useDefaultInstallables` because `defaultInstallables`. This takes
  over `prepare` for the one case that needs it, and provides enough
  flexiblity to handle `nix repl`'s idiosyncratic migration.

- We can use `ref` instead of `std::shared_ptr`. The former must be
  initialized (so it is like Rust's `Box` rather than `Option<Box>`,
  This expresses the invariant that the installable are in fact
  initialized much better.

  This is possible because since we just have local variables not
  fields, we can stop worrying about the not-yet-initialized case.

- Fewer lines of code! (Finally I have a large refactor that makes the
  number go down not up...)

- `nix repl` is now implemented in a clearer way.

The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.

To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.

This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.

Finally do note that I stopped overloading the `run` functions. The
reason was we are liable to get `-Woverloaded-virtual` warnings from
this. Yes, there is a workaround in putting `using
BaseClass::virtual_method;` next to overrides, but since I made the
`run`-delegation chain cheaper that would have required *far* more
`using` statements. I figured it was less obnoxious to just slightly
vary the names than force all the commands to do that.

Helps with NixOS/rfcs#134
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 15, 2023
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.

However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.

This isn't so clear to use.

What this commit does is make those command classes like the others,
with richer `run` functions.

Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:

- `prepare` and `load` are gone entirely! One command just hands just
  hands off to the next.

- `useDefaultInstallables` because `defaultInstallables`. This takes
  over `prepare` for the one case that needs it, and provides enough
  flexiblity to handle `nix repl`'s idiosyncratic migration.

- We can use `ref` instead of `std::shared_ptr`. The former must be
  initialized (so it is like Rust's `Box` rather than `Option<Box>`,
  This expresses the invariant that the installable are in fact
  initialized much better.

  This is possible because since we just have local variables not
  fields, we can stop worrying about the not-yet-initialized case.

- Fewer lines of code! (Finally I have a large refactor that makes the
  number go down not up...)

- `nix repl` is now implemented in a clearer way.

The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.

To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.

This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.

The "diagnostic ignored `-Woverloaded-virtual`" pragma helps because C++
doesn't like our many `run` methods. In our case, we don't mind the
shadowing it all --- it is *intentional* that the derived class only
provides a `run` method, and doesn't call any of the overridden `run`
methods.

Helps with NixOS/rfcs#134
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 15, 2023
These methods would previously fail on the other `Installable`s, so
moving them to this class is more correct as to where they actually
work.

Additionally, a `InstallableValueCommand` is created to make it easier
(or rather no worse than before) to write commands that just work on
`InstallableValue`s.

Besides being a cleanup to avoid failing default methods, this gets us
closer to NixOS/rfcs#134.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 15, 2023
Do this as opposed to associating it with each `DerivedPath` produced by
the installable. (Installables can produce multiple). `getExtraPathInfo`
is a new method which gets instead.

Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are moved
to `InstallableValue`.

To my pleasant surprise, the extra path infos associated with each
`DerivedPath` of a given installable were all the same, so this was an
easy refactor with no behavior change.

I did this because NixOS/rfcs#134 ; with these
things moved to `InstallableValue`, the base `Installable` no longer
depends on libexpr! This is a major step towards that.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 15, 2023
These methods would previously fail on the other `Installable`s, so
moving them to this class is more correct as to where they actually
work.

Additionally, a `InstallableValueCommand` is created to make it easier
(or rather no worse than before) to write commands that just work on
`InstallableValue`s.

Besides being a cleanup to avoid failing default methods, this gets us
closer to NixOS/rfcs#134.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 17, 2023
These methods would previously fail on the other `Installable`s, so
moving them to this class is more correct as to where they actually
work.

Additionally, a `InstallableValueCommand` is created to make it easier
(or rather no worse than before) to write commands that just work on
`InstallableValue`s.

Besides being a cleanup to avoid failing default methods, this gets us
closer to NixOS/rfcs#134.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 17, 2023
Do this as opposed to associating it with each `DerivedPath` produced by
the installable. (Installables can produce multiple). `getExtraPathInfo`
is a new method which gets instead.

Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are moved
to `InstallableValue`.

To my pleasant surprise, the extra path infos associated with each
`DerivedPath` of a given installable were all the same, so this was an
easy refactor with no behavior change.

I did this because NixOS/rfcs#134 ; with these
things moved to `InstallableValue`, the base `Installable` no longer
depends on libexpr! This is a major step towards that.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 24, 2023
Do this as opposed to associating it with each `DerivedPath` produced by
the installable. (Installables can produce multiple). `getExtraPathInfo`
is a new method which gets instead.

Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are moved
to `InstallableValue`.

To my pleasant surprise, the extra path infos associated with each
`DerivedPath` of a given installable were all the same, so this was an
easy refactor with no behavior change.

I did this because NixOS/rfcs#134 ; with these
things moved to `InstallableValue`, the base `Installable` no longer
depends on libexpr! This is a major step towards that.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Mar 24, 2023
Instead of having a bunch of optional fields, have a few subclasses
which can have mandatory fields.

Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are
moved to `InstallableValue`.

I did these things because NixOS/rfcs#134 ; with
these things moved to `InstallableValue`, the base `Installable` no
longer depends on libexpr! This is a major step towards that.

Also, add a bunch of doc comments for sake of the internal API docs.
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/german-federal-funding-for-foss-development/29036/4

@infinisil infinisil added status: accepted and removed status: FCP in Final Comment Period labels Aug 23, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nixcon-governance-workshop/32705/9

KAction pushed a commit to KAction/rfcs that referenced this pull request Apr 13, 2024
* nix-store-layer: Copy template

* nix-store-layer: First draft

* nix-store-layer: pluralism -> marketplace of ideas

Thanks @fricklerhandwerk for the suggestion.

* nix-store-layer: Tweak Tvix and go-nix section

* nix-store-layer: Make Store team future work

* nix-store-layer: Start tidying Guix section

* nix-store-layer: Boild down Guix section further

Implementation discussion is unneeded for now.

* nix-store-layer: Make incrementality of design clear

Do this by putting the steps in order with numbers, and showing how the
hardest part can come last.

Thanks @fricklerhandwerk for the suggestion.

* nix-store-layer: Give RFC number

* nix-store-layer: Fix typo

Meaning was reversed!

* Update rfcs/0134-nix-store-layer.md

Thanks!

Co-authored-by: Tor Bjornrud <bjornrud@users.noreply.github.com>

* nix-store-layer: Fix typo

Thanks!

Co-authored-by: Adam Joseph <54836058+amjoseph-nixpkgs@users.noreply.github.com>

* Apply suggestions from code review

Thanks so much!!!

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

* Apply suggestions from code review

Thanks @fricklerhandwerk, just tweaked a few things from your suggestions to make these.

* nix-store-layer: Cut stabilization in future work down

Can just refer to NixOS#136 now.

* Apply suggestions from code review

Co-authored-by: Linus Heckemann <git@sphalerite.org>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

* 134: We have a shepherd team!

Co-authored-by: Linus Heckemann <git@sphalerite.org>

* 134: Move manual and store-specific tests to future work

As discussed in today's meeting.

* 134: Alt name moved to future work

---------

Co-authored-by: Tor Bjornrud <bjornrud@users.noreply.github.com>
Co-authored-by: Adam Joseph <54836058+amjoseph-nixpkgs@users.noreply.github.com>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Co-authored-by: Linus Heckemann <git@sphalerite.org>
@Ericson2314 Ericson2314 deleted the nix-store-layer branch June 26, 2024 15:15
@Ericson2314
Copy link
Member Author

NixOS/nix#9063 general layering update — here's a PR for Flakes moved to a new library!

I can't believe it took me so long to get around to this fairy easy thing 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.