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.

Sign up for GitHub

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

Jump to bottom

Fix #6572 requires non-existent output #7283

Merged
merged 2 commits into from
Nov 15, 2022
Merged

Fix #6572 requires non-existent output #7283

merged 2 commits into from
Nov 15, 2022

Conversation

roberth
Copy link
Member

@roberth roberth commented Nov 9, 2022 •
edited
Loading

Reproduces #6572

Only issue_6572_dependent_outputs fails, suggesting that dependencies between the outputs of a multi-output drv are involved.

    + nix-store --delete /run/user/1000/nix-test/tests/build/store/5fv3k7lg7cgnswgiw43xarw07xg1z1wh-multiple-outputs-a-second
    finding garbage collector roots...
    deleting '/run/user/1000/nix-test/tests/build/store/5fv3k7lg7cgnswgiw43xarw07xg1z1wh-multiple-outputs-a-second'
    deleting unused links...
    note: currently hard linking saves 0.00 MiB
    1 store paths deleted, 0.00 MiB freed
    ++ nix build -f multiple-outputs.nix use-a --no-link --print-out-paths
    these 2 derivations will be built:
      /run/user/1000/nix-test/tests/build/store/f1qc3m41ffxjfz51rs3ix17xxvihix3r-multiple-outputs-a.drv
      /run/user/1000/nix-test/tests/build/store/p101iqkw9wwqpb7f81h5hxjficl7kizi-use-a.drv
    building '/run/user/1000/nix-test/tests/build/store/f1qc3m41ffxjfz51rs3ix17xxvihix3r-multiple-outputs-a.drv'...
    error: derivation '/run/user/1000/nix-test/tests/build/store/p101iqkw9wwqpb7f81h5hxjficl7kizi-use-a.drv' requires non-existent output 'first' from input derivation '/run/user/1000/nix-test/tests/build/store/f1qc3m41ffxjfz51rs3ix17xxvihix3r-multiple-outputs-a.drv'

@roberth roberth mentioned this pull request Nov 9, 2022
Closed
@roberth roberth changed the title tests: Reproduce #6572 Fix #6572 requires non-existent output Nov 14, 2022
@roberth roberth marked this pull request as ready for review November 14, 2022 15:29
@roberth
derivation-goal: Fix requires non-existing output error
7e162c6 
It occurred when a output of the dependency was already available,
so it didn't need rebuilding and didn't get added to the
inputDrvOutputs.
This process-related info wasn't suitable for the purpose of finding
the actual input paths for the builder. It is better to do this in
absolute terms by querying the store.
@roberth
tests: Reproduce NixOS#6572
c279ddb
@roberth roberth mentioned this pull request Nov 14, 2022
Merged
13 tasks
@roberth
Copy link
Member Author

roberth commented Nov 15, 2022

Bug was present since impure derivations, Nix 2.8, so backporting to current NixOS stable as well.

@dasJ
Copy link
Member

dasJ commented Nov 15, 2022

@roberth I agree, but the test relies on --print-out-paths which is only available on 2.9+ source

@roberth
Copy link
Member Author

roberth commented Nov 15, 2022

I'll fix that in the backport prs.

@thufschmitt thufschmitt self-requested a review November 15, 2022 12:56
thufschmitt
thufschmitt approved these changes Nov 15, 2022
View reviewed changes
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.

Thanks a lot for digging into this, it's really great to see this issue fixed.

Having to query the store again is a bit sad, but hopefully one day we'll have a proper structured control flow, and we won't have to rely on this any more 🤞

@thufschmitt thufschmitt merged commit 3ade5f5 into NixOS:master Nov 15, 2022
@github-actions
Copy link

Backport failed for 2.8-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.8-maintenance
git worktree add -d .worktree/backport-7283-to-2.8-maintenance origin/2.8-maintenance
cd .worktree/backport-7283-to-2.8-maintenance
git checkout -b backport-7283-to-2.8-maintenance
ancref=$(git merge-base 0efc314d4d1add3215810f9034b6041759d5175b c279ddb18cf3a34b0f6d4e3adcf9455da5397ad7)
git cherry-pick -x $ancref..c279ddb18cf3a34b0f6d4e3adcf9455da5397ad7

@github-actions github-actions bot mentioned this pull request Nov 15, 2022
Closed
@github-actions
Copy link

Successfully created backport PR #7304 for 2.9-maintenance.

@github-actions github-actions bot mentioned this pull request Nov 15, 2022
Closed
@github-actions
Copy link

Successfully created backport PR #7305 for 2.10-maintenance.

@github-actions github-actions bot mentioned this pull request Nov 15, 2022
Merged
@github-actions
Copy link

Successfully created backport PR #7306 for 2.11-maintenance.

Kha
Kha reviewed Nov 15, 2022
View reviewed changes
src/libstore/build/derivation-goal.cc
worker.store.printStorePath(drvPath), j, worker.store.printStorePath(depDrvPath));
}
else {
auto outMap = worker.evalStore.queryDerivationOutputMap(depDrvPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something expensive that should be evaluated at most once in the loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the vast majority of cases, inputDrvOutputs will have the result. Otherwise Nix would have been completely unusable since 2.8.

We can just lift it out of the loop, because that would create many more derivation file queries; for each inputDrv, instead for only very rare cases. Even then the chance of reusing the map is fairly low, because derivations typically don't have many outputs, and this only happens for multi output derivations.

It is important to keep in mind though when implementing the suggestion in the comment above, so I'll update that.

Copy link
Member Author

Choose a reason for hiding this comment

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

#7307

@Ericson2314
Copy link
Member

@thufschmitt

Having to query the store again is a bit sad, but hopefully one day we'll have a proper structured control flow, and we won't have to rely on this any more 🤞

We should talk about this at some point because I feel the exact opposite, that the store should be the single source of truth and we should have as little in-memory state as possible.

@thufschmitt thufschmitt mentioned this pull request Nov 17, 2022
Closed
@nixos-discourse
Copy link

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

@Radvendii Radvendii mentioned this pull request Dec 2, 2022
Merged
@thufschmitt thufschmitt mentioned this pull request Dec 2, 2022
Closed
@zhaofengli zhaofengli mentioned this pull request Jan 28, 2023
Closed
@roberth roberth mentioned this pull request Mar 29, 2023
Closed
@yorickvP yorickvP mentioned this pull request Apr 4, 2023
Open
@nixos-discourse
Copy link

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

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

@Kha Kha Kha left review comments

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@thufschmitt thufschmitt thufschmitt approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@roberth @dasJ @Ericson2314 @nixos-discourse @Kha @thufschmitt @Artturin