diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java index b14cd4361fe8ba..78585cf4902373 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java @@ -345,7 +345,7 @@ public void maybeReportSubcommand(Spawn spawn) { /* environmentVariablesToClear= */ null, getExecRoot().getPathString(), spawn.getConfigurationChecksum(), - spawn.getExecutionPlatformLabelString()); + spawn.getExecutionPlatformLabel()); getEventHandler().handle(Event.of(EventKind.SUBCOMMAND, null, "# " + reason + "\n" + message)); } 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 9c57f73edf3ade..5bcb26aad5d2f3 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 @@ -21,7 +21,10 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.TreeSet; /** Helper utility to create ActionInput instances. */ public final class ActionInputHelper { @@ -122,14 +125,24 @@ public static List expandArtifacts( ArtifactExpander artifactExpander, boolean keepEmptyTreeArtifacts) { List result = new ArrayList<>(); + Set emptyTreeArtifacts = new TreeSet<>(); + Set treeFileArtifactParents = new HashSet<>(); for (ActionInput input : inputs.toList()) { if (input instanceof Artifact) { - Artifact.addExpandedArtifact( - (Artifact) input, result, artifactExpander, keepEmptyTreeArtifacts); + Artifact inputArtifact = (Artifact) input; + Artifact.addExpandedArtifact(inputArtifact, result, artifactExpander, emptyTreeArtifacts); + if (inputArtifact.isChildOfDeclaredDirectory()) { + treeFileArtifactParents.add(inputArtifact.getParent()); + } } else { result.add(input); } } + + if (keepEmptyTreeArtifacts) { + emptyTreeArtifacts.removeAll(treeFileArtifactParents); + result.addAll(emptyTreeArtifacts); + } 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 1775144cfd400a..1a0b1ba55483cb 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 @@ -59,6 +59,7 @@ import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.UnaryOperator; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; @@ -1544,21 +1545,20 @@ public static String joinRootRelativePaths(String delimiter, Iterable /** * Adds an artifact to a collection, expanding it once if it's a middleman or tree artifact. * - *

A middleman artifact is never added to the collection. If {@code keepEmptyTreeArtifacts} is - * true, a tree artifact will be added to the collection when it expands into zero file artifacts. - * Otherwise, only the file artifacts the tree artifact expands into will be added. + *

The middleman or tree artifact is never added to the output collection. If a tree artifact + * expands into zero file artifacts, it is added to emptyTreeArtifacts. */ static void addExpandedArtifact( Artifact artifact, Collection output, ArtifactExpander artifactExpander, - boolean keepEmptyTreeArtifacts) { + Set emptyTreeArtifacts) { if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) { List expandedArtifacts = new ArrayList<>(); artifactExpander.expand(artifact, expandedArtifacts); output.addAll(expandedArtifacts); - if (keepEmptyTreeArtifacts && artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) { - output.add(artifact); + if (artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) { + emptyTreeArtifacts.add(artifact); } } else { output.add(artifact); 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..be99bbd147220b 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 @@ -169,11 +169,19 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) { /** * Optional materialization path. * - *

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 - * com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher} to implement zero-cost - * copies of remotely stored artifacts. + *

If present, this artifact is a copy of another artifact whose contents live at this path. + * This can happen when it is declared as a file and not as an unresolved symlink but the action + * that creates it materializes it in the filesystem as a symlink to another output artifact. This + * information is useful in two situations: + * + *

    + *
  1. When the symlink target is a remotely stored artifact, we can avoid downloading it + * multiple times when building without the bytes (see AbstractActionInputPrefetcher). + *
  2. When the symlink target is inaccessible from the sandboxed environment an action runs + * under, we can rewrite it accordingly (see SandboxHelpers). + *
+ * + * @see com.google.devtools.build.lib.skyframe.TreeArtifactValue#getMaterializationExecPath(). */ public Optional getMaterializationExecPath() { return Optional.empty(); @@ -214,6 +222,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 +453,25 @@ 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; @@ -462,7 +494,8 @@ public boolean equals(Object o) { RegularFileArtifactValue that = (RegularFileArtifactValue) o; return Arrays.equals(digest, that.digest) && Objects.equals(proxy, that.proxy) - && size == that.size; + && size == that.size + && Objects.equals(getMaterializationExecPath(), that.getMaterializationExecPath()); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java index 8fa244bf94ee96..282b7888ef8cae 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java @@ -21,7 +21,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.util.DescribableExecutionUnit; import java.util.Collection; -import java.util.Objects; import javax.annotation.Nullable; /** @@ -171,9 +170,9 @@ default boolean isMandatoryOutput(ActionInput output) { @Override @Nullable - default String getExecutionPlatformLabelString() { + default Label getExecutionPlatformLabel() { PlatformInfo executionPlatform = getExecutionPlatform(); - return executionPlatform == null ? null : Objects.toString(executionPlatform.label()); + return executionPlatform != null ? executionPlatform.label() : null; } @Override @@ -184,9 +183,8 @@ default String getConfigurationChecksum() { @Override @Nullable - default String getTargetLabel() { - Label label = getResourceOwner().getOwner().getLabel(); - return label == null ? null : label.toString(); + default Label getTargetLabel() { + return getResourceOwner().getOwner().getLabel(); } /** diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/BUILD index 7de94c3cf50b2e..2372a76de605e0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/BUILD @@ -129,7 +129,6 @@ java_library( ], deps = [ "//src/main/java/com/google/devtools/build/lib:runtime", - "//src/main/java/com/google/devtools/build/lib/bazel/execlog:stable_sort", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/exec:executor_builder", @@ -138,10 +137,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", - "//src/main/java/com/google/devtools/build/lib/util/io", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/protobuf:failure_details_java_proto", - "//src/main/protobuf:spawn_java_proto", "//third_party:guava", "//third_party:jsr305", ], diff --git a/src/main/java/com/google/devtools/build/lib/bazel/SpawnLogModule.java b/src/main/java/com/google/devtools/build/lib/bazel/SpawnLogModule.java index 99d457a5ad28da..9d21bc657f51f2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/SpawnLogModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/SpawnLogModule.java @@ -15,13 +15,13 @@ import static com.google.common.base.Preconditions.checkNotNull; -import com.google.devtools.build.lib.bazel.execlog.StableSort; import com.google.devtools.build.lib.buildtool.BuildRequest; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.ExecutorBuilder; +import com.google.devtools.build.lib.exec.ExpandedSpawnLogContext; +import com.google.devtools.build.lib.exec.ExpandedSpawnLogContext.Encoding; import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; -import com.google.devtools.build.lib.exec.Protos.SpawnExec; import com.google.devtools.build.lib.exec.SpawnLogContext; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.runtime.BlazeModule; @@ -32,41 +32,16 @@ import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.DetailedExitCode; -import com.google.devtools.build.lib.util.io.AsynchronousMessageOutputStream; -import com.google.devtools.build.lib.util.io.MessageOutputStream; -import com.google.devtools.build.lib.util.io.MessageOutputStreamWrapper.BinaryOutputStreamWrapper; -import com.google.devtools.build.lib.util.io.MessageOutputStreamWrapper.JsonOutputStreamWrapper; import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; -import java.io.InputStream; import javax.annotation.Nullable; /** Module providing on-demand spawn logging. */ public final class SpawnLogModule extends BlazeModule { @Nullable private SpawnLogContext spawnLogContext; - /** Output path for the raw output stream. */ - @Nullable private Path rawOutputPath; - - /** Output stream to write directly into during execution. */ - @Nullable private MessageOutputStream rawOutputStream; - - /** - * Output stream to convert the raw output into after the execution is done. - * - *

We open the stream at the beginning of the command so that any errors (e.g., unwritable - * location) are surfaced before execution begins. - */ - @Nullable private MessageOutputStream convertedOutputStream; - - private CommandEnvironment env; - private void clear() { spawnLogContext = null; - rawOutputPath = null; - rawOutputStream = null; - convertedOutputStream = null; - env = null; } private void initOutputs(CommandEnvironment env) throws IOException { @@ -99,45 +74,30 @@ private void initOutputs(CommandEnvironment env) throws IOException { return; } - this.env = env; - Path workingDirectory = env.getWorkingDirectory(); Path outputBase = env.getOutputBase(); - // Set up the raw output stream. - // This stream performs the writes in a separate thread to avoid blocking execution. - // If the unsorted binary format was requested, use the respective output path to avoid a - // pointless conversion at the end. Otherwise, use a temporary path. - if (executionOptions.executionLogBinaryFile != null && !executionOptions.executionLogSort) { - rawOutputPath = workingDirectory.getRelative(executionOptions.executionLogBinaryFile); - } else { - rawOutputPath = outputBase.getRelative("execution.log"); - } - rawOutputStream = new AsynchronousMessageOutputStream<>(rawOutputPath); - - // Set up the binary output stream, if distinct from the raw output stream. - if (executionOptions.executionLogBinaryFile != null && executionOptions.executionLogSort) { - convertedOutputStream = - new BinaryOutputStreamWrapper<>( - workingDirectory - .getRelative(executionOptions.executionLogBinaryFile) - .getOutputStream()); + Path outputPath = null; + Encoding encoding = null; + if (executionOptions.executionLogBinaryFile != null) { + encoding = Encoding.BINARY; + outputPath = workingDirectory.getRelative(executionOptions.executionLogBinaryFile); + } else if (executionOptions.executionLogJsonFile != null) { + encoding = Encoding.JSON; + outputPath = workingDirectory.getRelative(executionOptions.executionLogJsonFile); } - // Set up the text output stream. - if (executionOptions.executionLogJsonFile != null) { - convertedOutputStream = - new JsonOutputStreamWrapper<>( - workingDirectory - .getRelative(executionOptions.executionLogJsonFile) - .getOutputStream()); - } + // Use a well-known temporary path to avoid accumulation of potentially large files in /tmp + // due to abnormally terminated invocations (e.g., when running out of memory). + Path tempPath = outputBase.getRelative("execution.log"); spawnLogContext = - new SpawnLogContext( + new ExpandedSpawnLogContext( + checkNotNull(outputPath), + tempPath, + checkNotNull(encoding), + /* sorted= */ executionOptions.executionLogSort, env.getExecRoot().asFragment(), - rawOutputStream, - env.getOptions().getOptions(ExecutionOptions.class), env.getOptions().getOptions(RemoteOptions.class), env.getRuntime().getFileSystem().getDigestFunction(), env.getXattrProvider()); @@ -180,44 +140,13 @@ public void afterCommand() throws AbruptExitException { return; } - checkNotNull(rawOutputPath); - - boolean done = false; try { spawnLogContext.close(); - if (convertedOutputStream != null) { - InputStream in = rawOutputPath.getInputStream(); - if (spawnLogContext.shouldSort()) { - StableSort.stableSort(in, convertedOutputStream); - } else { - while (in.available() > 0) { - SpawnExec ex = SpawnExec.parseDelimitedFrom(in); - convertedOutputStream.write(ex); - } - } - convertedOutputStream.close(); - } - done = true; } catch (IOException e) { String message = e.getMessage() == null ? "Error writing execution log" : e.getMessage(); throw new AbruptExitException( createDetailedExitCode(message, Code.EXECUTION_LOG_WRITE_FAILURE), e); } finally { - if (convertedOutputStream != null) { - if (!done) { - env.getReporter() - .handle( - Event.warn( - "Execution log might not have been populated. Raw execution log is at " - + rawOutputPath)); - } else { - try { - rawOutputPath.delete(); - } catch (IOException e) { - // Intentionally ignored. - } - } - } clear(); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 711ce734ea5ebb..92e3aa5b2f8a77 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -29,9 +29,13 @@ import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileGlobals.ModuleExtensionUsageBuilder.ModuleExtensionProxy; import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.StarlarkExportable; +import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashMap; @@ -480,17 +484,31 @@ public ModuleExtensionProxy useExtension( } private String normalizeLabelString(String rawExtensionBzlFile) { - // Normalize the label by adding the current module's repo_name if the label doesn't specify a - // repository name. This is necessary as ModuleExtensionUsages are grouped by the string value - // of this label, but later mapped to their Label representation. If multiple strings map to the - // same Label, this would result in a crash. + // Normalize the label by parsing and stringifying it with a repo mapping that preserves the + // apparent repository name, except that a reference to the main repository via the empty + // repo name is translated to using the module repo name. This is necessary as + // ModuleExtensionUsages are grouped by the string value of this label, but later mapped to + // their Label representation. If multiple strings map to the same Label, this would result in a + // crash. // ownName can't change anymore as calling module() after this results in an error. String ownName = module.getRepoName().orElse(module.getName()); - if (module.getKey().equals(ModuleKey.ROOT) && rawExtensionBzlFile.startsWith("@//")) { - return "@" + ownName + rawExtensionBzlFile.substring(1); - } else if (rawExtensionBzlFile.startsWith("//")) { - return "@" + ownName + rawExtensionBzlFile; - } else { + RepositoryName ownRepoName = RepositoryName.createUnvalidated(ownName); + try { + ImmutableMap repoMapping = ImmutableMap.of(); + if (module.getKey().equals(ModuleKey.ROOT)) { + repoMapping = ImmutableMap.of("", ownRepoName); + } + Label label = + Label.parseWithPackageContext( + rawExtensionBzlFile, + Label.PackageContext.of( + PackageIdentifier.create(ownRepoName, PathFragment.EMPTY_FRAGMENT), + RepositoryMapping.createAllowingFallback(repoMapping))); + // Skip over the leading "@" of the unambiguous form. + return label.getUnambiguousCanonicalForm().substring(1); + } catch (LabelSyntaxException ignored) { + // Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and + // let the extension fail when evaluated. return rawExtensionBzlFile; } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java b/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java index 92267202f7be96..77c0be124615c8 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java @@ -278,7 +278,7 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe baseModule.getUnusedDeps())); } catch (InvalidArgumentException | OptionsParsingException e) { throw new InvalidArgumentException( - String.format("In extension argument: %s %s", arg, e.getMessage()), + String.format("In extension argument %s: %s", arg, e.getMessage()), Code.INVALID_ARGUMENTS, e); } @@ -494,13 +494,10 @@ private static ImmutableSortedSet extensionArgListToIds( private static BlazeCommandResult reportAndCreateFailureResult( CommandEnvironment env, String message, Code detailedCode) { - if (message.charAt(message.length() - 1) != '.') { - message = message.concat("."); - } String fullMessage = String.format( - message.concat(" Type '%s help mod' for syntax and help."), - env.getRuntime().getProductName()); + "%s%s Type '%s help mod' for syntax and help.", + message, message.endsWith(".") ? "" : ".", env.getRuntime().getProductName()); env.getReporter().handle(Event.error(fullMessage)); return createFailureResult(fullMessage, detailedCode); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorValue.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorValue.java index 98ca4b4fa7e0ee..134f1a0e37cb60 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorValue.java @@ -24,6 +24,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; +import java.nio.channels.ClosedByInterruptException; import java.util.Optional; import java.util.Set; import net.starlark.java.eval.Starlark; @@ -128,6 +129,8 @@ public static Path decompress(DecompressorDescriptor descriptor) throws RepositoryFunctionException, InterruptedException { try { return getDecompressor(descriptor.archivePath()).decompress(descriptor); + } catch (ClosedByInterruptException e) { + throw new InterruptedException(); } catch (IOException e) { Path destinationDirectory = descriptor.archivePath().getParentDirectory(); throw new RepositoryFunctionException( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index e9b641086aa33d..f34fb44603f63d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -494,9 +494,6 @@ public StructImpl download( if (executable) { outputPath.getPath().setExecutable(true); } - } catch (InterruptedException e) { - throw new RepositoryFunctionException( - new IOException("thread interrupted"), Transience.TRANSIENT); } catch (IOException e) { if (allowFail) { return StarlarkInfo.create( @@ -692,10 +689,6 @@ public StructImpl downloadAndExtract( env.getListener(), envVariables, getIdentifyingStringForLogging()); - } catch (InterruptedException e) { - env.getListener().post(w); - throw new RepositoryFunctionException( - new IOException("thread interrupted"), Transience.TRANSIENT); } catch (IOException e) { env.getListener().post(w); if (allowFail) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index dd8ad14c9b9fc8..246228875f8788 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -516,43 +516,56 @@ public String toString() { * potentially more expensive operations. */ // TODO(wyv): somehow migrate this to the base context too. - public void enforceLabelAttributes() throws EvalException, InterruptedException { + public void enforceLabelAttributes() + throws EvalException, InterruptedException, NeedsSkyframeRestartException { // TODO: If a labels fails to resolve to an existing regular file, we do not add a dependency on // that fact - if the file is created later or changes its type, it will not trigger a rerun of // the repository function. StructImpl attr = getAttr(); + // Batch restarts as they are expensive + boolean needsRestart = false; for (String name : attr.getFieldNames()) { Object value = attr.getValue(name); if (value instanceof Label) { - dependOnLabelIgnoringErrors((Label) value); + if (dependOnLabelIgnoringErrors((Label) value)) { + needsRestart = true; + } } if (value instanceof Sequence) { for (Object entry : (Sequence) value) { if (entry instanceof Label) { - dependOnLabelIgnoringErrors((Label) entry); + if (dependOnLabelIgnoringErrors((Label) entry)) { + needsRestart = true; + } } } } if (value instanceof Dict) { for (Object entry : ((Dict) value).keySet()) { if (entry instanceof Label) { - dependOnLabelIgnoringErrors((Label) entry); + if (dependOnLabelIgnoringErrors((Label) entry)) { + needsRestart = true; + } } } } } + + if (needsRestart) { + throw new NeedsSkyframeRestartException(); + } } - private void dependOnLabelIgnoringErrors(Label label) - throws InterruptedException, NeedsSkyframeRestartException { + private boolean dependOnLabelIgnoringErrors(Label label) throws InterruptedException { try { getPathFromLabel(label); } catch (NeedsSkyframeRestartException e) { - throw e; + return true; } catch (EvalException e) { // EvalExceptions indicate labels not referring to existing files. This is fine, // as long as they are never resolved to files in the execution of the rule; we allow // non-strict rules. } + return false; } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index b1b02b40f36159..424882321ebf20 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -257,13 +257,16 @@ java_library( java_library( name = "spawn_log_context", - srcs = ["SpawnLogContext.java"], + srcs = [ + "ExpandedSpawnLogContext.java", + "SpawnLogContext.java", + ], deps = [ - ":execution_options", "//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/platform:platform_utils", + "//src/main/java/com/google/devtools/build/lib/bazel/execlog:stable_sort", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/util/io", diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java new file mode 100644 index 00000000000000..8f5a6c451b1c6b --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java @@ -0,0 +1,323 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.exec; + +import static com.google.devtools.build.lib.exec.SpawnLogContext.computeDigest; +import static com.google.devtools.build.lib.exec.SpawnLogContext.getSpawnMetricsProto; + +import build.bazel.remote.execution.v2.Platform; +import com.google.common.collect.ImmutableSet; +import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.InputMetadataProvider; +import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.actions.SpawnResult; +import com.google.devtools.build.lib.actions.Spawns; +import com.google.devtools.build.lib.actions.cache.VirtualActionInput; +import com.google.devtools.build.lib.analysis.platform.PlatformUtils; +import com.google.devtools.build.lib.bazel.execlog.StableSort; +import com.google.devtools.build.lib.exec.Protos.Digest; +import com.google.devtools.build.lib.exec.Protos.File; +import com.google.devtools.build.lib.exec.Protos.SpawnExec; +import com.google.devtools.build.lib.profiler.Profiler; +import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.util.io.AsynchronousMessageOutputStream; +import com.google.devtools.build.lib.util.io.MessageOutputStream; +import com.google.devtools.build.lib.util.io.MessageOutputStreamWrapper.BinaryOutputStreamWrapper; +import com.google.devtools.build.lib.util.io.MessageOutputStreamWrapper.JsonOutputStreamWrapper; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.Dirent; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.devtools.build.lib.vfs.XattrProvider; +import java.io.IOException; +import java.io.InputStream; +import java.time.Duration; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.Map; +import java.util.SortedMap; +import java.util.TreeMap; +import java.util.TreeSet; +import java.util.function.Consumer; +import javax.annotation.Nullable; + +/** A {@link SpawnLogContext} implementation that produces a log in expanded format. */ +public class ExpandedSpawnLogContext implements SpawnLogContext { + + /** The log encoding. */ + public enum Encoding { + /** Length-delimited binary protos. */ + BINARY, + /** Newline-delimited JSON messages. */ + JSON + } + + private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + + private final Path tempPath; + private final boolean sorted; + + private final PathFragment execRoot; + @Nullable private final RemoteOptions remoteOptions; + private final DigestHashFunction digestHashFunction; + private final XattrProvider xattrProvider; + + /** Output stream to write directly into during execution. */ + private final MessageOutputStream rawOutputStream; + + /** Output stream to convert the raw output stream into after execution, if required. */ + @Nullable private final MessageOutputStream convertedOutputStream; + + public ExpandedSpawnLogContext( + Path outputPath, + Path tempPath, + Encoding encoding, + boolean sorted, + PathFragment execRoot, + @Nullable RemoteOptions remoteOptions, + DigestHashFunction digestHashFunction, + XattrProvider xattrProvider) + throws IOException { + this.tempPath = tempPath; + this.sorted = sorted; + this.execRoot = execRoot; + this.remoteOptions = remoteOptions; + this.digestHashFunction = digestHashFunction; + this.xattrProvider = xattrProvider; + + if (encoding == Encoding.BINARY && !sorted) { + // The unsorted binary format can be written directly into the output path during execution. + rawOutputStream = getRawOutputStream(outputPath); + convertedOutputStream = null; + } else { + // Otherwise, write the unsorted binary format into a temporary path first, then convert into + // the output format after execution. + rawOutputStream = getRawOutputStream(tempPath); + convertedOutputStream = getConvertedOutputStream(encoding, outputPath); + } + } + + private static MessageOutputStream getRawOutputStream(Path path) throws IOException { + // Use an AsynchronousMessageOutputStream so that writes occur in a separate thread. + // This ensures concurrent writes don't tear and avoids blocking execution. + return new AsynchronousMessageOutputStream<>(path); + } + + private static MessageOutputStream getConvertedOutputStream( + Encoding encoding, Path path) throws IOException { + switch (encoding) { + case BINARY: + return new BinaryOutputStreamWrapper<>(path.getOutputStream()); + case JSON: + return new JsonOutputStreamWrapper<>(path.getOutputStream()); + } + throw new IllegalArgumentException( + String.format("invalid execution log encoding: %s", encoding)); + } + + @Override + public void logSpawn( + Spawn spawn, + InputMetadataProvider inputMetadataProvider, + SortedMap inputMap, + FileSystem fileSystem, + Duration timeout, + SpawnResult result) + throws IOException, ExecException { + SortedMap existingOutputs = listExistingOutputs(spawn, fileSystem); + SpawnExec.Builder builder = SpawnExec.newBuilder(); + builder.addAllCommandArgs(spawn.getArguments()); + + Map env = spawn.getEnvironment(); + // Sorting the environment pairs by variable name. + TreeSet variables = new TreeSet<>(env.keySet()); + for (String var : variables) { + builder.addEnvironmentVariablesBuilder().setName(var).setValue(env.get(var)); + } + + ImmutableSet toolFiles = spawn.getToolFiles().toSet(); + + try (SilentCloseable c = Profiler.instance().profile("logSpawn/inputs")) { + for (Map.Entry e : inputMap.entrySet()) { + ActionInput input = e.getValue(); + if (input instanceof VirtualActionInput.EmptyActionInput) { + continue; + } + Path inputPath = fileSystem.getPath(execRoot.getRelative(input.getExecPathString())); + if (inputPath.isDirectory()) { + listDirectoryContents(inputPath, builder::addInputs, inputMetadataProvider); + continue; + } + Digest digest = + computeDigest( + input, inputPath, inputMetadataProvider, xattrProvider, digestHashFunction); + boolean isTool = + toolFiles.contains(input) + || (input instanceof TreeFileArtifact + && toolFiles.contains(((TreeFileArtifact) input).getParent())); + builder + .addInputsBuilder() + .setPath(input.getExecPathString()) + .setDigest(digest) + .setIsTool(isTool); + } + } catch (IOException e) { + logger.atWarning().withCause(e).log("Error computing spawn inputs"); + } + try (SilentCloseable c = Profiler.instance().profile("logSpawn/outputs")) { + ArrayList outputPaths = new ArrayList<>(); + for (ActionInput output : spawn.getOutputFiles()) { + outputPaths.add(output.getExecPathString()); + } + Collections.sort(outputPaths); + builder.addAllListedOutputs(outputPaths); + for (Map.Entry e : existingOutputs.entrySet()) { + Path path = e.getKey(); + if (path.isDirectory()) { + listDirectoryContents(path, builder::addActualOutputs, inputMetadataProvider); + } else { + File.Builder outputBuilder = builder.addActualOutputsBuilder(); + outputBuilder.setPath(path.relativeTo(fileSystem.getPath(execRoot)).toString()); + try { + outputBuilder.setDigest( + computeDigest( + e.getValue(), path, inputMetadataProvider, xattrProvider, digestHashFunction)); + } catch (IOException ex) { + logger.atWarning().withCause(ex).log("Error computing spawn event output properties"); + } + } + } + } + builder.setRemotable(Spawns.mayBeExecutedRemotely(spawn)); + + Platform execPlatform = PlatformUtils.getPlatformProto(spawn, remoteOptions); + if (execPlatform != null) { + builder.setPlatform(buildPlatform(execPlatform)); + } + if (result.status() != SpawnResult.Status.SUCCESS) { + builder.setStatus(result.status().toString()); + } + if (!timeout.isZero()) { + builder.setTimeoutMillis(timeout.toMillis()); + } + builder.setCacheable(Spawns.mayBeCached(spawn)); + builder.setRemoteCacheable(Spawns.mayBeCachedRemotely(spawn)); + builder.setExitCode(result.exitCode()); + builder.setCacheHit(result.isCacheHit()); + builder.setRunner(result.getRunnerName()); + + if (result.getDigest() != null) { + builder.setDigest(result.getDigest()); + } + + builder.setMnemonic(spawn.getMnemonic()); + + if (spawn.getTargetLabel() != null) { + builder.setTargetLabel(spawn.getTargetLabel().toString()); + } + + builder.setMetrics(getSpawnMetricsProto(result)); + + try (SilentCloseable c = Profiler.instance().profile("logSpawn/write")) { + rawOutputStream.write(builder.build()); + } + } + + @Override + public void close() throws IOException { + rawOutputStream.close(); + + if (convertedOutputStream == null) { + // No conversion required. + return; + } + + try (InputStream in = tempPath.getInputStream()) { + if (sorted) { + StableSort.stableSort(in, convertedOutputStream); + } else { + while (in.available() > 0) { + SpawnExec ex = SpawnExec.parseDelimitedFrom(in); + convertedOutputStream.write(ex); + } + } + } finally { + try { + tempPath.delete(); + } catch (IOException e) { + // Intentionally ignored. + } + } + } + + private static Protos.Platform buildPlatform(Platform platform) { + Protos.Platform.Builder platformBuilder = Protos.Platform.newBuilder(); + for (Platform.Property p : platform.getPropertiesList()) { + platformBuilder.addPropertiesBuilder().setName(p.getName()).setValue(p.getValue()); + } + return platformBuilder.build(); + } + + private SortedMap listExistingOutputs(Spawn spawn, FileSystem fileSystem) { + TreeMap result = new TreeMap<>(); + for (ActionInput output : spawn.getOutputFiles()) { + Path outputPath = fileSystem.getPath(execRoot.getRelative(output.getExecPathString())); + // TODO(olaola): once symlink API proposal is implemented, report symlinks here. + if (outputPath.exists()) { + result.put(outputPath, output); + } + } + return result; + } + + private void listDirectoryContents( + Path path, Consumer addFile, InputMetadataProvider inputMetadataProvider) { + try { + // TODO(olaola): once symlink API proposal is implemented, report symlinks here. + List sortedDirent = new ArrayList<>(path.readdir(Symlinks.NOFOLLOW)); + sortedDirent.sort(Comparator.comparing(Dirent::getName)); + for (Dirent dirent : sortedDirent) { + String name = dirent.getName(); + Path child = path.getRelative(name); + if (dirent.getType() == Dirent.Type.DIRECTORY) { + listDirectoryContents(child, addFile, inputMetadataProvider); + } else { + String pathString; + if (child.startsWith(execRoot)) { + pathString = child.asFragment().relativeTo(execRoot).getPathString(); + } else { + pathString = child.getPathString(); + } + addFile.accept( + File.newBuilder() + .setPath(pathString) + .setDigest( + computeDigest( + null, child, inputMetadataProvider, xattrProvider, digestHashFunction)) + .build()); + } + } + } catch (IOException e) { + logger.atWarning().withCause(e).log("Error computing spawn event file properties"); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java index cf0f3716a76ec3..bb655b530be28d 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java @@ -13,306 +13,70 @@ // limitations under the License. package com.google.devtools.build.lib.exec; -import build.bazel.remote.execution.v2.Platform; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableSet; -import com.google.common.flogger.GoogleLogger; import com.google.common.hash.HashCode; import com.google.devtools.build.lib.actions.ActionContext; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ExecException; 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.SpawnMetrics; import com.google.devtools.build.lib.actions.SpawnResult; -import com.google.devtools.build.lib.actions.Spawns; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; -import com.google.devtools.build.lib.analysis.platform.PlatformUtils; import com.google.devtools.build.lib.exec.Protos.Digest; -import com.google.devtools.build.lib.exec.Protos.File; -import com.google.devtools.build.lib.exec.Protos.SpawnExec; -import com.google.devtools.build.lib.profiler.Profiler; -import com.google.devtools.build.lib.profiler.SilentCloseable; -import com.google.devtools.build.lib.remote.options.RemoteOptions; -import com.google.devtools.build.lib.util.io.MessageOutputStream; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.DigestUtils; -import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.XattrProvider; import com.google.protobuf.util.Durations; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.time.Duration; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Comparator; -import java.util.List; -import java.util.Map; import java.util.SortedMap; -import java.util.TreeMap; -import java.util.TreeSet; -import java.util.function.Consumer; import javax.annotation.Nullable; -/** - * A logging utility for spawns. - */ -public class SpawnLogContext implements ActionContext { - - private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - - private final PathFragment execRoot; - private final MessageOutputStream executionLog; - @Nullable private final ExecutionOptions executionOptions; - @Nullable private final RemoteOptions remoteOptions; - private final DigestHashFunction digestHashFunction; - private final XattrProvider xattrProvider; - - public SpawnLogContext( - PathFragment execRoot, - MessageOutputStream executionLog, - @Nullable ExecutionOptions executionOptions, - @Nullable RemoteOptions remoteOptions, - DigestHashFunction digestHashFunction, - XattrProvider xattrProvider) { - this.execRoot = execRoot; - this.executionLog = executionLog; - this.executionOptions = executionOptions; - this.remoteOptions = remoteOptions; - this.digestHashFunction = digestHashFunction; - this.xattrProvider = xattrProvider; - } - +/** An {@link ActionContext} providing the ability to log executed spawns. */ +public interface SpawnLogContext extends ActionContext { /** - * Logs the executed spawn to the output stream. + * Logs an executed spawn. + * + *

May be called concurrently. * * @param spawn the spawn to log * @param inputMetadataProvider provides metadata for the spawn inputs + * @param inputMap the mapping from input paths to action inputs * @param fileSystem the filesystem containing the spawn inputs and outputs, which might be an * action filesystem when building without the bytes * @param timeout the timeout the spawn was run under * @param result the spawn result */ - public void logSpawn( + void logSpawn( Spawn spawn, InputMetadataProvider inputMetadataProvider, SortedMap inputMap, FileSystem fileSystem, Duration timeout, SpawnResult result) - throws IOException, ExecException { - SortedMap existingOutputs = listExistingOutputs(spawn, fileSystem); - SpawnExec.Builder builder = SpawnExec.newBuilder(); - builder.addAllCommandArgs(spawn.getArguments()); - - Map env = spawn.getEnvironment(); - // Sorting the environment pairs by variable name. - TreeSet variables = new TreeSet<>(env.keySet()); - for (String var : variables) { - builder.addEnvironmentVariablesBuilder().setName(var).setValue(env.get(var)); - } - - ImmutableSet toolFiles = spawn.getToolFiles().toSet(); - - try (SilentCloseable c = Profiler.instance().profile("logSpawn/inputs")) { - for (Map.Entry e : inputMap.entrySet()) { - ActionInput input = e.getValue(); - if (input instanceof VirtualActionInput.EmptyActionInput) { - continue; - } - Path inputPath = fileSystem.getPath(execRoot.getRelative(input.getExecPathString())); - if (inputPath.isDirectory()) { - listDirectoryContents(inputPath, builder::addInputs, inputMetadataProvider); - continue; - } - Digest digest = computeDigest(input, inputPath, inputMetadataProvider, xattrProvider); - boolean isTool = - toolFiles.contains(input) - || (input instanceof TreeFileArtifact - && toolFiles.contains(((TreeFileArtifact) input).getParent())); - builder - .addInputsBuilder() - .setPath(input.getExecPathString()) - .setDigest(digest) - .setIsTool(isTool); - } - } catch (IOException e) { - logger.atWarning().withCause(e).log("Error computing spawn inputs"); - } - try (SilentCloseable c = Profiler.instance().profile("logSpawn/outputs")) { - ArrayList outputPaths = new ArrayList<>(); - for (ActionInput output : spawn.getOutputFiles()) { - outputPaths.add(output.getExecPathString()); - } - Collections.sort(outputPaths); - builder.addAllListedOutputs(outputPaths); - for (Map.Entry e : existingOutputs.entrySet()) { - Path path = e.getKey(); - if (path.isDirectory()) { - listDirectoryContents(path, builder::addActualOutputs, inputMetadataProvider); - } else { - File.Builder outputBuilder = builder.addActualOutputsBuilder(); - outputBuilder.setPath(path.relativeTo(fileSystem.getPath(execRoot)).toString()); - try { - outputBuilder.setDigest( - computeDigest(e.getValue(), path, inputMetadataProvider, xattrProvider)); - } catch (IOException ex) { - logger.atWarning().withCause(ex).log("Error computing spawn event output properties"); - } - } - } - } - builder.setRemotable(Spawns.mayBeExecutedRemotely(spawn)); - - Platform execPlatform = PlatformUtils.getPlatformProto(spawn, remoteOptions); - if (execPlatform != null) { - builder.setPlatform(buildPlatform(execPlatform)); - } - if (result.status() != SpawnResult.Status.SUCCESS) { - builder.setStatus(result.status().toString()); - } - if (!timeout.isZero()) { - builder.setTimeoutMillis(timeout.toMillis()); - } - builder.setCacheable(Spawns.mayBeCached(spawn)); - builder.setRemoteCacheable(Spawns.mayBeCachedRemotely(spawn)); - builder.setExitCode(result.exitCode()); - builder.setCacheHit(result.isCacheHit()); - builder.setRunner(result.getRunnerName()); - - if (result.getDigest() != null) { - builder.setDigest(result.getDigest()); - } - - builder.setMnemonic(spawn.getMnemonic()); - - if (spawn.getTargetLabel() != null) { - builder.setTargetLabel(spawn.getTargetLabel()); - } - - SpawnMetrics metrics = result.getMetrics(); - Protos.SpawnMetrics.Builder metricsBuilder = builder.getMetricsBuilder(); - if (metrics.totalTimeInMs() != 0L) { - metricsBuilder.setTotalTime(millisToProto(metrics.totalTimeInMs())); - } - if (metrics.parseTimeInMs() != 0L) { - metricsBuilder.setParseTime(millisToProto(metrics.parseTimeInMs())); - } - if (metrics.networkTimeInMs() != 0L) { - metricsBuilder.setNetworkTime(millisToProto(metrics.networkTimeInMs())); - } - if (metrics.fetchTimeInMs() != 0L) { - metricsBuilder.setFetchTime(millisToProto(metrics.fetchTimeInMs())); - } - if (metrics.queueTimeInMs() != 0L) { - metricsBuilder.setQueueTime(millisToProto(metrics.queueTimeInMs())); - } - if (metrics.setupTimeInMs() != 0L) { - metricsBuilder.setSetupTime(millisToProto(metrics.setupTimeInMs())); - } - if (metrics.uploadTimeInMs() != 0L) { - metricsBuilder.setUploadTime(millisToProto(metrics.uploadTimeInMs())); - } - if (metrics.executionWallTimeInMs() != 0L) { - metricsBuilder.setExecutionWallTime(millisToProto(metrics.executionWallTimeInMs())); - } - if (metrics.processOutputsTimeInMs() != 0L) { - metricsBuilder.setProcessOutputsTime(millisToProto(metrics.processOutputsTimeInMs())); - } - if (metrics.retryTimeInMs() != 0L) { - metricsBuilder.setRetryTime(millisToProto(metrics.retryTimeInMs())); - } - metricsBuilder.setInputBytes(metrics.inputBytes()); - metricsBuilder.setInputFiles(metrics.inputFiles()); - metricsBuilder.setMemoryEstimateBytes(metrics.memoryEstimate()); - metricsBuilder.setInputBytesLimit(metrics.inputBytesLimit()); - metricsBuilder.setInputFilesLimit(metrics.inputFilesLimit()); - metricsBuilder.setOutputBytesLimit(metrics.outputBytesLimit()); - metricsBuilder.setOutputFilesLimit(metrics.outputFilesLimit()); - metricsBuilder.setMemoryBytesLimit(metrics.memoryLimit()); - if (metrics.timeLimitInMs() != 0L) { - metricsBuilder.setTimeLimit(millisToProto(metrics.timeLimitInMs())); - } - - try (SilentCloseable c = Profiler.instance().profile("logSpawn/write")) { - executionLog.write(builder.build()); - } - } + throws IOException, ExecException; - @VisibleForTesting - static com.google.protobuf.Duration millisToProto(int t) { - return Durations.fromMillis(t); - } - - public void close() throws IOException { - executionLog.close(); - } - - private static Protos.Platform buildPlatform(Platform platform) { - Protos.Platform.Builder platformBuilder = Protos.Platform.newBuilder(); - for (Platform.Property p : platform.getPropertiesList()) { - platformBuilder.addPropertiesBuilder().setName(p.getName()).setValue(p.getValue()); - } - return platformBuilder.build(); - } - - private SortedMap listExistingOutputs(Spawn spawn, FileSystem fileSystem) { - TreeMap result = new TreeMap<>(); - for (ActionInput output : spawn.getOutputFiles()) { - Path outputPath = fileSystem.getPath(execRoot.getRelative(output.getExecPathString())); - // TODO(olaola): once symlink API proposal is implemented, report symlinks here. - if (outputPath.exists()) { - result.put(outputPath, output); - } - } - return result; - } - - private void listDirectoryContents( - Path path, Consumer addFile, InputMetadataProvider inputMetadataProvider) { - try { - // TODO(olaola): once symlink API proposal is implemented, report symlinks here. - List sortedDirent = new ArrayList<>(path.readdir(Symlinks.NOFOLLOW)); - sortedDirent.sort(Comparator.comparing(Dirent::getName)); - for (Dirent dirent : sortedDirent) { - String name = dirent.getName(); - Path child = path.getRelative(name); - if (dirent.getType() == Dirent.Type.DIRECTORY) { - listDirectoryContents(child, addFile, inputMetadataProvider); - } else { - String pathString; - if (child.startsWith(execRoot)) { - pathString = child.asFragment().relativeTo(execRoot).getPathString(); - } else { - pathString = child.getPathString(); - } - addFile.accept( - File.newBuilder() - .setPath(pathString) - .setDigest(computeDigest(null, child, inputMetadataProvider, xattrProvider)) - .build()); - } - } - } catch (IOException e) { - logger.atWarning().withCause(e).log("Error computing spawn event file properties"); - } - } + /** Finishes writing the log and performs any required post-processing. */ + void close() throws IOException; /** - * Computes the digest of the given ActionInput or corresponding path. Will try to access the - * Metadata cache first, if it is available, and fall back to digesting the contents manually. + * Computes the digest of an ActionInput or its path. + * + *

Will try to obtain the digest from cached metadata first, falling back to digesting the + * contents manually. */ - private Digest computeDigest( + static Digest computeDigest( @Nullable ActionInput input, Path path, InputMetadataProvider inputMetadataProvider, - XattrProvider xattrProvider) + XattrProvider xattrProvider, + DigestHashFunction digestHashFunction) throws IOException { Digest.Builder builder = Digest.newBuilder().setHashFunctionName(digestHashFunction.toString()); @@ -353,7 +117,55 @@ private Digest computeDigest( .build(); } - public boolean shouldSort() { - return executionOptions.executionLogSort; + static Protos.SpawnMetrics getSpawnMetricsProto(SpawnResult result) { + SpawnMetrics metrics = result.getMetrics(); + Protos.SpawnMetrics.Builder builder = Protos.SpawnMetrics.newBuilder(); + if (metrics.totalTimeInMs() != 0L) { + builder.setTotalTime(millisToProto(metrics.totalTimeInMs())); + } + if (metrics.parseTimeInMs() != 0L) { + builder.setParseTime(millisToProto(metrics.parseTimeInMs())); + } + if (metrics.networkTimeInMs() != 0L) { + builder.setNetworkTime(millisToProto(metrics.networkTimeInMs())); + } + if (metrics.fetchTimeInMs() != 0L) { + builder.setFetchTime(millisToProto(metrics.fetchTimeInMs())); + } + if (metrics.queueTimeInMs() != 0L) { + builder.setQueueTime(millisToProto(metrics.queueTimeInMs())); + } + if (metrics.setupTimeInMs() != 0L) { + builder.setSetupTime(millisToProto(metrics.setupTimeInMs())); + } + if (metrics.uploadTimeInMs() != 0L) { + builder.setUploadTime(millisToProto(metrics.uploadTimeInMs())); + } + if (metrics.executionWallTimeInMs() != 0L) { + builder.setExecutionWallTime(millisToProto(metrics.executionWallTimeInMs())); + } + if (metrics.processOutputsTimeInMs() != 0L) { + builder.setProcessOutputsTime(millisToProto(metrics.processOutputsTimeInMs())); + } + if (metrics.retryTimeInMs() != 0L) { + builder.setRetryTime(millisToProto(metrics.retryTimeInMs())); + } + builder.setInputBytes(metrics.inputBytes()); + builder.setInputFiles(metrics.inputFiles()); + builder.setMemoryEstimateBytes(metrics.memoryEstimate()); + builder.setInputBytesLimit(metrics.inputBytesLimit()); + builder.setInputFilesLimit(metrics.inputFilesLimit()); + builder.setOutputBytesLimit(metrics.outputBytesLimit()); + builder.setOutputFilesLimit(metrics.outputFilesLimit()); + builder.setMemoryBytesLimit(metrics.memoryLimit()); + if (metrics.timeLimitInMs() != 0L) { + builder.setTimeLimit(millisToProto(metrics.timeLimitInMs())); + } + return builder.build(); + } + + @VisibleForTesting + static com.google.protobuf.Duration millisToProto(int t) { + return Durations.fromMillis(t); } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index a7000ebbc233b7..3e8ca341c80a2d 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -740,6 +740,12 @@ private TestAttemptResult runTestAttempt( .getOutputMetadataStore() .getTreeArtifactChildren( (SpecialArtifact) testAction.getCoverageDirectoryTreeArtifact()); + ImmutableSet coverageSpawnMetadata = + ImmutableSet.builder() + .addAll(expandedCoverageDir) + .add(testAction.getCoverageDirectoryTreeArtifact()) + .build(); + Spawn coveragePostProcessingSpawn = createCoveragePostProcessingSpawn( actionExecutionContext, @@ -759,7 +765,7 @@ private TestAttemptResult runTestAttempt( ActionExecutionContext coverageActionExecutionContext = actionExecutionContext .withFileOutErr(coverageOutErr) - .withOutputsAsInputs(expandedCoverageDir); + .withOutputsAsInputs(coverageSpawnMetadata); writeOutFile(coverageOutErr.getErrorPath(), coverageOutErr.getOutputPath()); appendCoverageLog(coverageOutErr, fileOutErr); diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java index 39282e6a206db7..916b5b41ba052c 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java @@ -131,8 +131,10 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) SpawnMetrics.Builder spawnMetrics = SpawnMetrics.Builder.forLocalExec(); Stopwatch totalTimeStopwatch = Stopwatch.createStarted(); Stopwatch setupTimeStopwatch = Stopwatch.createStarted(); - runfilesTreeUpdater.updateRunfiles( - spawn.getRunfilesSupplier(), spawn.getEnvironment(), context.getFileOutErr()); + try (var s = Profiler.instance().profile("updateRunfiles")) { + runfilesTreeUpdater.updateRunfiles( + spawn.getRunfilesSupplier(), spawn.getEnvironment(), context.getFileOutErr()); + } if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) { context.prefetchInputsAndWait(); } diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java index e1796b7a0798d5..f853e5de26342b 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java @@ -51,7 +51,6 @@ import java.util.Base64; import java.util.HashMap; import java.util.Map; -import java.util.Objects; import java.util.stream.Collectors; import net.starlark.java.eval.EvalException; @@ -260,9 +259,9 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream) /* environmentVariablesToClear= */ null, /* cwd= */ null, action.getOwner().getConfigurationChecksum(), - action.getExecutionPlatform() == null - ? null - : Objects.toString(action.getExecutionPlatform().label()))) + action.getExecutionPlatform() != null + ? action.getExecutionPlatform().label() + : null)) .append("\n"); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index a8f3c55dfcf9b8..a81718008a27ca 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -16,17 +16,20 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.util.concurrent.Futures.immediateFailedFuture; +import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static com.google.devtools.build.lib.remote.util.RxFutures.toCompletable; import static com.google.devtools.build.lib.remote.util.RxFutures.toListenableFuture; -import static com.google.devtools.build.lib.remote.util.RxUtils.mergeBulkTransfer; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; +import static com.google.devtools.build.lib.remote.util.Utils.mergeBulkTransfer; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.common.flogger.GoogleLogger; +import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; @@ -44,8 +47,6 @@ import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.util.AsyncTaskCache; -import com.google.devtools.build.lib.remote.util.RxUtils; -import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult; import com.google.devtools.build.lib.remote.util.TempPathGenerator; import com.google.devtools.build.lib.vfs.FileSymlinkLoopException; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -53,8 +54,6 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import io.reactivex.rxjava3.core.Completable; -import io.reactivex.rxjava3.core.Flowable; -import io.reactivex.rxjava3.core.Single; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; @@ -283,6 +282,10 @@ public ListenableFuture prefetchFiles( files.add(input); } + if (files.isEmpty()) { + return immediateVoidFuture(); + } + // Collect the set of directories whose output permissions must be set at the end of this call. // This responsibility cannot lie with the downloading of an individual file, because multiple // files may be concurrently downloaded into the same directory within a single call to @@ -291,30 +294,38 @@ public ListenableFuture prefetchFiles( // it must still synchronize on the output permissions having been set. Set dirsWithOutputPermissions = Sets.newConcurrentHashSet(); - Completable prefetch = - mergeBulkTransfer( - Flowable.fromIterable(files) - .flatMapSingle( - input -> - prefetchFile( - action, - dirsWithOutputPermissions, - metadataSupplier, - input, - priority))) - .doOnComplete( - // Set output permissions on tree artifact subdirectories, matching the behavior of - // SkyframeActionExecutor#checkOutputs for artifacts produced by local actions. - () -> { - for (Path dir : dirsWithOutputPermissions) { - directoryTracker.setOutputPermissions(dir); - } - }); - - return toListenableFuture(prefetch); + // Using plain futures to avoid RxJava overheads. + List> transfers = new ArrayList<>(files.size()); + try (var s = Profiler.instance().profile("compose prefetches")) { + for (var file : files) { + transfers.add( + prefetchFile(action, dirsWithOutputPermissions, metadataSupplier, file, priority)); + } + } + + ListenableFuture mergedTransfer; + try (var s = Profiler.instance().profile("mergeBulkTransfer")) { + mergedTransfer = mergeBulkTransfer(transfers); + } + + return Futures.transformAsync( + mergedTransfer, + unused -> { + try { + // Set output permissions on tree artifact subdirectories, matching the behavior of + // SkyframeActionExecutor#checkOutputs for artifacts produced by local actions. + for (Path dir : dirsWithOutputPermissions) { + directoryTracker.setOutputPermissions(dir); + } + } catch (IOException e) { + return immediateFailedFuture(e); + } + return immediateVoidFuture(); + }, + directExecutor()); } - private Single prefetchFile( + private ListenableFuture prefetchFile( ActionExecutionMetadata action, Set dirsWithOutputPermissions, MetadataSupplier metadataSupplier, @@ -323,14 +334,14 @@ private Single prefetchFile( try { if (input instanceof VirtualActionInput) { prefetchVirtualActionInput((VirtualActionInput) input); - return Single.just(TransferResult.ok()); + return immediateVoidFuture(); } PathFragment execPath = input.getExecPath(); FileArtifactValue metadata = metadataSupplier.getMetadata(input); if (metadata == null || !canDownloadFile(execRoot.getRelative(execPath), metadata)) { - return Single.just(TransferResult.ok()); + return immediateVoidFuture(); } @Nullable Symlink symlink = maybeGetSymlink(action, input, metadata, metadataSupplier); @@ -357,11 +368,9 @@ private Single prefetchFile( result = result.andThen(plantSymlink(symlink)); } - return RxUtils.toTransferResult(result); - } catch (IOException e) { - return Single.just(TransferResult.error(e)); - } catch (InterruptedException e) { - return Single.just(TransferResult.interrupted()); + return toListenableFuture(result); + } catch (IOException | InterruptedException e) { + return immediateFailedFuture(e); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index 3e8adc3a9de49c..61c4c4f4b9d9a4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -13,8 +13,12 @@ // limitations under the License. package com.google.devtools.build.lib.remote.util; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Strings.isNullOrEmpty; import static com.google.common.base.Throwables.getStackTraceAsString; +import static com.google.common.util.concurrent.Futures.immediateFailedFuture; +import static com.google.common.util.concurrent.Futures.immediateVoidFuture; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static java.util.stream.Collectors.joining; import build.bazel.remote.execution.v2.Action; @@ -29,7 +33,6 @@ import com.google.common.util.concurrent.FluentFuture; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; -import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.Spawn; @@ -417,11 +420,11 @@ public static ListenableFuture downloadAsActionResult( try { return Futures.immediateFuture(ActionResult.parseFrom(data.toByteArray())); } catch (InvalidProtocolBufferException e) { - return Futures.immediateFailedFuture(e); + return immediateFailedFuture(e); } }, - MoreExecutors.directExecutor()) - .catching(CacheNotFoundException.class, (e) -> null, MoreExecutors.directExecutor()); + directExecutor()) + .catching(CacheNotFoundException.class, (e) -> null, directExecutor()); } public static void verifyBlobContents(Digest expected, Digest actual) throws IOException { @@ -483,15 +486,15 @@ public ByteString getContents() { */ public static ListenableFuture refreshIfUnauthenticatedAsync( AsyncCallable call, CallCredentialsProvider callCredentialsProvider) { - Preconditions.checkNotNull(call); - Preconditions.checkNotNull(callCredentialsProvider); + checkNotNull(call); + checkNotNull(callCredentialsProvider); try { return Futures.catchingAsync( call.call(), Throwable.class, (e) -> refreshIfUnauthenticatedAsyncOnException(e, call, callCredentialsProvider), - MoreExecutors.directExecutor()); + directExecutor()); } catch (Throwable t) { return refreshIfUnauthenticatedAsyncOnException(t, call, callCredentialsProvider); } @@ -511,15 +514,15 @@ private static ListenableFuture refreshIfUnauthenticatedAsyncOnException( } } - return Futures.immediateFailedFuture(t); + return immediateFailedFuture(t); } /** Same as {@link #refreshIfUnauthenticatedAsync} but calling a synchronous code block. */ public static V refreshIfUnauthenticated( Callable call, CallCredentialsProvider callCredentialsProvider) throws IOException, InterruptedException { - Preconditions.checkNotNull(call); - Preconditions.checkNotNull(callCredentialsProvider); + checkNotNull(call); + checkNotNull(callCredentialsProvider); try { return call.call(); @@ -618,4 +621,49 @@ public static void waitForBulkTransfer( throw bulkTransferException; } } + + public static ListenableFuture mergeBulkTransfer( + Iterable> transfers) { + return Futures.whenAllComplete(transfers) + .callAsync( + () -> { + BulkTransferException bulkTransferException = null; + + for (var transfer : transfers) { + IOException error = null; + try { + transfer.get(); + } catch (CancellationException e) { + return immediateFailedFuture(new InterruptedException()); + } catch (InterruptedException e) { + return immediateFailedFuture(e); + } catch (ExecutionException e) { + var cause = e.getCause(); + if (cause instanceof InterruptedException) { + return immediateFailedFuture(cause); + } else if (cause instanceof IOException) { + error = (IOException) cause; + } else { + error = new IOException(cause); + } + } + + if (error == null) { + continue; + } + + if (bulkTransferException == null) { + bulkTransferException = new BulkTransferException(); + } + bulkTransferException.add(error); + } + + if (bulkTransferException != null) { + return immediateFailedFuture(bulkTransferException); + } + + return immediateVoidFuture(); + }, + directExecutor()); + } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index 7e31e3949f7f13..54c35c4960c174 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -264,7 +264,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti ENV_VARIABLES_TO_CLEAR, runCommandLine.workingDir.getPathString(), builtTargets.configuration.checksum(), - /* executionPlatformAsLabelString= */ null); + /* executionPlatformLabel= */ null); PathFragment shExecutable = ShToolchain.getPathForHost(builtTargets.configuration); if (shExecutable.isEmpty()) { diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java index 2a0fa882d93725..61868714abc722 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.sandbox; import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.stream.Collectors.joining; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -59,7 +60,8 @@ import java.time.Duration; import java.time.Instant; import java.util.Map; -import javax.annotation.Nullable; +import java.util.Optional; +import java.util.stream.Stream; /** Abstract common ancestor for sandbox spawn runners implementing the common parts. */ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { @@ -224,7 +226,7 @@ private final SpawnResult run( StringBuilder msg = new StringBuilder("Action failed to execute: java.io.IOException: "); msg.append(exceptionMsg); msg.append("\n"); - if (sandboxDebugOutput != null) { + if (!sandboxDebugOutput.isEmpty()) { msg.append("Sandbox debug output:\n"); msg.append(sandboxDebugOutput); msg.append("\n"); @@ -294,12 +296,12 @@ private final SpawnResult run( } String sandboxDebugOutput = getSandboxDebugOutput(sandbox); - if (sandboxDebugOutput != null) { + if (!sandboxDebugOutput.isEmpty()) { reporter.handle( Event.of( EventKind.DEBUG, String.format( - "Sandbox debug output for %s %s: %s", + "Sandbox debug output for %s %s:\n%s", originalSpawn.getMnemonic(), originalSpawn.getTargetLabel(), sandboxDebugOutput))); @@ -334,18 +336,21 @@ private final SpawnResult run( return spawnResultBuilder.build(); } - @Nullable private String getSandboxDebugOutput(SandboxedSpawn sandbox) throws IOException { + Optional sandboxDebugOutput = Optional.empty(); Path sandboxDebugPath = sandbox.getSandboxDebugPath(); if (sandboxDebugPath != null && sandboxDebugPath.exists()) { try (InputStream inputStream = sandboxDebugPath.getInputStream()) { String msg = new String(inputStream.readAllBytes(), UTF_8); if (!msg.isEmpty()) { - return msg; + sandboxDebugOutput = Optional.of(msg); } } } - return null; + Optional interactiveDebugInstructions = sandbox.getInteractiveDebugInstructions(); + return Stream.of(sandboxDebugOutput, interactiveDebugInstructions) + .flatMap(Optional::stream) + .collect(joining("\n")); } private boolean wasTimeout(Duration timeout, Duration wallTime) { 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..9e92eb4750b833 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", @@ -72,6 +73,7 @@ java_library( ":sandbox_helpers", ":sandbox_options", "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter", + "//src/main/java/com/google/devtools/build/lib/util:command", "//src/main/java/com/google/devtools/build/lib/util:describable_execution_unit", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", 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..3ad9c32531e0f0 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, @@ -258,29 +259,30 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context allowNetwork || Spawns.requiresNetwork(spawn, getSandboxOptions().defaultSandboxAllowNetwork); - return new SymlinkedSandboxedSpawn( - sandboxPath, - sandboxExecRoot, - commandLine, - environment, - inputs, - outputs, - writableDirs, - treeDeleter, - /* sandboxDebugPath= */ null, - statisticsPath, - spawn.getMnemonic()) { - @Override - public void createFileSystem() throws IOException, InterruptedException { - super.createFileSystem(); - writeConfig( - sandboxConfigPath, - writableDirs, - getInaccessiblePaths(), - allowNetworkForThisSpawn, - statisticsPath); - } - }; + return new SymlinkedSandboxedSpawn( + sandboxPath, + sandboxExecRoot, + commandLine, + environment, + inputs, + outputs, + writableDirs, + treeDeleter, + /* sandboxDebugPath= */ null, + statisticsPath, + /* interactiveDebugArguments= */ null, + spawn.getMnemonic()) { + @Override + public void createFileSystem() throws IOException, InterruptedException { + super.createFileSystem(); + writeConfig( + sandboxConfigPath, + writableDirs, + getInaccessiblePaths(), + allowNetworkForThisSpawn, + statisticsPath); + } + }; } private void writeConfig( 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/LinuxSandboxCommandLineBuilder.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilder.java index cf80265a85e6f0..967c08b5d6ce59 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilder.java @@ -51,7 +51,6 @@ public static BindMount of(Path mountPoint, Path source) { } private final Path linuxSandboxPath; - private final List commandArguments; private Path hermeticSandboxPath; private Path workingDirectory; private Duration timeout; @@ -72,15 +71,13 @@ public static BindMount of(Path mountPoint, Path source) { private boolean sigintSendsSigterm = false; private String cgroupsDir; - private LinuxSandboxCommandLineBuilder(Path linuxSandboxPath, List commandArguments) { + private LinuxSandboxCommandLineBuilder(Path linuxSandboxPath) { this.linuxSandboxPath = linuxSandboxPath; - this.commandArguments = commandArguments; } /** Returns a new command line builder for the {@code linux-sandbox} tool. */ - public static LinuxSandboxCommandLineBuilder commandLineBuilder( - Path linuxSandboxPath, List commandArguments) { - return new LinuxSandboxCommandLineBuilder(linuxSandboxPath, commandArguments); + public static LinuxSandboxCommandLineBuilder commandLineBuilder(Path linuxSandboxPath) { + return new LinuxSandboxCommandLineBuilder(linuxSandboxPath); } /** @@ -247,7 +244,7 @@ public LinuxSandboxCommandLineBuilder addExecutionInfo(Map execu } /** Builds the command line to invoke a specific command using the {@code linux-sandbox} tool. */ - public ImmutableList build() { + public ImmutableList buildForCommand(List commandArguments) { Preconditions.checkState( !(this.useFakeUsername && this.useFakeRoot), "useFakeUsername and useFakeRoot are exclusive"); 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..ac9c5ae72fcc13 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 @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.sandbox; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NETNS_WITH_LOOPBACK; import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NO_NETNS; @@ -61,6 +62,8 @@ import java.util.Map; import java.util.Optional; import java.util.SortedMap; +import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; @@ -106,10 +109,9 @@ private static boolean computeIsSupported(CommandEnvironment cmdEnv, Path linuxS throws InterruptedException { LocalExecutionOptions options = cmdEnv.getOptions().getOptions(LocalExecutionOptions.class); ImmutableList linuxSandboxArgv = - LinuxSandboxCommandLineBuilder.commandLineBuilder( - linuxSandbox, ImmutableList.of("/bin/true")) + LinuxSandboxCommandLineBuilder.commandLineBuilder(linuxSandbox) .setTimeout(options.getLocalSigkillGraceSeconds()) - .build(); + .buildForCommand(ImmutableList.of("/bin/true")); ImmutableMap env = ImmutableMap.of(); Path execRoot = cmdEnv.getExecRoot(); File cwd = execRoot.getPathFile(); @@ -281,6 +283,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + context.getInputMetadataProvider(), execRoot, withinSandboxExecRoot, packageRoots, @@ -309,7 +312,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context boolean createNetworkNamespace = !(allowNetwork || Spawns.requiresNetwork(spawn, sandboxOptions.defaultSandboxAllowNetwork)); LinuxSandboxCommandLineBuilder commandLineBuilder = - LinuxSandboxCommandLineBuilder.commandLineBuilder(linuxSandbox, spawn.getArguments()) + LinuxSandboxCommandLineBuilder.commandLineBuilder(linuxSandbox) .addExecutionInfo(spawn.getExecutionInfo()) .setWritableFilesAndDirectories(writableDirs) .setTmpfsDirectories(ImmutableSet.copyOf(getSandboxOptions().sandboxTmpfsPath)) @@ -354,7 +357,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context return new HardlinkedSandboxedSpawn( sandboxPath, sandboxExecRoot, - commandLineBuilder.build(), + commandLineBuilder.buildForCommand(spawn.getArguments()), environment, inputs, outputs, @@ -368,7 +371,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context return new SymlinkedSandboxedSpawn( sandboxPath, sandboxExecRoot, - commandLineBuilder.build(), + commandLineBuilder.buildForCommand(spawn.getArguments()), environment, inputs, outputs, @@ -376,6 +379,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context treeDeleter, sandboxDebugPath, statisticsPath, + makeInteractiveDebugArguments(commandLineBuilder, sandboxOptions), spawn.getMnemonic()); } } @@ -389,7 +393,7 @@ public String getName() { protected ImmutableSet getWritableDirs( Path sandboxExecRoot, Path withinSandboxExecRoot, Map env) throws IOException { - ImmutableSet.Builder writableDirs = ImmutableSet.builder(); + Set writableDirs = new TreeSet<>(); writableDirs.addAll(super.getWritableDirs(sandboxExecRoot, withinSandboxExecRoot, env)); if (getSandboxOptions().memoryLimitMb > 0) { CgroupsInfo cgroupsInfo = CgroupsInfo.getInstance(); @@ -399,7 +403,23 @@ protected ImmutableSet getWritableDirs( writableDirs.add(fs.getPath("/dev/shm").resolveSymbolicLinks()); writableDirs.add(fs.getPath("/tmp")); - return writableDirs.build(); + if (sandboxExecRoot.equals(withinSandboxExecRoot)) { + return ImmutableSet.copyOf(writableDirs); + } + + // If a writable directory is under the sandbox exec root, transform it so that its path will + // be the one that it will be available at after processing the bind mounts (this is how the + // sandbox interprets the corresponding arguments) + // + // Notably, this is usually the case for $TEST_TMPDIR because its default value is under the + // execroot. + return writableDirs.stream() + .map( + d -> + d.startsWith(sandboxExecRoot) + ? withinSandboxExecRoot.getRelative(d.relativeTo(sandboxExecRoot)) + : d) + .collect(toImmutableSet()); } private ImmutableList getBindMounts( @@ -538,4 +558,13 @@ public void cleanupSandboxBase(Path sandboxBase, TreeDeleter treeDeleter) throws super.cleanupSandboxBase(sandboxBase, treeDeleter); } + + @Nullable + private ImmutableList makeInteractiveDebugArguments( + LinuxSandboxCommandLineBuilder commandLineBuilder, SandboxOptions sandboxOptions) { + if (!sandboxOptions.sandboxDebug) { + return null; + } + return commandLineBuilder.buildForCommand(ImmutableList.of("/bin/sh", "-i")); + } } 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..14d2a63545a734 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,24 +100,26 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + context.getInputMetadataProvider(), execRoot, execRoot, packageRoots, null); SandboxOutputs outputs = helpers.getOutputs(spawn); - return new SymlinkedSandboxedSpawn( - sandboxPath, - sandboxExecRoot, - commandLineBuilder.build(), - environment, - inputs, - outputs, - getWritableDirs(sandboxExecRoot, sandboxExecRoot, environment), - treeDeleter, - /* sandboxDebugPath= */ null, - statisticsPath, - spawn.getMnemonic()); + return new SymlinkedSandboxedSpawn( + sandboxPath, + sandboxExecRoot, + commandLineBuilder.build(), + environment, + inputs, + outputs, + getWritableDirs(sandboxExecRoot, sandboxExecRoot, environment), + treeDeleter, + /* sandboxDebugPath= */ null, + statisticsPath, + /* interactiveDebugArguments= */ null, + spawn.getMnemonic()); } @Override 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..dfc18bda2c839a 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; @@ -29,6 +30,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 +447,29 @@ private static RootedPath processFilesetSymlink( symlink.getPathString())); } + private static RootedPath processResolvedSymlink( + Root absoluteRoot, + PathFragment execRootRelativeSymlinkTarget, + Root execRootWithinSandbox, + PathFragment execRootFragment, + ImmutableList packageRoots, + Function sourceRooWithinSandbox) { + PathFragment symlinkTarget = execRootFragment.getRelative(execRootRelativeSymlinkTarget); + for (Root packageRoot : packageRoots) { + if (packageRoot.contains(symlinkTarget)) { + return RootedPath.toRootedPath( + sourceRooWithinSandbox.apply(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 +484,7 @@ private static RootedPath processFilesetSymlink( */ public SandboxInputs processInputFiles( Map inputMap, + InputMetadataProvider inputMetadataProvider, Path execRootPath, Path withinSandboxExecRootPath, ImmutableList packageRoots, @@ -468,12 +495,24 @@ public SandboxInputs processInputFiles( withinSandboxExecRootPath.equals(execRootPath) ? withinSandboxExecRoot : Root.fromPath(execRootPath); + Root absoluteRoot = Root.absoluteRoot(execRootPath.getFileSystem()); Map inputFiles = new TreeMap<>(); Map inputSymlinks = new TreeMap<>(); 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(); @@ -503,23 +542,63 @@ 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) { - 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 (sandboxSourceRoots == null) { inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); + } else { + if (inputArtifact.isSourceArtifact()) { + Root sourceRoot = inputArtifact.getRoot().getRoot(); + inputPath = + RootedPath.toRootedPath( + sourceRootWithinSandbox.apply(sourceRoot), + inputArtifact.getRootRelativePath()); + } else { + PathFragment materializationExecPath = null; + if (inputArtifact.isChildOfDeclaredDirectory()) { + FileArtifactValue parentMetadata = + inputMetadataProvider.getInputMetadata(inputArtifact.getParent()); + if (parentMetadata.getMaterializationExecPath().isPresent()) { + materializationExecPath = + parentMetadata + .getMaterializationExecPath() + .get() + .getRelative(inputArtifact.getParentRelativePath()); + } + } else if (!inputArtifact.isTreeArtifact()) { + // Normally, one would not see tree artifacts here because they have already been + // expanded by the time the code gets here. However, there is one very special case: + // when an action has an archived tree artifact on its output and is executed on the + // local branch of the dynamic execution strategy, the tree artifact is zipped up + // in a little extra spawn, which has direct tree artifact on its inputs. Sadly, + // it's not easy to fix this because there isn't an easy way to inject this new + // tree artifact into the artifact expander being used. + // + // The best would be to not rely on spawn strategies for executing that little + // command: it's entirely under the control of Bazel so we can guarantee that it + // does not cause mischief. + FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(actionInput); + if (metadata.getMaterializationExecPath().isPresent()) { + materializationExecPath = metadata.getMaterializationExecPath().get(); + } + } + + if (materializationExecPath != null) { + inputPath = + processResolvedSymlink( + absoluteRoot, + materializationExecPath, + withinSandboxExecRoot, + execRootPath.asFragment(), + packageRoots, + sourceRootWithinSandbox); + } else { + inputPath = + RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); + } + } } } else { PathFragment execPath = actionInput.getExecPath(); @@ -544,6 +623,7 @@ public SandboxInputs processInputFiles( return new SandboxInputs(inputFiles, virtualInputs, inputSymlinks, sandboxRootToSourceRoot); } + /** The file and directory outputs of a sandboxed spawn. */ @AutoValue public abstract static class SandboxOutputs { diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java index 1a355bc90b6733..44657780bbb446 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java @@ -17,6 +17,7 @@ import com.google.devtools.build.lib.util.DescribableExecutionUnit; import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; +import java.util.Optional; import javax.annotation.Nullable; /** @@ -61,4 +62,12 @@ default boolean useSubprocessTimeout() { /** Deletes the sandbox directory. */ void delete(); + + /** + * Returns user-facing instructions for starting an interactive sandboxed environment identical to + * the one in which this spawn is executed. + */ + default Optional getInteractiveDebugInstructions() { + return Optional.empty(); + } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java index 57a09356f92cb2..4462d8a17df739 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java @@ -21,10 +21,13 @@ import com.google.devtools.build.lib.exec.TreeDeleter; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs; +import com.google.devtools.build.lib.util.CommandDescriptionForm; +import com.google.devtools.build.lib.util.CommandFailureUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.util.LinkedHashSet; +import java.util.Optional; import java.util.Set; import javax.annotation.Nullable; @@ -37,6 +40,8 @@ public class SymlinkedSandboxedSpawn extends AbstractContainerizingSandboxedSpaw /** Mnemonic of the action running in this spawn. */ private final String mnemonic; + @Nullable private final ImmutableList interactiveDebugArguments; + public SymlinkedSandboxedSpawn( Path sandboxPath, Path sandboxExecRoot, @@ -48,6 +53,7 @@ public SymlinkedSandboxedSpawn( TreeDeleter treeDeleter, @Nullable Path sandboxDebugPath, @Nullable Path statisticsPath, + @Nullable ImmutableList interactiveDebugArguments, String mnemonic) { super( sandboxPath, @@ -62,6 +68,7 @@ public SymlinkedSandboxedSpawn( statisticsPath, mnemonic); this.mnemonic = isNullOrEmpty(mnemonic) ? "_NoMnemonic_" : mnemonic; + this.interactiveDebugArguments = interactiveDebugArguments; } @Override @@ -96,4 +103,23 @@ public void delete() { SandboxStash.stashSandbox(sandboxPath, mnemonic); super.delete(); } + + @Nullable + @Override + public Optional getInteractiveDebugInstructions() { + if (interactiveDebugArguments == null) { + return Optional.empty(); + } + return Optional.of( + "Run this command to start an interactive shell in an identical sandboxed environment:\n" + + CommandFailureUtils.describeCommand( + CommandDescriptionForm.COMPLETE, + /* prettyPrintArgs= */ false, + interactiveDebugArguments, + getEnvironment(), + /* environmentVariablesToClear= */ null, + /* cwd= */ null, + /* configurationChecksum= */ null, + /* executionPlatformLabel= */ null)); + } } 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/server/GrpcServerImpl.java b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java index 8efb5da16d3473..c7c21960e63f6e 100644 --- a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java +++ b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.util.InterruptedFailureDetails; +import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -376,6 +377,26 @@ public void interrupt() { commandManager.interruptInflightCommands(); } + private Server bindIpv6WithRetries(InetSocketAddress address, int maxRetries) throws IOException { + Server server = null; + for (int attempt = 1; attempt <= maxRetries; attempt++) { + try { + server = + NettyServerBuilder.forAddress(address) + .addService(this) + .directExecutor() + .build() + .start(); + break; + } catch (IOException e) { + if (attempt == maxRetries) { + throw e; + } + } + } + return server; + } + @Override public void serve() throws AbruptExitException { Preconditions.checkState(!serving); @@ -391,8 +412,10 @@ public void serve() throws AbruptExitException { if (Epoll.isAvailable() && !Socket.isIPv6Preferred()) { throw new IOException("ipv6 is not preferred on the system."); } - server = - NettyServerBuilder.forAddress(address).addService(this).directExecutor().build().start(); + // For some strange reasons, Bazel server sometimes fails to bind to IPv6 localhost when + // running in macOS sandbox-exec with internet blocked. Retrying seems to help. + // See https://github.com/bazelbuild/bazel/issues/20743 + server = bindIpv6WithRetries(address, OS.getCurrent() == OS.DARWIN ? 3 : 1); } catch (IOException ipv6Exception) { address = new InetSocketAddress("127.0.0.1", port); try { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 79f864f541aba4..eef14e85af0ff8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -1246,8 +1246,7 @@ private R accumulateInputs( topLevelFilesets, input, value, - env, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + env); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java index 8971039c3a0a69..515056c489094b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyValue; import java.util.Map; @@ -48,8 +49,7 @@ static void addToMap( Map> topLevelFilesets, Artifact key, SkyValue value, - Environment env, - boolean requiresTreeMetadataWhenTreeFileIsInput) + Environment env) throws InterruptedException { addToMap( inputMap, @@ -60,8 +60,7 @@ static void addToMap( key, value, env, - MetadataConsumerForMetrics.NO_OP, - requiresTreeMetadataWhenTreeFileIsInput); + MetadataConsumerForMetrics.NO_OP); } /** @@ -77,8 +76,7 @@ static void addToMap( Artifact key, SkyValue value, Environment env, - MetadataConsumerForMetrics consumer, - boolean requiresTreeMetadataWhenTreeFileIsInput) + MetadataConsumerForMetrics consumer) throws InterruptedException { if (value instanceof RunfilesArtifactValue) { // Note: we don't expand the .runfiles/MANIFEST file into the inputs. The reason for that @@ -125,15 +123,17 @@ static void addToMap( /*depOwner=*/ key); consumer.accumulate(treeArtifactValue); } else if (value instanceof ActionExecutionValue) { - if (requiresTreeMetadataWhenTreeFileIsInput && key.isChildOfDeclaredDirectory()) { - // Actions resulting from the expansion of an ActionTemplate consume only one of the files - // in a tree artifact. However, the input prefetcher requires access to the tree metadata - // to determine the prefetch location of a tree artifact materialized as a symlink - // (cf. TreeArtifactValue#getMaterializationExecPath()). + // Actions resulting from the expansion of an ActionTemplate consume only one of the files + // in a tree artifact. However, the input prefetcher and the Linux sandbox require access to + // the tree metadata to determine the prefetch location of a tree artifact materialized as a + // symlink (cf. TreeArtifactValue#getMaterializationExecPath()). + if (key.isChildOfDeclaredDirectory()) { SpecialArtifact treeArtifact = key.getParent(); TreeArtifactValue treeArtifactValue = ((ActionExecutionValue) value).getTreeArtifactValue(treeArtifact); inputMap.putTreeArtifact(treeArtifact, treeArtifactValue, /* depOwner= */ treeArtifact); + addArchivedTreeArtifactMaybe( + treeArtifact, treeArtifactValue, archivedTreeArtifacts, inputMap, key); consumer.accumulate(treeArtifactValue); } FileArtifactValue metadata = ((ActionExecutionValue) value).getExistingFileArtifactValue(key); @@ -221,20 +221,27 @@ private static void expandTreeArtifactAndPopulateArtifactData( return; } - inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner); expandedArtifacts.put(treeArtifact, value.getChildren()); + inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner); + addArchivedTreeArtifactMaybe( + (SpecialArtifact) treeArtifact, value, archivedTreeArtifacts, inputMap, depOwner); + } + + private static void addArchivedTreeArtifactMaybe( + SpecialArtifact treeArtifact, + TreeArtifactValue value, + Map archivedTreeArtifacts, + ActionInputMapSink inputMap, + Artifact depOwner) { + if (!value.getArchivedRepresentation().isPresent()) { + return; + } - value - .getArchivedRepresentation() - .ifPresent( - archivedRepresentation -> { - inputMap.put( - archivedRepresentation.archivedTreeFileArtifact(), - archivedRepresentation.archivedFileValue(), - depOwner); - archivedTreeArtifacts.put( - (SpecialArtifact) treeArtifact, - archivedRepresentation.archivedTreeFileArtifact()); - }); + ArchivedRepresentation archivedRepresentation = value.getArchivedRepresentation().get(); + inputMap.put( + archivedRepresentation.archivedTreeFileArtifact(), + archivedRepresentation.archivedFileValue(), + depOwner); + archivedTreeArtifacts.put(treeArtifact, archivedRepresentation.archivedTreeFileArtifact()); } } 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..1bc2f4c7c0ac6a 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 @@ -264,7 +264,15 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa Path treeDir = artifactPathResolver.toPath(parent); boolean chmod = executionMode.get(); - FileStatus stat = treeDir.statIfFound(Symlinks.FOLLOW); + FileStatus lstat = treeDir.statIfFound(Symlinks.NOFOLLOW); + FileStatus stat; + if (lstat == null) { + stat = null; + } else if (!lstat.isSymbolicLink()) { + stat = lstat; + } else { + stat = treeDir.statIfFound(Symlinks.FOLLOW); + } // Make sure the tree artifact root exists and is a regular directory. Note that this is how the // action is initialized, so this should hold unless the action itself has deleted the root. @@ -332,12 +340,18 @@ 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 (lstat.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(); @@ -509,7 +523,13 @@ private FileArtifactValue constructFileArtifactValue( return value; } - if (type.isFile() && fileDigest != null) { + boolean isResolvedSymlink = + statAndValue.statNoFollow() != null + && statAndValue.statNoFollow().isSymbolicLink() + && statAndValue.realPath() != null + && !value.isRemote(); + + if (type.isFile() && !isResolvedSymlink && fileDigest != null) { // The digest is in the file value and that is all that is needed for this file's metadata. return value; } @@ -525,22 +545,30 @@ private FileArtifactValue constructFileArtifactValue( artifactPathResolver.toPath(artifact).getLastModifiedTime()); } - if (injectedDigest == null && type.isFile()) { + byte[] actualDigest = fileDigest != null ? fileDigest : injectedDigest; + + if (actualDigest == null && type.isFile()) { // We don't have an injected digest and there is no digest in the file value (which attempts a // fast digest). Manually compute the digest instead. - Path path = statAndValue.pathNoFollow(); - if (statAndValue.statNoFollow() != null - && statAndValue.statNoFollow().isSymbolicLink() - && statAndValue.realPath() != null) { - // If the file is a symlink, we compute the digest using the target path so that it's - // 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()); + // If the file is a symlink, we compute the digest using the target path so that it's + // possible to hit the digest cache - we probably already computed the digest for the + // target during previous action execution. + Path pathToDigest = isResolvedSymlink ? statAndValue.realPath() : statAndValue.pathNoFollow(); + actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest, value.getSize()); } - return FileArtifactValue.createFromInjectedDigest(value, injectedDigest); + + if (!isResolvedSymlink) { + return FileArtifactValue.createFromInjectedDigest(value, actualDigest); + } + + PathFragment realPathAsFragment = statAndValue.realPath().asFragment(); + PathFragment execRootRelativeRealPath = + realPathAsFragment.startsWith(execRoot) + ? realPathAsFragment.relativeTo(execRoot) + : realPathAsFragment; + return FileArtifactValue.createForResolvedSymlink( + execRootRelativeRealPath, value, actualDigest); } /** diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index 3311cdf513fb83..c9076034e60a6c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java @@ -39,7 +39,6 @@ import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.NoSuchPackageException; -import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClassProvider; @@ -148,10 +147,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) return BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE; } RepoSpec extRepoSpec = extensionEval.getGeneratedRepoSpecs().get(internalRepo); - Package pkg = createRuleFromSpec(extRepoSpec, starlarkSemantics, env).getRule().getPackage(); - Preconditions.checkNotNull(pkg); - - return new BzlmodRepoRuleValue(pkg, repositoryName.getName()); + return createRuleFromSpec(extRepoSpec, starlarkSemantics, env); } private static Optional checkRepoFromNonRegistryOverrides( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java index 9bd62c7d886f97..127fc19a374a99 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java @@ -205,8 +205,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) input, artifactValue, env, - currentConsumer, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + currentConsumer); if (!allArtifactsAreImportant && importantArtifactSet.contains(input)) { // Calling #addToMap a second time with `input` and `artifactValue` will perform no-op // updates to the secondary collections passed in (eg. expandedArtifacts, @@ -220,8 +219,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) topLevelFilesets, input, artifactValue, - env, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + env); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 81a2ef092f6ecf..6809ca303aa19c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -350,10 +350,6 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) { .test(action.getMnemonic()); } - boolean requiresTreeMetadataWhenTreeFileIsInput() { - return actionInputPrefetcher.requiresTreeMetadataWhenTreeFileIsInput(); - } - boolean publishTargetSummaries() { return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary; } diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD index 8801079580351a..a7519e1fe9adf7 100644 --- a/src/main/java/com/google/devtools/build/lib/util/BUILD +++ b/src/main/java/com/google/devtools/build/lib/util/BUILD @@ -83,6 +83,7 @@ java_library( name = "describable_execution_unit", srcs = ["DescribableExecutionUnit.java"], deps = [ + "//src/main/java/com/google/devtools/build/lib/cmdline", "//third_party:guava", "//third_party:jsr305", ], @@ -100,6 +101,7 @@ java_library( ":describable_execution_unit", ":os", ":shell_escaper", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/shell", "//src/main/java/com/google/devtools/build/lib/vfs", "//third_party:error_prone_annotations", diff --git a/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java b/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java index 91584f4733b41a..537d1d32642645 100644 --- a/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java +++ b/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java @@ -19,6 +19,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.Ordering; +import com.google.devtools.build.lib.cmdline.Label; import java.io.File; import java.util.Collection; import java.util.Comparator; @@ -173,7 +174,7 @@ public static String describeCommand( @Nullable List environmentVariablesToClear, @Nullable String cwd, @Nullable String configurationChecksum, - @Nullable String executionPlatformAsLabelString) { + @Nullable Label executionPlatformLabel) { Preconditions.checkNotNull(form); StringBuilder message = new StringBuilder(); @@ -267,9 +268,9 @@ public static String describeCommand( message.append("# Configuration: ").append(configurationChecksum); } - if (executionPlatformAsLabelString != null) { + if (executionPlatformLabel != null) { message.append("\n"); - message.append("# Execution platform: ").append(executionPlatformAsLabelString); + message.append("# Execution platform: ").append(executionPlatformLabel); } } @@ -288,8 +289,8 @@ static String describeCommandFailure( Map env, @Nullable String cwd, @Nullable String configurationChecksum, - @Nullable String targetLabel, - @Nullable String executionPlatformAsLabelString) { + @Nullable Label targetLabel, + @Nullable Label executionPlatformLabel) { String commandName = commandLineElements.iterator().next(); // Extract the part of the command name after the last "/", if any. @@ -318,7 +319,7 @@ static String describeCommandFailure( null, cwd, configurationChecksum, - executionPlatformAsLabelString)); + executionPlatformLabel)); return shortCommandName + " failed: " + output; } @@ -332,6 +333,6 @@ public static String describeCommandFailure( cwd, command.getConfigurationChecksum(), command.getTargetLabel(), - command.getExecutionPlatformLabelString()); + command.getExecutionPlatformLabel()); } } diff --git a/src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java b/src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java index afcc1ee18d4c2a..6ccc9a3b468f60 100644 --- a/src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java +++ b/src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java @@ -15,6 +15,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.cmdline.Label; import javax.annotation.Nullable; /** @@ -23,7 +24,7 @@ public interface DescribableExecutionUnit { @Nullable - default String getTargetLabel() { + default Label getTargetLabel() { return null; } @@ -38,7 +39,7 @@ default String getTargetLabel() { /** Returns the Label of the execution platform for the command, if any, as a String. */ @Nullable - default String getExecutionPlatformLabelString() { + default Label getExecutionPlatformLabel() { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java index 9d0509181a8b53..06453e46c6a233 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java +++ b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java @@ -181,7 +181,7 @@ protected Subprocess createProcess() throws IOException, UserExecException { // Mostly tests require network, and some blaze run commands, but no workers. LinuxSandboxCommandLineBuilder commandLineBuilder = LinuxSandboxCommandLineBuilder.commandLineBuilder( - this.hardenedSandboxOptions.sandboxBinary(), args) + this.hardenedSandboxOptions.sandboxBinary()) .setWritableFilesAndDirectories(getWritableDirs(workDir)) .setTmpfsDirectories(ImmutableSet.copyOf(this.hardenedSandboxOptions.tmpfsPath())) .setPersistentProcess(true) @@ -202,7 +202,7 @@ protected Subprocess createProcess() throws IOException, UserExecException { commandLineBuilder.setUseFakeUsername(true); } - args = commandLineBuilder.build(); + args = commandLineBuilder.buildForCommand(args); } return createProcessBuilder(args).start(); } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java index e20296beadcf16..92754b0569af4d 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java @@ -213,6 +213,6 @@ public String toString() { /* environmentVariablesToClear= */ null, execRoot.getPathString(), /* configurationChecksum= */ null, - /* executionPlatformAsLabelString= */ null); + /* executionPlatformLabel= */ 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 6718b7e709a561..7adf6eaf188203 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 @@ -176,8 +176,10 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) String.format( "%s worker %s", spawn.getMnemonic(), spawn.getResourceOwner().describe()))) { - runfilesTreeUpdater.updateRunfiles( - spawn.getRunfilesSupplier(), spawn.getEnvironment(), context.getFileOutErr()); + try (var s = Profiler.instance().profile("updateRunfiles")) { + runfilesTreeUpdater.updateRunfiles( + spawn.getRunfilesSupplier(), spawn.getEnvironment(), context.getFileOutErr()); + } InputMetadataProvider inputFileCache = context.getInputMetadataProvider(); @@ -188,6 +190,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/main/starlark/builtins_bzl/common/cc/semantics.bzl b/src/main/starlark/builtins_bzl/common/cc/semantics.bzl index 922e24dffd2e12..914b39107ff1ab 100644 --- a/src/main/starlark/builtins_bzl/common/cc/semantics.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/semantics.bzl @@ -86,35 +86,7 @@ def _get_coverage_attrs(): } def _get_coverage_env(ctx): - runfiles = ctx.runfiles() - test_env = {} - if ctx.configuration.coverage_enabled: - # Bazel’s coverage runner - # (https://github.com/bazelbuild/bazel/blob/3.0.0/tools/test/collect_coverage.sh) - # needs a binary called “lcov_merge.” Its location is passed in the - # LCOV_MERGER environmental variable. For builtin rules, this variable - # is set automatically based on a magic “$lcov_merger” or - # “:lcov_merger” attribute, but it’s not possible to create such - # attributes in Starlark. Therefore we specify the variable ourselves. - # Note that the coverage runner runs in the runfiles root instead of - # the execution root, therefore we use “path” instead of “short_path.” - test_env["LCOV_MERGER"] = ctx.executable._lcov_merger.path - - # C/C++ coverage instrumentation needs another binary that wraps gcov; - # see - # https://github.com/bazelbuild/bazel/blob/5.0.0/tools/test/collect_coverage.sh#L199. - # This is normally set from a hidden “$collect_cc_coverage” attribute; - # see - # https://github.com/bazelbuild/bazel/blob/5.0.0/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java#L253-L258. - test_env["CC_CODE_COVERAGE_SCRIPT"] = ctx.executable._collect_cc_coverage.path - - # The test runfiles need all applicable runfiles for the tools above. - runfiles = runfiles.merge_all([ - ctx.attr._lcov_merger[DefaultInfo].default_runfiles, - ctx.attr._collect_cc_coverage[DefaultInfo].default_runfiles, - ]) - - return runfiles, test_env + return ctx.runfiles(), {} def _get_cc_runtimes(ctx, is_library): if is_library: diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index b36b5614ff4663..c6dfee8ae0d3ac 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -363,7 +363,10 @@ public void simpleExtension_nonCanonicalLabel() throws Exception { "use_repo(ext2, 'bar')", "ext3 = use_extension('@//:defs.bzl', 'ext')", "ext3.tag(name='quz', data='qu')", - "use_repo(ext3, 'quz')"); + "use_repo(ext3, 'quz')", + "ext4 = use_extension('defs.bzl', 'ext')", + "ext4.tag(name='qor', data='qo')", + "use_repo(ext4, 'qor')"); scratch.file( workspaceRoot.getRelative("defs.bzl").getPathString(), "load('@data_repo//:defs.bzl','data_repo')", @@ -379,7 +382,8 @@ public void simpleExtension_nonCanonicalLabel() throws Exception { "load('@foo//:data.bzl', foo_data='data')", "load('@bar//:data.bzl', bar_data='data')", "load('@quz//:data.bzl', quz_data='data')", - "data = 'foo:'+foo_data+' bar:'+bar_data+' quz:'+quz_data"); + "load('@qor//:data.bzl', qor_data='data')", + "data = 'foo:'+foo_data+' bar:'+bar_data+' quz:'+quz_data+' qor:'+qor_data"); SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); EvaluationResult result = @@ -387,7 +391,8 @@ public void simpleExtension_nonCanonicalLabel() throws Exception { if (result.hasError()) { throw result.getError().getException(); } - assertThat(result.get(skyKey).getModule().getGlobal("data")).isEqualTo("foo:fu bar:ba quz:qu"); + assertThat(result.get(skyKey).getModule().getGlobal("data")) + .isEqualTo("foo:fu bar:ba quz:qu qor:qo"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTest.java b/src/test/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContextTest.java similarity index 70% rename from src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTest.java rename to src/test/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContextTest.java index 9cfd8d6e110028..a5dc8c2c8e92ee 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContextTest.java @@ -14,9 +14,9 @@ package com.google.devtools.build.lib.exec; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.exec.SpawnLogContext.millisToProto; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.mockito.Mockito.verify; import com.google.common.base.Utf8; import com.google.common.collect.ImmutableList; @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.actions.StaticInputMetadataProvider; 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.ExpandedSpawnLogContext.Encoding; import com.google.devtools.build.lib.exec.Protos.Digest; import com.google.devtools.build.lib.exec.Protos.EnvironmentVariable; import com.google.devtools.build.lib.exec.Protos.File; @@ -48,7 +49,6 @@ import com.google.devtools.build.lib.server.FailureDetails.Crash; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.skyframe.TreeArtifactValue; -import com.google.devtools.build.lib.util.io.MessageOutputStream; import com.google.devtools.build.lib.vfs.DelegateFileSystem; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.Dirent; @@ -63,22 +63,17 @@ import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.io.IOException; +import java.io.InputStream; import java.time.Duration; +import java.util.ArrayList; import java.util.Map; import java.util.SortedMap; -import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnit; -import org.mockito.junit.MockitoRule; -/** Tests for {@link SpawnLogContext}. */ +/** Tests for {@link ExpandedSpawnLogContext}. */ @RunWith(TestParameterInjector.class) -public final class SpawnLogContextTest { - @Rule public final MockitoRule mockito = MockitoJUnit.rule(); - @Mock private MessageOutputStream outputStream; - +public final class ExpandedSpawnLogContextTest { private final DigestHashFunction digestHashFunction = DigestHashFunction.SHA256; private final FileSystem fs = new InMemoryFileSystem(digestHashFunction); private final Path execRoot = fs.getPath("/execroot"); @@ -86,6 +81,9 @@ public final class SpawnLogContextTest { private final ArtifactRoot outputDir = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "out"); + private final Path logPath = fs.getPath("/log"); + private final Path tempPath = fs.getPath("/temp"); + // A fake action filesystem that provides a fast digest, but refuses to compute it from the // file contents (which won't be available when building without the bytes). private static final class FakeActionFileSystem extends DelegateFileSystem { @@ -145,9 +143,9 @@ public void testFileInput(@TestParameter InputsMode inputsMode) throws Exception spawn.withTools(fileInput); } - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( spawn.build(), createInputMetadataProvider(fileInput), createInputMap(fileInput), @@ -155,15 +153,15 @@ public void testFileInput(@TestParameter InputsMode inputsMode) throws Exception defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addInputs( - File.newBuilder() - .setPath("file") - .setDigest(getDigest("abc")) - .setIsTool(inputsMode.isTool())) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addInputs( + File.newBuilder() + .setPath("file") + .setDigest(getDigest("abc")) + .setIsTool(inputsMode.isTool())) + .build()); } @Test @@ -182,9 +180,9 @@ public void testDirectoryInput( spawn.withTools(dirInput); } - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( spawn.build(), createInputMetadataProvider(dirInput), createInputMap(dirInput), @@ -193,18 +191,15 @@ public void testDirectoryInput( defaultSpawnResult()); // TODO(tjgq): Propagate tool bit to files inside source directories. - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addAllInputs( - dirContents.isEmpty() - ? ImmutableList.of() - : ImmutableList.of( - File.newBuilder() - .setPath("dir/file") - .setDigest(getDigest("abc")) - .build())) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addAllInputs( + dirContents.isEmpty() + ? ImmutableList.of() + : ImmutableList.of( + File.newBuilder().setPath("dir/file").setDigest(getDigest("abc")).build())) + .build()); } @Test @@ -224,9 +219,9 @@ public void testTreeInput( spawn.withTools(treeInput); } - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( spawn.build(), createInputMetadataProvider(treeInput), createInputMap(treeInput), @@ -234,19 +229,19 @@ public void testTreeInput( defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addAllInputs( - dirContents.isEmpty() - ? ImmutableList.of() - : ImmutableList.of( - File.newBuilder() - .setPath("out/tree/child") - .setDigest(getDigest("abc")) - .setIsTool(inputsMode.isTool()) - .build())) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addAllInputs( + dirContents.isEmpty() + ? ImmutableList.of() + : ImmutableList.of( + File.newBuilder() + .setPath("out/tree/child") + .setDigest(getDigest("abc")) + .setIsTool(inputsMode.isTool()) + .build())) + .build()); } @Test @@ -255,9 +250,9 @@ public void testRunfilesInput() throws Exception { writeFile(runfilesInput, "abc"); - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( // In reality, the spawn would have a RunfilesSupplier and a runfiles middleman input. defaultSpawn(), createInputMetadataProvider(runfilesInput), @@ -268,11 +263,11 @@ public void testRunfilesInput() throws Exception { defaultSpawnResult()); // TODO(tjgq): The path should be foo.runfiles/data.txt. - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addInputs(File.newBuilder().setPath("data.txt").setDigest(getDigest("abc"))) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addInputs(File.newBuilder().setPath("data.txt").setDigest(getDigest("abc"))) + .build()); } @Test @@ -283,9 +278,9 @@ public void testAbsolutePathInput() throws Exception { writeFile(absolutePath, "abc"); - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( defaultSpawn(), new StaticInputMetadataProvider( ImmutableMap.of(absolutePathInput, FileArtifactValue.createForTesting(absolutePath))), @@ -294,18 +289,18 @@ public void testAbsolutePathInput() throws Exception { defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addInputs(File.newBuilder().setPath("/some/file.txt").setDigest(getDigest("abc"))) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addInputs(File.newBuilder().setPath("/some/file.txt").setDigest(getDigest("abc"))) + .build()); } @Test public void testEmptyInput() throws Exception { - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( defaultSpawn(), createInputMetadataProvider(), /* inputMap= */ ImmutableSortedMap.of( @@ -315,7 +310,7 @@ public void testEmptyInput() throws Exception { defaultSpawnResult()); // TODO(tjgq): It would make more sense to report an empty file. - verify(outputStream).write(defaultSpawnExec()); + closeAndAssertLog(context, defaultSpawnExec()); } @Test @@ -326,9 +321,9 @@ public void testFileOutput(@TestParameter OutputsMode outputsMode) throws Except Spawn spawn = defaultSpawnBuilder().withOutputs(fileOutput).build(); - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( spawn, createInputMetadataProvider(), createInputMap(), @@ -336,12 +331,12 @@ public void testFileOutput(@TestParameter OutputsMode outputsMode) throws Except defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addListedOutputs("out/file") - .addActualOutputs(File.newBuilder().setPath("out/file").setDigest(getDigest("abc"))) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addListedOutputs("out/file") + .addActualOutputs(File.newBuilder().setPath("out/file").setDigest(getDigest("abc"))) + .build()); } @Test @@ -357,9 +352,9 @@ public void testDirectoryOutput( SpawnBuilder spawn = defaultSpawnBuilder().withOutputs(dirOutput); - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( spawn.build(), createInputMetadataProvider(), createInputMap(), @@ -367,19 +362,19 @@ public void testDirectoryOutput( defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addListedOutputs("out/dir") - .addAllActualOutputs( - dirContents.isEmpty() - ? ImmutableList.of() - : ImmutableList.of( - File.newBuilder() - .setPath("out/dir/file") - .setDigest(getDigest("abc")) - .build())) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addListedOutputs("out/dir") + .addAllActualOutputs( + dirContents.isEmpty() + ? ImmutableList.of() + : ImmutableList.of( + File.newBuilder() + .setPath("out/dir/file") + .setDigest(getDigest("abc")) + .build())) + .build()); } @Test @@ -396,9 +391,9 @@ public void testTreeOutput( Spawn spawn = defaultSpawnBuilder().withOutputs(treeOutput).build(); - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( spawn, createInputMetadataProvider(), createInputMap(), @@ -406,19 +401,19 @@ public void testTreeOutput( defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addListedOutputs("out/tree") - .addAllActualOutputs( - dirContents.isEmpty() - ? ImmutableList.of() - : ImmutableList.of( - File.newBuilder() - .setPath("out/tree/child") - .setDigest(getDigest("abc")) - .build())) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addListedOutputs("out/tree") + .addAllActualOutputs( + dirContents.isEmpty() + ? ImmutableList.of() + : ImmutableList.of( + File.newBuilder() + .setPath("out/tree/child") + .setDigest(getDigest("abc")) + .build())) + .build()); } @Test @@ -426,9 +421,9 @@ public void testEnvironment() throws Exception { Spawn spawn = defaultSpawnBuilder().withEnvironment("SPAM", "eggs").withEnvironment("FOO", "bar").build(); - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( spawn, createInputMetadataProvider(), createInputMap(), @@ -436,21 +431,21 @@ public void testEnvironment() throws Exception { defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .addEnvironmentVariables( - EnvironmentVariable.newBuilder().setName("FOO").setValue("bar")) - .addEnvironmentVariables( - EnvironmentVariable.newBuilder().setName("SPAM").setValue("eggs")) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addEnvironmentVariables( + EnvironmentVariable.newBuilder().setName("FOO").setValue("bar")) + .addEnvironmentVariables( + EnvironmentVariable.newBuilder().setName("SPAM").setValue("eggs")) + .build()); } @Test public void testDefaultPlatformProperties() throws Exception { - SpawnLogContext spawnLogContext = createSpawnLogContext(ImmutableMap.of("a", "1", "b", "2")); + SpawnLogContext context = createSpawnLogContext(ImmutableMap.of("a", "1", "b", "2")); - spawnLogContext.logSpawn( + context.logSpawn( defaultSpawn(), createInputMetadataProvider(), createInputMap(), @@ -458,15 +453,15 @@ public void testDefaultPlatformProperties() throws Exception { defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .setPlatform( - Platform.newBuilder() - .addProperties(Platform.Property.newBuilder().setName("a").setValue("1")) - .addProperties(Platform.Property.newBuilder().setName("b").setValue("2")) - .build()) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .setPlatform( + Platform.newBuilder() + .addProperties(Platform.Property.newBuilder().setName("a").setValue("1")) + .addProperties(Platform.Property.newBuilder().setName("b").setValue("2")) + .build()) + .build()); } @Test @@ -474,9 +469,9 @@ public void testSpawnPlatformProperties() throws Exception { Spawn spawn = defaultSpawnBuilder().withExecProperties(ImmutableMap.of("a", "3", "c", "4")).build(); - SpawnLogContext spawnLogContext = createSpawnLogContext(ImmutableMap.of("a", "1", "b", "2")); + SpawnLogContext context = createSpawnLogContext(ImmutableMap.of("a", "1", "b", "2")); - spawnLogContext.logSpawn( + context.logSpawn( spawn, createInputMetadataProvider(), createInputMap(), @@ -485,16 +480,16 @@ public void testSpawnPlatformProperties() throws Exception { defaultSpawnResult()); // The spawn properties should override the default properties. - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .setPlatform( - Platform.newBuilder() - .addProperties(Platform.Property.newBuilder().setName("a").setValue("3")) - .addProperties(Platform.Property.newBuilder().setName("b").setValue("2")) - .addProperties(Platform.Property.newBuilder().setName("c").setValue("4")) - .build()) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .setPlatform( + Platform.newBuilder() + .addProperties(Platform.Property.newBuilder().setName("a").setValue("3")) + .addProperties(Platform.Property.newBuilder().setName("b").setValue("2")) + .addProperties(Platform.Property.newBuilder().setName("c").setValue("4")) + .build()) + .build()); } @Test @@ -503,9 +498,9 @@ public void testExecutionInfo( throws Exception { Spawn spawn = defaultSpawnBuilder().withExecutionInfo(requirement, "").build(); - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( spawn, createInputMetadataProvider(), createInputMap(), @@ -513,25 +508,25 @@ public void testExecutionInfo( defaultTimeout(), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .setRemotable(!requirement.equals("no-remote")) - .setCacheable(!requirement.equals("no-cache")) - .setRemoteCacheable( - !requirement.equals("no-cache") - && !requirement.equals("no-remote") - && !requirement.equals("no-remote-cache")) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .setRemotable(!requirement.equals("no-remote")) + .setCacheable(!requirement.equals("no-cache")) + .setRemoteCacheable( + !requirement.equals("no-cache") + && !requirement.equals("no-remote") + && !requirement.equals("no-remote-cache")) + .build()); } @Test public void testCacheHit() throws Exception { - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); SpawnResult result = defaultSpawnResultBuilder().setCacheHit(true).build(); - spawnLogContext.logSpawn( + context.logSpawn( defaultSpawn(), createInputMetadataProvider(), createInputMap(), @@ -539,16 +534,16 @@ public void testCacheHit() throws Exception { defaultTimeout(), result); - verify(outputStream).write(defaultSpawnExecBuilder().setCacheHit(true).build()); + closeAndAssertLog(context, defaultSpawnExecBuilder().setCacheHit(true).build()); } @Test public void testDigest() throws Exception { - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); SpawnResult result = defaultSpawnResultBuilder().setDigest(getDigest("abc")).build(); - spawnLogContext.logSpawn( + context.logSpawn( defaultSpawn(), createInputMetadataProvider(), createInputMap(), @@ -556,14 +551,14 @@ public void testDigest() throws Exception { defaultTimeout(), result); - verify(outputStream).write(defaultSpawnExecBuilder().setDigest(getDigest("abc")).build()); + closeAndAssertLog(context, defaultSpawnExecBuilder().setDigest(getDigest("abc")).build()); } @Test public void testTimeout() throws Exception { - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( defaultSpawn(), createInputMetadataProvider(), createInputMap(), @@ -571,18 +566,18 @@ public void testTimeout() throws Exception { /* timeout= */ Duration.ofSeconds(42), defaultSpawnResult()); - verify(outputStream) - .write( - defaultSpawnExecBuilder().setTimeoutMillis(Duration.ofSeconds(42).toMillis()).build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder().setTimeoutMillis(Duration.ofSeconds(42).toMillis()).build()); } @Test public void testSpawnMetrics() throws Exception { SpawnMetrics metrics = SpawnMetrics.Builder.forLocalExec().setTotalTimeInMs(1).build(); - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); - spawnLogContext.logSpawn( + context.logSpawn( defaultSpawn(), createInputMetadataProvider(), createInputMap(), @@ -590,16 +585,16 @@ public void testSpawnMetrics() throws Exception { defaultTimeout(), defaultSpawnResultBuilder().setSpawnMetrics(metrics).build()); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .setMetrics(Protos.SpawnMetrics.newBuilder().setTotalTime(millisToProto(1))) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .setMetrics(Protos.SpawnMetrics.newBuilder().setTotalTime(millisToProto(1))) + .build()); } @Test public void testStatus() throws Exception { - SpawnLogContext spawnLogContext = createSpawnLogContext(); + SpawnLogContext context = createSpawnLogContext(); // SpawnResult requires a non-zero exit code and non-null failure details when the status isn't // successful. @@ -614,7 +609,7 @@ public void testStatus() throws Exception { .build()) .build(); - spawnLogContext.logSpawn( + context.logSpawn( defaultSpawn(), createInputMetadataProvider(), createInputMap(), @@ -622,12 +617,12 @@ public void testStatus() throws Exception { defaultTimeout(), result); - verify(outputStream) - .write( - defaultSpawnExecBuilder() - .setExitCode(37) - .setStatus(Status.NON_ZERO_EXIT.toString()) - .build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .setExitCode(37) + .setStatus(Status.NON_ZERO_EXIT.toString()) + .build()); } private static Duration defaultTimeout() { @@ -724,18 +719,21 @@ private static TreeArtifactValue createTreeArtifactValue(Artifact tree) throws E return builder.build(); } - private SpawnLogContext createSpawnLogContext() { + private SpawnLogContext createSpawnLogContext() throws IOException { return createSpawnLogContext(ImmutableSortedMap.of()); } - private SpawnLogContext createSpawnLogContext(ImmutableMap platformProperties) { + private SpawnLogContext createSpawnLogContext(ImmutableMap platformProperties) + throws IOException { RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); remoteOptions.remoteDefaultExecProperties = platformProperties.entrySet().asList(); - return new SpawnLogContext( + return new ExpandedSpawnLogContext( + logPath, + tempPath, + Encoding.BINARY, + /* sorted= */ false, execRoot.asFragment(), - outputStream, - Options.getDefaults(ExecutionOptions.class), remoteOptions, DigestHashFunction.SHA256, SyscallCache.NO_CACHE); @@ -757,4 +755,19 @@ private static void writeFile(Path path, String contents) throws IOException { path.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.writeContent(path, UTF_8, contents); } + + private void closeAndAssertLog(SpawnLogContext context, SpawnExec... expected) + throws IOException { + context.close(); + + ArrayList actual = new ArrayList<>(); + try (InputStream in = logPath.getInputStream()) { + while (in.available() > 0) { + SpawnExec ex = SpawnExec.parseDelimitedFrom(in); + actual.add(ex); + } + } + + assertThat(actual).containsExactlyElementsIn(expected).inOrder(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java index 76b949eaf6d798..b39f2cf6836091 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java @@ -71,7 +71,6 @@ import java.util.HashMap; import java.util.Map; import java.util.concurrent.Callable; -import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.LinkedBlockingQueue; @@ -760,7 +759,7 @@ public void prefetchFile_interruptingMetadataSupplier_interruptsDownload() throw prefetcher.prefetchFiles( action, ImmutableList.of(a1), interruptedMetadataSupplier, Priority.MEDIUM); - assertThrows(CancellationException.class, future::get); + assertThrows(InterruptedException.class, () -> getFromFuture(future)); } @Test 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/LinuxSandboxCommandLineBuilderTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilderTest.java index a8caa07f8836d2..9f8df6d9c30997 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilderTest.java @@ -53,11 +53,10 @@ public void testLinuxSandboxCommandLineBuilder_fakeRootAndFakeUsernameAreExclusi assertThrows( IllegalStateException.class, () -> - LinuxSandboxCommandLineBuilder.commandLineBuilder( - linuxSandboxPath, commandArguments) + LinuxSandboxCommandLineBuilder.commandLineBuilder(linuxSandboxPath) .setUseFakeRoot(true) .setUseFakeUsername(true) - .build()); + .buildForCommand(commandArguments)); assertThat(e).hasMessageThat().contains("exclusive"); } @@ -75,8 +74,8 @@ public void testLinuxSandboxCommandLineBuilder_buildsWithoutOptionalArguments() .build(); List commandLine = - LinuxSandboxCommandLineBuilder.commandLineBuilder(linuxSandboxPath, commandArguments) - .build(); + LinuxSandboxCommandLineBuilder.commandLineBuilder(linuxSandboxPath) + .buildForCommand(commandArguments); assertThat(commandLine).containsExactlyElementsIn(expectedCommandLine).inOrder(); } @@ -163,7 +162,7 @@ public void testLinuxSandboxCommandLineBuilder_buildsWithOptionalArguments() { .build(); List commandLine = - LinuxSandboxCommandLineBuilder.commandLineBuilder(linuxSandboxPath, commandArguments) + LinuxSandboxCommandLineBuilder.commandLineBuilder(linuxSandboxPath) .setWorkingDirectory(workingDirectory) .setStdoutPath(stdoutPath) .setStderrPath(stderrPath) @@ -180,7 +179,7 @@ public void testLinuxSandboxCommandLineBuilder_buildsWithOptionalArguments() { .setSandboxDebugPath(sandboxDebugPath.getPathString()) .setPersistentProcess(true) .setCgroupsDir(cgroupsDir) - .build(); + .buildForCommand(commandArguments); assertThat(commandLine).containsExactlyElementsIn(expectedCommandLine).inOrder(); } 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..78fe5c244ee282 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,17 +24,23 @@ 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; 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; +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; @@ -68,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; @@ -94,6 +101,79 @@ 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(); @@ -106,7 +186,12 @@ 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 +212,12 @@ 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 +263,12 @@ 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 +276,12 @@ 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(); diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java index 1dd8e66f6640a9..dfc659bcb6d3ec 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java @@ -83,6 +83,7 @@ public void createFileSystem() throws Exception { new SynchronousTreeDeleter(), /* sandboxDebugPath= */ null, /* statisticsPath= */ null, + /* interactiveDebugArguments= */ null, "SomeMnemonic"); symlinkedExecRoot.createFileSystem(); @@ -114,6 +115,7 @@ public void copyOutputs() throws Exception { new SynchronousTreeDeleter(), /* sandboxDebugPath= */ null, /* statisticsPath= */ null, + /* interactiveDebugArguments= */ null, "SomeMnemonic"); symlinkedExecRoot.createFileSystem(); diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java index 0a36c62c3d615f..f4278c5e6406e5 100644 --- a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java +++ b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java @@ -76,8 +76,8 @@ public void testLinuxSandboxedCommand_echo() throws Exception { ImmutableList commandArguments = ImmutableList.of("echo", "sleep furiously"); List fullCommandLine = - LinuxSandboxCommandLineBuilder.commandLineBuilder(getLinuxSandboxPath(), commandArguments) - .build(); + LinuxSandboxCommandLineBuilder.commandLineBuilder(getLinuxSandboxPath()) + .buildForCommand(commandArguments); Command command = new Command(fullCommandLine.toArray(new String[0])); CommandResult commandResult = command.execute(); @@ -98,9 +98,9 @@ private void checkLinuxSandboxStatistics(Duration userTimeToSpend, Duration syst Path statisticsFilePath = outputDir.getRelative("stats.out"); List fullCommandLine = - LinuxSandboxCommandLineBuilder.commandLineBuilder(getLinuxSandboxPath(), commandArguments) + LinuxSandboxCommandLineBuilder.commandLineBuilder(getLinuxSandboxPath()) .setStatisticsPath(statisticsFilePath) - .build(); + .buildForCommand(commandArguments); ExecutionStatisticsTestUtil.executeCommandAndCheckStatisticsAboutCpuTimeSpent( userTimeToSpend, systemTimeToSpend, fullCommandLine, statisticsFilePath); 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..afcf8281bb36b8 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 @@ -82,10 +82,6 @@ private enum TreeComposition { FULLY_LOCAL, FULLY_REMOTE, MIXED; - - boolean isPartiallyRemote() { - return this == FULLY_REMOTE || this == MIXED; - } } private final Map chmodCalls = Maps.newConcurrentMap(); @@ -422,11 +418,10 @@ public void fileArtifactMaterializedAsSymlink( .getPath(outputArtifact.getPath().getPathString()) .createSymbolicLink(targetArtifact.getPath().asFragment()); - PathFragment expectedMaterializationExecPath = null; - if (location == FileLocation.REMOTE) { - expectedMaterializationExecPath = - preexistingPath != null ? preexistingPath : targetArtifact.getExecPath(); - } + PathFragment expectedMaterializationExecPath = + location == FileLocation.REMOTE && preexistingPath != null + ? preexistingPath + : targetArtifact.getExecPath(); assertThat(store.getOutputMetadata(outputArtifact)) .isEqualTo(createFileMetadataForSymlinkTest(location, expectedMaterializationExecPath)); @@ -436,7 +431,12 @@ private FileArtifactValue createFileMetadataForSymlinkTest( FileLocation location, @Nullable PathFragment materializationExecPath) { switch (location) { case LOCAL: - return FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10); + FileArtifactValue target = + FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10); + return materializationExecPath == null + ? target + : FileArtifactValue.createForResolvedSymlink( + materializationExecPath, target, target.getDigest()); case REMOTE: return RemoteFileArtifactValue.create( new byte[] {1, 2, 3}, 10, 1, -1, materializationExecPath); @@ -481,11 +481,8 @@ public void treeArtifactMaterializedAsSymlink( .getPath(outputArtifact.getPath().getPathString()) .createSymbolicLink(targetArtifact.getPath().asFragment()); - PathFragment expectedMaterializationExecPath = null; - if (composition.isPartiallyRemote()) { - expectedMaterializationExecPath = - preexistingPath != null ? preexistingPath : targetArtifact.getExecPath(); - } + PathFragment expectedMaterializationExecPath = + preexistingPath != null ? preexistingPath : targetArtifact.getExecPath(); assertThat(store.getTreeArtifactValue(outputArtifact)) .isEqualTo( @@ -814,7 +811,7 @@ public void fileArtifactValueForSymlink_readFromCache() throws Exception { var symlinkMetadata = store.getOutputMetadata(symlink); - assertThat(symlinkMetadata).isEqualTo(targetMetadata); + assertThat(symlinkMetadata.getDigest()).isEqualTo(targetMetadata.getDigest()); assertThat(DigestUtils.getCacheStats().hitCount()).isEqualTo(1); } } diff --git a/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java b/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java index cf5341895661ff..d4949df013118c 100644 --- a/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java @@ -31,7 +31,7 @@ public class CommandFailureUtilsTest { @Test public void describeCommandFailure() throws Exception { - String target = "//foo:bar"; + Label target = Label.parseCanonicalUnchecked("//foo:bar"); String[] args = new String[3]; args[0] = "/bin/sh"; args[1] = "-c"; @@ -51,7 +51,7 @@ public void describeCommandFailure() throws Exception { cwd, "cfg12345", target, - executionPlatform.label().toString()); + executionPlatform.label()); assertThat(message) .isEqualTo( "sh failed: error executing Mnemonic command (from target //foo:bar) " @@ -60,7 +60,7 @@ public void describeCommandFailure() throws Exception { @Test public void describeCommandFailure_verbose() throws Exception { - String target = "//foo:bar"; + Label target = Label.parseCanonicalUnchecked("//foo:bar"); String[] args = new String[3]; args[0] = "/bin/sh"; args[1] = "-c"; @@ -80,7 +80,7 @@ public void describeCommandFailure_verbose() throws Exception { cwd, "cfg12345", target, - executionPlatform.label().toString()); + executionPlatform.label()); assertThat(message) .isEqualTo( "sh failed: error executing Mnemonic command (from target //foo:bar) \n" @@ -94,7 +94,7 @@ public void describeCommandFailure_verbose() throws Exception { @Test public void describeCommandFailure_longMessage() throws Exception { - String target = "//foo:bar"; + Label target = Label.parseCanonicalUnchecked("//foo:bar"); String[] args = new String[40]; args[0] = "some_command"; for (int i = 1; i < args.length; i++) { @@ -117,7 +117,7 @@ public void describeCommandFailure_longMessage() throws Exception { cwd, "cfg12345", target, - executionPlatform.label().toString()); + executionPlatform.label()); assertThat(message) .isEqualTo( "some_command failed: error executing Mnemonic command (from target //foo:bar) " @@ -130,7 +130,7 @@ public void describeCommandFailure_longMessage() throws Exception { @Test public void describeCommandFailure_longMessage_verbose() throws Exception { - String target = "//foo:bar"; + Label target = Label.parseCanonicalUnchecked("//foo:bar"); String[] args = new String[40]; args[0] = "some_command"; for (int i = 1; i < args.length; i++) { @@ -153,7 +153,7 @@ public void describeCommandFailure_longMessage_verbose() throws Exception { cwd, "cfg12345", target, - executionPlatform.label().toString()); + executionPlatform.label()); assertThat(message) .isEqualTo( "some_command failed: error executing Mnemonic command (from target //foo:bar) \n" @@ -172,7 +172,7 @@ public void describeCommandFailure_longMessage_verbose() throws Exception { @Test public void describeCommandFailure_singleSkippedArgument() throws Exception { - String target = "//foo:bar"; + Label target = Label.parseCanonicalUnchecked("//foo:bar"); String[] args = new String[35]; // Long enough to make us skip 1 argument below. args[0] = "some_command"; for (int i = 1; i < args.length; i++) { @@ -191,7 +191,7 @@ public void describeCommandFailure_singleSkippedArgument() throws Exception { cwd, "cfg12345", target, - executionPlatform.label().toString()); + executionPlatform.label()); assertThat(message) .isEqualTo( "some_command failed: error executing Mnemonic command (from target //foo:bar)" @@ -230,7 +230,7 @@ public void describeCommandPrettyPrintArgs() throws Exception { envToClear, cwd, "cfg12345", - executionPlatform.label().toString()); + executionPlatform.label()); assertThat(message) .isEqualTo( 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 diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java index 54955ff5fdc6a6..7336879f246fe0 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java @@ -383,8 +383,8 @@ private static Path prepareSandboxRunner(FileSystem fs, RemoteWorkerOptions remo CommandResult cmdResult = null; Command cmd = new Command( - LinuxSandboxCommandLineBuilder.commandLineBuilder(sandboxPath, ImmutableList.of("true")) - .build() + LinuxSandboxCommandLineBuilder.commandLineBuilder(sandboxPath) + .buildForCommand(ImmutableList.of("true")) .toArray(new String[0]), ImmutableMap.of(), sandboxPath.getParentDirectory().getPathFile());