diff options
| -rw-r--r-- | src/main/cpp/blaze.cc | 93 | ||||
| -rw-r--r-- | src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java | 66 | ||||
| -rwxr-xr-x | src/test/shell/integration/client_test.sh | 11 |
3 files changed, 103 insertions, 67 deletions
diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc index 9c72fb55e8..f637dfa355 100644 --- a/src/main/cpp/blaze.cc +++ b/src/main/cpp/blaze.cc @@ -519,16 +519,22 @@ static vector<string> GetArgumentArray( } if (globals->options->oom_more_eagerly) { result.push_back("--experimental_oom_more_eagerly"); + } else { + result.push_back("--noexperimental_oom_more_eagerly"); } result.push_back("--experimental_oom_more_eagerly_threshold=" + ToString(globals->options->oom_more_eagerly_threshold)); - if (!globals->options->write_command_log) { + if (globals->options->write_command_log) { + result.push_back("--write_command_log"); + } else { result.push_back("--nowrite_command_log"); } if (globals->options->watchfs) { result.push_back("--watchfs"); + } else { + result.push_back("--nowatchfs"); } if (globals->options->fatal_event_bus_exceptions) { result.push_back("--fatal_event_bus_exceptions"); @@ -536,25 +542,25 @@ static vector<string> GetArgumentArray( result.push_back("--nofatal_event_bus_exceptions"); } - // We use this syntax so that the logic in ServerNeedsToBeKilled() that + // We use this syntax so that the logic in AreStartupOptionsDifferent() that // decides whether the server needs killing is simpler. This is parsed by the // Java code where --noclient_debug and --client_debug=false are equivalent. // Note that --client_debug false (separated by space) won't work either, - // because the logic in ServerNeedsToBeKilled() assumes that every argument - // is in the --arg=value form. + // because the logic in AreStartupOptionsDifferent() assumes that every + // argument is in the --arg=value form. if (globals->options->client_debug) { result.push_back("--client_debug=true"); } else { result.push_back("--client_debug=false"); } + // These flags are passed to the java process only for Blaze reporting + // purposes; the real interpretation of the jvm flags occurs when we set up + // the java command line. if (!globals->options->GetExplicitHostJavabase().empty()) { result.push_back("--host_javabase=" + globals->options->GetExplicitHostJavabase()); } - - // This is only for Blaze reporting purposes; the real interpretation of the - // jvm flags occurs when we set up the java command line. if (globals->options->host_jvm_debug) { result.push_back("--host_jvm_debug"); } @@ -1066,41 +1072,48 @@ static void ExtractData(const string &self_path) { } } -const char *volatile_startup_options[] = { - "--option_sources=", - "--max_idle_secs=", - "--connect_timeout_secs=", - "--client_debug=", - NULL, -}; +// TODO(ccalvarin) when --batch is gone and the startup_options field in the +// gRPC message is always set, there is no reason for client options that are +// not used at server startup to be part of the startup command line. The server +// command line difference logic can be simplified then. +const std::vector<string> volatile_startup_options = { + "--option_sources=", "--max_idle_secs=", "--connect_timeout_secs=", + "--client_debug="}; // Returns true if the server needs to be restarted to accommodate changes // between the two argument lists. -static bool ServerNeedsToBeKilled(const vector<string> &args1, - const vector<string> &args2) { +static bool AreStartupOptionsDifferent( + const vector<string> &running_server_args, + const vector<string> &requested_args) { // We need not worry about one side missing an argument and the other side - // having the default value, since this command line is already the - // canonicalized one that always contains every switch (with default values - // if it was not present on the real command line). Same applies for argument - // ordering. - if (args1.size() != args2.size()) { + // having the default value, since this command line is the canonical one for + // this version of Bazel: either the default value is listed explicitly or it + // is not, but this has nothing to do with the user's command line: it is + // defined by GetArgumentArray(). Same applies for argument ordering. + if (running_server_args.size() != requested_args.size()) { + BAZEL_LOG(INFO) << "The new command line has a different length from the " + "running server's."; return true; } - for (int i = 0; i < args1.size(); i++) { - bool option_volatile = false; - for (const char **candidate = volatile_startup_options; *candidate != NULL; - candidate++) { - string candidate_string(*candidate); - if (args1[i].substr(0, candidate_string.size()) == candidate_string && - args2[i].substr(0, candidate_string.size()) == candidate_string) { - option_volatile = true; - break; + for (int i = 0; i < running_server_args.size(); i++) { + if (running_server_args[i] != requested_args[i]) { + bool option_volatile = false; + // Only check if this is a volatile option for dissimilar args. + for (const string &candidate : volatile_startup_options) { + if (running_server_args[i].substr(0, candidate.size()) == candidate && + requested_args[i].substr(0, candidate.size()) == candidate) { + option_volatile = true; + break; + } + } + if (!option_volatile) { + BAZEL_LOG(INFO) + << "A difference was found between the command lines at position " + << i << ": the running server has option " << running_server_args[i] + << ", and requested option is: " << requested_args[i]; + return true; } - } - - if (!option_volatile && args1[i] != args2[i]) { - return true; } } @@ -1116,19 +1129,20 @@ static void KillRunningServerIfDifferentStartupOptions( string cmdline_path = blaze_util::JoinPath(globals->options->output_base, "server/cmdline"); - string joined_arguments; + string old_joined_arguments; // No, /proc/$PID/cmdline does not work, because it is limited to 4K. Even // worse, its behavior differs slightly between kernels (in some, when longer // command lines are truncated, the last 4 bytes are replaced with // "..." + NUL. - blaze_util::ReadFile(cmdline_path, &joined_arguments); - vector<string> arguments = blaze_util::Split(joined_arguments, '\0'); + blaze_util::ReadFile(cmdline_path, &old_joined_arguments); + vector<string> old_arguments = blaze_util::Split(old_joined_arguments, '\0'); // These strings contain null-separated command line arguments. If they are // the same, the server can stay alive, otherwise, it needs shuffle off this // mortal coil. - if (ServerNeedsToBeKilled(arguments, GetArgumentArray(workspace_layout))) { + if (AreStartupOptionsDifferent(old_arguments, + GetArgumentArray(workspace_layout))) { globals->restart_reason = NEW_OPTIONS; BAZEL_LOG(WARNING) << "Running " << globals->options->product_name << " server needs to be killed, because the startup " @@ -1155,6 +1169,9 @@ static void EnsureCorrectRunningVersion(BlazeServer *server) { if (!ok || !CompareAbsolutePaths(prev_installation, globals->options->install_base)) { if (server->Connected()) { + BAZEL_LOG(INFO) + << "Killing running server because it is using another version of " + << globals->options->product_name; server->KillRunningServer(); globals->restart_reason = NEW_VERSION; } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java index aaa7f05806..e5a8069a88 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java @@ -25,14 +25,18 @@ import com.google.devtools.common.options.OptionsBase; import java.util.Map; /** - * Options that will be evaluated by the blaze client startup code and passed - * to the blaze server upon startup. + * Options that will be evaluated by the blaze client startup code and passed to the blaze server + * upon startup. * - * <h4>IMPORTANT</h4> These options and their defaults must be kept in sync with those in the - * source of the launcher. The latter define the actual default values; this class exists only to - * provide the help message, which displays the default values. + * <h4>IMPORTANT</h4> * - * The same relationship holds between {@link HostJvmStartupOptions} and the launcher. + * These options and their defaults must be kept in sync with those in the source of the launcher. + * The latter define the actual default values, most startup options are passed every time, + * regardless of whether a value was set explicitly or if the default was used. Some options are + * omitted by default, though this should only be true for options where "omitted" is a distinct + * value. + * + * <p>The same relationship holds between {@link HostJvmStartupOptions} and the launcher. */ public class BlazeServerStartupOptions extends OptionsBase { /** @@ -79,7 +83,7 @@ public class BlazeServerStartupOptions extends OptionsBase { */ @Option( name = "install_base", - defaultValue = "", // NOTE: purely decorative! See class docstring. + defaultValue = "", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.LOSES_INCREMENTAL_STATE}, metadataTags = {OptionMetadataTag.HIDDEN}, @@ -94,7 +98,7 @@ public class BlazeServerStartupOptions extends OptionsBase { */ @Option( name = "install_md5", - defaultValue = "", // NOTE: purely decorative! See class docstring. + defaultValue = "", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE, OptionEffectTag.BAZEL_MONITORING}, metadataTags = {OptionMetadataTag.HIDDEN}, @@ -109,7 +113,7 @@ public class BlazeServerStartupOptions extends OptionsBase { */ @Option( name = "output_base", - defaultValue = "null", // NOTE: purely decorative! See class docstring. + defaultValue = "null", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOSES_INCREMENTAL_STATE}, converter = OptionsUtils.PathFragmentConverter.class, @@ -129,7 +133,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "output_user_root", - defaultValue = "null", // NOTE: purely decorative! See class docstring. + defaultValue = "null", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOSES_INCREMENTAL_STATE}, converter = OptionsUtils.PathFragmentConverter.class, @@ -148,7 +152,7 @@ public class BlazeServerStartupOptions extends OptionsBase { */ @Option( name = "server_jvm_out", - defaultValue = "null", // NOTE: purely decorative! See class docstring. + defaultValue = "null", documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOSES_INCREMENTAL_STATE}, converter = OptionsUtils.PathFragmentConverter.class, @@ -161,7 +165,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "workspace_directory", - defaultValue = "", + defaultValue = "", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.LOSES_INCREMENTAL_STATE}, metadataTags = {OptionMetadataTag.HIDDEN}, @@ -174,7 +178,9 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "max_idle_secs", - defaultValue = "" + (3 * 3600), // NOTE: purely decorative! See class docstring. + // NOTE: default value only used for documentation, value is always passed by the client when + // not in --batch mode. + defaultValue = "" + (3 * 3600), documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.EAGERNESS_TO_EXIT, OptionEffectTag.LOSES_INCREMENTAL_STATE}, valueHelp = "<integer>", @@ -186,7 +192,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "batch", - defaultValue = "false", // NOTE: purely decorative! See class docstring. + defaultValue = "false", documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = { OptionEffectTag.LOSES_INCREMENTAL_STATE, @@ -200,7 +206,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "deep_execroot", - defaultValue = "true", // NOTE: purely decorative! See class docstring. + defaultValue = "true", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE, OptionEffectTag.EXECUTION}, help = @@ -211,7 +217,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "experimental_oom_more_eagerly", - defaultValue = "false", // NOTE: purely decorative! See class docstring. + defaultValue = "false", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE, OptionEffectTag.EAGERNESS_TO_EXIT}, help = @@ -224,7 +230,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "experimental_oom_more_eagerly_threshold", - defaultValue = "100", // NOTE: purely decorative! See class docstring. + defaultValue = "100", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE, OptionEffectTag.EAGERNESS_TO_EXIT}, help = @@ -236,7 +242,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "block_for_lock", - defaultValue = "true", // NOTE: purely decorative! See class docstring. + defaultValue = "true", // NOTE: only for documentation, value never passed to the server. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.EAGERNESS_TO_EXIT}, help = @@ -247,7 +253,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "io_nice_level", - defaultValue = "-1", // NOTE: purely decorative! + defaultValue = "-1", // NOTE: only for documentation, value never passed to the server. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, valueHelp = "{-1,0,1,2,3,4,5,6,7}", @@ -261,7 +267,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "batch_cpu_scheduling", - defaultValue = "false", // NOTE: purely decorative! + defaultValue = "false", // NOTE: only for documentation, value never passed to the server. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, help = @@ -273,7 +279,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "blazerc", - defaultValue = "null", // NOTE: purely decorative! + defaultValue = "null", // NOTE: purely decorative, rc files are read by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.CHANGES_INPUTS}, valueHelp = "<path>", @@ -288,7 +294,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "master_blazerc", - defaultValue = "true", // NOTE: purely decorative! + defaultValue = "true", // NOTE: purely decorative, rc files are read by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.CHANGES_INPUTS}, help = "If this option is false, the master %{product}rc next to the binary is not read." @@ -297,7 +303,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "fatal_event_bus_exceptions", - defaultValue = "false", // NOTE: purely decorative! + defaultValue = "false", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.EAGERNESS_TO_EXIT, OptionEffectTag.LOSES_INCREMENTAL_STATE}, help = "Whether or not to exit if an exception is thrown by an internal EventBus handler." @@ -319,7 +325,7 @@ public class BlazeServerStartupOptions extends OptionsBase { // turn this into a non-startup option. @Option( name = "watchfs", - defaultValue = "false", + defaultValue = "false", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.UNKNOWN}, metadataTags = OptionMetadataTag.DEPRECATED, @@ -329,6 +335,8 @@ public class BlazeServerStartupOptions extends OptionsBase { ) public boolean watchFS; + // This option is only passed in --batch mode. The value is otherwise passed as part of the + // server request. @Option( name = "invocation_policy", defaultValue = "", @@ -355,7 +363,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "product_name", - defaultValue = "bazel", // NOTE: purely decorative! + defaultValue = "bazel", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = { OptionEffectTag.LOSES_INCREMENTAL_STATE, @@ -373,7 +381,7 @@ public class BlazeServerStartupOptions extends OptionsBase { // TODO(ulfjack): Make this a command option. @Option( name = "write_command_log", - defaultValue = "true", + defaultValue = "true", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOSES_INCREMENTAL_STATE}, help = "Whether or not to write the command.log file" @@ -382,7 +390,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "client_debug", - defaultValue = "false", // NOTE: purely decorative! + defaultValue = "false", // NOTE: only for documentation, value is set and used by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.BAZEL_MONITORING}, help = @@ -393,7 +401,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "connect_timeout_secs", - defaultValue = "30", + defaultValue = "30", // NOTE: only for documentation, value is set and used by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION}, help = "The amount of time the client waits for each attempt to connect to the server" @@ -402,7 +410,7 @@ public class BlazeServerStartupOptions extends OptionsBase { @Option( name = "expand_configs_in_place", - defaultValue = "true", // NOTE: purely decorative! + defaultValue = "true", // NOTE: only for documentation, value is always passed by the client. documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, OptionEffectTag.CHANGES_INPUTS}, metadataTags = {OptionMetadataTag.EXPERIMENTAL}, diff --git a/src/test/shell/integration/client_test.sh b/src/test/shell/integration/client_test.sh index 4717883a70..53f132a914 100755 --- a/src/test/shell/integration/client_test.sh +++ b/src/test/shell/integration/client_test.sh @@ -67,6 +67,17 @@ function test_shutdown() { bazel shutdown >& $TEST_log || fail "Expected success" local server_pid2=$(bazel info server_pid 2>$TEST_log) assert_not_equals "$server_pid1" "$server_pid2" + expect_not_log "WARNING: Running B\\(azel\\|laze\\) server needs to be killed" +} + +function test_server_restart_due_to_startup_options_with_client_debug_information() { + # Using --write_command_log for no particular reason, if that flag is removed, another startup + # option will do just fine. + local server_pid1=$(bazel --client_debug --write_command_log info server_pid 2>$TEST_log) + local server_pid2=$(bazel --client_debug --nowrite_command_log info server_pid 2>$TEST_log) + assert_not_equals "$server_pid1" "$server_pid2" # pid changed. + expect_log "\\[bazel WARNING .*\\] Running B\\(azel\\|laze\\) server needs to be killed" + expect_log "\\[bazel INFO .*\\] .* the running server has option --write_command_log, and requested option is: --nowrite_command_log" } function test_exit_code() { |