Skip to content

Comments

Try to fix build failure#7604

Merged
roberth merged 1 commit intoNixOS:masterfrom
obsidiansystems:fix-variant-missing-raw
Jan 16, 2023
Merged

Try to fix build failure#7604
roberth merged 1 commit intoNixOS:masterfrom
obsidiansystems:fix-variant-missing-raw

Conversation

@Ericson2314
Copy link
Member

Failure: https://hydra.nixos.org/build/205357257/nixlog/1

The problem seems to be trying to std::visit a derived class of std::variant. Per
https://stackoverflow.com/questions/63616709/incomplete-type-stdvariant-used-in-nested-name-specifier certain C++ standard library implementations allow this, but others do not.

The solution is simply to call the raw method, which upcasts the reference back to the std::variant.

Failure: https://hydra.nixos.org/build/205357257/nixlog/1

The problem seems to be trying to `std::visit` a derived class of
`std::variant`. Per
https://stackoverflow.com/questions/63616709/incomplete-type-stdvariant-used-in-nested-name-specifier
certain C++ standard library implementations allow this, but others do
not.

The solution is simply to call the `raw` method, which upcasts the
reference back to the `std::variant`.
Ericson2314 referenced this pull request Jan 15, 2023
`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.
@roberth
Copy link
Member

roberth commented Jan 16, 2023

Perhaps we shouldn't subclass std::variant then?

@roberth roberth merged commit c133e66 into NixOS:master Jan 16, 2023
@Ericson2314
Copy link
Member Author

We could private subclass It which will force the use of raw but still allow using for the constructors I think, as another option.

@edolstra
Copy link
Member

@Ericson2314
Copy link
Member Author

OK will keep looking.

@Ericson2314
Copy link
Member Author

#7613

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants