Skip to content

Commit 4c76db8

Browse files
committed
Make sure settings.sandboxedPaths is closed outside DerivationBuilder
This is a nicer separation of concerns --- `DerivationBuilder` just mounts the extra paths you tell it too, and the outside world is responsible for making sure those extra paths make sense. Since the closure only depends on global settings, and not per-derivation information, we also have the option of moving this up further and caching it across all local builds. (I only just realized this after having done this refactor. I am not doing that change at this time, however.)
1 parent 08e42e2 commit 4c76db8

File tree

3 files changed

+27
-17
lines changed

3 files changed

+27
-17
lines changed

src/libstore/build/derivation-building-goal.cc

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,9 +677,26 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
677677
auto * localStoreP = dynamic_cast<LocalStore *>(&worker.store);
678678
assert(localStoreP);
679679

680+
decltype(DerivationBuilderParams::defaultPathsInChroot) defaultPathsInChroot = settings.sandboxPaths.get();
680681
decltype(DerivationBuilderParams::finalEnv) finalEnv;
681682
decltype(DerivationBuilderParams::extraFiles) extraFiles;
682683

684+
/* Add the closure of store paths to the chroot. */
685+
StorePathSet closure;
686+
for (auto & i : defaultPathsInChroot)
687+
try {
688+
if (worker.store.isInStore(i.second.source))
689+
worker.store.computeFSClosure(worker.store.toStorePath(i.second.source).first, closure);
690+
} catch (InvalidPath & e) {
691+
} catch (Error & e) {
692+
e.addTrace({}, "while processing sandbox path '%s'", i.second.source);
693+
throw;
694+
}
695+
for (auto & i : closure) {
696+
auto p = worker.store.printStorePath(i);
697+
defaultPathsInChroot.insert_or_assign(p, ChrootPath{.source = p});
698+
}
699+
683700
try {
684701
if (drv->structuredAttrs) {
685702
auto json = drv->structuredAttrs->prepareStructuredAttrs(
@@ -748,6 +765,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
748765
*drvOptions,
749766
inputPaths,
750767
initialOutputs,
768+
std::move(defaultPathsInChroot),
751769
std::move(finalEnv),
752770
std::move(extraFiles),
753771
});

src/libstore/include/nix/store/build/derivation-builder.hh

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,12 @@ struct DerivationBuilderParams
5959

6060
const BuildMode & buildMode;
6161

62+
/**
63+
* Extra paths we want to be in the chroot, regardless of the
64+
* derivation we are building.
65+
*/
66+
PathsInChroot defaultPathsInChroot;
67+
6268
struct EnvEntry
6369
{
6470
/**
@@ -96,6 +102,7 @@ struct DerivationBuilderParams
96102
const DerivationOptions & drvOptions,
97103
const StorePathSet & inputPaths,
98104
std::map<std::string, InitialOutput> & initialOutputs,
105+
PathsInChroot defaultPathsInChroot,
99106
std::map<std::string, EnvEntry, std::less<>> finalEnv,
100107
StringMap extraFiles)
101108
: drvPath{drvPath}
@@ -105,6 +112,7 @@ struct DerivationBuilderParams
105112
, inputPaths{inputPaths}
106113
, initialOutputs{initialOutputs}
107114
, buildMode{buildMode}
115+
, defaultPathsInChroot{std::move(defaultPathsInChroot)}
108116
, finalEnv{std::move(finalEnv)}
109117
, extraFiles{std::move(extraFiles)}
110118
{

src/libstore/unix/build/derivation-builder.cc

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -836,29 +836,13 @@ PathsInChroot DerivationBuilderImpl::getPathsInSandbox()
836836
{
837837
/* Allow a user-configurable set of directories from the
838838
host file system. */
839-
PathsInChroot pathsInChroot = settings.sandboxPaths.get();
839+
PathsInChroot pathsInChroot = defaultPathsInChroot;
840840

841841
if (hasPrefix(store.storeDir, tmpDirInSandbox())) {
842842
throw Error("`sandbox-build-dir` must not contain the storeDir");
843843
}
844844
pathsInChroot[tmpDirInSandbox()] = {.source = tmpDir};
845845

846-
/* Add the closure of store paths to the chroot. */
847-
StorePathSet closure;
848-
for (auto & i : pathsInChroot)
849-
try {
850-
if (store.isInStore(i.second.source))
851-
store.computeFSClosure(store.toStorePath(i.second.source).first, closure);
852-
} catch (InvalidPath & e) {
853-
} catch (Error & e) {
854-
e.addTrace({}, "while processing sandbox path '%s'", i.second.source);
855-
throw;
856-
}
857-
for (auto & i : closure) {
858-
auto p = store.printStorePath(i);
859-
pathsInChroot.insert_or_assign(p, ChrootPath{.source = p});
860-
}
861-
862846
PathSet allowedPaths = settings.allowedImpureHostPrefixes;
863847

864848
/* This works like the above, except on a per-derivation level */

0 commit comments

Comments
 (0)