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

Make buildPathsWithResults() only return info on wanted outputs #6311

Merged
merged 6 commits into from
Mar 25, 2022
Merged

Make buildPathsWithResults() only return info on wanted outputs #6311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
175c785
Random cleanup
edolstra Mar 24, 2022
09796c0
Random cleanup
edolstra Mar 24, 2022
fe5509d
Only return wanted outputs
edolstra Mar 24, 2022
540d7e3
Retry substitution after an incomplete closure only once
edolstra Mar 24, 2022
187dc08
tests/build.sh: Test that 'nix build' only prints wanted outputs
edolstra Mar 24, 2022
50c229a
Use wantOutput
edolstra Mar 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions src/libstore/build/derivation-goal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,6 @@ void DerivationGoal::haveDerivation()
if (!drv->type().hasKnownOutputPaths())
settings.requireExperimentalFeature(Xp::CaDerivations);

retrySubstitution = false;

for (auto & i : drv->outputsAndOptPaths(worker.store))
if (i.second.second)
worker.store.addTempRoot(*i.second.second);
Expand Down Expand Up @@ -311,14 +309,11 @@ void DerivationGoal::outputsSubstitutionTried()
gaveUpOnSubstitution();
}


/* At least one of the output paths could not be
produced using a substitute. So we have to build instead. */
void DerivationGoal::gaveUpOnSubstitution()
{
/* Make sure checkPathValidity() from now on checks all
outputs. */
wantedOutputs.clear();
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't see any reason why checkPathValidity() needs to check all outputs. In any case, messing with wantedOutputs seems dirty...

Copy link
Member

Choose a reason for hiding this comment

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

I agree it is dirty, but the idea I think (I did not write that but remember reading it IIRC) that since we are now doing the build, we will be making all afresh outputs, and we should fail if the derivation did not produce all the outputs even if we already had the missing outputs before.

Without this, we run the risk of some wonky non-deterministic derivation that produces a different subset of the outputs each time not failing as it should.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, another reason is that if during a build we discover we in fact need other outputs, it is nice to know we don't need to restart a build that got so upgraded. See DerivationGoal::addWantedOutputs

Copy link
Member

@Ericson2314 Ericson2314 Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A consequence of this is that I think buildPathsWithResults is going to need to filter the results based on the derived path. I will make a PR removing the derived path from the build result stored by the derivation goal, because I no longer think it is correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I can tell, what outputs get build/registered (or whose absence causes the build to fail) is unaffected by wantedOutputs or initialOutputs.*.wanted. So I think it should be okay. Restarts are only needed in the substitution case, not when we're actually building.

Copy link
Member

Choose a reason for hiding this comment

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

@edolstra On what basis do you think this?

See Worker::makeDerivationGoalCommon. If a new goal needs to be created that overlaps with an existing goal, this function is called. This may occur at any point over the derivation goal's lifecycle.

Copy link
Member

Choose a reason for hiding this comment

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

I will fix this in the other PR too, sigh.

Copy link
Member Author

Choose a reason for hiding this comment

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

Think what? I'm not sure I understand the problem...

Copy link
Member

Choose a reason for hiding this comment

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

See the long PR description in #6312


/* The inputs must be built before we can build this goal. */
if (useDerivation)
for (auto & i : dynamic_cast<Derivation *>(drv.get())->inputDrvs)
Expand Down Expand Up @@ -426,7 +421,8 @@ void DerivationGoal::inputsRealised()
return;
}

if (retrySubstitution) {
if (retrySubstitution && !retriedSubstitution) {
retriedSubstitution = true;
haveDerivation();
return;
}
Expand Down
8 changes: 6 additions & 2 deletions src/libstore/build/derivation-goal.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,12 @@ struct DerivationGoal : public Goal
bool needRestart = false;

/* Whether to retry substituting the outputs after building the
inputs. */
bool retrySubstitution;
inputs. This is done in case of an incomplete closure. */
bool retrySubstitution = false;

/* Whether we've retried substitution, in which case we won't try
again. */
bool retriedSubstitution = false;

/* The derivation stored at drvPath. */
std::unique_ptr<Derivation> drv;
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/build/goal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ void Goal::addWaitee(GoalPtr waitee)

void Goal::waiteeDone(GoalPtr waitee, ExitCode result)
{
assert(waitees.find(waitee) != waitees.end());
assert(waitees.count(waitee));
waitees.erase(waitee);

trace(fmt("waitee '%s' done; %d left", waitee->name, waitees.size()));
Expand Down
13 changes: 5 additions & 8 deletions src/libstore/build/goal.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,21 @@ struct Goal : public std::enable_shared_from_this<Goal>
WeakGoals waiters;

/* Number of goals we are/were waiting for that have failed. */
unsigned int nrFailed;
size_t nrFailed = 0;

/* Number of substitution goals we are/were waiting for that
failed because there are no substituters. */
unsigned int nrNoSubstituters;
size_t nrNoSubstituters = 0;

/* Number of substitution goals we are/were waiting for that
failed because they had unsubstitutable references. */
unsigned int nrIncompleteClosure;
size_t nrIncompleteClosure = 0;

/* Name of this goal for debugging purposes. */
std::string name;

/* Whether the goal is finished. */
ExitCode exitCode;
ExitCode exitCode = ecBusy;

/* Build result. */
BuildResult buildResult;
Expand All @@ -65,10 +65,7 @@ struct Goal : public std::enable_shared_from_this<Goal>
Goal(Worker & worker, DerivedPath path)
: worker(worker)
, buildResult { .path = std::move(path) }
{
nrFailed = nrNoSubstituters = nrIncompleteClosure = 0;
exitCode = ecBusy;
}
{ }

virtual ~Goal()
{
Expand Down
3 changes: 2 additions & 1 deletion src/libstore/build/local-derivation-goal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2613,7 +2613,8 @@ DrvOutputs LocalDerivationGoal::registerOutputs()
signRealisation(thisRealisation);
worker.store.registerDrvOutput(thisRealisation);
}
builtOutputs.emplace(thisRealisation.id, thisRealisation);
if (wantOutput(outputName, wantedOutputs))
builtOutputs.emplace(thisRealisation.id, thisRealisation);
}

return builtOutputs;
Expand Down
14 changes: 13 additions & 1 deletion tests/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,27 @@
source common.sh

expectedJSONRegex='\[\{"drvPath":".*multiple-outputs-a.drv","outputs":\{"first":".*multiple-outputs-a-first","second":".*multiple-outputs-a-second"}},\{"drvPath":".*multiple-outputs-b.drv","outputs":\{"out":".*multiple-outputs-b"}}]'
clearStore

# Make sure that 'nix build' only returns the outputs we asked for.
nix build -f multiple-outputs.nix --json a --no-link | jq --exit-status '
(.[0] |
(.drvPath | match(".*multiple-outputs-a.drv")) and
(.outputs | keys | length == 1) and
(.outputs.first | match(".*multiple-outputs-a-first")))
'

nix build -f multiple-outputs.nix --json a.all b.all --no-link | jq --exit-status '
(.[0] |
(.drvPath | match(".*multiple-outputs-a.drv")) and
(.outputs | keys | length == 2) and
(.outputs.first | match(".*multiple-outputs-a-first")) and
(.outputs.second | match(".*multiple-outputs-a-second")))
and (.[1] |
(.drvPath | match(".*multiple-outputs-b.drv")) and
(.outputs | keys | length == 1) and
(.outputs.out | match(".*multiple-outputs-b")))
'

testNormalization () {
clearStore
outPath=$(nix-build ./simple.nix --no-out-link)
Expand Down