aboutsummaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorGravatar ccalvarin <ccalvarin@google.com>2017-08-14 21:09:07 +0200
committerGravatar Irina Iancu <elenairina@google.com>2017-08-16 11:04:41 +0200
commit1cbe62a09b37f2db76e11ebb18fb46616076ef87 (patch)
treef02a66fc345507f3eb3ab3bbe404fdcb161cbabc
parentfc6412ca67a9d98c1b6b0c8b237c119e543bc266 (diff)
Send Bazel startup options to server.
Send the startup options tagged with their origin so that the server has correct information about the command line as the client received it. Removes the unconditional stderr printing of all bazelrc startup options in the bazel client. Instead, the startup options are sent to the server and the same informational printing is gated on the --announce_rc option. This avoids unconditional log spam to stderr early in startup. If the server is unreachable or there are errors parsing startup options, the message is still printed to stderr. Fixes https://github.com/bazelbuild/bazel/issues/2530. RELNOTES: --announce_rc now controls whether bazelrc startup options are printed to stderr. PiperOrigin-RevId: 165211007
-rw-r--r--WORKSPACE9
-rw-r--r--src/main/cpp/blaze.cc13
-rw-r--r--src/main/cpp/option_processor.cc56
-rw-r--r--src/main/cpp/option_processor.h15
-rw-r--r--src/main/cpp/startup_options.cc35
-rw-r--r--src/main/cpp/startup_options.h7
-rw-r--r--src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java80
-rw-r--r--src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java18
-rw-r--r--src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java19
-rw-r--r--src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java5
-rw-r--r--src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java16
-rw-r--r--src/main/java/com/google/devtools/build/lib/server/ServerCommand.java15
-rw-r--r--src/main/protobuf/command_server.proto15
-rw-r--r--src/test/cpp/BUILD2
-rw-r--r--src/test/cpp/option_processor_test.cc168
-rw-r--r--src/test/cpp/startup_options_test.cc73
-rwxr-xr-xsrc/test/shell/integration/loading_phase_tests.sh5
17 files changed, 491 insertions, 60 deletions
diff --git a/WORKSPACE b/WORKSPACE
index a86ba4e6b4..e9665ae9a2 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -119,3 +119,12 @@ http_archive(
strip_prefix = "bazel-toolchains-bccee4855c049d34bac481083b4c68e2fab8cc50",
sha256 = "3903fd93b96b42067e00b7973a2c16c34e761ad7a0b55e1557d408f352849e41",
)
+
+http_archive(
+ name = "com_googlesource_code_re2",
+ urls = [
+ "https://github.com/google/re2/archive/2017-08-01.tar.gz",
+ ],
+ strip_prefix = "re2-2017-08-01",
+ sha256 = "938723dc197125392698c5fcf41acb74877866ff140b81fd50b7314bf26f1636",
+)
diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc
index cca002de10..ec5b8f3676 100644
--- a/src/main/cpp/blaze.cc
+++ b/src/main/cpp/blaze.cc
@@ -761,6 +761,7 @@ static void StartServerAndConnect(const WorkspaceLayout *workspace_layout,
std::this_thread::sleep_until(next_attempt_time);
if (!server_startup->IsStillAlive()) {
+ globals->option_processor->PrintStartupOptionsProvenanceMessage();
fprintf(stderr,
"\nunexpected pipe read status: %s\n"
"Server presumed dead. Now printing '%s':\n",
@@ -1148,6 +1149,7 @@ static void ParseOptions(int argc, const char *argv[]) {
args, globals->workspace, globals->cwd, &error);
if (parse_exit_code != blaze_exit_code::SUCCESS) {
+ globals->option_processor->PrintStartupOptionsProvenanceMessage();
die(parse_exit_code, "%s", error.c_str());
}
globals->options = globals->option_processor->GetParsedStartupOptions();
@@ -1624,6 +1626,17 @@ unsigned int GrpcBlazeServer::Communicate() {
request.set_invocation_policy(globals->options->invocation_policy);
}
+ const StartupOptions *startup_options(
+ globals->option_processor->GetParsedStartupOptions());
+ for (const auto &startup_option :
+ startup_options->original_startup_options_) {
+ command_server::StartupOption *proto_option_field =
+ request.add_startup_options();
+ request.add_startup_options();
+ proto_option_field->set_source(startup_option.source);
+ proto_option_field->set_option(startup_option.value);
+ }
+
grpc::ClientContext context;
command_server::RunResponse response;
std::unique_ptr<grpc::ClientReader<command_server::RunResponse>> reader(
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc
index b3b4afa009..325e8ce881 100644
--- a/src/main/cpp/option_processor.cc
+++ b/src/main/cpp/option_processor.cc
@@ -86,7 +86,6 @@ blaze_exit_code::ExitCode OptionProcessor::RcFile::Parse(
// A '\' at the end of a line continues the line.
blaze_util::Replace("\\\r\n", "", &contents);
blaze_util::Replace("\\\n", "", &contents);
- vector<string> startup_options;
vector<string> lines = blaze_util::Split(contents, '\n');
for (string& line : lines) {
@@ -153,19 +152,10 @@ blaze_exit_code::ExitCode OptionProcessor::RcFile::Parse(
words_it++; // Advance past command.
for (; words_it != words.end(); words_it++) {
(*rcoptions)[command].push_back(RcOption(index, *words_it));
- if (command == "startup") {
- startup_options.push_back(*words_it);
- }
}
}
}
- if (!startup_options.empty()) {
- string startup_args;
- blaze_util::JoinStrings(startup_options, ' ', &startup_args);
- fprintf(stderr, "INFO: Reading 'startup' options from %s: %s\n",
- filename.c_str(), startup_args.c_str());
- }
return blaze_exit_code::SUCCESS;
}
@@ -389,8 +379,50 @@ blaze_exit_code::ExitCode OptionProcessor::ParseOptions(
return blaze_exit_code::SUCCESS;
}
+static void PrintStartupOptions(const std::string& source,
+ const std::vector<std::string>& options) {
+ if (!source.empty()) {
+ std::string startup_args;
+ blaze_util::JoinStrings(options, ' ', &startup_args);
+ fprintf(stderr, "INFO: Reading 'startup' options from %s: %s\n",
+ source.c_str(), startup_args.c_str());
+ }
+}
+
+void OptionProcessor::PrintStartupOptionsProvenanceMessage() const {
+ StartupOptions* parsed_startup_options = GetParsedStartupOptions();
+
+ // Print the startup flags in the order they are parsed, to keep the
+ // precendence clear. In order to minimize the number of lines of output in
+ // the terminal, group sequential flags by origin. Note that an rc file may
+ // turn up multiple times in this list, if, for example, it imports another
+ // rc file and contains startup options on either side of the import
+ // statement. This is done intentionally to make option priority clear.
+ std::string command_line_source;
+ std::string& most_recent_blazerc = command_line_source;
+ std::vector<std::string> accumulated_options;
+ for (auto flag : parsed_startup_options->original_startup_options_) {
+ if (flag.source == most_recent_blazerc) {
+ accumulated_options.push_back(flag.value);
+ } else {
+ PrintStartupOptions(most_recent_blazerc, accumulated_options);
+ // Start accumulating again.
+ accumulated_options.clear();
+ accumulated_options.push_back(flag.value);
+ most_recent_blazerc = flag.source;
+ }
+ }
+ // Don't forget to print out the last ones.
+ PrintStartupOptions(most_recent_blazerc, accumulated_options);
+}
+
blaze_exit_code::ExitCode OptionProcessor::ParseStartupOptions(
std::string *error) {
+ // Rc files can import other files at any point, and these imported rcs are
+ // expanded in place. The effective ordering of rc flags is stored in
+ // rcoptions_ and should be processed in that order. Here, we isolate just the
+ // startup options, but keep the file they came from attached for the
+ // option_sources tracking and for sending to the server.
std::vector<RcStartupFlag> rcstartup_flags;
auto iter = rcoptions_.find("startup");
@@ -482,8 +514,8 @@ std::vector<std::string> OptionProcessor::GetBlazercAndEnvCommandArgs(
result.push_back("--rc_source=" + blaze::ConvertPath(blazerc->Filename()));
}
- // Process RcOptions exept for the startup flags that are already parsed
- // by the client and shouldn't be handled by defult_overrides.
+ // Process RcOptions except for the startup flags that are already parsed
+ // by the client and shouldn't be included in the command args.
for (auto it = rcoptions.begin(); it != rcoptions.end(); ++it) {
if (it->first != "startup") {
for (const RcOption& rcoption : it->second) {
diff --git a/src/main/cpp/option_processor.h b/src/main/cpp/option_processor.h
index 7054ac1dc1..0fe45ba015 100644
--- a/src/main/cpp/option_processor.h
+++ b/src/main/cpp/option_processor.h
@@ -28,6 +28,10 @@ namespace blaze {
class WorkspaceLayout;
+// Broken down structure of the command line into logical components. The raw
+// arguments should not be referenced after this structure exists. This
+// breakdown should suffice to access the parts of the command line that the
+// client cares about, notably the binary and startup startup options.
struct CommandLine {
const std::string path_to_binary;
const std::vector<std::string> startup_args;
@@ -63,7 +67,7 @@ class OptionProcessor {
// result.path_to_binary = "output/bazel"
// result.startup_args = {"--foo", "--bar=42", "--bar=blah"}
// result.command = "build"
- // result.command_args = {"--some_flag", "value", ":mytarget"}
+ // result.command_args = {"--myflag", "value", ":mytarget"}
//
// Note that result.startup_args is guaranteed to contain only valid
// startup options (w.r.t. StartupOptions::IsUnary and
@@ -104,6 +108,11 @@ class OptionProcessor {
const std::string& workspace, std::string* user_blazerc_file,
std::string* error);
+ // Prints a message about the origin of startup options. This should be called
+ // if the server is not started or called, in case the options are related
+ // to the failure. Otherwise, the server will handle any required logging.
+ void PrintStartupOptionsProvenanceMessage() const;
+
private:
class RcOption {
public:
@@ -123,7 +132,7 @@ class OptionProcessor {
blaze_exit_code::ExitCode Parse(
const std::string& workspace, const WorkspaceLayout* workspace_layout,
std::vector<RcFile*>* rcfiles,
- std::map<std::string, std::vector<RcOption> >* rcoptions,
+ std::map<std::string, std::vector<RcOption>>* rcoptions,
std::string* error);
const std::string& Filename() const { return filename_; }
const int Index() const { return index_; }
@@ -133,7 +142,7 @@ class OptionProcessor {
const std::string& workspace, const std::string& filename,
const int index, const WorkspaceLayout* workspace_layout,
std::vector<RcFile*>* rcfiles,
- std::map<std::string, std::vector<RcOption> >* rcoptions,
+ std::map<std::string, std::vector<RcOption>>* rcoptions,
std::list<std::string>* import_stack, std::string* error);
std::string filename_;
diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc
index d54dbb2969..27c3d71848 100644
--- a/src/main/cpp/startup_options.cc
+++ b/src/main/cpp/startup_options.cc
@@ -90,7 +90,8 @@ StartupOptions::StartupOptions(const string &product_name,
invocation_policy(NULL),
client_debug(false),
java_logging_formatter(
- "com.google.devtools.build.lib.util.SingleLineFormatter") {
+ "com.google.devtools.build.lib.util.SingleLineFormatter"),
+ original_startup_options_(std::vector<RcStartupFlag>()) {
bool testing = !blaze::GetEnv("TEST_TMPDIR").empty();
if (testing) {
output_root = MakeAbsolute(blaze::GetEnv("TEST_TMPDIR"));
@@ -381,21 +382,31 @@ blaze_exit_code::ExitCode StartupOptions::ProcessArgs(
std::string *error) {
std::vector<RcStartupFlag>::size_type i = 0;
while (i < rcstartup_flags.size()) {
- bool is_space_separated;
- const bool is_last_elem = i == rcstartup_flags.size() - 1;
+ bool is_space_separated = false;
+ const std::string next_value =
+ (i == rcstartup_flags.size() - 1) ? "" : rcstartup_flags[i + 1].value;
const blaze_exit_code::ExitCode process_arg_exit_code =
- ProcessArg(rcstartup_flags[i].value,
- is_last_elem ? "" : rcstartup_flags[i + 1].value,
- rcstartup_flags[i].source,
- &is_space_separated,
- error);
- if (process_arg_exit_code != blaze_exit_code::SUCCESS) {
- return process_arg_exit_code;
- }
+ ProcessArg(rcstartup_flags[i].value, next_value,
+ rcstartup_flags[i].source, &is_space_separated, error);
+ // Store the provided option in --flag(=value)? form. Store these before
+ // propagating any error code, since we want to have the correct
+ // information for the output. The fact that the options aren't parseable
+ // doesn't matter for this step.
if (is_space_separated) {
+ const std::string combined_value =
+ rcstartup_flags[i].value + "=" + next_value;
+ original_startup_options_.push_back(
+ RcStartupFlag(rcstartup_flags[i].source, combined_value));
+ i += 2;
+ } else {
+ original_startup_options_.push_back(
+ RcStartupFlag(rcstartup_flags[i].source, rcstartup_flags[i].value));
i++;
}
- i++;
+
+ if (process_arg_exit_code != blaze_exit_code::SUCCESS) {
+ return process_arg_exit_code;
+ }
}
return blaze_exit_code::SUCCESS;
}
diff --git a/src/main/cpp/startup_options.h b/src/main/cpp/startup_options.h
index 81cfe787ac..9ddc98d48c 100644
--- a/src/main/cpp/startup_options.h
+++ b/src/main/cpp/startup_options.h
@@ -61,7 +61,8 @@ class UnaryStartupFlag : public StartupFlag {
const std::string name_;
};
-// A startup flag parsed from a bazelrc file.
+// A startup flag tagged with its origin, either an rc file or the empty
+// string for the ones specified in the command line.
// For instance, RcStartupFlag("somepath/.bazelrc", "--foo") is used to
// represent that the line "startup --foo" was found when parsing
// "somepath/.bazelrc".
@@ -267,6 +268,10 @@ class StartupOptions {
// Value of the java.util.logging.FileHandler.formatter Java property.
std::string java_logging_formatter;
+ // The startup options as received from the user and rc files, tagged with
+ // their origin. This is populated by ProcessArgs.
+ std::vector<RcStartupFlag> original_startup_options_;
+
protected:
// Constructor for subclasses only so that site-specific extensions of this
// class can override the product name. The product_name must be the
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index 453cd7c24b..54803ea944 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -65,6 +65,7 @@ import java.util.Collection;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
+import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import java.util.logging.Level;
@@ -261,9 +262,9 @@ public class BlazeCommandDispatcher {
}
/**
- * Executes a single command. Returns the Unix exit status for the Blaze
- * client process, or throws {@link ShutdownBlazeServerException} to
- * indicate that a command wants to shutdown the Blaze server.
+ * Executes a single command. Returns the Unix exit status for the Blaze client process, or throws
+ * {@link ShutdownBlazeServerException} to indicate that a command wants to shutdown the Blaze
+ * server.
*/
int exec(
InvocationPolicy invocationPolicy,
@@ -271,7 +272,9 @@ public class BlazeCommandDispatcher {
OutErr outErr,
LockingMode lockingMode,
String clientDescription,
- long firstContactTime) throws ShutdownBlazeServerException, InterruptedException {
+ long firstContactTime,
+ Optional<List<Pair<String, String>>> startupOptionsTaggedWithBazelRc)
+ throws ShutdownBlazeServerException, InterruptedException {
OriginalCommandLineEvent originalCommandLine = new OriginalCommandLineEvent(args);
Preconditions.checkNotNull(clientDescription);
if (args.isEmpty()) { // Default to help command if no arguments specified.
@@ -327,8 +330,16 @@ public class BlazeCommandDispatcher {
outErr.printErrLn("Server shut down " + shutdownReason);
return ExitCode.LOCAL_ENVIRONMENTAL_ERROR.getNumericExitCode();
}
- return execExclusively(originalCommandLine, invocationPolicy, args, outErr, firstContactTime,
- commandName, command, waitTimeInMs);
+ return execExclusively(
+ originalCommandLine,
+ invocationPolicy,
+ args,
+ outErr,
+ firstContactTime,
+ commandName,
+ command,
+ waitTimeInMs,
+ startupOptionsTaggedWithBazelRc);
} catch (ShutdownBlazeServerException e) {
shutdownReason = "explicitly by client " + currentClientDescription;
throw e;
@@ -348,7 +359,8 @@ public class BlazeCommandDispatcher {
long firstContactTime,
String commandName,
BlazeCommand command,
- long waitTimeInMs)
+ long waitTimeInMs,
+ Optional<List<Pair<String, String>>> startupOptionsTaggedWithBazelRc)
throws ShutdownBlazeServerException {
// Record the start time for the profiler. Do not put anything before this!
long execStartTimeNanos = runtime.getClock().nanoTime();
@@ -489,6 +501,39 @@ public class BlazeCommandDispatcher {
}
if (commonOptions.announceRcOptions) {
+ if (startupOptionsTaggedWithBazelRc.isPresent()) {
+ String lastBlazerc = "";
+ List<String> accumulatedStartupOptions = new ArrayList<>();
+ for (Pair<String, String> option : startupOptionsTaggedWithBazelRc.get()) {
+ // Do not include the command line options, marked by the empty string.
+ if (option.getFirst().isEmpty()) {
+ continue;
+ }
+
+ // If we've moved to a new blazerc in the list, print out the info from the last one,
+ // and clear the accumulated list.
+ if (!lastBlazerc.isEmpty() && !option.getFirst().equals(lastBlazerc)) {
+ String logMessage =
+ String.format(
+ "Reading 'startup' options from %s: %s",
+ lastBlazerc, String.join(", ", accumulatedStartupOptions));
+ reporter.handle(Event.info(logMessage));
+ accumulatedStartupOptions = new ArrayList<>();
+ }
+
+ lastBlazerc = option.getFirst();
+ accumulatedStartupOptions.add(option.getSecond());
+ }
+ // Print out the final blazerc-grouped list, if any startup options were provided by
+ // blazerc.
+ if (!lastBlazerc.isEmpty()) {
+ String logMessage =
+ String.format(
+ "Reading 'startup' options from %s: %s",
+ lastBlazerc, String.join(", ", accumulatedStartupOptions));
+ reporter.handle(Event.info(logMessage));
+ }
+ }
for (String note : rcfileNotes) {
reporter.handle(Event.info(note));
}
@@ -546,14 +591,21 @@ public class BlazeCommandDispatcher {
}
/**
- * For testing ONLY. Same as {@link #exec}, but automatically
- * uses the current time.
+ * For testing ONLY. Same as {@link #exec(InvocationPolicy, List, OutErr, LockingMode, String,
+ * long, Optional<List<Pair<String, String>>>)}, but automatically uses the current time.
*/
@VisibleForTesting
- public int exec(List<String> args, LockingMode lockingMode, String clientDescription,
- OutErr originalOutErr) throws ShutdownBlazeServerException, InterruptedException {
- return exec(InvocationPolicy.getDefaultInstance(), args, originalOutErr, LockingMode.ERROR_OUT,
- clientDescription, runtime.getClock().currentTimeMillis());
+ public int exec(
+ List<String> args, LockingMode lockingMode, String clientDescription, OutErr originalOutErr)
+ throws ShutdownBlazeServerException, InterruptedException {
+ return exec(
+ InvocationPolicy.getDefaultInstance(),
+ args,
+ originalOutErr,
+ LockingMode.ERROR_OUT,
+ clientDescription,
+ runtime.getClock().currentTimeMillis(),
+ Optional.empty() /* startupOptionBundles */);
}
/**
@@ -590,7 +642,7 @@ public class BlazeCommandDispatcher {
// handler, and later replayed.
ExitCode earlyExitCode =
checkCwdInWorkspace(workspace, commandAnnotation, commandName, eventHandler);
- if (earlyExitCode != ExitCode.SUCCESS) {
+ if (!earlyExitCode.equals(ExitCode.SUCCESS)) {
return earlyExitCode;
}
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
index d7bd5941d8..2d5e8f0510 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -63,6 +63,7 @@ import com.google.devtools.build.lib.util.CustomExitCodePublisher;
import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.util.LoggingUtil;
import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.ProcessUtils;
import com.google.devtools.build.lib.util.ThreadUtils;
@@ -95,6 +96,7 @@ import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
+import java.util.Optional;
import java.util.UUID;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
@@ -741,12 +743,24 @@ public final class BlazeRuntime {
return e.getExitCode().getNumericExitCode();
}
+ ImmutableList.Builder<Pair<String, String>> startupOptionsFromCommandLine =
+ ImmutableList.builder();
+ for (String option : commandLineOptions.getStartupArgs()) {
+ startupOptionsFromCommandLine.add(new Pair<>("", option));
+ }
+
BlazeCommandDispatcher dispatcher = new BlazeCommandDispatcher(runtime);
try {
LOG.info(getRequestLogString(commandLineOptions.getOtherArgs()));
- return dispatcher.exec(policy, commandLineOptions.getOtherArgs(), OutErr.SYSTEM_OUT_ERR,
- LockingMode.ERROR_OUT, "batch client", runtime.getClock().currentTimeMillis());
+ return dispatcher.exec(
+ policy,
+ commandLineOptions.getOtherArgs(),
+ OutErr.SYSTEM_OUT_ERR,
+ LockingMode.ERROR_OUT,
+ "batch client",
+ runtime.getClock().currentTimeMillis(),
+ Optional.of(startupOptionsFromCommandLine.build()));
} catch (BlazeCommandDispatcher.ShutdownBlazeServerException e) {
return e.getExitStatus();
} catch (InterruptedException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
index 3b70c8656a..3d2b46d310 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
@@ -13,12 +13,15 @@
// limitations under the License.
package com.google.devtools.build.lib.runtime;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.LockingMode;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
import com.google.devtools.build.lib.server.ServerCommand;
+import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.OutErr;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.List;
+import java.util.Optional;
import java.util.logging.Logger;
/**
@@ -44,14 +47,22 @@ public class CommandExecutor implements ServerCommand {
InvocationPolicy invocationPolicy,
List<String> args,
OutErr outErr,
- BlazeCommandDispatcher.LockingMode lockingMode,
+ LockingMode lockingMode,
String clientDescription,
- long firstContactTime) throws InterruptedException {
+ long firstContactTime,
+ Optional<List<Pair<String, String>>> startupOptionsTaggedWithBazelRc)
+ throws InterruptedException {
LOG.info(BlazeRuntime.getRequestLogString(args));
try {
- return dispatcher.exec(invocationPolicy, args, outErr, lockingMode, clientDescription,
- firstContactTime);
+ return dispatcher.exec(
+ invocationPolicy,
+ args,
+ outErr,
+ lockingMode,
+ clientDescription,
+ firstContactTime,
+ startupOptionsTaggedWithBazelRc);
} catch (BlazeCommandDispatcher.ShutdownBlazeServerException e) {
if (e.getCause() != null) {
StringWriter message = new StringWriter();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
index 9284db7688..d5131c29d7 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
@@ -190,9 +190,10 @@ public class CommonCommandOptions extends OptionsBase {
public boolean announceRcOptions;
/**
- * These are the actual default overrides. Each value is a pair of (command name, value).
+ * These are the actual default overrides. Each value is a tuple of (bazelrc index, command name,
+ * value). The blazerc index is a number used to find the blazerc in --rc_source's values.
*
- * <p>For example: "--default_override=build=--cpu=piii"
+ * <p>For example: "--default_override=rc:build=--cpu=piii"
*/
@Option(
name = "default_override",
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 6abef0e4ac..3f3b46b413 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
@@ -31,9 +31,11 @@ import com.google.devtools.build.lib.server.CommandProtos.PingRequest;
import com.google.devtools.build.lib.server.CommandProtos.PingResponse;
import com.google.devtools.build.lib.server.CommandProtos.RunRequest;
import com.google.devtools.build.lib.server.CommandProtos.RunResponse;
+import com.google.devtools.build.lib.server.CommandProtos.StartupOption;
import com.google.devtools.build.lib.util.BlazeClock;
import com.google.devtools.build.lib.util.Clock;
import com.google.devtools.build.lib.util.ExitCode;
+import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.ThreadUtils;
import com.google.devtools.build.lib.util.io.OutErr;
@@ -59,6 +61,7 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.UUID;
import java.util.concurrent.Exchanger;
import java.util.concurrent.ExecutionException;
@@ -809,6 +812,16 @@ public class GrpcServerImpl implements RPCServer {
String commandId;
int exitCode;
+ // TODO(b/63925394): This information needs to be passed to the GotOptionsEvent, which does not
+ // currently have the explicit startup options. See Improved Command Line Reporting design doc
+ // for details.
+ // Convert the startup options record to Java strings, source first.
+ ImmutableList.Builder<Pair<String, String>> startupOptions = ImmutableList.builder();
+ for (StartupOption option : request.getStartupOptionsList()) {
+ startupOptions.add(
+ new Pair<>(option.getSource().toString(CHARSET), option.getOption().toString(CHARSET)));
+ }
+
try (RunningCommand command = new RunningCommand()) {
commandId = command.id;
@@ -837,7 +850,8 @@ public class GrpcServerImpl implements RPCServer {
rpcOutErr,
request.getBlockForLock() ? LockingMode.WAIT : LockingMode.ERROR_OUT,
request.getClientDescription(),
- clock.currentTimeMillis());
+ clock.currentTimeMillis(),
+ Optional.of(startupOptions.build()));
} catch (OptionsParsingException e) {
rpcOutErr.printErrLn(e.getMessage());
exitCode = ExitCode.COMMAND_LINE_ERROR.getNumericExitCode();
diff --git a/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java b/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
index ef5f727bdb..f0574cdbaa 100644
--- a/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
@@ -15,8 +15,10 @@ package com.google.devtools.build.lib.server;
import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.OutErr;
import java.util.List;
+import java.util.Optional;
/**
* The {@link RPCServer} calls an arbitrary command implementing this
@@ -25,8 +27,13 @@ import java.util.List;
public interface ServerCommand {
/**
- * Executes the request, writing any output or error messages into err.
- * Returns 0 on success; any other value or exception indicates an error.
+ * Executes the request, writing any output or error messages into err. Returns 0 on success; any
+ * other value or exception indicates an error.
+ *
+ * @param startupOptionsTaggedWithBazelRc List of startup options in Pair(bazelRc, option) form.
+ * The empty string bazelRc is interpreted as the command line, and option should be in
+ * --[no]flag or --flag=value form. If we don't have access to this information (--batch),
+ * leave this parameter as Optional.empty().
*/
int exec(
InvocationPolicy policy,
@@ -34,7 +41,9 @@ public interface ServerCommand {
OutErr outErr,
BlazeCommandDispatcher.LockingMode lockingMode,
String clientDescription,
- long firstContactTime) throws InterruptedException;
+ long firstContactTime,
+ Optional<List<Pair<String, String>>> startupOptionsTaggedWithBazelRc)
+ throws InterruptedException;
/**
* Whether the server needs to be shut down.
diff --git a/src/main/protobuf/command_server.proto b/src/main/protobuf/command_server.proto
index 79f44b9930..b55a68817d 100644
--- a/src/main/protobuf/command_server.proto
+++ b/src/main/protobuf/command_server.proto
@@ -51,6 +51,21 @@ message RunRequest {
// between batch and server mode, so the parsing logic is only in the Java
// code.
string invocation_policy = 5;
+
+ // Startup arguments, in the order they were applied, tagged with where they
+ // came from. These options have already been parsed and already have had
+ // their effect. This information should only be used for logging.
+ repeated StartupOption startup_options = 6;
+}
+
+// Contains the a startup option with its source file. Uses bytes to preserve
+// the way the user inputted the arguments, like the args in RunRequest.
+message StartupOption {
+ // Startup option in --nullaryflag or --unaryflag=value form.
+ bytes option = 1;
+ // Where the option came from, such as an rc file or an empty string for the
+ // command line.
+ bytes source = 2;
}
// Contains metadata and result data for a command execution.
diff --git a/src/test/cpp/BUILD b/src/test/cpp/BUILD
index 20c36d552e..500974501d 100644
--- a/src/test/cpp/BUILD
+++ b/src/test/cpp/BUILD
@@ -37,12 +37,14 @@ cc_test(
name = "option_processor_test",
size = "small",
srcs = ["option_processor_test.cc"],
+ tags = ["requires_internet"],
deps = [
"//src/main/cpp:blaze_util",
"//src/main/cpp:option_processor",
"//src/main/cpp:workspace_layout",
"//src/main/cpp/util",
"//third_party:gtest",
+ "@com_googlesource_code_re2//:re2",
],
)
diff --git a/src/test/cpp/option_processor_test.cc b/src/test/cpp/option_processor_test.cc
index ab8ea59847..88fb52f42c 100644
--- a/src/test/cpp/option_processor_test.cc
+++ b/src/test/cpp/option_processor_test.cc
@@ -17,10 +17,11 @@
#include "src/main/cpp/blaze_util.h"
#include "src/main/cpp/blaze_util_platform.h"
#include "src/main/cpp/option_processor-internal.h"
-#include "src/main/cpp/workspace_layout.h"
#include "src/main/cpp/util/file.h"
#include "src/main/cpp/util/file_platform.h"
+#include "src/main/cpp/workspace_layout.h"
#include "gtest/gtest.h"
+#include "re2/re2.h"
namespace blaze {
@@ -167,8 +168,7 @@ TEST_F(OptionProcessorTest, CanParseDifferentStartupArgs) {
option_processor_->GetExplicitCommandArguments());
}
-
-TEST_F(OptionProcessorTest, CommandLineBlazercTest) {
+TEST_F(OptionProcessorTest, CommandLineBazelrcTest) {
const std::string cmdline_rc_path =
blaze_util::JoinPath(workspace_, "mybazelrc");
ASSERT_TRUE(blaze_util::MakeDirectories(
@@ -185,9 +185,25 @@ TEST_F(OptionProcessorTest, CommandLineBlazercTest) {
option_processor_->ParseOptions(args, workspace_, cwd_, &error))
<< error;
ASSERT_EQ(expected_error, error);
+
+ // Check that the startup option option provenance message prints the correct
+ // information for the incorrect flag, and does not print the command-line
+ // provided startup flags.
+ testing::internal::CaptureStderr();
+ option_processor_->PrintStartupOptionsProvenanceMessage();
+ const std::string& output = testing::internal::GetCapturedStderr();
+
+ EXPECT_PRED1(
+ [](std::string actualOutput) {
+ return RE2::FullMatch(
+ actualOutput,
+ "INFO: Reading 'startup' options from .*mybazelrc: "
+ "--foo\n");
+ },
+ output);
}
-TEST_F(OptionProcessorTest, NoMasterBlazercAndBlazercWorkTogetherCorrectly) {
+TEST_F(OptionProcessorTest, NoMasterBazelrcAndBazelrcWorkTogetherCorrectly) {
const std::string cmdline_rc_path =
blaze_util::JoinPath(workspace_, "mybazelrc");
ASSERT_TRUE(blaze_util::MakeDirectories(
@@ -211,6 +227,150 @@ TEST_F(OptionProcessorTest, NoMasterBlazercAndBlazercWorkTogetherCorrectly) {
<< error;
EXPECT_EQ(123, option_processor_->GetParsedStartupOptions()->max_idle_secs);
+
+ // Check that the startup option option provenance message prints the correct
+ // information for the provided rc, and prints nothing for the master bazelrc.
+ testing::internal::CaptureStderr();
+ option_processor_->PrintStartupOptionsProvenanceMessage();
+ const std::string& output = testing::internal::GetCapturedStderr();
+
+ EXPECT_PRED1(
+ [](std::string actualOutput) {
+ return RE2::FullMatch(
+ actualOutput,
+ "INFO: Reading 'startup' options from .*mybazelrc: "
+ "--max_idle_secs=123\n");
+ },
+ output);
+}
+
+TEST_F(OptionProcessorTest, MultipleStartupArgsInMasterBazelrcWorksCorrectly) {
+ // Add startup flags to the master bazelrc.
+ const std::string master_rc_path =
+ blaze_util::JoinPath(workspace_, "tools/bazel.rc");
+ ASSERT_TRUE(
+ blaze_util::MakeDirectories(blaze_util::Dirname(master_rc_path), 0755));
+ ASSERT_TRUE(blaze_util::WriteFile(
+ "startup --max_idle_secs=42\nstartup --io_nice_level=6", master_rc_path,
+ 0755));
+
+ const std::vector<std::string> args = {"bazel", "build"};
+ std::string error;
+ ASSERT_EQ(blaze_exit_code::SUCCESS,
+ option_processor_->ParseOptions(args, workspace_, cwd_, &error))
+ << error;
+
+ EXPECT_EQ(42, option_processor_->GetParsedStartupOptions()->max_idle_secs);
+ EXPECT_EQ(6, option_processor_->GetParsedStartupOptions()->io_nice_level);
+
+ // Check that the startup options get grouped together properly in the output
+ // message.
+ testing::internal::CaptureStderr();
+ option_processor_->PrintStartupOptionsProvenanceMessage();
+ const std::string& output = testing::internal::GetCapturedStderr();
+
+ EXPECT_PRED1(
+ [](std::string actualOutput) {
+ return RE2::FullMatch(
+ actualOutput,
+ "INFO: Reading 'startup' options from .*tools.*bazel.rc: "
+ "--max_idle_secs=42 --io_nice_level=6\n");
+ },
+ output);
+}
+
+TEST_F(OptionProcessorTest, CustomBazelrcOverridesMasterBazelrc) {
+ // Add startup flags to the master bazelrc.
+ const std::string master_rc_path =
+ blaze_util::JoinPath(workspace_, "tools/bazel.rc");
+ ASSERT_TRUE(
+ blaze_util::MakeDirectories(blaze_util::Dirname(master_rc_path), 0755));
+ ASSERT_TRUE(blaze_util::WriteFile(
+ "startup --max_idle_secs=42\nstartup --io_nice_level=6", master_rc_path,
+ 0755));
+
+ // Override one of the master bazelrc's flags in the custom bazelrc.
+ const std::string cmdline_rc_path =
+ blaze_util::JoinPath(workspace_, "mybazelrc");
+ ASSERT_TRUE(
+ blaze_util::MakeDirectories(blaze_util::Dirname(cmdline_rc_path), 0755));
+ ASSERT_TRUE(blaze_util::WriteFile("startup --max_idle_secs=123",
+ cmdline_rc_path, 0755));
+ const std::vector<std::string> args = {
+ "bazel", "--bazelrc=" + cmdline_rc_path, "build"};
+ std::string error;
+ ASSERT_EQ(blaze_exit_code::SUCCESS,
+ option_processor_->ParseOptions(args, workspace_, cwd_, &error))
+ << error;
+
+ EXPECT_EQ(123, option_processor_->GetParsedStartupOptions()->max_idle_secs);
+ EXPECT_EQ(6, option_processor_->GetParsedStartupOptions()->io_nice_level);
+
+ // Check that the options are reported in the correct order in the provenance
+ // message.
+ testing::internal::CaptureStderr();
+ option_processor_->PrintStartupOptionsProvenanceMessage();
+ const std::string& output = testing::internal::GetCapturedStderr();
+
+ EXPECT_PRED1(
+ [](std::string actualOutput) {
+ return RE2::FullMatch(
+ actualOutput,
+ "INFO: Reading 'startup' options from .*tools.*bazel.rc: "
+ "--max_idle_secs=42 --io_nice_level=6\n"
+ "INFO: Reading 'startup' options from .*mybazelrc: "
+ "--max_idle_secs=123\n");
+ },
+ output);
+}
+
+TEST_F(OptionProcessorTest, BazelRcImportsMaintainsFlagOrdering) {
+ // Override one of the master bazelrc's flags in the custom bazelrc.
+ const std::string imported_rc_path =
+ blaze_util::JoinPath(workspace_, "myimportedbazelrc");
+ ASSERT_TRUE(
+ blaze_util::MakeDirectories(blaze_util::Dirname(imported_rc_path), 0755));
+ ASSERT_TRUE(blaze_util::WriteFile(
+ "startup --max_idle_secs=123\nstartup --io_nice_level=4",
+ imported_rc_path, 0755));
+
+ // Add startup flags the imported bazelrc.
+ const std::string master_rc_path =
+ blaze_util::JoinPath(workspace_, "tools/bazel.rc");
+ ASSERT_TRUE(
+ blaze_util::MakeDirectories(blaze_util::Dirname(master_rc_path), 0755));
+ ASSERT_TRUE(blaze_util::WriteFile("startup --max_idle_secs=42\nimport " +
+ imported_rc_path +
+ "\nstartup --io_nice_level=6",
+ master_rc_path, 0755));
+
+ const std::vector<std::string> args = {"bazel", "build"};
+ std::string error;
+ ASSERT_EQ(blaze_exit_code::SUCCESS,
+ option_processor_->ParseOptions(args, workspace_, cwd_, &error))
+ << error;
+
+ EXPECT_EQ(123, option_processor_->GetParsedStartupOptions()->max_idle_secs);
+ EXPECT_EQ(6, option_processor_->GetParsedStartupOptions()->io_nice_level);
+
+ // Check that the options are reported in the correct order in the provenance
+ // message, the imported file between the two master flags
+ testing::internal::CaptureStderr();
+ option_processor_->PrintStartupOptionsProvenanceMessage();
+ const std::string& output = testing::internal::GetCapturedStderr();
+
+ EXPECT_PRED1(
+ [](std::string actualOutput) {
+ return RE2::FullMatch(
+ actualOutput,
+ "INFO: Reading 'startup' options from .*tools.*bazel.rc: "
+ "--max_idle_secs=42\n"
+ "INFO: Reading 'startup' options from .*myimportedbazelrc: "
+ "--max_idle_secs=123 --io_nice_level=4\n"
+ "INFO: Reading 'startup' options from .*tools.*bazel.rc: "
+ "--io_nice_level=6\n");
+ },
+ output);
}
TEST_F(OptionProcessorTest, SplitCommandLineWithEmptyArgs) {
diff --git a/src/test/cpp/startup_options_test.cc b/src/test/cpp/startup_options_test.cc
index 164bdc3c90..a4c52a2621 100644
--- a/src/test/cpp/startup_options_test.cc
+++ b/src/test/cpp/startup_options_test.cc
@@ -155,4 +155,77 @@ TEST_F(StartupOptionsTest, ValidStartupFlagsTest) {
SuccessfulIsUnaryTest("output_user_root");
}
+TEST_F(StartupOptionsTest, ProcessSpaceSeparatedArgsTest) {
+ std::string error;
+ const std::vector<RcStartupFlag> flags{
+ RcStartupFlag("somewhere", "--max_idle_secs"),
+ RcStartupFlag("somewhere", "42")};
+
+ const blaze_exit_code::ExitCode ec =
+ startup_options_->ProcessArgs(flags, &error);
+ ASSERT_EQ(blaze_exit_code::SUCCESS, ec)
+ << "ProcessArgs failed with error " << error;
+ EXPECT_EQ(42, startup_options_->max_idle_secs);
+
+ EXPECT_EQ("somewhere", startup_options_->original_startup_options_[0].source);
+ EXPECT_EQ("--max_idle_secs=42",
+ startup_options_->original_startup_options_[0].value);
+}
+
+TEST_F(StartupOptionsTest, ProcessEqualsSeparatedArgsTest) {
+ std::string error;
+ const std::vector<RcStartupFlag> flags{
+ RcStartupFlag("somewhere", "--max_idle_secs=36")};
+
+ const blaze_exit_code::ExitCode ec =
+ startup_options_->ProcessArgs(flags, &error);
+ ASSERT_EQ(ec, blaze_exit_code::SUCCESS)
+ << "ProcessArgs failed with error " << error;
+ EXPECT_EQ(36, startup_options_->max_idle_secs);
+
+ EXPECT_EQ("somewhere", startup_options_->original_startup_options_[0].source);
+ EXPECT_EQ("--max_idle_secs=36",
+ startup_options_->original_startup_options_[0].value);
+}
+
+TEST_F(StartupOptionsTest, ProcessIncorrectArgValueTest) {
+ std::string error;
+ const std::vector<RcStartupFlag> flags{
+ RcStartupFlag("somewhere", "--max_idle_secs=notANumber")};
+
+ const blaze_exit_code::ExitCode ec =
+ startup_options_->ProcessArgs(flags, &error);
+ ASSERT_EQ(blaze_exit_code::BAD_ARGV, ec)
+ << "ProcessArgs failed with the wrong error " << error;
+
+ // Even for a failing args processing step, expect the original value
+ // to be stored.
+ EXPECT_EQ("somewhere", startup_options_->original_startup_options_[0].source);
+ EXPECT_EQ("--max_idle_secs=notANumber",
+ startup_options_->original_startup_options_[0].value);
+}
+
+TEST_F(StartupOptionsTest, ProcessArgsWithMultipleArgstest) {
+ const std::vector<RcStartupFlag> flags{
+ RcStartupFlag("somewhere", "--max_idle_secs=36"),
+ RcStartupFlag("somewhereElse", "--nowrite_command_log")};
+
+ std::string error;
+ const blaze_exit_code::ExitCode ec =
+ startup_options_->ProcessArgs(flags, &error);
+ ASSERT_EQ(ec, blaze_exit_code::SUCCESS)
+ << "ProcessArgs failed with error " << error;
+ EXPECT_EQ(36, startup_options_->max_idle_secs);
+ EXPECT_FALSE(startup_options_->write_command_log);
+
+ EXPECT_EQ("somewhere", startup_options_->original_startup_options_[0].source);
+ EXPECT_EQ("--max_idle_secs=36",
+ startup_options_->original_startup_options_[0].value);
+
+ EXPECT_EQ("somewhereElse",
+ startup_options_->original_startup_options_[1].source);
+ EXPECT_EQ("--nowrite_command_log",
+ startup_options_->original_startup_options_[1].value);
+}
+
} // namespace blaze
diff --git a/src/test/shell/integration/loading_phase_tests.sh b/src/test/shell/integration/loading_phase_tests.sh
index 5c13b8fb18..33d3539b56 100755
--- a/src/test/shell/integration/loading_phase_tests.sh
+++ b/src/test/shell/integration/loading_phase_tests.sh
@@ -110,13 +110,14 @@ function test_bazelrc_option() {
cp ${bazelrc} ${new_workspace_dir}/.${PRODUCT_NAME}rc
echo "build --cpu=armeabi-v7a" >>.${PRODUCT_NAME}rc # default bazelrc
- $PATH_TO_BAZEL_BIN info >/dev/null 2>$TEST_log
+ $PATH_TO_BAZEL_BIN info --announce_rc >/dev/null 2>$TEST_log
expect_log "Reading.*$(pwd)/.${PRODUCT_NAME}rc:
.*--cpu=armeabi-v7a"
cp .${PRODUCT_NAME}rc foo
echo "build --cpu=armeabi-v7a" >>foo # non-default bazelrc
- $PATH_TO_BAZEL_BIN --${PRODUCT_NAME}rc=foo info >/dev/null 2>$TEST_log
+ $PATH_TO_BAZEL_BIN --${PRODUCT_NAME}rc=foo info --announce_rc >/dev/null \
+ 2>$TEST_log
expect_log "Reading.*$(pwd)/foo:
.*--cpu=armeabi-v7a"
}