Make command infra less stateful and more regular

[Ericson2314]

Motivation

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.

Context

Helps with NixOS/rfcs#134

Depends on #7748

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
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or bug fix: updated release notes

[nixos-discourse]

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

https://discourse.nixos.org/t/2023-02-17-nix-team-meeting-minutes-33/25624/1

[dpulls]

🎉 All dependencies have been resolved !

[Ericson2314]

This is the next one for RFC 134. I assigned @edolstra too in addition to @tomberek because @edolstra expressed an interest in reviewing these at some point, and since this one is a good bit bigger it could probably use a second pair of eyes reviewing anyways.

[tomberek]

Makes the flow of command processing more clear. Less state and more explicit passing of values.

[Ericson2314]

The macOS output seems a bit out of order? at least I am trouble reading it.

Regardless, I think this is ready for review. I don't anticipate needing to change it much to fix this error.

[Ericson2314]

(Ah good, it went away)

[nixos-discourse]

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

https://discourse.nixos.org/t/2023-03-10-nix-team-meeting-minutes-39/26279/1

[edolstra]

Could we use -Wno-overloaded-virtual to shut up the compiler?

[Ericson2314]

Could we use -Wno-overloaded-virtual to shut up the compiler?

I added a pramga for that. I can't do the push-pop thing very well because the warning would also apply in every single sub-command, as opposed to just the run in the header. But it is still better than just, say, adding the flag to CXXFLAGS.

It says pragma gcc, but It works with Clang too :).

Thanks for the review!