diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java index 35f8019f85db01..986c0677cbe60c 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java @@ -117,28 +117,34 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) if (!file.getFileName().toString().endsWith(".class")) { return FileVisitResult.CONTINUE; } - // TODO(bazel-team): filter with coverage_instrumentation_filter? - // It's not clear whether there is any advantage in not instrumenting *Test classes, - // apart from lowering the covered percentage in the aggregate statistics. - // We first move the original .class file to our metadata directory, then instrument it - // and output the instrumented version in the regular classes output directory. - Path instrumentedCopy = file; - Path uninstrumentedCopy; - if (isNewCoverageImplementation) { - Path absoluteUninstrumentedCopy = Paths.get(file + ".uninstrumented"); - uninstrumentedCopy = - instrumentedClassesDirectory.resolve(root.relativize(absoluteUninstrumentedCopy)); - } else { - uninstrumentedCopy = instrumentedClassesDirectory.resolve(root.relativize(file)); - } - Files.createDirectories(uninstrumentedCopy.getParent()); - Files.move(file, uninstrumentedCopy); - try (InputStream input = - new BufferedInputStream(Files.newInputStream(uninstrumentedCopy)); - OutputStream output = - new BufferedOutputStream(Files.newOutputStream(instrumentedCopy))) { - instr.instrument(input, output, file.toString()); + try { + // We first move the original .class file to our metadata directory, then instrument + // it and output the instrumented version in the regular classes output directory. + Path instrumentedCopy = file; + Path uninstrumentedCopy; + if (isNewCoverageImplementation) { + Path absoluteUninstrumentedCopy = Paths.get(file + ".uninstrumented"); + uninstrumentedCopy = + instrumentedClassesDirectory.resolve( + root.relativize(absoluteUninstrumentedCopy)); + } else { + uninstrumentedCopy = instrumentedClassesDirectory.resolve(root.relativize(file)); + } + Files.createDirectories(uninstrumentedCopy.getParent()); + Files.move(file, uninstrumentedCopy); + try (InputStream input = + new BufferedInputStream(Files.newInputStream(uninstrumentedCopy)); + OutputStream output = + new BufferedOutputStream(Files.newOutputStream(instrumentedCopy))) { + instr.instrument(input, output, file.toString()); + } catch (Exception e) { + Files.delete(instrumentedCopy); + Files.copy(uninstrumentedCopy, instrumentedCopy); + throw e; // Bubble up to the outer broader safety catch block for logging. + } + } catch (Exception e) { + System.err.printf("WARNING: %s was not instrumented: %s%n\n", file, e.getMessage()); } return FileVisitResult.CONTINUE; } 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 441b34abcda0da..912411212f8a9e 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 @@ -41,7 +41,9 @@ import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.events.ExtendedEventHandler.FetchProgress; +import com.google.devtools.build.lib.events.NullEventHandler; import com.google.devtools.build.lib.packages.StarlarkInfo; import com.google.devtools.build.lib.packages.StructImpl; import com.google.devtools.build.lib.packages.StructProvider; @@ -615,8 +617,8 @@ private StructImpl completeDownload(PendingDownload pendingDownload) defaultValue = "False", named = true, doc = - "If set, indicate the error in the return value" - + " instead of raising an error for failed downloads"), + "If set, indicate the error in the return value instead of raising an error for" + + " failed downloads. This silences errors and warnings."), @Param( name = "canonical_id", defaultValue = "''", @@ -726,7 +728,7 @@ public Object download( canonicalId, Optional.empty(), outputPath.getPath(), - env.getListener(), + allowFail ? NullEventHandler.INSTANCE : env.getListener(), envVariables, getIdentifyingStringForLogging()); download = @@ -815,8 +817,8 @@ public Object download( defaultValue = "False", named = true, doc = - "If set, indicate the error in the return value" - + " instead of raising an error for failed downloads"), + "If set, indicate the error in the return value instead of raising an error for" + + " failed downloads. This silences errors and warnings."), @Param( name = "canonical_id", defaultValue = "''", @@ -927,7 +929,7 @@ public StructImpl downloadAndExtract( canonicalId, Optional.of(type), downloadDirectory, - env.getListener(), + allowFail ? NullEventHandler.INSTANCE : env.getListener(), envVariables, getIdentifyingStringForLogging()); downloadedPath = downloadManager.finalizeDownload(pendingDownload); diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java index 6210747c99286a..07807c8cbecb20 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java @@ -530,6 +530,21 @@ public boolean usingLocalTestJobs() { + " code 39.") public int remoteRetryOnCacheEviction; + @Option( + name = "experimental_use_remote_cache_for_cache_unaware_spawns", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "If set to true (the default), allow spawns other than \"remote\" to use remote caching. " + + "Setting this to false is useful when troubleshooting non-determinism cases in a " + + "\"mixed\" configuration where certain spawns are forced to run remotely (for " + + "example, to hide their non-determinism) and others are allowed to run via dynamic " + + "execution. In such cases, we must allow the remote spawns to leverage the remote " + + "cache, but not those that are configured to run locally. This flag is similar in " + + "purpose to --remote_accept_cached.") + public boolean useRemoteCacheForCacheUnawareSpawns; + /** An enum for specifying different formats of test output. */ public enum TestOutputFormat { SUMMARY, // Provide summary output only. diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/BUILD b/src/main/java/com/google/devtools/build/lib/exec/local/BUILD index bad3cb8abac5a5..c2638c74005eab 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/local/BUILD @@ -31,6 +31,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:resource_manager", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", + "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/exec:runfiles_tree_updater", "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/profiler", 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 916b5b41ba052c..42ecbfbebce610 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 @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.exec.BinTools; +import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; import com.google.devtools.build.lib.exec.SpawnExecutingEvent; import com.google.devtools.build.lib.exec.SpawnRunner; @@ -100,9 +101,11 @@ public class LocalSpawnRunner implements SpawnRunner { private final BinTools binTools; private final RunfilesTreeUpdater runfilesTreeUpdater; + private final boolean handlesCaching; public LocalSpawnRunner( Path execRoot, + ExecutionOptions executionOptions, LocalExecutionOptions localExecutionOptions, ResourceManager resourceManager, LocalEnvProvider localEnvProvider, @@ -117,6 +120,7 @@ public LocalSpawnRunner( this.localEnvProvider = localEnvProvider; this.binTools = binTools; this.runfilesTreeUpdater = runfilesTreeUpdater; + this.handlesCaching = !executionOptions.useRemoteCacheForCacheUnawareSpawns; } @Override @@ -182,7 +186,7 @@ public boolean canExec(Spawn spawn) { @Override public boolean handlesCaching() { - return false; + return handlesCaching; } protected Path createActionTemp(Path execRoot) throws IOException { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 934cdff7382f15..43aba3c437d72f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -72,9 +72,11 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.longrunning.Operation; +import com.google.protobuf.Any; import com.google.protobuf.Timestamp; import com.google.protobuf.util.Durations; import com.google.protobuf.util.Timestamps; +import com.google.rpc.RetryInfo; import io.grpc.Status.Code; import java.io.IOException; import java.time.Duration; @@ -179,6 +181,29 @@ public void reportExecutingIfNot() { } } + /** + * Guess if an action failed due to a transient remote error or not. + * + *

As described in #18319, the + * Remote Build protocol does not yet provide a way to distinguish between regular action crashes + * and unexpected worker crashes. This provides a heuristic to retry on likely worker crashes. + * The specific error code we may get depends on the remote executor implementation so we need to + * be quite liberal here: Go funnels all non-regular exit codes into {@code -1} and the shell + * returns {@code 128+SIGNO}. + */ + private boolean isTransientError(RemoteAction action, RemoteActionResult result) { + if (!remoteOptions.remoteExitSignalsAreTransientErrors) { + return false; + } + + boolean isTestAction = action.getSpawn().getMnemonic().equals("TestRunner"); + if (isTestAction) { + return false; + } + + return result.getExitCode() < 0 || result.getExitCode() > 127; + } + @Override public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) throws ExecException, InterruptedException, IOException, ForbiddenActionInputException { @@ -306,6 +331,21 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) maybeDownloadServerLogs(action, result.getResponse()); } + if (isTransientError(action, result)) { + // This contraption is required to convince the ExecuteRetrier that the failure can + // be retried. + com.google.rpc.Status synthesizedStatus = com.google.rpc.Status.newBuilder() + .setCode(com.google.rpc.Code.FAILED_PRECONDITION.getNumber()) + .setMessage( + "Remote action seems to have terminated due to a signal (exit code " + + result.getExitCode() + + "); assuming worker crash to retry") + .addDetails(Any.pack(RetryInfo.newBuilder().build())) + .build(); + Exception cause = new ExecutionStatusException(synthesizedStatus, null); + throw new IOException(cause); + } + try { return downloadAndFinalizeSpawnResult( action, diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 31bd22fd56915a..a3b3a0a95c03eb 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -348,6 +348,22 @@ public RemoteBuildEventUploadModeConverter() { + "If set to 0, retries are disabled.") public int remoteMaxRetryAttempts; + // TODO(https://github.com/bazelbuild/bazel/issues/18319): Remove once the Remote Build + // protocol can distinguish between abrupt worker termination and action termination. + @Option( + name = "snowflake_remote_exit_signals_are_transient_errors", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "Consider abrupt terminations of remote non-test actions as transient failures. " + + "This can be used to retry actions that fail due to their remote worker " + + "crashing instead of failing the build. Note that this cannot differentiate " + + "between those failures and legitimate action crashes so, if you enable this, " + + "you should also set --remote_retries to non-zero or enable local fallback " + + "to avoid getting into an infinite retry loop.") + public boolean remoteExitSignalsAreTransientErrors; + @Option( name = "remote_retry_max_delay", defaultValue = "5s", diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index 6aa9ba7f36e6fc..eac1fcd0c30116 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java @@ -547,7 +547,7 @@ private LazyData doCostlyEstimation() { case LINUX: resourceSet = ResourceSet.createWithRamCpu( - /* memoryMb= */ Math.max(50, -100 + 0.1 * inputsCount), /* cpu= */ 1); + /* memoryMb= */ 100 + 2.4 * (inputsBytes / 1024.0 / 1024.0), /* cpuUsage= */ 1); break; default: resourceSet = diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java index 9c862cb7019863..a2d89175d5aaa4 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java @@ -490,7 +490,7 @@ DetailedExitCode parseOptions(List args, ExtendedEventHandler eventHandl // to improve the user experience, but not required for safety or correctness. optionsPolicyEnforcer.enforce(optionsParser, commandAnnotation.name()); // Print warnings for odd options usage - for (String warning : optionsParser.getWarnings()) { + for (String warning : getFilteredWarnings(optionsParser)) { eventHandler.handle(Event.warn(warning)); } CommonCommandOptions commonOptions = optionsParser.getOptions(CommonCommandOptions.class); @@ -704,4 +704,20 @@ private static void partitionCommandLineArgs( } } } + + public static ImmutableList getFilteredWarnings(OptionsParser parser) { + CommonCommandOptions commonOptions = parser.getOptions(CommonCommandOptions.class); + Preconditions.checkNotNull(commonOptions, + "getWarnings can only be called after options parsing"); + + ImmutableList.Builder warnings = ImmutableList.builder(); + for (String warning : parser.getWarnings()) { + if (!commonOptions.reportDuplicateOptionsAndConfigs && warning.contains( + "was expanded to from both")) { + continue; + } + warnings.add(warning); + } + return warnings.build(); + } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandLineEvent.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandLineEvent.java index 9267fdc395c0ff..6b8f2a76439768 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandLineEvent.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandLineEvent.java @@ -30,6 +30,8 @@ import com.google.devtools.build.lib.runtime.proto.CommandLineOuterClass.CommandLineSection; import com.google.devtools.build.lib.runtime.proto.CommandLineOuterClass.Option; import com.google.devtools.build.lib.runtime.proto.CommandLineOuterClass.OptionList; +import com.google.devtools.build.lib.util.OptionsUtils; +import com.google.devtools.build.lib.util.OptionsUtils.OptionSensitivity; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.common.options.OptionDefinition; import com.google.devtools.common.options.OptionEffectTag; @@ -138,11 +140,13 @@ List

It is tempting (and difficult) to make this list customizable via a command-line flag. + * However, if we allowed customization, we would not be able to trust that a given Bazel + * version does the right thing because we'd be subject to the contents of the bazelrc or + * the flags that a user passes.

+ */ + private final static ImmutableSet allowedEnvVars = ImmutableSet.of( + "BAZEL_SRCDIR", + "BAZEL_TEST", + "HOME", + "LD_LIBRARY_PATH", + "PATH", + "PWD", + "SHLVL", + "TEST_BINARY", + "TEST_SRCDIR", + "TEST_TARGET", + "TEST_TIMEOUT", + "TEST_TMPDIR", + "TEST_WORKSPACE", + "TMPDIR", + "TZ", + "USER", + "WORKSPACE_DIR" + ); + + /** + * List of environment variable name prefixes that are allowed to propagate to the BEP. + * + *

This list must only contain prefixes for clients we control as otherwise we could be + * allowing unknown variables by mistake.

+ * + *

See {@link #allowedEnvVars} for more details.

+ */ + private final static ImmutableSet allowedEnvVarPrefixes = ImmutableSet.of( + "BESD_", + "CI_SNOWCI_", + "IJWB_" + ); + + /** List of commands that have sensitive residual arguments. */ + public final static ImmutableSet sensitiveResidualCommands = ImmutableSet.of("run"); + + /** Tag for the sensitivity of an option's value. */ + public enum OptionSensitivity { + /** The option has a value that is not sensitive, or it has no value. */ + NONE, + + /** The option has a value of the form name=val and only val is sensitive. */ + PARTIAL, + + /** The option has a value and the whole value is sensitive. */ + FULL, + } + + /** Predicate that returns true if the given string corresponds to an option that carries a + * fully sensitive value. */ + private final static Predicate isFullySensitiveOption = + Pattern.compile("_arg([ =].*|)$").asPredicate(); + + /** Predicate that returns true if the given string corresponds to an option that carries a + * partially sensitive value. */ + private final static Predicate isPartiallySensitiveOption = + Pattern.compile("_(env|header)([ =].*|)$").asPredicate(); + + /** Returns true if the given environment variable can be propagated to the BEP. */ + private static boolean isEnvVarAllowed(String name) { + if (allowedEnvVars.contains(name)) { + return true; + } + + for (String prefix : allowedEnvVarPrefixes) { + if (name.startsWith(prefix)) { + return true; + } + } + + return false; + } + + /** Determines the sensitivity of the given option name, where the provided string may include the + * option's value as well. */ + public static OptionSensitivity getOptionSensitivity(String raw) { + if (isFullySensitiveOption.test(raw)) { + return OptionSensitivity.FULL; + } else if (isPartiallySensitiveOption.test(raw)) { + return OptionSensitivity.PARTIAL; + } else { + return OptionSensitivity.NONE; + } + } + + /** Determines if the given string contains an option name/value pair. */ + private static boolean containsOptionValue(String raw) { + return raw.contains(" ") || raw.contains("="); + } + + /** + * Given a name/value pair, rewrites the string to redact the value unless the name is in the + * {@link #allowedEnvVars} list. + * + * @param raw a string of the form name=value + * @return the rewritten string + */ + public static String maybeScrubAssignment(OptionSensitivity sensitivity, String raw) { + switch (sensitivity) { + case NONE: + return raw; + + case PARTIAL: + String[] parts = raw.split("=", 2); + if (parts.length < 2) { + // If we find a lone value, we could redact it with the rationale being that the user + // might have made a mistake and typed `--test_env=secret` instead of + // `--test_env=VAR=secret`. However, redacting lone values would render the telemetry for + // all flags nearly useless and a determined user would pass these checks anyway. + return raw; + } + assert parts.length == 2; + + String name = parts[0]; + if (isEnvVarAllowed(name)) { + return raw; + } + // We do not add the word REDACTED here because we want the equal sign to be explicitly + // followed by a space, which simplifies detecting that there really is no value attached + // to the variable. + return String.format("%s= ", name); + + default: + assert sensitivity.equals(OptionSensitivity.FULL); // Cope with non-exhaustive switches. + return "REDACTED"; + } + } + + /** + * Given a literal argument with name/value pairs that may contain a secret, rewrites the string + * to redact the value unless the name is in the {@link #allowedEnvVars} or + * {@link #allowedEnvVarPrefixes} lists. + * + * @param raw a string of the form option=name=value or option name=value. The + * string may or may not be prefixed by --. + * @return the rewritten string + */ + public static String maybeScrubCombinedForm(OptionSensitivity sensitivity, String raw) { + int pos = 0; + int[] chars = raw.chars().toArray(); + for (int ch : chars) { + if (ch == ' ' || ch == '=') { + break; + } + pos += 1; + } + if (pos == chars.length) { + if (sensitivity == OptionSensitivity.NONE) { + return raw; + } else { + // This should never happen, but better propagate a placeholder string rather than crash + // due to the complexity of option parsing and the fact that we might have missed some + // corner case. + return "INVALID-OPTION-VALUE"; + } + } + pos += 1; + return String.format( + "%s%s", raw.substring(0, pos), maybeScrubAssignment(sensitivity, raw.substring(pos))); + } + + /** + * Scrubs secrets from a list of arguments. + * + *

The semantics for scrubbing arguments are the same as those defined by + * {@link #maybeScrubCombinedForm}. However, this must take care of options that were split + * across two arguments (one with the option's name and one with the value) as well as all + * residual arguments.

+ * + * @param args the arguments to scrub + * @return the scrubbed arguments list + */ + public static ImmutableList scrubArgs(ImmutableList args) { + ImmutableList.Builder builder = ImmutableList.builder(); + + Iterator iter = args.iterator(); + + // Handle Bazel's own arguments first. + OptionSensitivity previousSensitivity = OptionSensitivity.NONE; + while (iter.hasNext()) { + String canonicalForm = iter.next(); + if (canonicalForm.equals("--")) { + builder.add(canonicalForm); + break; + } + + if (previousSensitivity != OptionSensitivity.NONE) { + canonicalForm = maybeScrubAssignment(previousSensitivity, canonicalForm); + previousSensitivity = OptionSensitivity.NONE; + } else { + OptionSensitivity sensitivity = getOptionSensitivity(canonicalForm); + if (sensitivity != OptionSensitivity.NONE) { + if (containsOptionValue(canonicalForm)) { + canonicalForm = maybeScrubCombinedForm(sensitivity, canonicalForm); + } else { + previousSensitivity = sensitivity; + } + } + } + builder.add(canonicalForm); + } + + // Handle residual arguments. + while (iter.hasNext()) { + iter.next(); + builder.add("REDACTED"); + } + + return builder.build(); + } + /** * Returns a string representation of the non-hidden specified options; option values are * shell-escaped. @@ -42,7 +266,11 @@ public static String asShellEscapedString(Iterable opti if (result.length() != 0) { result.append(' '); } - result.append(option.getCanonicalFormWithValueEscaper(ShellEscaper::escapeString)); + OptionSensitivity sensitivity = + getOptionSensitivity(option.getOptionDefinition().getOptionName()); + result.append(option.getCanonicalFormWithValueEscaper( + (unescaped) -> + ShellEscaper.escapeString(maybeScrubAssignment(sensitivity, unescaped)))); } return result.toString(); } @@ -67,7 +295,7 @@ public static List asArgumentList(Iterable opti } builder.add(option.getCanonicalForm()); } - return builder.build(); + return scrubArgs(builder.build()); } /** diff --git a/src/main/java/com/google/devtools/build/lib/worker/BUILD b/src/main/java/com/google/devtools/build/lib/worker/BUILD index 03a625a5cc160c..92750bbc7c306d 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/BUILD +++ b/src/main/java/com/google/devtools/build/lib/worker/BUILD @@ -65,6 +65,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", + "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/exec:runfiles_tree_updater", "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/exec/local", diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index 5defc2f9457c50..30d37e67db3220 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java @@ -226,6 +226,7 @@ public void registerSpawnStrategies( env.getBlazeWorkspace().getBinTools(), env.getLocalResourceManager(), RunfilesTreeUpdater.forCommandEnvironment(env), + env.getOptions().getOptions(ExecutionOptions.class), env.getOptions().getOptions(WorkerOptions.class), WorkerMetricsCollector.instance(), env.getClock()); 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 7adf6eaf188203..6e5f45bcbb6e56 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 @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.exec.BinTools; +import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; import com.google.devtools.build.lib.exec.SpawnExecutingEvent; import com.google.devtools.build.lib.exec.SpawnRunner; @@ -98,6 +99,7 @@ final class WorkerSpawnRunner implements SpawnRunner { private final WorkerParser workerParser; private final AtomicInteger requestIdCounter = new AtomicInteger(1); private final WorkerMetricsCollector metricsCollector; + private final boolean handlesCaching; public WorkerSpawnRunner( SandboxHelpers helpers, @@ -109,6 +111,7 @@ public WorkerSpawnRunner( BinTools binTools, ResourceManager resourceManager, RunfilesTreeUpdater runfilesTreeUpdater, + ExecutionOptions executionOptions, WorkerOptions workerOptions, WorkerMetricsCollector workerMetricsCollector, Clock clock) { @@ -123,6 +126,7 @@ public WorkerSpawnRunner( this.resourceManager.setWorkerPool(workers); this.metricsCollector = workerMetricsCollector; this.metricsCollector.setClock(clock); + this.handlesCaching = !executionOptions.useRemoteCacheForCacheUnawareSpawns; } @Override @@ -149,7 +153,7 @@ public boolean canExec(Spawn spawn) { @Override public boolean handlesCaching() { - return false; + return handlesCaching; } @Override diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/BUILD b/src/test/java/com/google/devtools/build/lib/exec/local/BUILD index 845d2786a4e1c0..9cf9d4ff749ada 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/local/BUILD @@ -28,6 +28,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/actions:resource_manager", "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", + "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/exec:runfiles_tree_updater", "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/exec/local", diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java index 57b1ec42f1c38d..28c653d8143e67 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.exec.BinTools; +import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; import com.google.devtools.build.lib.exec.SpawnExecutingEvent; import com.google.devtools.build.lib.exec.SpawnSchedulingEvent; @@ -107,6 +108,7 @@ public TestedLocalSpawnRunner( LocalEnvProvider localEnvProvider) { super( execRoot, + Options.getDefaults(ExecutionOptions.class), localExecutionOptions, resourceManager, localEnvProvider, @@ -621,6 +623,7 @@ public void interruptWaitsForProcessExit() throws Exception { LocalSpawnRunner runner = new LocalSpawnRunner( tempDir, + Options.getDefaults(ExecutionOptions.class), Options.getDefaults(LocalExecutionOptions.class), resourceManager, LocalEnvProvider.forCurrentOs(ImmutableMap.of()), @@ -858,6 +861,7 @@ public void hasExecutionStatistics() throws Exception { FileSystem fs = new UnixFileSystem(DigestHashFunction.SHA256, /*hashAttributeName=*/ ""); + ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class); LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class); int minimumWallTimeToSpendInMs = 10 * 1000; @@ -883,6 +887,7 @@ public void hasExecutionStatistics() throws Exception { LocalSpawnRunner runner = new LocalSpawnRunner( execRoot, + executionOptions, options, resourceManager, LocalSpawnRunnerTest::keepLocalEnvUnchanged, diff --git a/src/test/java/com/google/devtools/build/lib/runtime/CommandLineEventTest.java b/src/test/java/com/google/devtools/build/lib/runtime/CommandLineEventTest.java index 9f67cc11dd06bf..011a07fbbc51df 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/CommandLineEventTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/CommandLineEventTest.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.bazel.BazelStartupOptionsModule.Options; +import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId.StructuredCommandLineId; import com.google.devtools.build.lib.runtime.CommandLineEvent.CanonicalCommandLineEvent; import com.google.devtools.build.lib.runtime.CommandLineEvent.OriginalCommandLineEvent; @@ -26,6 +27,7 @@ import com.google.devtools.build.lib.runtime.proto.CommandLineOuterClass.CommandLine; import com.google.devtools.build.lib.runtime.proto.CommandLineOuterClass.CommandLineSection; import com.google.devtools.build.lib.runtime.proto.CommandLineOuterClass.CommandLineSection.SectionTypeCase; +import com.google.devtools.build.lib.runtime.proto.CommandLineOuterClass.Option; import com.google.devtools.build.lib.runtime.proto.CommandLineOuterClass.OptionList; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.common.options.OptionPriority.PriorityCategory; @@ -511,4 +513,68 @@ public void testSimpleStringToolCommandLine() throws OptionsParsingException { assertThat(line.getSections(0).getChunkList().getChunk(0)) .isEqualTo("The quick brown fox jumps over the lazy dog"); } + + @Test + public void testScrubEnvOption() throws OptionsParsingException { + OptionsParser fakeStartupOptions = + OptionsParser.builder().optionsClasses(BlazeServerStartupOptions.class).build(); + OptionsParser fakeCommandOptions = + OptionsParser.builder().optionsClasses(TestOptions.class).build(); + fakeCommandOptions.parse( + PriorityCategory.COMMAND_LINE, + "command line", + ImmutableList.of( + "--stub_env=HOME=/home/jmmv", + "--stub_env=NOT_ALLOWED=1234", + "--stub_env", "ANOTHER=foo=1234", + "--stub_inherit_env=HOME", + "--stub_inherit_env=NOT_ALLOWED" + )); + + CommandLine line = + new OriginalCommandLineEvent( + "testblaze", + fakeStartupOptions, + "someCommandName", + fakeCommandOptions, + Optional.of(ImmutableList.of())) + .asStreamProto(null) + .getStructuredCommandLine(); + + assertThat(line.getCommandLineLabel()).isEqualTo("original"); + checkCommandLineSectionLabels(line); + + assertThat(line.getSections(0).getChunkList().getChunk(0)).isEqualTo("testblaze"); + assertThat(line.getSections(1).getOptionList().getOptionCount()).isEqualTo(0); + assertThat(line.getSections(2).getChunkList().getChunk(0)).isEqualTo("someCommandName"); + // Expect the rc file options and invocation policy options to not be listed with the explicit + // command line options. + assertThat(line.getSections(3).getOptionList().getOptionCount()).isEqualTo(5); + { + Option option = line.getSections(3).getOptionList().getOption(0); + assertThat(option.getCombinedForm()).isEqualTo("--stub_env=HOME=/home/jmmv"); + assertThat(option.getOptionValue()).isEqualTo("HOME=/home/jmmv"); + } + { + Option option = line.getSections(3).getOptionList().getOption(1); + assertThat(option.getCombinedForm()).isEqualTo("--stub_env=NOT_ALLOWED= "); + assertThat(option.getOptionValue()).isEqualTo("NOT_ALLOWED= "); + } + { + Option option = line.getSections(3).getOptionList().getOption(2); + assertThat(option.getCombinedForm()).isEqualTo("--stub_env ANOTHER= "); + assertThat(option.getOptionValue()).isEqualTo("ANOTHER= "); + } + { + Option option = line.getSections(3).getOptionList().getOption(3); + assertThat(option.getCombinedForm()).isEqualTo("--stub_inherit_env=HOME"); + assertThat(option.getOptionValue()).isEqualTo("HOME"); + } + { + Option option = line.getSections(3).getOptionList().getOption(4); + assertThat(option.getCombinedForm()).isEqualTo("--stub_inherit_env=NOT_ALLOWED"); + assertThat(option.getOptionValue()).isEqualTo("NOT_ALLOWED"); + } + assertThat(line.getSections(4).getChunkList().getChunkCount()).isEqualTo(0); + } } diff --git a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java index eb66c9afd05c23..b4c5e55c6b06ea 100644 --- a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java @@ -138,6 +138,7 @@ public final void setUp() throws Exception { OptionsParser optionsParser = OptionsParser.builder().optionsClasses(ExecutionOptions.class).build(); optionsParser.parse("--verbose_failures"); + ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class); LocalExecutionOptions localExecutionOptions = Options.getDefaults(LocalExecutionOptions.class); ResourceManager resourceManager = ResourceManager.instanceForTestingOnly(); @@ -150,6 +151,7 @@ public final void setUp() throws Exception { execRoot, new LocalSpawnRunner( execRoot, + executionOptions, localExecutionOptions, resourceManager, (env, binTools1, fallbackTmpDir) -> ImmutableMap.copyOf(env), diff --git a/src/test/java/com/google/devtools/build/lib/util/OptionsUtilsTest.java b/src/test/java/com/google/devtools/build/lib/util/OptionsUtilsTest.java index dcdb4a97e98f01..ac7f6fcdf6cb02 100644 --- a/src/test/java/com/google/devtools/build/lib/util/OptionsUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/OptionsUtilsTest.java @@ -16,6 +16,8 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.util.OptionsUtils.OptionSensitivity; import com.google.devtools.build.lib.util.OptionsUtils.PathFragmentConverter; import com.google.devtools.build.lib.util.OptionsUtils.PathFragmentListConverter; import com.google.devtools.build.lib.vfs.PathFragment; @@ -231,4 +233,175 @@ public void absolutePathFragmentConverter_failsForRelativePath() { assertThat(e).hasMessageThat().isEqualTo("Not an absolute path: 'relative/path'"); } + + @Test + public void getOptionSensitivity_None() { + assertThat(OptionsUtils.getOptionSensitivity("x")).isEqualTo(OptionSensitivity.NONE); + assertThat(OptionsUtils.getOptionSensitivity("x_arg_ok")).isEqualTo(OptionSensitivity.NONE); + assertThat(OptionsUtils.getOptionSensitivity("x_env_ok")).isEqualTo(OptionSensitivity.NONE); + assertThat(OptionsUtils.getOptionSensitivity("x_header_ok")).isEqualTo(OptionSensitivity.NONE); + } + + @Test + public void getOptionSensitivity_Partial() { + assertThat(OptionsUtils.getOptionSensitivity("x_env")).isEqualTo(OptionSensitivity.PARTIAL); + assertThat(OptionsUtils.getOptionSensitivity("x_env=a")).isEqualTo(OptionSensitivity.PARTIAL); + assertThat(OptionsUtils.getOptionSensitivity("x_env a")).isEqualTo(OptionSensitivity.PARTIAL); + + assertThat(OptionsUtils.getOptionSensitivity("x_header")).isEqualTo(OptionSensitivity.PARTIAL); + assertThat( + OptionsUtils.getOptionSensitivity("x_header=a")).isEqualTo(OptionSensitivity.PARTIAL); + assertThat( + OptionsUtils.getOptionSensitivity("x_header a")).isEqualTo(OptionSensitivity.PARTIAL); + } + + @Test + public void getOptionSensitivity_Full() { + assertThat(OptionsUtils.getOptionSensitivity("x_arg")).isEqualTo(OptionSensitivity.FULL); + assertThat(OptionsUtils.getOptionSensitivity("x_arg=a")).isEqualTo(OptionSensitivity.FULL); + assertThat(OptionsUtils.getOptionSensitivity("x_arg a")).isEqualTo(OptionSensitivity.FULL); + } + + @Test + public void maybeScrubAssignment_None() { + assertThat(OptionsUtils.maybeScrubAssignment(OptionSensitivity.NONE, "A=B")).isEqualTo("A=B"); + } + + @Test + public void maybeScrubAssignment_Partial_OnlyName() { + assertThat(OptionsUtils.maybeScrubAssignment(OptionSensitivity.PARTIAL, "AB")).isEqualTo("AB"); + } + + @Test + public void maybeScrubAssignment_Partial_NameAndValue() { + assertThat( + OptionsUtils.maybeScrubAssignment(OptionSensitivity.PARTIAL, "A=B")) + .isEqualTo("A= "); + assertThat( + OptionsUtils.maybeScrubAssignment(OptionSensitivity.PARTIAL, "A=B=C")) + .isEqualTo("A= "); + } + + @Test + public void maybeScrubAssignment_Full() { + assertThat(OptionsUtils.maybeScrubAssignment(OptionSensitivity.FULL, "")).isEqualTo("REDACTED"); + assertThat( + OptionsUtils.maybeScrubAssignment(OptionSensitivity.FULL, "x")).isEqualTo("REDACTED"); + } + + @Test + public void maybeScrubCombinedForm_None() { + assertThat(OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.NONE, "")).isEqualTo(""); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.NONE, "--a_env x")) + .isEqualTo("--a_env x"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.NONE, "--a_env=x")) + .isEqualTo("--a_env=x"); + } + + @Test + public void maybeScrubCombinedForm_Partial() { + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.PARTIAL, "")) + .isEqualTo("INVALID-OPTION-VALUE"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.PARTIAL, "--a")) + .isEqualTo("INVALID-OPTION-VALUE"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.PARTIAL, "--a_env x")) + .isEqualTo("--a_env x"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.PARTIAL, "--a_env x=y")) + .isEqualTo("--a_env x= "); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.PARTIAL, "--a_env=x")) + .isEqualTo("--a_env=x"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.PARTIAL, "--a_env=x=y")) + .isEqualTo("--a_env=x= "); + } + + @Test + public void maybeScrubCombinedForm_Full() { + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.FULL, "")) + .isEqualTo("INVALID-OPTION-VALUE"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.FULL, "--a")) + .isEqualTo("INVALID-OPTION-VALUE"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.FULL, "--a_env x")) + .isEqualTo("--a_env REDACTED"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.FULL, "--a_env x=y")) + .isEqualTo("--a_env REDACTED"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.FULL, "--a_env=x")) + .isEqualTo("--a_env=REDACTED"); + assertThat( + OptionsUtils.maybeScrubCombinedForm(OptionSensitivity.FULL, "--a_env=x=y")) + .isEqualTo("--a_env=REDACTED"); + } + + @Test + public void scrubArgs_None() { + ImmutableList args = ImmutableList.of(); + ImmutableList expArgs = ImmutableList.of(); + assertThat(OptionsUtils.scrubArgs(args)).isEqualTo(expArgs); + } + + @Test + public void scrubArgs_Partial_SameArg() { + ImmutableList args = + ImmutableList.of("foo", "--test_env=UNKNOWN=1234", "--test_env=HOME=dir"); + ImmutableList expArgs = + ImmutableList.of("foo", "--test_env=UNKNOWN= ", "--test_env=HOME=dir"); + assertThat(OptionsUtils.scrubArgs(args)).isEqualTo(expArgs); + } + + @Test + public void scrubArgs_Partial_SeparateArg() { + ImmutableList args = + ImmutableList.of("foo", "--test_env", "UNKNOWN=1234", "--test_env=HOME=dir"); + ImmutableList expArgs = + ImmutableList.of("foo", "--test_env", "UNKNOWN= ", "--test_env=HOME=dir"); + assertThat(OptionsUtils.scrubArgs(args)).isEqualTo(expArgs); + } + + @Test + public void scrubArgs_Full_SameArg() { + ImmutableList args = + ImmutableList.of("foo", "--test_arg=UNKNOWN=1234", "--test_arg=HOME=dir"); + ImmutableList expArgs = + ImmutableList.of("foo", "--test_arg=REDACTED", "--test_arg=REDACTED"); + assertThat(OptionsUtils.scrubArgs(args)).isEqualTo(expArgs); + } + + @Test + public void scrubArgs_Full_SeparateArg() { + ImmutableList args = + ImmutableList.of("foo", "--test_arg", "UNKNOWN=1234", "--test_arg=HOME=dir"); + ImmutableList expArgs = + ImmutableList.of("foo", "--test_arg", "REDACTED", "--test_arg=REDACTED"); + assertThat(OptionsUtils.scrubArgs(args)).isEqualTo(expArgs); + } + + @Test + public void scrubArgs_Residue_NotSensitive() { + ImmutableList args = + ImmutableList.of("test", "--", "abc", "--def"); + ImmutableList expArgs = + ImmutableList.of("test", "--", "REDACTED", "REDACTED"); + assertThat(OptionsUtils.scrubArgs(args)).isEqualTo(expArgs); + } + + @Test + public void scrubArgs_Residue_Sensitive() { + ImmutableList args = + ImmutableList.of("run", "--", "abc", "--def"); + ImmutableList expArgs = + ImmutableList.of("run", "--", "REDACTED", "REDACTED"); + assertThat(OptionsUtils.scrubArgs(args)).isEqualTo(expArgs); + } } diff --git a/src/test/java/com/google/devtools/build/lib/worker/BUILD b/src/test/java/com/google/devtools/build/lib/worker/BUILD index 8235102ec10124..ae9e0c192cb21b 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/BUILD +++ b/src/test/java/com/google/devtools/build/lib/worker/BUILD @@ -94,6 +94,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", + "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/metrics:ps_info_collector", diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java index b7b55eb054e6bd..4691d08549204f 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java @@ -51,6 +51,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.SpawnExecutingEvent; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.local.LocalEnvProvider; @@ -178,6 +179,7 @@ public void testExecInWorker_virtualInputs_doesntQueryInputFileCache() /* binTools= */ null, resourceManager, /* runfilesTreeUpdater= */ null, + new ExecutionOptions(), new WorkerOptions(), metricsCollector, new JavaClock()); @@ -578,6 +580,7 @@ private WorkerSpawnRunner createWorkerSpawnRunner(WorkerOptions workerOptions) { /* binTools= */ null, resourceManager, /* runfilesTreeUpdater= */ null, + new ExecutionOptions(), workerOptions, metricsCollector, new JavaClock()); diff --git a/src/test/java/com/google/devtools/common/options/TestOptions.java b/src/test/java/com/google/devtools/common/options/TestOptions.java index 2e95799f406e1d..157349b6df3020 100644 --- a/src/test/java/com/google/devtools/common/options/TestOptions.java +++ b/src/test/java/com/google/devtools/common/options/TestOptions.java @@ -248,4 +248,26 @@ public class TestOptions extends OptionsBase { documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, effectTags = {OptionEffectTag.NO_OP}) public String testDeprecated; + + /* + * Flags with potential secrets. + */ + + @Option( + name = "stub_env", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "null", + allowMultiple = true, + help = "a repeatable string-valued flag with its own unhelpful help text") + public List testEnvString; + + @Option( + name = "stub_inherit_env", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "null", + allowMultiple = true, + help = "a repeatable string-valued flag with its own unhelpful help text") + public List testInheritEnvString; } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 060b322574539d..42a6e08c33d941 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1084,6 +1084,48 @@ EOF expect_not_log "remote cache hit" } +function test_no_use_remote_cache_for_cache_unaware_spawns() { + mkdir -p a + cat > a/BUILD <<'EOF' +genrule( + name = "foo", + srcs = [], + outs = ["foo.txt"], + cmd = "echo \"$$RANDOM\" > \"$@\"", +) +EOF + + # Build the non-deterministic target to inject its output into the cache. + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + //a:foo >& $TEST_log || fail "Failed to build //a:foo" + cp bazel-bin/a/foo.txt before.txt + + # Do a second build with the default value of the + # --experimental_use_remote_cache_for_cache_unaware_spawns flag, which we + # expect it to be true (so the build should reuse the cached result and not + # be subject to non-determinism). + bazel clean + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --spawn_strategy=sandboxed,local \ + //a:foo >& $TEST_log || fail "Failed to build //a:foo" + diff -u before.txt bazel-bin/a/foo.txt \ + || fail "Non-deterministic action should have been reused from the cache" + + # Do a third build now, disallowing the use of the remote cache and thus + # observing the non-determinism. + bazel clean + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_use_remote_cache_for_cache_unaware_spawns=false \ + --spawn_strategy=sandboxed,local \ + //a:foo >& $TEST_log || fail "Failed to build //a:foo" + if diff -u before.txt bazel-bin/a/foo.txt; then + fail "Non-deterministic action should have been reexecuted, not cached" + fi +} + function test_tag_no_cache() { mkdir -p a cat > a/BUILD <<'EOF' diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh index d429985fac4816..d925d0d7dec751 100755 --- a/src/test/shell/integration/build_event_stream_test.sh +++ b/src/test/shell/integration/build_event_stream_test.sh @@ -51,6 +51,10 @@ EOF touch pkg/sourcefileA pkg/sourcefileB pkg/sourcefileC cat > pkg/BUILD <<'EOF' exports_files(["somesourcefile"]) +sh_test( + name = "true-bin", + srcs = ["true.sh"], +) sh_test( name = "true", srcs = ["true.sh"], @@ -1504,4 +1508,68 @@ EOF expect_log 'packages_loaded: 2' } +function test_secrets_in_env() { + THE_SECRET=secret1234 bazel test -k --build_event_text_file=$TEST_log pkg:true \ + || fail "bazel test failed" + + # The obvious: make sure the secrets don't leak in any way and check that we did + # not hit any unexpected corner cases. All other checks below are just "nice-to-have"s. + expect_not_log 'secret1234' + expect_not_log 'INVALID-OPTION-VALUE' + + # Check that some allowed environment variables are passed through. + expect_log_n "--client_env=PATH=${PATH}" 3 + expect_log_once "args.*--client_env=PATH=${PATH}" + expect_log_n "--client_env=USER=${USER}" 3 + expect_log_once "args.*--client_env=USER=${USER}" + + # Check argument rewrites in the unstructured and structured command lines. + expect_log_n '--client_env=THE_SECRET= ' 3 + + # Check that we also see the flag's value on its own. + expect_log_n 'THE_SECRET= ' 5 +} + +function test_secrets_in_options() { + bazel test -k --build_event_text_file=$TEST_log \ + --bes_header=NAME=the-secret-xyz \ + --test_env=HOME=/foo \ + --test_arg flat \ + --test_arg=PATH=/bar \ + --test_env=A_SECRET=secret1234 \ + --test_arg ANOTHER_SECRET=secret5678 \ + pkg:true \ + || fail "bazel test failed" + + # The obvious: make sure the secrets don't leak in any way and check that we did + # not hit any unexpected corner cases. All other checks below are just "nice-to-have"s. + expect_not_log 'secret1234' + expect_not_log 'secret5678' + expect_not_log 'the-secret-xyz' + expect_not_log 'INVALID-OPTION-VALUE' + + # Check argument rewrites throughout the log. + expect_log_n '--bes_header=NAME= ' 5 + expect_log_n '--test_env=HOME=/foo' 5 + expect_log_n '--test_arg REDACTED' 4 + expect_log_n '--test_env=A_SECRET= ' 5 + expect_log_n '--test_arg=REDACTED' 10 + + # Check argument rewrites in the unstructured and structured command lines. + expect_log_once "args.*--bes_header=NAME= " + expect_log_once "args.*--test_env=HOME=/foo" + expect_log "args.*\"REDACTED\"" + expect_not_log "args.*\"flat\"" + expect_log_once "args.*--test_arg=REDACTED" + + # Check the shell representation of the command line. + expect_log_once "options_description.*--bes_header=\\\'NAME= \\\'" + expect_log_once 'options_description.*--test_arg=REDACTED' + expect_log_once "options_description.*--test_env=\\\'A_SECRET= \\\'" + + # Check that we also see the flags' values on their own. + expect_log_n 'A_SECRET= ' 8 + expect_not_log 'ANOTHER_SECRET= ' +} + run_suite "Integration tests for the build event stream" diff --git a/user.bazelrc b/user.bazelrc new file mode 100644 index 00000000000000..707d23289c5351 --- /dev/null +++ b/user.bazelrc @@ -0,0 +1,12 @@ +startup --host_jvm_args=-Xmx4g + +build --action_env=PATH=/opt/rh/devtoolset-10/root/usr/bin:/opt/rh/rh-python36/root/usr/bin:/bin:/usr/bin:/usr/local/bin +build --action_env=LD_LIBRARY_PATH=/opt/rh/devtoolset-10/root/usr/lib64:/opt/rh/devtoolset-10/root/usr/lib:/opt/rh/devtoolset-10/root/usr/lib64/dyninst:/opt/rh/devtoolset-10/root/usr/lib/dyninst:/opt/rh/devtoolset-10/root/usr/lib64:/opt/rh/devtoolset-10/root/usr/lib +build --host_action_env=PATH=/opt/rh/devtoolset-10/root/usr/bin:/opt/rh/rh-python36/root/usr/bin:/bin:/usr/bin:/usr/local/bin +build --host_action_env=LD_LIBRARY_PATH=/opt/rh/devtoolset-10/root/usr/lib64:/opt/rh/devtoolset-10/root/usr/lib:/opt/rh/devtoolset-10/root/usr/lib64/dyninst:/opt/rh/devtoolset-10/root/usr/lib/dyninst:/opt/rh/devtoolset-10/root/usr/lib64:/opt/rh/devtoolset-10/root/usr/lib + +build --java_runtime_version=remotejdk_11 + +# Ensure a clean /tmp across actions rather than using /tmp from the real filesystem +# to workaround JVM crashes (https://github.com/bazelbuild/bazel/issues/3236). +build --sandbox_tmpfs_path=/tmp