`OutputSpec` for `DerivationGoal` and `DerivedPath`, today's `OutputSpec` -> `ExtendedOutputSpec` #6815

Ericson2314 · 2022-07-17T03:27:18Z

DerivedPath::Built and DerivationGoal were previously using a regular std::set<std::string< with the convention that the empty set means all outputs. But it is easy to forget about this rule when processing those sets. Using OutputsSpec forces us to get it right.

I did this because of the deduplication @edolstra requested in #4543 (comment). We can't do much good code reuse when DerivedPath doesn't use OutputsSpec like it should.

We need to split OutputsSpec from ExtendedOutputsSpec because the "default" case only makes sense for high-level installables with an outputsToInstall field. For DerivationGoal, and DerivedPath itself, only concrete sets and the "all" wild-card make sense.

Ericson2314 · 2022-11-14T14:50:19Z

Hooray it is fixed!

nixos-discourse · 2022-11-24T10:48:37Z

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

https://discourse.nixos.org/t/2022-11-14-nix-team-meeting-minutes-8/23452/1

Ericson2314 · 2022-12-13T18:29:34Z

@edolstra I rebased and in the final 4th commit cleaned up the parser, following up from #4543. This is ready for review!

Ericson2314 · 2022-12-13T18:29:58Z

It is possible to review this one commit-by-commit, and that might make it easier.

src/libcmd/installables.cc

edolstra · 2022-12-19T13:15:13Z

src/libcmd/installables.cc

@@ -662,7 +664,9 @@ std::tuple<std::string, FlakeRef, InstallableValue::DerivationInfo> InstallableF
            priority = aPriority->getInt();
    }

-    if (outputsToInstall.empty() || std::get_if<AllOutputs>(&extendedOutputsSpec)) {
+    auto * outputsSpec = std::get_if<ExtendedOutputsSpec::Explicit>(&extendedOutputsSpec);


I rewrote this logic to hopefully be much easier to read. There was some stuff that just seemed redundant? I deleted it but do check.

src/libstore/outputs-spec.cc

edolstra · 2022-12-19T13:23:00Z

src/libstore/build/derivation-goal.cc

+        [&](const OutputsSpec::Names & names) {
+            return names;
+        },
+    }, wantedOutputs.raw());


This is very similar to the code above (realWantedOutputs), so maybe this could be merged into a method on OutputSpec, e.g. outputSpec.names_or(...).

tomberek · 2022-12-19T13:25:18Z

src/libstore/build/derivation-goal.cc

-    auto wantedOutputsLeft = wantedOutputs;
+    auto wantedOutputsLeft = std::visit(overloaded {
+        [&](const OutputsSpec::All &) {
+            return StringSet {};


presumably this is to minimize changes to the rest of this function. Should there be a TODO here?

Hehe I thought the same, but I think this is actually the natural way for this one specific check --- which is "did we refer to any outputs that don't exist?"

edolstra · 2022-12-19T13:27:43Z

src/libstore/misc.cc

@@ -301,4 +301,49 @@ std::map<DrvOutput, StorePath> drvOutputReferences(
    return drvOutputReferences(Realisation::closure(store, inputRealisations), info->references);
 }

+OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd, Store * evalStore0)


Maybe rename evalStore0 to evalStore_. That's what we usually do for shadowed names.

Actually why does evalStore need to be passed as a pointer instead of a reference?

Did the rename, I will try to keep that pattern in mind going forward! :)

Responding to the question: In the header it's has a default argument: evalStore = nullptr. I would like to do evalStore = *this and make it a reference but I don't think that is valid C++?

src/libstore/outputs-spec.cc

src/libcmd/installables.cc

nixos-discourse · 2022-12-19T16:59:46Z

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

https://discourse.nixos.org/t/2022-12-12-nix-team-meeting-minutes-16/24119/1

nixos-discourse · 2022-12-19T17:23:17Z

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

https://discourse.nixos.org/t/2022-12-19-nix-team-meeting-minutes-18/24121/1

roberth

Some comments and suggestions for testing. I'm not too clever and I have the memory of a goldfish, so we have to do better with testing. This code in particular would benefit from unit tests, for the obvious reason I just stated, but also as a way to "document" the json interface, which should be stable. Lastly testing may reveal aspects that we might not otherwise think of.

Unofficial checklist.

is the idea good? has it been discussed by the Nix team?
unit tests
functional tests (tests/**.sh)
- reused
documentation in the manual
- is this purely refactoring? yes. was the documentation for outputs up to par? might be best to separate that out; scoped out for now
documentation in the code (if necessary; ideally code is already clear)
- will judge when the tests are written
documentation in the commit message (why was this change made? for future reference when maintaining the code)
n/a documentation in the changelog (to announce features and fixes to existing users who might have to do something to finally solve their problem, and to summarize the development history)

src/libstore/tests/outputs-spec.cc

src/libstore/outputs-spec.cc

src/libstore/tests/outputs-spec.cc

src/libstore/outputs-spec.hh

- Add a comment - Put `OutputsSpec` in a different header (First part of NixOS#6815) - Make a few stray uses of it in new code use `DerivedPath` instead.

Ericson2314 · 2023-01-10T22:50:15Z

I taught myself in #7543 how to write the tests, I'll return to this once that one is merge (they overlap slightly, so conflicts either way).

tfc · 2023-01-11T17:51:12Z

src/libstore/outputs-spec.hh

 #include "nlohmann/json_fwd.hpp"

 namespace nix {

-typedef std::set<std::string> OutputNames;
+struct OutputNames : std::set<std::string> {


the STL container classes are not meant to be inherited from. Please don't.

Is it a problem when it's a "newtype" like class --- exactly one superclass/field and so hopefully the same representation?

Hm... I see that you try to make it harder to create an empty set.
Unfortunately I wouldn't deem this approach to be very effective since you can still write auto myEmptyInstance = OutputNames({}); and it does not even look wrong, because the typename OutputNames does not convey that it's meant to never be empty. For this reason, one still has to put the same effort into the review to find out if we really never construct non-empty sets.

From a design perspective, this is not a case for inheritance.
I suggest to either drop this subclass which is from a design perspective more harm than good, or implement a class template <typename T> class NonEmptySet that has an std::set and that really implements the semantics of a non-empty data structure.

For what it's worth, there was one "builder pattern" type situation where I did need to start with the empty set as an identity for merging proper ones in. So I dont really mind that there is an escape hatch. Of course yes the best thing to do would be to use a separate type for this one instance and make it ironclad.

A few little changes preparing for the rest.

Do this prior to making a new more limitted `OutputPath` we will use in more places.

`DerivedPath::Built` and `DerivationGoal` were previously using a regular set with the convention that the empty set means all outputs. But it is easy to forget about this rule when processing those sets. Using `OutputSpec` forces us to get it right.

This should be a non-empty set, and so we don't want people doing this by accident. We remove the zero-0 constructor with a little inheritance trickery.

This forces us to be explicit. It also requires to rework how `from_json` works. A `JSON_IMPL` is added to assist with this.

tfc · 2023-01-12T07:12:59Z

src/libstore/outputs-spec.hh

 };

-struct DefaultOutputs {
-    bool operator < (const DefaultOutputs & _) const { return false; }
+struct AllOutputs : std::monostate { };


I also suggest to not subclass this standard class. Looking over the code, it does not seem to bring much advantage.

https://en.cppreference.com/w/cpp/utility/variant/monostate It gives me all the comparison functions and other such things I need.

It seems like that would be an appropriate use of monostate. It is a small class with a clear purpose, and by subclassing we make its meaning explicit.

What we're really doing here is declaring an algebraic data type, in a verbose, C++ way. Maybe that's why I don't see a problem, or may that's why there won't be a problem. Or maybe I'm misreading @tfc who isn't saying that this will be a problem per se and it isn't quite related to the comment about subclassing stl containers.

tfc · 2023-01-12T07:13:35Z

src/libstore/outputs-spec.hh

+
+typedef std::variant<AllOutputs, OutputNames> _OutputsSpecRaw;
+
+struct OutputsSpec : _OutputsSpecRaw {


And here we have another subclass of STL standard classes... please, please, don't.

See #7479 we have an issue discussing this pattern. I am for it as the best that can be done for Sum types until we switch to Rust.

tfc · 2023-01-12T07:14:06Z

src/libstore/outputs-spec.hh

 };

-typedef std::variant<DefaultOutputs, AllOutputs, OutputNames> OutputsSpec;
+struct DefaultOutputs : std::monostate { };


same pattern here. I have seen many code bases where developers subclassed stl classes - none of them aged well.

Additionally get rid of the evil time we made an empty `OutputSpec::Names()`.

roberth

Looks really good, especially with the new tests, thanks @Ericson2314!
I'm also approving of using the established algebraic data type pattern, considering that it is better to discuss it as part of #7479. This new usage of it would not constitute any significant tech debt, as already a number of ADTs would have to be migrated to a different pattern.

roberth · 2023-01-13T14:37:48Z

src/libstore/outputs-spec.hh

 };

-struct DefaultOutputs {
-    bool operator < (const DefaultOutputs & _) const { return false; }
+struct AllOutputs : std::monostate { };


It seems like that would be an appropriate use of monostate. It is a small class with a clear purpose, and by subclassing we make its meaning explicit.

What we're really doing here is declaring an algebraic data type, in a verbose, C++ way. Maybe that's why I don't see a problem, or may that's why there won't be a problem. Or maybe I'm misreading @tfc who isn't saying that this will be a problem per se and it isn't quite related to the comment about subclassing stl containers.

roberth · 2023-01-13T15:04:14Z

Merged to unblock further work. The remaining discussion wasn't critical and can be continued in #7479

edolstra · 2023-01-16T12:47:32Z

@Ericson2314 Looks like this broke build.aarch64-linux, see https://hydra.nixos.org/build/205195243.

Ericson2314 · 2023-01-16T14:10:58Z

@edolstra @roberth told me about the problem and I made #7604 which I hope fixes it? Though I wasn't able to test it.

roberth · 2023-01-21T22:53:25Z

I suspect that this PR has caused the regression:

Regression builtins.getContext / outputs = [ "" ] #7655

nixos-discourse · 2023-04-21T11:58:03Z

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

https://discourse.nixos.org/t/nix-team-report-2022-10-2023-03/27486/1

Ericson2314 mentioned this pull request Jul 17, 2022

Low level <drvPath>^<outputName> installable syntax to match existing <highLevelInstallable>^<outputNames> syntax #4543

Merged

Ericson2314 force-pushed the better-wanted-outputs branch from 3f3a8da to 9042c5a Compare November 1, 2022 15:29

Ericson2314 force-pushed the better-wanted-outputs branch from 9042c5a to 65b39ea Compare November 14, 2022 13:54

Ericson2314 marked this pull request as ready for review November 14, 2022 13:55

Ericson2314 changed the title ~~OutputSpec for DerivationGoal and DerivedPath, today's OutputSpec -> ExtendedOutputSpec`~~ OutputSpec for DerivationGoal and DerivedPath, today's OutputSpec -> ExtendedOutputSpec Nov 14, 2022

Ericson2314 requested a review from edolstra November 14, 2022 20:38

thufschmitt assigned edolstra Nov 18, 2022

Ericson2314 mentioned this pull request Nov 21, 2022

Tracking issue for RFC 92: Dynamic derivations #6316

Open

4 tasks

Ericson2314 force-pushed the better-wanted-outputs branch from 65b39ea to 4cb5959 Compare December 12, 2022 13:03

Ericson2314 requested a review from thufschmitt as a code owner December 12, 2022 13:03

Ericson2314 force-pushed the better-wanted-outputs branch from 4cb5959 to f41e2b2 Compare December 13, 2022 18:01

Ericson2314 force-pushed the better-wanted-outputs branch from f41e2b2 to 5cdd58a Compare December 13, 2022 18:36