Bazel client: Refactor startup options parsing to gracefully handle user errors, rather than crash.
blaze::OptionProcessor::SplitCommandLine is a nice standalone helper function, and so it's undesirable for blaze::StartupOptions::IsNullary to intentionally crash the host program via BAZEL_DIE.
Even ignoring that, option_processor.h's comment for SplitCommandLine says "... If the method fails then error will contain the cause..." and so transitive BAZEL_DIE usage makes this comment technically incorrect. Therefore, this change here can be motivated by a desire to make that comment correct.
RELNOTES: None
PiperOrigin-RevId: 300896347
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc
index 78df306..e44af24 100644
--- a/src/main/cpp/option_processor.cc
+++ b/src/main/cpp/option_processor.cc
@@ -88,10 +88,17 @@
vector<string>::size_type i = 1;
while (i < args.size() && IsArg(args[i])) {
string& current_arg = args[i];
+
+ bool is_nullary;
+ if (!startup_options_->MaybeCheckValidNullary(current_arg, &is_nullary,
+ error)) {
+ return nullptr;
+ }
+
// If the current argument is a valid nullary startup option such as
// --master_bazelrc or --nomaster_bazelrc proceed to examine the next
// argument.
- if (startup_options_->IsNullary(current_arg)) {
+ if (is_nullary) {
startup_args.push_back(current_arg);
i++;
} else if (startup_options_->IsUnary(current_arg)) {
diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc
index bf93cdd..6bd300d 100644
--- a/src/main/cpp/startup_options.cc
+++ b/src/main/cpp/startup_options.cc
@@ -182,21 +182,24 @@
}
}
-bool StartupOptions::IsNullary(const string &arg) const {
+bool StartupOptions::MaybeCheckValidNullary(const string &arg, bool *result,
+ std::string *error) const {
std::string::size_type i = arg.find_first_of('=');
if (i == std::string::npos) {
- return all_nullary_startup_flags_.find(arg) !=
- all_nullary_startup_flags_.end();
- } else {
- std::string f = arg.substr(0, i);
- if (all_nullary_startup_flags_.find(f) !=
- all_nullary_startup_flags_.end()) {
- BAZEL_DIE(blaze_exit_code::BAD_ARGV)
- << "In argument '" << arg << "': option '" << f
- << "' does not take a value.";
- }
- return false;
+ *result = all_nullary_startup_flags_.find(arg) !=
+ all_nullary_startup_flags_.end();
+ return true;
}
+ std::string f = arg.substr(0, i);
+ if (all_nullary_startup_flags_.find(f) == all_nullary_startup_flags_.end()) {
+ *result = false;
+ return true;
+ }
+
+ blaze_util::StringPrintf(
+ error, "In argument '%s': option '%s' does not take a value.",
+ arg.c_str(), f.c_str());
+ return false;
}
void StartupOptions::AddExtraOptions(vector<string> *result) const {
@@ -217,7 +220,13 @@
const char* next_arg = next_argstr.empty() ? NULL : next_argstr.c_str();
const char* value = NULL;
- if (IsNullary(argstr)) {
+ bool is_nullary;
+ if (!MaybeCheckValidNullary(argstr, &is_nullary, error)) {
+ *is_space_separated = false;
+ return blaze_exit_code::BAD_ARGV;
+ }
+
+ if (is_nullary) {
// 'enabled' is true if 'argstr' is "--foo", and false if it's "--nofoo".
bool enabled = (argstr.compare(0, 4, "--no") != 0);
if (no_rc_nullary_startup_flags_.find(argstr) !=
diff --git a/src/main/cpp/startup_options.h b/src/main/cpp/startup_options.h
index adabe74..efe99e8 100644
--- a/src/main/cpp/startup_options.h
+++ b/src/main/cpp/startup_options.h
@@ -106,9 +106,21 @@
const blaze_util::Path &server_javabase, std::vector<std::string> *result,
const std::vector<std::string> &user_options, std::string *error) const;
- // Checks whether the argument is a valid nullary option.
- // E.g. --master_bazelrc, --nomaster_bazelrc.
- bool IsNullary(const std::string& arg) const;
+ // Checks whether "arg" is a valid nullary option (e.g. "--master_bazelrc" or
+ // "--nomaster_bazelrc").
+ //
+ // Returns true, if "arg" looks like either a valid nullary option or a
+ // potentially valid unary option. In this case, "result" will be populated
+ // with true iff "arg" is definitely a valid nullary option.
+ //
+ // Returns false, if "arg" looks like an attempt to pass a value to nullary
+ // option (e.g. "--nullary_option=idontknowwhatimdoing"). In this case,
+ // "error" will be populated with a user-friendly error message.
+ //
+ // Therefore, callers of this function should look at the return value and
+ // then either look at "result" (on true) or "error" (on false).
+ bool MaybeCheckValidNullary(const std::string &arg, bool *result,
+ std::string *error) const;
// Checks whether the argument is a valid unary option.
// E.g. --blazerc=foo, --blazerc foo.
diff --git a/src/test/cpp/bazel_startup_options_test.cc b/src/test/cpp/bazel_startup_options_test.cc
index 2a07b80..5fc432a 100644
--- a/src/test/cpp/bazel_startup_options_test.cc
+++ b/src/test/cpp/bazel_startup_options_test.cc
@@ -66,8 +66,21 @@
}
TEST_F(BazelStartupOptionsTest, EmptyFlagsAreInvalid) {
- EXPECT_FALSE(startup_options_->IsNullary(""));
- EXPECT_FALSE(startup_options_->IsNullary("--"));
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(startup_options_->MaybeCheckValidNullary("", &result, &error));
+ EXPECT_FALSE(result);
+ }
+
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(
+ startup_options_->MaybeCheckValidNullary("--", &result, &error));
+ EXPECT_FALSE(result);
+ }
+
EXPECT_FALSE(startup_options_->IsUnary(""));
EXPECT_FALSE(startup_options_->IsUnary("--"));
}
@@ -80,23 +93,23 @@
// member that knows the Google-internal procedure for adding/deprecating
// startup flags.
const StartupOptions* options = startup_options_.get();
- ExpectIsNullaryOption(options, "batch");
- ExpectIsNullaryOption(options, "batch_cpu_scheduling");
- ExpectIsNullaryOption(options, "block_for_lock");
- ExpectIsNullaryOption(options, "client_debug");
- ExpectIsNullaryOption(options, "deep_execroot");
- ExpectIsNullaryOption(options, "experimental_oom_more_eagerly");
- ExpectIsNullaryOption(options, "fatal_event_bus_exceptions");
- ExpectIsNullaryOption(options, "home_rc");
- ExpectIsNullaryOption(options, "host_jvm_debug");
- ExpectIsNullaryOption(options, "ignore_all_rc_files");
- ExpectIsNullaryOption(options, "incompatible_enable_execution_transition");
- ExpectIsNullaryOption(options, "master_bazelrc");
- ExpectIsNullaryOption(options, "shutdown_on_low_sys_mem");
- ExpectIsNullaryOption(options, "system_rc");
- ExpectIsNullaryOption(options, "watchfs");
- ExpectIsNullaryOption(options, "workspace_rc");
- ExpectIsNullaryOption(options, "write_command_log");
+ ExpectValidNullaryOption(options, "batch");
+ ExpectValidNullaryOption(options, "batch_cpu_scheduling");
+ ExpectValidNullaryOption(options, "block_for_lock");
+ ExpectValidNullaryOption(options, "client_debug");
+ ExpectValidNullaryOption(options, "deep_execroot");
+ ExpectValidNullaryOption(options, "experimental_oom_more_eagerly");
+ ExpectValidNullaryOption(options, "fatal_event_bus_exceptions");
+ ExpectValidNullaryOption(options, "home_rc");
+ ExpectValidNullaryOption(options, "host_jvm_debug");
+ ExpectValidNullaryOption(options, "ignore_all_rc_files");
+ ExpectValidNullaryOption(options, "incompatible_enable_execution_transition");
+ ExpectValidNullaryOption(options, "master_bazelrc");
+ ExpectValidNullaryOption(options, "shutdown_on_low_sys_mem");
+ ExpectValidNullaryOption(options, "system_rc");
+ ExpectValidNullaryOption(options, "watchfs");
+ ExpectValidNullaryOption(options, "workspace_rc");
+ ExpectValidNullaryOption(options, "write_command_log");
ExpectIsUnaryOption(options, "bazelrc");
ExpectIsUnaryOption(options, "command_port");
ExpectIsUnaryOption(options, "connect_timeout_secs");
@@ -115,9 +128,24 @@
}
TEST_F(BazelStartupOptionsTest, BlazercFlagsAreNotAccepted) {
- EXPECT_FALSE(startup_options_->IsNullary("--master_blazerc"));
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(startup_options_->MaybeCheckValidNullary("--master_blazerc",
+ &result, &error));
+ EXPECT_FALSE(result);
+ }
+
EXPECT_FALSE(startup_options_->IsUnary("--master_blazerc"));
- EXPECT_FALSE(startup_options_->IsNullary("--blazerc"));
+
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(
+ startup_options_->MaybeCheckValidNullary("--blazerc", &result, &error));
+ EXPECT_FALSE(result);
+ }
+
EXPECT_FALSE(startup_options_->IsUnary("--blazerc"));
}
diff --git a/src/test/cpp/startup_options_test.cc b/src/test/cpp/startup_options_test.cc
index 1968fda..a5bec67 100644
--- a/src/test/cpp/startup_options_test.cc
+++ b/src/test/cpp/startup_options_test.cc
@@ -84,7 +84,7 @@
TEST_F(StartupOptionsTest, JavaLoggingOptions) {
ASSERT_EQ("com.google.devtools.build.lib.util.SingleLineFormatter",
- startup_options_->java_logging_formatter);
+ startup_options_->java_logging_formatter);
}
// TODO(bazel-team): remove the ifdef guard once the implementation of
@@ -108,8 +108,21 @@
#endif // __linux
TEST_F(StartupOptionsTest, EmptyFlagsAreInvalidTest) {
- EXPECT_FALSE(startup_options_->IsNullary(""));
- EXPECT_FALSE(startup_options_->IsNullary("--"));
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(startup_options_->MaybeCheckValidNullary("", &result, &error));
+ EXPECT_FALSE(result);
+ }
+
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(
+ startup_options_->MaybeCheckValidNullary("--", &result, &error));
+ EXPECT_FALSE(result);
+ }
+
EXPECT_FALSE(startup_options_->IsUnary(""));
EXPECT_FALSE(startup_options_->IsUnary("--"));
}
diff --git a/src/test/cpp/test_util.cc b/src/test/cpp/test_util.cc
index e39c96c..6015d88 100644
--- a/src/test/cpp/test_util.cc
+++ b/src/test/cpp/test_util.cc
@@ -22,22 +22,51 @@
// but also that they are parsed. StartupOptions* options would need to be
// non-const to call ProcessArgs and test that the value is recognized by the
// command line.
-void ExpectIsNullaryOption(const StartupOptions* options,
- const std::string& flag_name) {
- EXPECT_TRUE(options->IsNullary("--" + flag_name));
- EXPECT_TRUE(options->IsNullary("--no" + flag_name));
+void ExpectValidNullaryOption(const StartupOptions* options,
+ const std::string& flag_name) {
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(
+ options->MaybeCheckValidNullary("--" + flag_name, &result, &error));
+ EXPECT_TRUE(result);
+ }
- EXPECT_FALSE(options->IsNullary("--" + flag_name + "__invalid"));
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(
+ options->MaybeCheckValidNullary("--no" + flag_name, &result, &error));
+ EXPECT_TRUE(result);
+ }
- EXPECT_DEATH(options->IsNullary("--" + flag_name + "=foo"),
- ("In argument '--" + flag_name + "=foo': option '--" +
- flag_name + "' does not take a value")
- .c_str());
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(options->MaybeCheckValidNullary("--" + flag_name + "__invalid",
+ &result, &error));
+ EXPECT_FALSE(result);
+ }
- EXPECT_DEATH(options->IsNullary("--no" + flag_name + "=foo"),
- ("In argument '--no" + flag_name + "=foo': option '--no" +
- flag_name + "' does not take a value")
- .c_str());
+ {
+ bool result;
+ std::string error;
+ EXPECT_FALSE(options->MaybeCheckValidNullary("--" + flag_name + "=foo",
+ &result, &error));
+ EXPECT_EQ("In argument '--" + flag_name + "=foo': option '--" + flag_name +
+ "' does not take a value.",
+ error);
+ }
+
+ {
+ bool result;
+ std::string error;
+ EXPECT_FALSE(options->MaybeCheckValidNullary("--no" + flag_name + "=foo",
+ &result, &error));
+ EXPECT_EQ("In argument '--no" + flag_name + "=foo': option '--no" +
+ flag_name + "' does not take a value.",
+ error);
+ }
EXPECT_FALSE(options->IsUnary("--" + flag_name));
EXPECT_FALSE(options->IsUnary("--no" + flag_name));
@@ -50,8 +79,22 @@
EXPECT_TRUE(options->IsUnary("--" + flag_name + "=foo"));
EXPECT_FALSE(options->IsUnary("--" + flag_name + "__invalid"));
- EXPECT_FALSE(options->IsNullary("--" + flag_name));
- EXPECT_FALSE(options->IsNullary("--no" + flag_name));
+
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(
+ options->MaybeCheckValidNullary("--" + flag_name, &result, &error));
+ EXPECT_FALSE(result);
+ }
+
+ {
+ bool result;
+ std::string error;
+ EXPECT_TRUE(
+ options->MaybeCheckValidNullary("--no" + flag_name, &result, &error));
+ EXPECT_FALSE(result);
+ }
}
void ParseStartupOptionsAndExpectWarning(
diff --git a/src/test/cpp/test_util.h b/src/test/cpp/test_util.h
index 6c23c2e..cdbfcf2 100644
--- a/src/test/cpp/test_util.h
+++ b/src/test/cpp/test_util.h
@@ -19,8 +19,8 @@
namespace blaze {
-void ExpectIsNullaryOption(const StartupOptions* options,
- const std::string& flag_name);
+void ExpectValidNullaryOption(const StartupOptions* options,
+ const std::string& flag_name);
void ExpectIsUnaryOption(const StartupOptions* options,
const std::string& flag_name);
void ParseStartupOptionsAndExpectWarning(
