diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java index ea5f758f408121..ae91285a43d1dd 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java @@ -103,12 +103,13 @@ public PathFragment getExecPath() { } /** - * Expands middleman artifacts in a sequence of {@link ActionInput}s. + * Expands middleman and tree artifacts in a sequence of {@link ActionInput}s. * - *

Non-middleman artifacts are returned untouched. + *

Non-middleman, non-tree artifacts are returned untouched. */ public static List expandArtifacts( - NestedSet inputs, ArtifactExpander artifactExpander) { + NestedSet inputs, ArtifactExpander artifactExpander, + boolean keepEmptyTreeArtifacts) { List result = new ArrayList<>(); List containedArtifacts = new ArrayList<>(); for (ActionInput input : inputs.toList()) { @@ -118,7 +119,8 @@ public static List expandArtifacts( } containedArtifacts.add((Artifact) input); } - Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander); + Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander, + keepEmptyTreeArtifacts); return result; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 780e263ea3ba27..7005acc9f2fd84 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -1520,26 +1520,31 @@ public static String joinRootRelativePaths(String delimiter, Iterable return Joiner.on(delimiter).join(toRootRelativePaths(artifacts)); } - /** Adds a collection of artifacts to a given collection, with middleman actions expanded once. */ + /** + * Adds a collection of artifacts to a given collection, with middleman actions and tree artifacts + * expanded once. + */ static void addExpandedArtifacts( Iterable artifacts, Collection output, - ArtifactExpander artifactExpander) { - addExpandedArtifacts(artifacts, output, Functions.identity(), artifactExpander); + ArtifactExpander artifactExpander, + boolean keepEmptyTreeArtifacts) { + addExpandedArtifacts(artifacts, output, Functions.identity(), artifactExpander, + keepEmptyTreeArtifacts); } /** - * Converts a collection of artifacts into the outputs computed by - * outputFormatter and adds them to a given collection. Middleman artifacts - * are expanded once. + * Converts a collection of artifacts into the outputs computed by outputFormatter and adds them + * to a given collection. Middleman artifacts and tree artifacts are expanded once. */ private static void addExpandedArtifacts(Iterable artifacts, - Collection output, - Function outputFormatter, - ArtifactExpander artifactExpander) { + Collection output, + Function outputFormatter, + ArtifactExpander artifactExpander, + boolean keepEmptyTreeArtifacts) { for (Artifact artifact : artifacts) { if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) { - expandArtifact(artifact, output, outputFormatter, artifactExpander); + expandArtifact(artifact, output, outputFormatter, artifactExpander, keepEmptyTreeArtifacts); } else { output.add(outputFormatter.apply(artifact)); } @@ -1549,13 +1554,17 @@ private static void addExpandedArtifacts(Iterable artifa private static void expandArtifact(Artifact middleman, Collection output, Function outputFormatter, - ArtifactExpander artifactExpander) { + ArtifactExpander artifactExpander, + boolean keepEmptyTreeArtifacts) { Preconditions.checkArgument(middleman.isMiddlemanArtifact() || middleman.isTreeArtifact()); List artifacts = new ArrayList<>(); artifactExpander.expand(middleman, artifacts); for (Artifact artifact : artifacts) { output.add(outputFormatter.apply(artifact)); } + if (keepEmptyTreeArtifacts && middleman.isTreeArtifact() && artifacts.isEmpty()) { + output.add(outputFormatter.apply(middleman)); + } } /** diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java index 44cb0210b92584..f7e54f18c222d7 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java @@ -130,7 +130,8 @@ void addRunfilesToInputs( if (localArtifact.isTreeArtifact()) { List expandedInputs = ActionInputHelper.expandArtifacts( - NestedSetBuilder.create(Order.STABLE_ORDER, localArtifact), artifactExpander); + NestedSetBuilder.create(Order.STABLE_ORDER, localArtifact), artifactExpander, + false); for (ActionInput input : expandedInputs) { addMapping( inputMap, @@ -222,7 +223,8 @@ private static void addInputs( NestedSet inputFiles, ArtifactExpander artifactExpander, PathFragment baseDirectory) { - List inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander); + List inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander, + true); for (ActionInput input : inputs) { addMapping(inputMap, input.getExecPath(), input, baseDirectory); } 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 d1f5ec24143e56..630a4551485e7f 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 @@ -233,8 +233,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); 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 f56eafd8cf9573..1a46186e76c991 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 @@ -223,8 +223,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); 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 ffb7a5f3b7fd02..b9fdbf3c69f69a 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 @@ -184,8 +184,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); 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 79b625ea896c0c..6b6835eff2b549 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 @@ -112,8 +112,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); 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 ea9f071b89ba3c..7e0a90145fac66 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 @@ -487,27 +487,8 @@ private static void writeVirtualInputTo(VirtualActionInput input, Path target) */ public SandboxInputs processInputFiles( Map inputMap, - Spawn spawn, - ArtifactExpander artifactExpander, Path execRoot) throws IOException { - // SpawnInputExpander#getInputMapping uses ArtifactExpander#expandArtifacts to expand - // middlemen and tree artifacts, which expands empty tree artifacts to no entry. However, - // actions that accept TreeArtifacts as inputs generally expect that the empty directory is - // created. So we add those explicitly here. - // TODO(ulfjack): Move this code to SpawnInputExpander. - for (ActionInput input : spawn.getInputFiles().toList()) { - if (input instanceof Artifact && ((Artifact) input).isTreeArtifact()) { - List containedArtifacts = new ArrayList<>(); - artifactExpander.expand((Artifact) input, containedArtifacts); - // Attempting to mount a non-empty directory results in ERR_DIRECTORY_NOT_EMPTY, so we - // only mount empty TreeArtifacts as directories. - if (containedArtifacts.isEmpty()) { - inputMap.put(input.getExecPath(), input); - } - } - } - Map inputFiles = new TreeMap<>(); Set virtualInputs = new HashSet<>(); Map inputSymlinks = new TreeMap<>(); 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 f602fab1adbc8a..3be3a5496bedcd 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 @@ -72,8 +72,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs readablePaths = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); readablePaths.materializeVirtualInputs(execRoot); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java index e3afa41371c43c..483e17805e86da 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java @@ -64,7 +64,7 @@ public static SortedMap getWorkerFilesWithDigests( TreeMap workerFilesMap = new TreeMap<>(); List tools = - ActionInputHelper.expandArtifacts(spawn.getToolFiles(), artifactExpander); + ActionInputHelper.expandArtifacts(spawn.getToolFiles(), artifactExpander, false); for (ActionInput tool : tools) { @Nullable FileArtifactValue metadata = actionInputFileCache.getMetadata(tool); if (metadata == null) { 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 2a7425b9110e5c..b0b9fcf04df5dd 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 @@ -202,8 +202,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) inputFiles = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); } SandboxOutputs outputs = helpers.getOutputs(spawn); @@ -258,7 +256,8 @@ private WorkRequest createWorkRequest( } List inputs = - ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander()); + ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander(), + false); for (ActionInput input : inputs) { byte[] digestBytes = inputFileCache.getMetadata(input).getDigest(); 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 61fa76665192c1..9083a1e36f57e6 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,14 +24,11 @@ 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.ArtifactExpander; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; -import com.google.devtools.build.lib.actions.Spawn; 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.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.testutil.Scratch; @@ -66,9 +63,6 @@ @RunWith(JUnit4.class) public class SandboxHelpersTest { - private static final ArtifactExpander EMPTY_EXPANDER = (ignored1, ignored2) -> {}; - private static final Spawn SPAWN = new SpawnBuilder().build(); - private final Scratch scratch = new Scratch(); private Path execRoot; @Nullable private ExecutorService executorToCleanup; @@ -99,7 +93,7 @@ public void processInputFiles_materializesParamFile() throws Exception { UTF_8); SandboxInputs inputs = - sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot); + sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot); assertThat(inputs.getFiles()) .containsExactly(PathFragment.create("paramFile"), execRoot.getChild("paramFile")); @@ -119,7 +113,7 @@ public void processInputFiles_materializesBinToolsFile() throws Exception { PathFragment.create("_bin/say_hello")); SandboxInputs inputs = - sandboxHelpers.processInputFiles(inputMap(tool), SPAWN, EMPTY_EXPANDER, execRoot); + sandboxHelpers.processInputFiles(inputMap(tool), execRoot); assertThat(inputs.getFiles()) .containsExactly( @@ -143,7 +137,7 @@ public void processInputFiles_delayVirtualInputMaterialization_doesNotStoreVirtu UTF_8); SandboxInputs inputs = - sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot); + sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot); assertThat(inputs.getFiles()).isEmpty(); assertThat(inputs.getSymlinks()).isEmpty(); @@ -165,7 +159,7 @@ public void sandboxInputMaterializeVirtualInputs_delayMaterialization_writesCorr scratch.file("tool", "tool_code"), PathFragment.create("tools/tool")); SandboxInputs inputs = sandboxHelpers.processInputFiles( - inputMap(paramFile, tool), SPAWN, EMPTY_EXPANDER, execRoot); + inputMap(paramFile, tool), execRoot); inputs.materializeVirtualInputs(scratch.dir("/sandbox")); @@ -213,13 +207,13 @@ protected void setExecutable(PathFragment path, boolean executable) throws IOExc () -> { try { sandboxHelpers.processInputFiles( - inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot); + inputMap(input), customExecRoot); finishProcessingSemaphore.release(); } catch (IOException e) { throw new IllegalArgumentException(e); } }); - sandboxHelpers.processInputFiles(inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot); + sandboxHelpers.processInputFiles(inputMap(input), customExecRoot); finishProcessingSemaphore.release(); future.get(); diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh index ef84ca980a0017..c76cba3b653a2e 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh @@ -794,7 +794,7 @@ EOF } # regression test for https://github.com/bazelbuild/bazel/issues/6262 -function test_create_tree_artifact_inputs() { +function test_create_tree_artifact_outputs() { create_workspace_with_default_repos WORKSPACE cat > def.bzl <<'EOF' @@ -818,6 +818,36 @@ EOF bazel build --test_output=streamed :a &>$TEST_log || fail "expected build to succeed" } +function test_create_tree_artifact_inputs() { + create_workspace_with_default_repos WORKSPACE + + cat > def.bzl <<'EOF' +def _r(ctx): + empty_d = ctx.actions.declare_directory("%s_empty_dir" % ctx.label.name) + ctx.actions.run_shell( + outputs = [empty_d], + command = "mkdir -p %s" % empty_d.path, + ) + f = ctx.actions.declare_file("%s" % ctx.label.name) + ctx.actions.run_shell( + inputs = [empty_d], + outputs = [f], + command = "cd %s && pwd > %s" % (empty_d.path, f.path), + ) + return [DefaultInfo(files = depset([f]))] + +r = rule(implementation = _r) +EOF + +cat > BUILD <<'EOF' +load(":def.bzl", "r") + +r(name = "a") +EOF + + bazel build :a &>$TEST_log || fail "expected build to succeed" +} + # The test shouldn't fail if the environment doesn't support running it. check_sandbox_allowed || exit 0