From faa139f8e47f2f5f8ad7a8052a1190e1fc42e3aa Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Tue, 19 Dec 2023 11:46:01 +0000 Subject: [PATCH 1/5] Fix output base under /tmp. Resolved symlink action outputs and param files are both fixed. --- .../build/lib/actions/FileArtifactValue.java | 23 ++++++++++- .../google/devtools/build/lib/sandbox/BUILD | 1 + .../sandbox/DarwinSandboxedSpawnRunner.java | 1 + .../sandbox/DockerSandboxedSpawnRunner.java | 1 + .../sandbox/LinuxSandboxedSpawnRunner.java | 1 + .../ProcessWrapperSandboxedSpawnRunner.java | 1 + .../build/lib/sandbox/SandboxHelpers.java | 39 ++++++++++++++++++- .../sandbox/WindowsSandboxedSpawnRunner.java | 1 + .../skyframe/ActionOutputMetadataStore.java | 2 + .../build/lib/worker/WorkerSpawnRunner.java | 1 + .../build/lib/sandbox/SandboxHelpersTest.java | 13 +++++-- 11 files changed, 78 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index 1afcd70e2b533b..48e91a9dcbee26 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -214,6 +214,12 @@ public static FileArtifactValue createForSourceArtifact( xattrProvider); } + public static FileArtifactValue createForResolvedSymlink( + PathFragment realPath, FileArtifactValue metadata, @Nullable byte[] digest) { + return new ResolvedSymlinkFileArtifactValue( + realPath, digest, metadata.getContentsProxy(), metadata.getSize()); + } + public static FileArtifactValue createFromInjectedDigest( FileArtifactValue metadata, @Nullable byte[] digest) { return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize()); @@ -439,7 +445,22 @@ public String toString() { } } - private static final class RegularFileArtifactValue extends FileArtifactValue { + private static final class ResolvedSymlinkFileArtifactValue extends RegularFileArtifactValue { + private final PathFragment realPath; + + private ResolvedSymlinkFileArtifactValue(PathFragment realPath, @Nullable byte[] digest, + @Nullable FileContentsProxy proxy, long size) { + super(digest, proxy, size); + this.realPath = realPath; + } + + @Override + public Optional getMaterializationExecPath() { + return Optional.of(realPath); + } + } + + private static class RegularFileArtifactValue extends FileArtifactValue { private final byte[] digest; @Nullable private final FileContentsProxy proxy; private final long size; diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD index 87ac37c3644eb0..6d49080690bf08 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD +++ b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD @@ -17,6 +17,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/vfs", diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java index fad5cfa108b11e..2bf50dffa528d2 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java @@ -228,6 +228,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + context.getInputMetadataProvider(), execRoot, execRoot, packageRoots, diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java index 3dacaeaa05ee9a..971b05cd11b99b 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java @@ -226,6 +226,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + context.getInputMetadataProvider(), execRoot, execRoot, packageRoots, diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java index 2d593e7a28003e..9c623357e66dae 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java @@ -281,6 +281,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + context.getInputMetadataProvider(), execRoot, withinSandboxExecRoot, packageRoots, diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java index 2f7608dcc8b857..c9b1509ca06107 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java @@ -100,6 +100,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + context.getInputMetadataProvider(), execRoot, execRoot, packageRoots, diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java index fb72d591e0781e..777faeb15e2ba2 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java @@ -29,6 +29,8 @@ import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; @@ -444,6 +446,25 @@ private static RootedPath processFilesetSymlink( symlink.getPathString())); } + private static RootedPath processResolvedSymlink( + Root absoluteRoot, + PathFragment symlinkTarget, + Root execRootWithinSandbox, + PathFragment execRootFragment, + ImmutableList packageRoots) { + for (Root packageRoot : packageRoots) { + if (packageRoot.contains(symlinkTarget)) { + return RootedPath.toRootedPath(packageRoot, packageRoot.relativize(symlinkTarget)); + } + } + + if (symlinkTarget.startsWith(execRootFragment)) { + return RootedPath.toRootedPath(execRootWithinSandbox, symlinkTarget.relativeTo(execRootFragment)); + } + + return RootedPath.toRootedPath(absoluteRoot, symlinkTarget); + } + /** * Returns the inputs of a Spawn as a map of PathFragments relative to an execRoot to paths in the * host filesystem where the input files can be found. @@ -458,6 +479,7 @@ private static RootedPath processFilesetSymlink( */ public SandboxInputs processInputFiles( Map inputMap, + InputMetadataProvider inputMetadataProvider, Path execRootPath, Path withinSandboxExecRootPath, ImmutableList packageRoots, @@ -468,6 +490,7 @@ public SandboxInputs processInputFiles( withinSandboxExecRootPath.equals(execRootPath) ? withinSandboxExecRoot : Root.fromPath(execRootPath); + Root absoluteRoot = Root.absoluteRoot(execRootPath.getFileSystem()); Map inputFiles = new TreeMap<>(); Map inputSymlinks = new TreeMap<>(); @@ -503,9 +526,11 @@ public SandboxInputs processInputFiles( if (actionInput instanceof EmptyActionInput) { inputPath = null; + } else if (actionInput instanceof VirtualActionInput) { + inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, actionInput.getExecPath()); } else if (actionInput instanceof Artifact) { Artifact inputArtifact = (Artifact) actionInput; - if (inputArtifact.isSourceArtifact() && sandboxSourceRoots != null) { + if (inputArtifact.isSourceArtifact() && !inputArtifact.isDirectory() && sandboxSourceRoots != null) { Root sourceRoot = inputArtifact.getRoot().getRoot(); if (!sourceRootToSandboxSourceRoot.containsKey(sourceRoot)) { int next = sourceRootToSandboxSourceRoot.size(); @@ -518,6 +543,18 @@ public SandboxInputs processInputFiles( RootedPath.toRootedPath( sourceRootToSandboxSourceRoot.get(sourceRoot), inputArtifact.getRootRelativePath()); + } else if (!inputArtifact.isSourceArtifact() && sandboxSourceRoots != null) { + FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(actionInput); + if (!metadata.isRemote() && metadata.getMaterializationExecPath().isPresent()) { + inputPath = processResolvedSymlink( + absoluteRoot, + metadata.getMaterializationExecPath().get(), + execRoot, + execRootPath.asFragment(), + packageRoots); + } else { + inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); + } } else { inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java index c7996e38582fa1..505e2417850161 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java @@ -76,6 +76,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs readablePaths = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + context.getInputMetadataProvider(), execRoot, execRoot, packageRoots, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java index a1b1a77d3d0cbb..f47089dfe65ad0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java @@ -536,6 +536,8 @@ private FileArtifactValue constructFileArtifactValue( // possible to hit the digest cache - we probably already computed the digest for the // target during previous action execution. path = statAndValue.realPath(); + injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize()); + return FileArtifactValue.createForResolvedSymlink(path.asFragment(), value, injectedDigest); } injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize()); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index 988e2f8f347c2d..d623cedfb2f91a 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -189,6 +189,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) helpers.processInputFiles( context.getInputMapping( PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + context.getInputMetadataProvider(), execRoot, execRoot, packageRoots, diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java index 8df844739de17e..5b403d7dd98e78 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.exec.BinTools; +import com.google.devtools.build.lib.exec.util.FakeActionInputFileCache; import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs; @@ -106,7 +107,9 @@ public void processInputFiles_materializesParamFile() throws Exception { SandboxInputs inputs = sandboxHelpers.processInputFiles( - inputMap(paramFile), execRootPath, execRootPath, ImmutableList.of(), null); + inputMap(paramFile), + new FakeActionInputFileCache(), + execRootPath, execRootPath, ImmutableList.of(), null); assertThat(inputs.getFiles()) .containsExactly(PathFragment.create("paramFile"), execRootedPath("paramFile")); @@ -127,7 +130,9 @@ public void processInputFiles_materializesBinToolsFile() throws Exception { SandboxInputs inputs = sandboxHelpers.processInputFiles( - inputMap(tool), execRootPath, execRootPath, ImmutableList.of(), null); + inputMap(tool), + new FakeActionInputFileCache(), + execRootPath, execRootPath, ImmutableList.of(), null); assertThat(inputs.getFiles()) .containsExactly(PathFragment.create("_bin/say_hello"), execRootedPath("_bin/say_hello")); @@ -173,7 +178,7 @@ protected void setExecutable(PathFragment path, boolean executable) throws IOExc try { var unused = sandboxHelpers.processInputFiles( - inputMap(input), customExecRoot, customExecRoot, ImmutableList.of(), null); + inputMap(input), new FakeActionInputFileCache(), customExecRoot, customExecRoot, ImmutableList.of(), null); finishProcessingSemaphore.release(); } catch (IOException | InterruptedException e) { throw new IllegalArgumentException(e); @@ -181,7 +186,7 @@ protected void setExecutable(PathFragment path, boolean executable) throws IOExc }); var unused = sandboxHelpers.processInputFiles( - inputMap(input), customExecRoot, customExecRoot, ImmutableList.of(), null); + inputMap(input), new FakeActionInputFileCache(), customExecRoot, customExecRoot, ImmutableList.of(), null); finishProcessingSemaphore.release(); future.get(); From 41bca0e0ebc388b140b32ba058446791545a2871 Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Wed, 20 Dec 2023 13:39:08 +0000 Subject: [PATCH 2/5] fix trees --- .../build/lib/sandbox/SandboxHelpers.java | 69 ++++++++------ .../skyframe/ActionOutputMetadataStore.java | 19 ++-- src/test/shell/bazel/bazel_sandboxing_test.sh | 89 +++++++++++++++++++ 3 files changed, 145 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java index 777faeb15e2ba2..4916a679d1b685 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java @@ -448,10 +448,11 @@ private static RootedPath processFilesetSymlink( private static RootedPath processResolvedSymlink( Root absoluteRoot, - PathFragment symlinkTarget, + PathFragment execRootRelativeSymlinkTarget, Root execRootWithinSandbox, PathFragment execRootFragment, ImmutableList packageRoots) { + PathFragment symlinkTarget = execRootFragment.getRelative(execRootRelativeSymlinkTarget); for (Root packageRoot : packageRoots) { if (packageRoot.contains(symlinkTarget)) { return RootedPath.toRootedPath(packageRoot, packageRoot.relativize(symlinkTarget)); @@ -530,33 +531,49 @@ public SandboxInputs processInputFiles( inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, actionInput.getExecPath()); } else if (actionInput instanceof Artifact) { Artifact inputArtifact = (Artifact) actionInput; - if (inputArtifact.isSourceArtifact() && !inputArtifact.isDirectory() && sandboxSourceRoots != null) { - Root sourceRoot = inputArtifact.getRoot().getRoot(); - if (!sourceRootToSandboxSourceRoot.containsKey(sourceRoot)) { - int next = sourceRootToSandboxSourceRoot.size(); - sourceRootToSandboxSourceRoot.put( - sourceRoot, - Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next)))); - } - - inputPath = - RootedPath.toRootedPath( - sourceRootToSandboxSourceRoot.get(sourceRoot), - inputArtifact.getRootRelativePath()); - } else if (!inputArtifact.isSourceArtifact() && sandboxSourceRoots != null) { - FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(actionInput); - if (!metadata.isRemote() && metadata.getMaterializationExecPath().isPresent()) { - inputPath = processResolvedSymlink( - absoluteRoot, - metadata.getMaterializationExecPath().get(), - execRoot, - execRootPath.asFragment(), - packageRoots); + if (sandboxSourceRoots == null) { + inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); + } else { + if (inputArtifact.isSourceArtifact()) { + Root sourceRoot = inputArtifact.getRoot().getRoot(); + if (!sourceRootToSandboxSourceRoot.containsKey(sourceRoot)) { + int next = sourceRootToSandboxSourceRoot.size(); + sourceRootToSandboxSourceRoot.put( + sourceRoot, + Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next)))); + } + + inputPath = + RootedPath.toRootedPath( + sourceRootToSandboxSourceRoot.get(sourceRoot), + inputArtifact.getRootRelativePath()); } else { - inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); + PathFragment materializationExecPath = null; + if (inputArtifact.isChildOfDeclaredDirectory()) { + FileArtifactValue parentMetadata = inputMetadataProvider.getInputMetadata(inputArtifact.getParent()); + if (!parentMetadata.isRemote() && parentMetadata.getMaterializationExecPath().isPresent()) { + materializationExecPath = + parentMetadata.getMaterializationExecPath().get() + .getRelative(inputArtifact.getParentRelativePath()); + } + } else { + FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(actionInput); + if (metadata.getMaterializationExecPath().isPresent()) { + materializationExecPath = metadata.getMaterializationExecPath().get(); + } + } + + if (materializationExecPath != null) { + inputPath = processResolvedSymlink( + absoluteRoot, + materializationExecPath, + withinSandboxExecRoot, + execRootPath.asFragment(), + packageRoots); + } else { + inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); + } } - } else { - inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); } } else { PathFragment execPath = actionInput.getExecPath(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java index f47089dfe65ad0..25d9adac9a9f86 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java @@ -332,12 +332,19 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa } // Same rationale as for constructFileArtifactValue. - if (anyRemote.get() && treeDir.isSymbolicLink() && stat instanceof FileStatusWithMetadata) { - FileArtifactValue metadata = ((FileStatusWithMetadata) stat).getMetadata(); - tree.setMaterializationExecPath( - metadata - .getMaterializationExecPath() - .orElse(treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot))); + if (treeDir.isSymbolicLink()) { + PathFragment materializationExecPath = null; + if (stat instanceof FileStatusWithMetadata) { + materializationExecPath = ((FileStatusWithMetadata) stat).getMetadata() + .getMaterializationExecPath() + .orElse(null); + } + + if (materializationExecPath == null) { + materializationExecPath = treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot); + } + + tree.setMaterializationExecPath(materializationExecPath); } return tree.build(); diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh index 3e65184291777a..86fa7ad1807cd4 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh @@ -334,6 +334,95 @@ EOF assert_contains GOOD bazel-bin/pkg/gen.txt } +function test_symlink_with_output_base_under_tmp() { + if [[ "$PLATFORM" == "darwin" ]]; then + # Tests Linux-specific functionality + return 0 + fi + + create_workspace_with_default_repos WORKSPACE + + sed -i.bak '/sandbox_tmpfs_path/d' $TEST_TMPDIR/bazelrc + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load(":r.bzl", "symlink_rule") + +genrule(name="r1", srcs=[], outs=[":r1"], cmd="echo CONTENT > $@") +symlink_rule(name="r2", input=":r1") +genrule(name="r3", srcs=[":r2"], outs=[":r3"], cmd="cp $< $@") +EOF + + cat > pkg/r.bzl <<'EOF' +def _symlink_impl(ctx): + output = ctx.actions.declare_file(ctx.label.name) + ctx.actions.symlink(output = output, target_file = ctx.file.input) + return [DefaultInfo(files = depset([output]))] + +symlink_rule = rule( + implementation = _symlink_impl, + attrs = {"input": attr.label(allow_single_file=True)}) +EOF + + local tmp_output_base=$(mktemp -d "/tmp/bazel_output_base.XXXXXXXX") + trap "chmod -R u+w $tmp_output_base && rm -fr $tmp_output_base" EXIT + + bazel --output_base="$tmp_output_base" build //pkg:r3 + assert_contains CONTENT bazel-bin/pkg/r3 + bazel --output_base="$tmp_output_base" shutdown +} + +function test_symlink_to_directory_with_output_base_under_tmp() { + if [[ "$PLATFORM" == "darwin" ]]; then + # Tests Linux-specific functionality + return 0 + fi + + create_workspace_with_default_repos WORKSPACE + + sed -i.bak '/sandbox_tmpfs_path/d' $TEST_TMPDIR/bazelrc + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +load(":r.bzl", "symlink_rule", "tree_rule") + +tree_rule(name="t1") +symlink_rule(name="t2", input=":t1") +genrule(name="t3", srcs=[":t2"], outs=[":t3"], cmd=";\n".join( + ["cat $(location :t2)/{a/a,b/b} > $@"])) +EOF + + cat > pkg/r.bzl <<'EOF' +def _symlink_impl(ctx): + output = ctx.actions.declare_directory(ctx.label.name) + ctx.actions.symlink(output = output, target_file = ctx.file.input) + return [DefaultInfo(files = depset([output]))] + +symlink_rule = rule( + implementation = _symlink_impl, + attrs = {"input": attr.label(allow_single_file=True)}) + +def _tree_impl(ctx): + output = ctx.actions.declare_directory(ctx.label.name) + ctx.actions.run_shell( + outputs = [output], + command = "export TREE=%s && mkdir $TREE/a $TREE/b && echo -n A > $TREE/a/a && echo -n B > $TREE/b/b" % output.path) + return [DefaultInfo(files = depset([output]))] + +tree_rule = rule( + implementation = _tree_impl, + attrs = {}) + +EOF + + local tmp_output_base=$(mktemp -d "/tmp/bazel_output_base.XXXXXXXX") + trap "chmod -R u+w $tmp_output_base && rm -fr $tmp_output_base" EXIT + + bazel --output_base="$tmp_output_base" build //pkg:t3 + assert_contains AB bazel-bin/pkg/t3 + bazel --output_base="$tmp_output_base" shutdown +} + # The test shouldn't fail if the environment doesn't support running it. check_sandbox_allowed || exit 0 From 9dc505ed963c9ad64f0bafd53dd0fd5e54ce5214 Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Wed, 20 Dec 2023 14:36:54 +0000 Subject: [PATCH 3/5] fix BwoBIntegrationTest --- .../build/lib/sandbox/SandboxHelpers.java | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java index 4916a679d1b685..b7f5f338ce2934 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java @@ -20,6 +20,7 @@ import static com.google.devtools.build.lib.vfs.Dirent.Type.SYMLINK; import com.google.auto.value.AutoValue; +import com.google.common.base.Function; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -451,11 +452,14 @@ private static RootedPath processResolvedSymlink( PathFragment execRootRelativeSymlinkTarget, Root execRootWithinSandbox, PathFragment execRootFragment, - ImmutableList packageRoots) { + ImmutableList packageRoots, + Function sourceRooWithinSandbox) { PathFragment symlinkTarget = execRootFragment.getRelative(execRootRelativeSymlinkTarget); for (Root packageRoot : packageRoots) { if (packageRoot.contains(symlinkTarget)) { - return RootedPath.toRootedPath(packageRoot, packageRoot.relativize(symlinkTarget)); + return RootedPath.toRootedPath( + sourceRooWithinSandbox.apply(packageRoot), + packageRoot.relativize(symlinkTarget)); } } @@ -498,6 +502,17 @@ public SandboxInputs processInputFiles( Map virtualInputs = new HashMap<>(); Map sourceRootToSandboxSourceRoot = new TreeMap<>(); + Function sourceRootWithinSandbox = r -> { + if (!sourceRootToSandboxSourceRoot.containsKey(r)) { + int next = sourceRootToSandboxSourceRoot.size(); + sourceRootToSandboxSourceRoot.put( + r, + Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next)))); + } + + return sourceRootToSandboxSourceRoot.get(r); + }; + for (Map.Entry e : inputMap.entrySet()) { if (Thread.interrupted()) { throw new InterruptedException(); @@ -536,22 +551,15 @@ public SandboxInputs processInputFiles( } else { if (inputArtifact.isSourceArtifact()) { Root sourceRoot = inputArtifact.getRoot().getRoot(); - if (!sourceRootToSandboxSourceRoot.containsKey(sourceRoot)) { - int next = sourceRootToSandboxSourceRoot.size(); - sourceRootToSandboxSourceRoot.put( - sourceRoot, - Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next)))); - } - inputPath = RootedPath.toRootedPath( - sourceRootToSandboxSourceRoot.get(sourceRoot), + sourceRootWithinSandbox.apply(sourceRoot), inputArtifact.getRootRelativePath()); } else { PathFragment materializationExecPath = null; if (inputArtifact.isChildOfDeclaredDirectory()) { FileArtifactValue parentMetadata = inputMetadataProvider.getInputMetadata(inputArtifact.getParent()); - if (!parentMetadata.isRemote() && parentMetadata.getMaterializationExecPath().isPresent()) { + if (parentMetadata.getMaterializationExecPath().isPresent()) { materializationExecPath = parentMetadata.getMaterializationExecPath().get() .getRelative(inputArtifact.getParentRelativePath()); @@ -569,7 +577,8 @@ public SandboxInputs processInputFiles( materializationExecPath, withinSandboxExecRoot, execRootPath.asFragment(), - packageRoots); + packageRoots, + sourceRootWithinSandbox); } else { inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); } From b8bc95cb2eb91ba1972400c9a4c9a0c47333296b Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Thu, 21 Dec 2023 09:22:23 +0000 Subject: [PATCH 4/5] some more tests --- .../build/lib/actions/FileArtifactValue.java | 7 +- .../google/devtools/build/lib/sandbox/BUILD | 2 + .../build/lib/sandbox/SandboxHelpersTest.java | 75 +++++++++++++++++++ .../ActionOutputMetadataStoreTest.java | 5 +- 4 files changed, 83 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index 48e91a9dcbee26..827dfb7d53df83 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -171,9 +171,12 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) { * *

If present, this artifact is a copy of another artifact. It is still tracked as a * non-symlink by Bazel, but materialized in the local filesystem as a symlink to the original - * artifact, whose contents live at this location. This is used by {@link + * artifact, whose contents live at this location. + * + *

This is used by {@link * com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher} to implement zero-cost - * copies of remotely stored artifacts. + * copies of remotely stored artifacts and to tack which real files (artifact or otherwise) + * resolved symlink artifacts point to. */ public Optional getMaterializationExecPath() { return Optional.empty(); diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/BUILD b/src/test/java/com/google/devtools/build/lib/sandbox/BUILD index 74873ef72d3311..671997db568c43 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/BUILD +++ b/src/test/java/com/google/devtools/build/lib/sandbox/BUILD @@ -59,12 +59,14 @@ java_test( deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter", "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_helpers", "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_options", "//src/main/java/com/google/devtools/build/lib/sandbox:sandboxed_spawns", "//src/main/java/com/google/devtools/build/lib/sandbox:tree_deleter", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java index 5b403d7dd98e78..cfc62cd3f8de6c 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java @@ -24,8 +24,12 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.Spawn; @@ -36,6 +40,7 @@ import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -69,6 +74,7 @@ /** Tests for {@link SandboxHelpers}. */ @RunWith(JUnit4.class) public class SandboxHelpersTest { + private static final byte[] FAKE_DIGEST = new byte[] {1}; private final Scratch scratch = new Scratch(); private Path execRootPath; @@ -95,6 +101,75 @@ private RootedPath execRootedPath(String execPath) { return RootedPath.toRootedPath(execRoot, PathFragment.create(execPath)); } + @Test + public void processInputFiles_resolvesMaterializationPath_fileArtifact() throws Exception { + ArtifactRoot outputRoot = + ArtifactRoot.asDerivedRoot(execRootPath, ArtifactRoot.RootType.Output, "outputs"); + Path sandboxSourceRoot = scratch.dir("/faketmp/sandbox-source-roots"); + + Artifact input = ActionsTestUtil.createArtifact(outputRoot, "a/a"); + FileArtifactValue symlinkTargetMetadata = + FileArtifactValue.createForNormalFile(FAKE_DIGEST, null, 0L); + FileArtifactValue inputMetadata = FileArtifactValue.createForResolvedSymlink( + PathFragment.create("b/b"), symlinkTargetMetadata, FAKE_DIGEST); + + FakeActionInputFileCache inputMetadataProvider = new FakeActionInputFileCache(); + inputMetadataProvider.put(input, inputMetadata); + + SandboxHelpers sandboxHelpers = new SandboxHelpers(); + SandboxInputs inputs = + sandboxHelpers.processInputFiles( + inputMap(input), + inputMetadataProvider, + execRootPath, execRootPath, + ImmutableList.of(), + sandboxSourceRoot); + + assertThat(inputs.getFiles()).containsEntry( + input.getExecPath(), + RootedPath.toRootedPath(execRoot, PathFragment.create("b/b"))); + } + + @Test + public void processInputFiles_resolvesMaterializationPath_treeArtifact() throws Exception { + ArtifactRoot outputRoot = + ArtifactRoot.asDerivedRoot(execRootPath, ArtifactRoot.RootType.Output, "outputs"); + Path sandboxSourceRoot = scratch.dir("/faketmp/sandbox-source-roots"); + SpecialArtifact parent = + ActionsTestUtil.createTreeArtifactWithGeneratingAction( + outputRoot, "bin/config/other_dir/subdir"); + + TreeFileArtifact childA = TreeFileArtifact.createTreeOutput(parent, "a/a"); + TreeFileArtifact childB = TreeFileArtifact.createTreeOutput(parent, "b/b"); + FileArtifactValue childMetadata = FileArtifactValue.createForNormalFile(FAKE_DIGEST, + null, 0L); + TreeArtifactValue parentMetadata = TreeArtifactValue.newBuilder(parent) + .putChild(childA, childMetadata) + .putChild(childB, childMetadata) + .setMaterializationExecPath(PathFragment.create("materialized")) + .build(); + + FakeActionInputFileCache inputMetadataProvider = new FakeActionInputFileCache(); + inputMetadataProvider.put(parent, parentMetadata.getMetadata()); + + SandboxHelpers sandboxHelpers = new SandboxHelpers(); + SandboxInputs inputs = + sandboxHelpers.processInputFiles( + inputMap(childA, childB), + inputMetadataProvider, + execRootPath, execRootPath, + ImmutableList.of(), + sandboxSourceRoot); + + assertThat(inputs.getFiles()).containsEntry( + childA.getExecPath(), + RootedPath.toRootedPath(execRoot, PathFragment.create("materialized/a/a"))); + assertThat(inputs.getFiles()).containsEntry( + childB.getExecPath(), + RootedPath.toRootedPath(execRoot, PathFragment.create("materialized/b/b"))); + } + + @Test public void processInputFiles_materializesParamFile() throws Exception { SandboxHelpers sandboxHelpers = new SandboxHelpers(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java index 430e809fb50f31..b1ba2180945968 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java @@ -481,11 +481,8 @@ public void treeArtifactMaterializedAsSymlink( .getPath(outputArtifact.getPath().getPathString()) .createSymbolicLink(targetArtifact.getPath().asFragment()); - PathFragment expectedMaterializationExecPath = null; - if (composition.isPartiallyRemote()) { - expectedMaterializationExecPath = + PathFragment expectedMaterializationExecPath = preexistingPath != null ? preexistingPath : targetArtifact.getExecPath(); - } assertThat(store.getTreeArtifactValue(outputArtifact)) .isEqualTo( From a80c48a57b851a9e020eeb0d8bc74dc1ab379feb Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Thu, 21 Dec 2023 09:30:24 +0000 Subject: [PATCH 5/5] relativize symlink target wrt. execroot --- .../build/lib/skyframe/ActionOutputMetadataStore.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java index 25d9adac9a9f86..7bf241b201611b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java @@ -544,7 +544,15 @@ private FileArtifactValue constructFileArtifactValue( // target during previous action execution. path = statAndValue.realPath(); injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize()); - return FileArtifactValue.createForResolvedSymlink(path.asFragment(), value, injectedDigest); + + PathFragment execRootRelativeRealPath; + if (path.asFragment().startsWith(execRoot)) { + execRootRelativeRealPath = path.asFragment().relativeTo(execRoot); + } else { + execRootRelativeRealPath = path.asFragment(); + } + + return FileArtifactValue.createForResolvedSymlink(execRootRelativeRealPath, value, injectedDigest); } injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize());