Skip to content

Commit

Permalink
Demote OptionProcessor from globals
Browse files Browse the repository at this point in the history 
Some more cutting back on global mutable state that's hard to reason about the
validity of. This makes it more obvious what actually depends on what aspects
of the option processor and should make it easier to come even on the TODO to
make GlobalVariables.options const.

PiperOrigin-RevId: 251294543
  • Loading branch information
@michajlo
michajlo authored and Copybara-Service committed Jun 3, 2019
1 parent fe81b49 commit ce5e718
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 41 deletions.
82 changes: 50 additions & 32 deletions src/main/cpp/blaze.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,10 @@ class BlazeServer final {
// Send the command line to the server and forward whatever it says to stdout
// and stderr. Returns the desired exit code. Only call this when the server
// is in connected state.
unsigned int Communicate();
unsigned int Communicate(
const StartupOptions &startup_options,
const std::string &command,
const std::vector<std::string> &command_args);

// Disconnects and kills an existing server. Only call this when this object
// is in connected state.
Expand Down Expand Up @@ -572,8 +575,11 @@ static int StartServer(const WorkspaceLayout *workspace_layout,
//
// This function passes the commands array to the blaze process.
// This array should start with a command ("build", "info", etc.).
static void StartStandalone(const WorkspaceLayout *workspace_layout,
BlazeServer *server, uint64_t start_time) {
static void StartStandalone(
const WorkspaceLayout *workspace_layout,
const OptionProcessor &option_processor,
const uint64_t start_time,
BlazeServer *server) {
if (server->Connected()) {
server->KillRunningServer();
}
Expand All @@ -584,9 +590,9 @@ static void StartStandalone(const WorkspaceLayout *workspace_layout,
BAZEL_LOG(INFO) << "Starting " << globals->options->product_name
<< " in batch mode.";

string command = globals->option_processor->GetCommand();
const string command = option_processor.GetCommand();
const vector<string> command_arguments =
globals->option_processor->GetCommandArguments();
option_processor.GetCommandArguments();

if (!command_arguments.empty() && command == "shutdown") {
string product = globals->options->product_name;
Expand Down Expand Up @@ -662,8 +668,10 @@ static void SetRestartReasonIfNotSet(RestartReason restart_reason) {
}

// Starts up a new server and connects to it. Exits if it didn't work out.
static void StartServerAndConnect(const WorkspaceLayout *workspace_layout,
BlazeServer *server) {
static void StartServerAndConnect(
const WorkspaceLayout *workspace_layout,
const OptionProcessor &option_processor,
BlazeServer *server) {
string server_dir =
blaze_util::JoinPath(globals->options->output_base, "server");

Expand Down Expand Up @@ -739,7 +747,7 @@ static void StartServerAndConnect(const WorkspaceLayout *workspace_layout,

std::this_thread::sleep_until(next_attempt_time);
if (!server_startup->IsStillAlive()) {
globals->option_processor->PrintStartupOptionsProvenanceMessage();
option_processor.PrintStartupOptionsProvenanceMessage();
if (globals->jvm_log_file_append) {
// Don't dump the log if we were appending - the user should know where
// to find it, and who knows how much content they may have accumulated.
Expand Down Expand Up @@ -1048,11 +1056,13 @@ static void CancelServer() { blaze_server->Cancel(); }
// Performs all I/O for a single client request to the server, and
// shuts down the client (by exit or signal).
static ATTRIBUTE_NORETURN void SendServerRequest(
const WorkspaceLayout *workspace_layout, BlazeServer *server,
uint64_t start_time) {
const WorkspaceLayout *workspace_layout,
const OptionProcessor &option_processor,
const uint64_t start_time,
BlazeServer *server) {
while (true) {
if (!server->Connected()) {
StartServerAndConnect(workspace_layout, server);
StartServerAndConnect(workspace_layout, option_processor, server);
}

// Check for the case when the workspace directory deleted and then gets
Expand Down Expand Up @@ -1090,23 +1100,29 @@ static ATTRIBUTE_NORETURN void SendServerRequest(
globals->startup_time = GetMillisecondsMonotonic() - start_time;

SignalHandler::Get().Install(globals, CancelServer);
SignalHandler::Get().PropagateSignalOrExit(server->Communicate());
SignalHandler::Get().PropagateSignalOrExit(
server->Communicate(
*option_processor.GetParsedStartupOptions(),
option_processor.GetCommand(),
option_processor.GetCommandArguments()));
}

// Parse the options, storing parsed values into globals->options.
static void ParseOptions(const string &cwd, int argc, const char *argv[]) {
// Parse the options.
static void ParseOptionsOrDie(
const string &cwd,
OptionProcessor &option_processor,
int argc,
const char *argv[]) {
std::string error;
std::vector<std::string> args;
args.insert(args.end(), argv, argv + argc);
const blaze_exit_code::ExitCode parse_exit_code =
globals->option_processor->ParseOptions(
args, globals->workspace, cwd, &error);
option_processor.ParseOptions(args, globals->workspace, cwd, &error);

if (parse_exit_code != blaze_exit_code::SUCCESS) {
globals->option_processor->PrintStartupOptionsProvenanceMessage();
option_processor.PrintStartupOptionsProvenanceMessage();
BAZEL_DIE(parse_exit_code) << error;
}
globals->options = globals->option_processor->GetParsedStartupOptions();
}

static string GetCanonicalCwd() {
Expand Down Expand Up @@ -1313,7 +1329,7 @@ int Main(int argc, const char *argv[], WorkspaceLayout *workspace_layout,
return blaze_exit_code::SUCCESS;
}

globals = new GlobalVariables(option_processor);
globals = new GlobalVariables();
blaze::SetupStdStreams();
if (argc == 1 && blaze::WarnIfStartedFromDesktop()) {
// Only check and warn for from-desktop start if there were no args.
Expand All @@ -1335,21 +1351,23 @@ int Main(int argc, const char *argv[], WorkspaceLayout *workspace_layout,

#if defined(_WIN32) || defined(__CYGWIN__)
// Must be done before command line parsing.
// ParseOptions already populate --client_env, so detect bash before it
// ParseOptionsOrDie already populate --client_env, so detect bash before it
// happens.
(void)DetectBashAndExportBazelSh();
#endif // if defined(_WIN32) || defined(__CYGWIN__)

globals->binary_path = CheckAndGetBinaryPath(cwd, argv[0]);
ParseOptions(cwd, argc, argv);
ParseOptionsOrDie(cwd, *option_processor, argc, argv);
globals->options = option_processor->GetParsedStartupOptions();

SetDebugLog(globals->options->client_debug);
// If client_debug was false, this is ignored, so it's accurate.
BAZEL_LOG(INFO) << "Debug logging requested, sending all client log "
"statements to stderr";
// TODO(b/79206210): Can't log this before SetDebugLog is called, since the
// warning might get swallowed. Once the bug is fixed, move this call to
// OptionProcessor::ParseOptions where the order of operations is more clear.
// OptionProcessor::ParseOptionsOrDie where the order of operations is more
// clear.
globals->options->MaybeLogStartupOptionWarnings();

if (globals->options->unlimit_coredumps) {
Expand All @@ -1375,7 +1393,7 @@ int Main(int argc, const char *argv[], WorkspaceLayout *workspace_layout,
blaze_server->Connect();

if (!globals->options->batch &&
"shutdown" == globals->option_processor->GetCommand() &&
"shutdown" == option_processor->GetCommand() &&
!blaze_server->Connected()) {
return 0;
}
Expand All @@ -1386,9 +1404,11 @@ int Main(int argc, const char *argv[], WorkspaceLayout *workspace_layout,
if (globals->options->batch) {
SetScheduling(globals->options->batch_cpu_scheduling,
globals->options->io_nice_level);
StartStandalone(workspace_layout, blaze_server, start_time);
StartStandalone(
workspace_layout, *option_processor, start_time, blaze_server);
} else {
SendServerRequest(workspace_layout, blaze_server, start_time);
SendServerRequest(
workspace_layout, *option_processor, start_time, blaze_server);
}
return 0;
}
Expand Down Expand Up @@ -1666,19 +1686,19 @@ void BlazeServer::KillRunningServer() {
connected_ = false;
}

unsigned int BlazeServer::Communicate() {
unsigned int BlazeServer::Communicate(
const StartupOptions &startup_options,
const string &command,
const vector<string> &command_args) {
assert(connected_);
assert(globals->server_pid > 0);

vector<string> arg_vector;
string command = globals->option_processor->GetCommand();
if (!command.empty()) {
arg_vector.push_back(command);
AddLoggingArgs(&arg_vector);
}

const vector<string> command_args =
globals->option_processor->GetCommandArguments();
if (!command_args.empty()) {
arg_vector.insert(arg_vector.end(),
command_args.begin(),
Expand All @@ -1696,10 +1716,8 @@ unsigned int BlazeServer::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_) {
startup_options.original_startup_options_) {
command_server::StartupOption *proto_option_field =
request.add_startup_options();
request.add_startup_options();
Expand Down
5 changes: 2 additions & 3 deletions src/main/cpp/global_variables.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@

namespace blaze {

GlobalVariables::GlobalVariables(OptionProcessor* option_processor)
: option_processor(option_processor),
server_pid(-1),
GlobalVariables::GlobalVariables()
: server_pid(-1),
options(NULL), /* Initialized after parsing with option_processor. */
startup_time(0),
extract_data_time(0),
Expand Down
7 changes: 1 addition & 6 deletions src/main/cpp/global_variables.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

namespace blaze {

class OptionProcessor;
class StartupOptions;

// The reason for a blaze server restart.
Expand All @@ -42,7 +41,7 @@ enum RestartReason {
};

struct GlobalVariables {
GlobalVariables(OptionProcessor *option_processor);
GlobalVariables();

std::string ServerJarPath() const {
// The server jar is called "A-server.jar" so it's the first binary we
Expand All @@ -61,10 +60,6 @@ struct GlobalVariables {
// If not under a workspace directory, this is equal to cwd.
std::string workspace;

// Option processor responsible for parsing RC files and converting them into
// the argument list passed on to the server.
OptionProcessor *option_processor;

// The path of the JVM executable that should be used to launch Blaze.
std::string jvm_path;

Expand Down

0 comments on commit ce5e718

Please sign in to comment.