Skip to content

Commit

Permalink
Allow substitutes for builds that have preferLocalBuild set
Browse files Browse the repository at this point in the history
Not substituting builds with "preferLocalBuild = true" was a bad idea,
because it didn't take the cost of dependencies into account. For
instance, if we can't substitute a fetchgit call, then we have to
download/build git and all its dependencies.

Partially reverts 5558652 and adds a
new derivation attribute "allowSubstitutes" to specify whether a
derivation may be substituted.
  • Loading branch information
edolstra committed Jun 4, 2015
1 parent b190f77 commit b64988b
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 10 deletions.
22 changes: 14 additions & 8 deletions src/libstore/build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,7 @@ void DerivationGoal::haveDerivation()
/* We are first going to try to create the invalid output paths
through substitutes. If that doesn't work, we'll build
them. */
if (settings.useSubstitutes && !willBuildLocally(drv))
if (settings.useSubstitutes && substitutesAllowed(drv))
foreach (PathSet::iterator, i, invalidOutputs)
addWaitee(worker.makeSubstitutionGoal(*i, buildMode == bmRepair));

Expand Down Expand Up @@ -1196,13 +1196,6 @@ PathSet outputPaths(const DerivationOutputs & outputs)
}


static string get(const StringPairs & map, const string & key)
{
StringPairs::const_iterator i = map.find(key);
return i == map.end() ? (string) "" : i->second;
}


static bool canBuildLocally(const string & platform)
{
return platform == settings.thisSystem
Expand All @@ -1213,12 +1206,25 @@ static bool canBuildLocally(const string & platform)
}


static string get(const StringPairs & map, const string & key, const string & def = "")
{
StringPairs::const_iterator i = map.find(key);
return i == map.end() ? def : i->second;
}


bool willBuildLocally(const Derivation & drv)
{
return get(drv.env, "preferLocalBuild") == "1" && canBuildLocally(drv.platform);
}


bool substitutesAllowed(const Derivation & drv)
{
return get(drv.env, "allowSubstitutes", "1") == "1";
}


void DerivationGoal::tryToBuild()
{
trace("trying to build");
Expand Down
4 changes: 2 additions & 2 deletions src/libstore/misc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ void queryMissing(StoreAPI & store, const PathSet & targets,
if (invalid.empty()) continue;

todoDrv.insert(*i);
if (settings.useSubstitutes && !willBuildLocally(drv))
if (settings.useSubstitutes && substitutesAllowed(drv))
query.insert(invalid.begin(), invalid.end());
}

Expand All @@ -144,7 +144,7 @@ void queryMissing(StoreAPI & store, const PathSet & targets,

PathSet outputs;
bool mustBuild = false;
if (settings.useSubstitutes && !willBuildLocally(drv)) {
if (settings.useSubstitutes && substitutesAllowed(drv)) {
foreach (DerivationOutputs::iterator, j, drv.outputs) {
if (!wantOutput(j->first, i2.second)) continue;
if (!store.isValidPath(j->second.path)) {
Expand Down
2 changes: 2 additions & 0 deletions src/libstore/misc.hh
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,7 @@ void queryMissing(StoreAPI & store, const PathSet & targets,

bool willBuildLocally(const Derivation & drv);

bool substitutesAllowed(const Derivation & drv);


}

3 comments on commit b64988b

@copumpkin
Copy link
Member

Choose a reason for hiding this comment

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

So writeText would have allowSubstitutes = false to minimize cache request noise?

@edolstra
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah.

@vcunat
Copy link
Member

@vcunat vcunat commented on b64988b Jun 5, 2015

Choose a reason for hiding this comment

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

It would be nice if preferLocalBuild and allowSubstitutes were not accounted in hash computation (and, to be safe, they shouldn't be in builder's scope).

Please sign in to comment.