Skip to content

Commit

Permalink
Client, refactor: simplify nullary flag handling
Browse files Browse the repository at this point in the history
Nullary startup flags (e.g. --[no]master_bazelrc)
are registered with
StartupOptions::RegisterNullaryStartupOption.

Important change: `--host_jvm_debug` now supports
the negative flag (`--nohost_jvm_debug`).
I suspect the missing support was a bug resulting
from the code duplication this PR eliminates.

Until now, nullary flags also had their special
handling logic, even though the logic was always
the same.

Now, nullary flags are registered together with
their member variable's pointer, and there's only
one code path to handle all. Startup flags
forbidden in .bazelrc files are handled specially.

Until now, we searched the handler code linearly
via a long if-elseif-elseif-... chain, so parsing
N flags out of M possible flags performed O(N*M)
string comparisons.

Now, we look up the nullary flag names from a
std::unordered_map, which has O(1) lookup
complexity, reducing the total complexity to O(M).

Also a drive-by improvement in blaze.cc: use
std::unique_ptr instead of manual new/delete.

Change-Id: Ie3dcba4ae6afa959e84657b7ff9bb8f4c87777fd

Closes bazelbuild#9121.

Change-Id: I794cdc553ea833620b174375fbfdaa7492113d13
PiperOrigin-RevId: 265010164
  • Loading branch information
laszlocsomor authored and copybara-github committed Aug 23, 2019
1 parent 166efd6 commit 5697da9
Show file tree
Hide file tree
Showing 6 changed files with 150 additions and 198 deletions.
67 changes: 6 additions & 61 deletions src/main/cpp/bazel_startup_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ BazelStartupOptions::BazelStartupOptions(
use_workspace_rc(true),
use_home_rc(true),
use_master_bazelrc_(true) {
RegisterNullaryStartupFlag("home_rc");
RegisterNullaryStartupFlag("master_bazelrc");
RegisterNullaryStartupFlag("system_rc");
RegisterNullaryStartupFlag("workspace_rc");
RegisterNullaryStartupFlagNoRc("home_rc", &use_home_rc);
RegisterNullaryStartupFlagNoRc("master_bazelrc", &use_master_bazelrc_);
OverrideOptionSourcesKey("master_bazelrc", "blazerc");
RegisterNullaryStartupFlagNoRc("system_rc", &use_system_rc);
RegisterNullaryStartupFlagNoRc("workspace_rc", &use_workspace_rc);
RegisterUnaryStartupFlag("bazelrc");
}

Expand All @@ -44,66 +45,10 @@ blaze_exit_code::ExitCode BazelStartupOptions::ProcessArgExtra(

if ((*value = GetUnaryOption(arg, next_arg, "--bazelrc")) != nullptr) {
if (!rcfile.empty()) {
*error = "Can't specify --bazelrc in the .bazelrc file.";
*error = "Can't specify --bazelrc in the RC file.";
return blaze_exit_code::BAD_ARGV;
}
user_bazelrc_ = *value;
} else if (GetNullaryOption(arg, "--system_rc")) {
if (!rcfile.empty()) {
*error = "Can't specify --system_rc in .bazelrc file.";
return blaze_exit_code::BAD_ARGV;
}
use_system_rc = true;
option_sources["system_rc"] = rcfile;
} else if (GetNullaryOption(arg, "--nosystem_rc")) {
if (!rcfile.empty()) {
*error = "Can't specify --nosystem_rc in .bazelrc file.";
return blaze_exit_code::BAD_ARGV;
}
use_system_rc = false;
option_sources["system_rc"] = rcfile;
} else if (GetNullaryOption(arg, "--workspace_rc")) {
if (!rcfile.empty()) {
*error = "Can't specify --workspace_rc in .bazelrc file.";
return blaze_exit_code::BAD_ARGV;
}
use_workspace_rc = true;
option_sources["workspace_rc"] = rcfile;
} else if (GetNullaryOption(arg, "--noworkspace_rc")) {
if (!rcfile.empty()) {
*error = "Can't specify --noworkspace_rc in .bazelrc file.";
return blaze_exit_code::BAD_ARGV;
}
use_workspace_rc = false;
option_sources["workspace_rc"] = rcfile;
} else if (GetNullaryOption(arg, "--home_rc")) {
if (!rcfile.empty()) {
*error = "Can't specify --home_rc in .bazelrc file.";
return blaze_exit_code::BAD_ARGV;
}
use_home_rc = true;
option_sources["home_rc"] = rcfile;
} else if (GetNullaryOption(arg, "--nohome_rc")) {
if (!rcfile.empty()) {
*error = "Can't specify --nohome_rc in .bazelrc file.";
return blaze_exit_code::BAD_ARGV;
}
use_home_rc = false;
option_sources["home_rc"] = rcfile;
} else if (GetNullaryOption(arg, "--master_bazelrc")) {
if (!rcfile.empty()) {
*error = "Can't specify --master_bazelrc in .bazelrc file.";
return blaze_exit_code::BAD_ARGV;
}
use_master_bazelrc_ = true;
option_sources["blazerc"] = rcfile;
} else if (GetNullaryOption(arg, "--nomaster_bazelrc")) {
if (!rcfile.empty()) {
*error = "Can't specify --nomaster_bazelrc in .bazelrc file.";
return blaze_exit_code::BAD_ARGV;
}
use_master_bazelrc_ = false;
option_sources["blazerc"] = rcfile;
} else {
*is_processed = false;
return blaze_exit_code::SUCCESS;
Expand Down
3 changes: 3 additions & 0 deletions src/main/cpp/bazel_startup_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ class BazelStartupOptions : public StartupOptions {

void MaybeLogStartupOptionWarnings() const override;

protected:
std::string GetRcFileBaseName() const override { return ".bazelrc"; }

private:
std::string user_bazelrc_;
bool use_system_rc;
Expand Down
12 changes: 3 additions & 9 deletions src/main/cpp/blaze.cc
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,6 @@ class BlazeServer final {
const bool block_for_lock,
const string &output_base,
const string &server_jvm_out);
~BlazeServer();

// Acquire a lock for the server running in this output base. Returns the
// number of milliseconds spent waiting for the lock.
Expand Down Expand Up @@ -302,7 +301,7 @@ class BlazeServer final {

// Pipe that the main thread sends actions to and the cancel thread receives
// actions from.
blaze_util::IPipe *pipe_;
std::unique_ptr<blaze_util::IPipe> pipe_;

bool TryConnect(CommandServer::Stub *client);
void CancelThread();
Expand Down Expand Up @@ -1664,18 +1663,13 @@ BlazeServer::BlazeServer(
output_base_(output_base) {
gpr_set_log_function(null_grpc_log_function);

pipe_ = blaze_util::CreatePipe();
if (pipe_ == NULL) {
pipe_.reset(blaze_util::CreatePipe());
if (!pipe_) {
BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
<< "Couldn't create pipe: " << GetLastErrorString();
}
}

BlazeServer::~BlazeServer() {
delete pipe_;
pipe_ = NULL;
}

bool BlazeServer::TryConnect(
CommandServer::Stub *client) {
grpc::ClientContext context;
Expand Down
Loading

0 comments on commit 5697da9

Please sign in to comment.