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

Fix why-depends for CA derivations #7337

Merged
merged 2 commits into from
Nov 23, 2022
Merged

Conversation

Radvendii
Copy link
Contributor

@Radvendii Radvendii commented Nov 23, 2022

why-depends assumed that we knew the output path of the second argument. For CA derivations, we might not know until it's built. One way to solve this would be to build the second installable to get the output path.

In this case we don't need to, though. If the first installable (A) depends on the second (B), then getting the store path of A will necessitate having the store path B. The contrapositive is, if the store path of B is not known (i.e. it's a CA derivation which hasn't been built), then A does not depend on B.

Fix the remaining issue in #4661

why-depends assumed that we knew the output path of the second argument.
For CA derivations, we might not know until it's built. One way to solve
this would be to build the second installable to get the output path.

In this case we don't need to, though. If the first installable (A)
depends on the second (B), then getting the store path of A will
necessitate having the store path B. The contrapositive is, if the store
path of B is not known (i.e. it's a CA derivation which hasn't been
built), then A does not depend on B.
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

A minor request, but 👍 otherwise. Thanks :)

Can you just add a test? I think it's enough to do like in https://github.com/NixOS/nix/blob/master/tests/ca/build-dry.sh, just re-source the why-depends.sh test but with NIX_TESTS_CA_BY_DEFAULT=1

src/nix/why-depends.cc Show resolved Hide resolved
@Radvendii
Copy link
Contributor Author

Is it worth wrapping this:

std::visit(overloaded {
            [](const DerivedPath::Opaque & nodrv) -> std::optional<StorePath> {
                return { nodrv.path };
            },
            [&](const DerivedPath::Built & hasdrv) -> std::optional<StorePath> {
                if (hasdrv.outputs.size() != 1) {
                    throw Error("argument '%s' should evaluate to one store path", dependency->what());
                }
                auto outputMap = store->queryPartialDerivationOutputMap(hasdrv.drvPath);
                auto maybePath = outputMap.find(*hasdrv.outputs.begin());
                if (maybePath == outputMap.end()) {
                    throw Error("unexpected end of iterator");
                }
                return maybePath->second;
            },
        }, derivedDependency.raw())

into a function? This seems like a common thing to want "give me the store path if you already have it, otherwise nullopt"

@thufschmitt
Copy link
Member

Yes, good idea. But we probably also need to rethink the whole Installable::toStorePath logic because it's just broken when realise isn't Realise::Outputs. So I'd rather keep this for later.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-43/25185/1

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