Skip to content

Commit fb6658c

Browse files
lberkicopybara-github
lberki
authored and
copybara-github
committed
Fix two issues with --incompatible_sandbox_hermetic_tmp that manifested themselves when the output base was under /tmp:
1. Virtual action inputs didn't work. This was a fairly simple omission in the path transformation logic. 2. Artifacts which are resolved symlinks (i.e. declared using `declare_file`) did not work. This is fixed by keeping track of the realpath of the symlink in `FileArtifactValue` / `TreeArtifactValue`. Fixes #20515 and #20518. RELNOTES: None. PiperOrigin-RevId: 595145191 Change-Id: I833025928374c78bc719d8e3be1efb2b2950b9f1
1 parent 9bf9ced commit fb6658c

14 files changed

Lines changed: 397 additions & 61 deletions

‎src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java‎

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -169,11 +169,19 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) {
169169
/**
170170
* Optional materialization path.
171171
*
172-
* <p>If present, this artifact is a copy of another artifact. It is still tracked as a
173-
* non-symlink by Bazel, but materialized in the local filesystem as a symlink to the original
174-
* artifact, whose contents live at this location. This is used by {@link
175-
* com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher} to implement zero-cost
176-
* copies of remotely stored artifacts.
172+
* <p>If present, this artifact is a copy of another artifact whose contents live at this path.
173+
* This can happen when it is declared as a file and not as an unresolved symlink but the action
174+
* that creates it materializes it in the filesystem as a symlink to another output artifact. This
175+
* information is useful in two situations:
176+
*
177+
* <ol>
178+
* <li>When the symlink target is a remotely stored artifact, we can avoid downloading it
179+
* multiple times when building without the bytes (see AbstractActionInputPrefetcher).
180+
* <li>When the symlink target is inaccessible from the sandboxed environment an action runs
181+
* under, we can rewrite it accordingly (see SandboxHelpers).
182+
* </ol>
183+
*
184+
* @see com.google.devtools.build.lib.skyframe.TreeArtifactValue#getMaterializationExecPath().
177185
*/
178186
public Optional<PathFragment> getMaterializationExecPath() {
179187
return Optional.empty();
@@ -214,6 +222,12 @@ public static FileArtifactValue createForSourceArtifact(
214222
xattrProvider);
215223
}
216224

225+
public static FileArtifactValue createForResolvedSymlink(
226+
PathFragment realPath, FileArtifactValue metadata, @Nullable byte[] digest) {
227+
return new ResolvedSymlinkFileArtifactValue(
228+
realPath, digest, metadata.getContentsProxy(), metadata.getSize());
229+
}
230+
217231
public static FileArtifactValue createFromInjectedDigest(
218232
FileArtifactValue metadata, @Nullable byte[] digest) {
219233
return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize());
@@ -439,7 +453,25 @@ public String toString() {
439453
}
440454
}
441455

442-
private static final class RegularFileArtifactValue extends FileArtifactValue {
456+
private static final class ResolvedSymlinkFileArtifactValue extends RegularFileArtifactValue {
457+
private final PathFragment realPath;
458+
459+
private ResolvedSymlinkFileArtifactValue(
460+
PathFragment realPath,
461+
@Nullable byte[] digest,
462+
@Nullable FileContentsProxy proxy,
463+
long size) {
464+
super(digest, proxy, size);
465+
this.realPath = realPath;
466+
}
467+
468+
@Override
469+
public Optional<PathFragment> getMaterializationExecPath() {
470+
return Optional.of(realPath);
471+
}
472+
}
473+
474+
private static class RegularFileArtifactValue extends FileArtifactValue {
443475
private final byte[] digest;
444476
@Nullable private final FileContentsProxy proxy;
445477
private final long size;
@@ -462,7 +494,8 @@ public boolean equals(Object o) {
462494
RegularFileArtifactValue that = (RegularFileArtifactValue) o;
463495
return Arrays.equals(digest, that.digest)
464496
&& Objects.equals(proxy, that.proxy)
465-
&& size == that.size;
497+
&& size == that.size
498+
&& Objects.equals(getMaterializationExecPath(), that.getMaterializationExecPath());
466499
}
467500

468501
@Override

‎src/main/java/com/google/devtools/build/lib/sandbox/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ java_library(
1717
deps = [
1818
"//src/main/java/com/google/devtools/build/lib/actions",
1919
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
20+
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
2021
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
2122
"//src/main/java/com/google/devtools/build/lib/cmdline",
2223
"//src/main/java/com/google/devtools/build/lib/vfs",

‎src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
228228
SandboxInputs inputs =
229229
helpers.processInputFiles(
230230
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
231+
context.getInputMetadataProvider(),
231232
execRoot,
232233
execRoot,
233234
packageRoots,

‎src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
226226
SandboxInputs inputs =
227227
helpers.processInputFiles(
228228
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
229+
context.getInputMetadataProvider(),
229230
execRoot,
230231
execRoot,
231232
packageRoots,

‎src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
281281
SandboxInputs inputs =
282282
helpers.processInputFiles(
283283
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
284+
context.getInputMetadataProvider(),
284285
execRoot,
285286
withinSandboxExecRoot,
286287
packageRoots,

‎src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
100100
SandboxInputs inputs =
101101
helpers.processInputFiles(
102102
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
103+
context.getInputMetadataProvider(),
103104
execRoot,
104105
execRoot,
105106
packageRoots,

‎src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java‎

Lines changed: 94 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static com.google.devtools.build.lib.vfs.Dirent.Type.SYMLINK;
2121

2222
import com.google.auto.value.AutoValue;
23+
import com.google.common.base.Function;
2324
import com.google.common.base.Preconditions;
2425
import com.google.common.collect.ImmutableList;
2526
import com.google.common.collect.ImmutableMap;
@@ -29,6 +30,8 @@
2930
import com.google.common.flogger.GoogleLogger;
3031
import com.google.devtools.build.lib.actions.ActionInput;
3132
import com.google.devtools.build.lib.actions.Artifact;
33+
import com.google.devtools.build.lib.actions.FileArtifactValue;
34+
import com.google.devtools.build.lib.actions.InputMetadataProvider;
3235
import com.google.devtools.build.lib.actions.Spawn;
3336
import com.google.devtools.build.lib.actions.UserExecException;
3437
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
@@ -444,6 +447,29 @@ private static RootedPath processFilesetSymlink(
444447
symlink.getPathString()));
445448
}
446449

450+
private static RootedPath processResolvedSymlink(
451+
Root absoluteRoot,
452+
PathFragment execRootRelativeSymlinkTarget,
453+
Root execRootWithinSandbox,
454+
PathFragment execRootFragment,
455+
ImmutableList<Root> packageRoots,
456+
Function<Root, Root> sourceRooWithinSandbox) {
457+
PathFragment symlinkTarget = execRootFragment.getRelative(execRootRelativeSymlinkTarget);
458+
for (Root packageRoot : packageRoots) {
459+
if (packageRoot.contains(symlinkTarget)) {
460+
return RootedPath.toRootedPath(
461+
sourceRooWithinSandbox.apply(packageRoot), packageRoot.relativize(symlinkTarget));
462+
}
463+
}
464+
465+
if (symlinkTarget.startsWith(execRootFragment)) {
466+
return RootedPath.toRootedPath(
467+
execRootWithinSandbox, symlinkTarget.relativeTo(execRootFragment));
468+
}
469+
470+
return RootedPath.toRootedPath(absoluteRoot, symlinkTarget);
471+
}
472+
447473
/**
448474
* Returns the inputs of a Spawn as a map of PathFragments relative to an execRoot to paths in the
449475
* host filesystem where the input files can be found.
@@ -458,6 +484,7 @@ private static RootedPath processFilesetSymlink(
458484
*/
459485
public SandboxInputs processInputFiles(
460486
Map<PathFragment, ActionInput> inputMap,
487+
InputMetadataProvider inputMetadataProvider,
461488
Path execRootPath,
462489
Path withinSandboxExecRootPath,
463490
ImmutableList<Root> packageRoots,
@@ -468,12 +495,24 @@ public SandboxInputs processInputFiles(
468495
withinSandboxExecRootPath.equals(execRootPath)
469496
? withinSandboxExecRoot
470497
: Root.fromPath(execRootPath);
498+
Root absoluteRoot = Root.absoluteRoot(execRootPath.getFileSystem());
471499

472500
Map<PathFragment, RootedPath> inputFiles = new TreeMap<>();
473501
Map<PathFragment, PathFragment> inputSymlinks = new TreeMap<>();
474502
Map<VirtualActionInput, byte[]> virtualInputs = new HashMap<>();
475503
Map<Root, Root> sourceRootToSandboxSourceRoot = new TreeMap<>();
476504

505+
Function<Root, Root> sourceRootWithinSandbox =
506+
r -> {
507+
if (!sourceRootToSandboxSourceRoot.containsKey(r)) {
508+
int next = sourceRootToSandboxSourceRoot.size();
509+
sourceRootToSandboxSourceRoot.put(
510+
r, Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next))));
511+
}
512+
513+
return sourceRootToSandboxSourceRoot.get(r);
514+
};
515+
477516
for (Map.Entry<PathFragment, ActionInput> e : inputMap.entrySet()) {
478517
if (Thread.interrupted()) {
479518
throw new InterruptedException();
@@ -503,23 +542,63 @@ public SandboxInputs processInputFiles(
503542

504543
if (actionInput instanceof EmptyActionInput) {
505544
inputPath = null;
545+
} else if (actionInput instanceof VirtualActionInput) {
546+
inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, actionInput.getExecPath());
506547
} else if (actionInput instanceof Artifact) {
507548
Artifact inputArtifact = (Artifact) actionInput;
508-
if (inputArtifact.isSourceArtifact() && sandboxSourceRoots != null) {
509-
Root sourceRoot = inputArtifact.getRoot().getRoot();
510-
if (!sourceRootToSandboxSourceRoot.containsKey(sourceRoot)) {
511-
int next = sourceRootToSandboxSourceRoot.size();
512-
sourceRootToSandboxSourceRoot.put(
513-
sourceRoot,
514-
Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next))));
515-
}
516-
517-
inputPath =
518-
RootedPath.toRootedPath(
519-
sourceRootToSandboxSourceRoot.get(sourceRoot),
520-
inputArtifact.getRootRelativePath());
521-
} else {
549+
if (sandboxSourceRoots == null) {
522550
inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath());
551+
} else {
552+
if (inputArtifact.isSourceArtifact()) {
553+
Root sourceRoot = inputArtifact.getRoot().getRoot();
554+
inputPath =
555+
RootedPath.toRootedPath(
556+
sourceRootWithinSandbox.apply(sourceRoot),
557+
inputArtifact.getRootRelativePath());
558+
} else {
559+
PathFragment materializationExecPath = null;
560+
if (inputArtifact.isChildOfDeclaredDirectory()) {
561+
FileArtifactValue parentMetadata =
562+
inputMetadataProvider.getInputMetadata(inputArtifact.getParent());
563+
if (parentMetadata.getMaterializationExecPath().isPresent()) {
564+
materializationExecPath =
565+
parentMetadata
566+
.getMaterializationExecPath()
567+
.get()
568+
.getRelative(inputArtifact.getParentRelativePath());
569+
}
570+
} else if (!inputArtifact.isTreeArtifact()) {
571+
// Normally, one would not see tree artifacts here because they have already been
572+
// expanded by the time the code gets here. However, there is one very special case:
573+
// when an action has an archived tree artifact on its output and is executed on the
574+
// local branch of the dynamic execution strategy, the tree artifact is zipped up
575+
// in a little extra spawn, which has direct tree artifact on its inputs. Sadly,
576+
// it's not easy to fix this because there isn't an easy way to inject this new
577+
// tree artifact into the artifact expander being used.
578+
//
579+
// The best would be to not rely on spawn strategies for executing that little
580+
// command: it's entirely under the control of Bazel so we can guarantee that it
581+
// does not cause mischief.
582+
FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(actionInput);
583+
if (metadata.getMaterializationExecPath().isPresent()) {
584+
materializationExecPath = metadata.getMaterializationExecPath().get();
585+
}
586+
}
587+
588+
if (materializationExecPath != null) {
589+
inputPath =
590+
processResolvedSymlink(
591+
absoluteRoot,
592+
materializationExecPath,
593+
withinSandboxExecRoot,
594+
execRootPath.asFragment(),
595+
packageRoots,
596+
sourceRootWithinSandbox);
597+
} else {
598+
inputPath =
599+
RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath());
600+
}
601+
}
523602
}
524603
} else {
525604
PathFragment execPath = actionInput.getExecPath();
@@ -544,6 +623,7 @@ public SandboxInputs processInputFiles(
544623
return new SandboxInputs(inputFiles, virtualInputs, inputSymlinks, sandboxRootToSourceRoot);
545624
}
546625

626+
547627
/** The file and directory outputs of a sandboxed spawn. */
548628
@AutoValue
549629
public abstract static class SandboxOutputs {

‎src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
7676
SandboxInputs readablePaths =
7777
helpers.processInputFiles(
7878
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
79+
context.getInputMetadataProvider(),
7980
execRoot,
8081
execRoot,
8182
packageRoots,

‎src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java‎

Lines changed: 48 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,15 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
264264
Path treeDir = artifactPathResolver.toPath(parent);
265265
boolean chmod = executionMode.get();
266266

267-
FileStatus stat = treeDir.statIfFound(Symlinks.FOLLOW);
267+
FileStatus lstat = treeDir.statIfFound(Symlinks.NOFOLLOW);
268+
FileStatus stat;
269+
if (lstat == null) {
270+
stat = null;
271+
} else if (!lstat.isSymbolicLink()) {
272+
stat = lstat;
273+
} else {
274+
stat = treeDir.statIfFound(Symlinks.FOLLOW);
275+
}
268276

269277
// Make sure the tree artifact root exists and is a regular directory. Note that this is how the
270278
// action is initialized, so this should hold unless the action itself has deleted the root.
@@ -332,12 +340,18 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
332340
}
333341

334342
// Same rationale as for constructFileArtifactValue.
335-
if (anyRemote.get() && treeDir.isSymbolicLink() && stat instanceof FileStatusWithMetadata) {
336-
FileArtifactValue metadata = ((FileStatusWithMetadata) stat).getMetadata();
337-
tree.setMaterializationExecPath(
338-
metadata
339-
.getMaterializationExecPath()
340-
.orElse(treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot)));
343+
if (lstat.isSymbolicLink()) {
344+
PathFragment materializationExecPath = null;
345+
if (stat instanceof FileStatusWithMetadata) {
346+
materializationExecPath =
347+
((FileStatusWithMetadata) stat).getMetadata().getMaterializationExecPath().orElse(null);
348+
}
349+
350+
if (materializationExecPath == null) {
351+
materializationExecPath = treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot);
352+
}
353+
354+
tree.setMaterializationExecPath(materializationExecPath);
341355
}
342356

343357
return tree.build();
@@ -509,7 +523,13 @@ private FileArtifactValue constructFileArtifactValue(
509523
return value;
510524
}
511525

512-
if (type.isFile() && fileDigest != null) {
526+
boolean isResolvedSymlink =
527+
statAndValue.statNoFollow() != null
528+
&& statAndValue.statNoFollow().isSymbolicLink()
529+
&& statAndValue.realPath() != null
530+
&& !value.isRemote();
531+
532+
if (type.isFile() && !isResolvedSymlink && fileDigest != null) {
513533
// The digest is in the file value and that is all that is needed for this file's metadata.
514534
return value;
515535
}
@@ -525,22 +545,30 @@ private FileArtifactValue constructFileArtifactValue(
525545
artifactPathResolver.toPath(artifact).getLastModifiedTime());
526546
}
527547

528-
if (injectedDigest == null && type.isFile()) {
548+
byte[] actualDigest = fileDigest != null ? fileDigest : injectedDigest;
549+
550+
if (actualDigest == null && type.isFile()) {
529551
// We don't have an injected digest and there is no digest in the file value (which attempts a
530552
// fast digest). Manually compute the digest instead.
531-
Path path = statAndValue.pathNoFollow();
532-
if (statAndValue.statNoFollow() != null
533-
&& statAndValue.statNoFollow().isSymbolicLink()
534-
&& statAndValue.realPath() != null) {
535-
// If the file is a symlink, we compute the digest using the target path so that it's
536-
// possible to hit the digest cache - we probably already computed the digest for the
537-
// target during previous action execution.
538-
path = statAndValue.realPath();
539-
}
540553

541-
injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize());
554+
// If the file is a symlink, we compute the digest using the target path so that it's
555+
// possible to hit the digest cache - we probably already computed the digest for the
556+
// target during previous action execution.
557+
Path pathToDigest = isResolvedSymlink ? statAndValue.realPath() : statAndValue.pathNoFollow();
558+
actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest, value.getSize());
542559
}
543-
return FileArtifactValue.createFromInjectedDigest(value, injectedDigest);
560+
561+
if (!isResolvedSymlink) {
562+
return FileArtifactValue.createFromInjectedDigest(value, actualDigest);
563+
}
564+
565+
PathFragment realPathAsFragment = statAndValue.realPath().asFragment();
566+
PathFragment execRootRelativeRealPath =
567+
realPathAsFragment.startsWith(execRoot)
568+
? realPathAsFragment.relativeTo(execRoot)
569+
: realPathAsFragment;
570+
return FileArtifactValue.createForResolvedSymlink(
571+
execRootRelativeRealPath, value, actualDigest);
544572
}
545573

546574
/**

‎src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
189189
helpers.processInputFiles(
190190
context.getInputMapping(
191191
PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
192+
context.getInputMetadataProvider(),
192193
execRoot,
193194
execRoot,
194195
packageRoots,

0 commit comments

Comments
 (0)