Add a warning if the same rc file is read multiple times.
Prerequisite for fixing #5765, to minimize the risk that multiple rc files import the same file, which is slightly more likely with try-import.
RELNOTES: None.
PiperOrigin-RevId: 209772907
diff --git a/src/main/cpp/option_processor-internal.h b/src/main/cpp/option_processor-internal.h
index c552db4..b25db9c 100644
--- a/src/main/cpp/option_processor-internal.h
+++ b/src/main/cpp/option_processor-internal.h
@@ -31,6 +31,11 @@
std::vector<std::string> DedupeBlazercPaths(
const std::vector<std::string>& paths);
+// Given the set of already-ready files, warns if any of the newly loaded_rcs
+// are duplicates. All paths are expected to be canonical.
+void WarnAboutDuplicateRcFiles(const std::set<std::string>& read_files,
+ const std::deque<std::string>& loaded_rcs);
+
// Get the legacy list of rc files that would have been loaded - this is to
// provide a useful warning if files are being ignored that were loaded in a
// previous version of Bazel.
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc
index 9e40ddd..07f6792 100644
--- a/src/main/cpp/option_processor.cc
+++ b/src/main/cpp/option_processor.cc
@@ -237,7 +237,12 @@
case RcFile::ParseError::NONE:
return blaze_exit_code::SUCCESS;
case RcFile::ParseError::UNREADABLE_FILE:
- // We check readability before parsing, so this is unexpected.
+ // We check readability before parsing, so this is unexpected for
+ // top-level rc files, so is an INTERNAL_ERROR. It can happen for imported
+ // files, however, which should be BAD_ARGV, but we don't currently
+ // differentiate.
+ // TODO(bazel-team): fix RcFile to reclassify unreadable files that were
+ // read from a recursive call due to a malformed import.
return blaze_exit_code::INTERNAL_ERROR;
case RcFile::ParseError::INVALID_FORMAT:
case RcFile::ParseError::IMPORT_LOOP:
@@ -247,6 +252,41 @@
}
}
+void WarnAboutDuplicateRcFiles(const std::set<std::string>& read_files,
+ const std::deque<std::string>& loaded_rcs) {
+ // The first rc file in the queue is the top-level one, the one that would
+ // have imported all the others in the queue. The top-level rc is one of the
+ // default locations (system, workspace, home) or the explicit path passed by
+ // --bazelrc.
+ const std::string& top_level_rc = loaded_rcs.front();
+
+ const std::set<std::string> unique_loaded_rcs(loaded_rcs.begin(),
+ loaded_rcs.end());
+ // First check if each of the newly loaded rc files was already read.
+ for (const std::string& loaded_rc : unique_loaded_rcs) {
+ if (read_files.count(loaded_rc) > 0) {
+ if (loaded_rc == top_level_rc) {
+ BAZEL_LOG(WARNING)
+ << "Duplicate rc file: " << loaded_rc
+ << " is read multiple times, it is a standard rc file location "
+ "but must have been unnecessarilly imported earlier.";
+ } else {
+ BAZEL_LOG(WARNING)
+ << "Duplicate rc file: " << loaded_rc
+ << " is read multiple times, most recently imported from "
+ << top_level_rc;
+ }
+ }
+ // Now check if the top-level rc file loads up its own duplicates (it can't
+ // be a cycle, since that would be an error and we would have already
+ // exited, but it could have a diamond dependency of some sort.)
+ if (std::count(loaded_rcs.begin(), loaded_rcs.end(), loaded_rc) > 1) {
+ BAZEL_LOG(WARNING) << "Duplicate rc file: " << loaded_rc
+ << " is imported multiple times from " << top_level_rc;
+ }
+ }
+}
+
} // namespace internal
// TODO(#4502) Consider simplifying result_rc_files to a vector of RcFiles, no
@@ -320,18 +360,26 @@
// It's possible that workspace == home, that files are symlinks for each
// other, or that the --bazelrc flag is a duplicate. Dedupe them to minimize
// the likelihood of repeated options. Since bazelrcs can include one another,
- // this isn't sufficient to prevent duplicate options, but it's good enough.
- // This also has the effect of removing paths that don't point to real files.
+ // this isn't sufficient to prevent duplicate options, so we also warn if we
+ // discover duplicate loads later. This also has the effect of removing paths
+ // that don't point to real files.
rc_files = internal::DedupeBlazercPaths(rc_files);
+ std::set<std::string> read_files;
// Parse these potential files, in priority order;
- for (const auto& bazelrc_path : rc_files) {
+ for (const std::string& top_level_bazelrc_path : rc_files) {
std::unique_ptr<RcFile> parsed_rc;
blaze_exit_code::ExitCode parse_rcfile_exit_code = ParseRcFile(
- workspace_layout, workspace, bazelrc_path, &parsed_rc, error);
+ workspace_layout, workspace, top_level_bazelrc_path, &parsed_rc, error);
if (parse_rcfile_exit_code != blaze_exit_code::SUCCESS) {
return parse_rcfile_exit_code;
}
+
+ // Check that none of the rc files loaded this time are duplicate.
+ const std::deque<std::string>& sources = parsed_rc->sources();
+ internal::WarnAboutDuplicateRcFiles(read_files, sources);
+ read_files.insert(sources.begin(), sources.end());
+
result_rc_files->push_back(std::move(parsed_rc));
}
@@ -341,12 +389,6 @@
// TODO(b/36168162): Remove this warning along with
// internal::GetOldRcPaths and internal::FindLegacyUserBazelrc after
// the transition period has passed.
- std::set<std::string> read_files;
- for (auto& result_rc : *result_rc_files) {
- const std::deque<std::string>& sources = result_rc->sources();
- read_files.insert(sources.begin(), sources.end());
- }
-
const std::set<std::string> old_files =
internal::GetOldRcPaths(workspace_layout, workspace, cwd,
cmd_line->path_to_binary, cmd_line->startup_args);
diff --git a/src/test/cpp/rc_file_test.cc b/src/test/cpp/rc_file_test.cc
index f23d639..af7254d 100644
--- a/src/test/cpp/rc_file_test.cc
+++ b/src/test/cpp/rc_file_test.cc
@@ -28,6 +28,7 @@
#include "googletest/include/gtest/gtest.h"
namespace blaze {
+using ::testing::ContainsRegex;
using ::testing::HasSubstr;
using ::testing::MatchesRegex;
@@ -154,6 +155,22 @@
return false;
}
+ void ParseOptionsAndCheckOutput(
+ const std::vector<std::string>& args,
+ const blaze_exit_code::ExitCode expected_exit_code,
+ const std::string& expected_error_regex,
+ const std::string& expected_output_regex) {
+ std::string error;
+ testing::internal::CaptureStderr();
+ const blaze_exit_code::ExitCode exit_code =
+ option_processor_->ParseOptions(args, workspace_, cwd_, &error);
+ const std::string& output = testing::internal::GetCapturedStderr();
+
+ ASSERT_EQ(expected_exit_code, exit_code) << error;
+ ASSERT_THAT(error, ContainsRegex(expected_error_regex));
+ ASSERT_THAT(output, ContainsRegex(expected_output_regex));
+ }
+
const std::string workspace_;
std::string cwd_;
const std::string binary_dir_;
@@ -530,10 +547,8 @@
const std::vector<std::string> args = {"bazel", "--noworkspace_rc",
"--workspace_rc", "build"};
- std::string error;
- ASSERT_EQ(blaze_exit_code::SUCCESS,
- option_processor_->ParseOptions(args, workspace_, cwd_, &error))
- << error;
+ ParseOptionsAndCheckOutput(args, blaze_exit_code::SUCCESS, "", "");
+
EXPECT_EQ(123, option_processor_->GetParsedStartupOptions()->max_idle_secs);
// Check that the startup options' provenance message contains the correct
@@ -554,10 +569,7 @@
"startup --max_idle_secs=42\nstartup --io_nice_level=6", &workspace_rc));
const std::vector<std::string> args = {binary_path_, "build"};
- std::string error;
- ASSERT_EQ(blaze_exit_code::SUCCESS,
- option_processor_->ParseOptions(args, workspace_, cwd_, &error))
- << error;
+ ParseOptionsAndCheckOutput(args, blaze_exit_code::SUCCESS, "", "");
EXPECT_EQ(42, option_processor_->GetParsedStartupOptions()->max_idle_secs);
EXPECT_EQ(6, option_processor_->GetParsedStartupOptions()->io_nice_level);
@@ -590,10 +602,7 @@
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;
+ ParseOptionsAndCheckOutput(args, blaze_exit_code::SUCCESS, "", "");
EXPECT_EQ(123, option_processor_->GetParsedStartupOptions()->max_idle_secs);
EXPECT_EQ(6, option_processor_->GetParsedStartupOptions()->io_nice_level);
@@ -630,10 +639,7 @@
&workspace_rc));
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;
+ ParseOptionsAndCheckOutput(args, blaze_exit_code::SUCCESS, "", "");
EXPECT_EQ(123, option_processor_->GetParsedStartupOptions()->max_idle_secs);
EXPECT_EQ(6, option_processor_->GetParsedStartupOptions()->io_nice_level);
@@ -654,4 +660,136 @@
"--io_nice_level=6\n"));
}
+TEST_F(ParseOptionsTest, BazelRcImportFailsForMissingFile) {
+ const std::string missing_imported_rc_path =
+ blaze_util::JoinPath(workspace_, "myimportedbazelrc");
+ std::string workspace_rc;
+ ASSERT_TRUE(SetUpWorkspaceRcFile("import " + missing_imported_rc_path,
+ &workspace_rc));
+
+ const std::vector<std::string> args = {"bazel", "build"};
+ ParseOptionsAndCheckOutput(
+ args, blaze_exit_code::INTERNAL_ERROR,
+ "Unexpected error reading .blazerc file '.*myimportedbazelrc'", "");
+}
+
+TEST_F(ParseOptionsTest, DoubleImportsCauseAWarning) {
+ 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("", imported_rc_path, 0755));
+
+ // Import the custom location twice.
+ std::string workspace_rc;
+ ASSERT_TRUE(SetUpWorkspaceRcFile(
+ "import " + imported_rc_path + "\n"
+ "import " + imported_rc_path + "\n",
+ &workspace_rc));
+
+ const std::vector<std::string> args = {"bazel", "build"};
+ ParseOptionsAndCheckOutput(
+ args, blaze_exit_code::SUCCESS, "",
+ "WARNING: Duplicate rc file: .*myimportedbazelrc is imported multiple "
+ "times from .*workspace.*bazelrc\n");
+}
+
+TEST_F(ParseOptionsTest, DeepDoubleImportCausesAWarning) {
+ const std::string dual_imported_rc_path =
+ blaze_util::JoinPath(workspace_, "dual_imported.bazelrc");
+ ASSERT_TRUE(blaze_util::MakeDirectories(
+ blaze_util::Dirname(dual_imported_rc_path), 0755));
+ ASSERT_TRUE(blaze_util::WriteFile("", dual_imported_rc_path, 0755));
+
+ const std::string intermediate_import_1 =
+ blaze_util::JoinPath(workspace_, "intermediate_import_1");
+ ASSERT_TRUE(blaze_util::MakeDirectories(
+ blaze_util::Dirname(intermediate_import_1), 0755));
+ ASSERT_TRUE(blaze_util::WriteFile("import " + dual_imported_rc_path,
+ intermediate_import_1, 0755));
+
+ const std::string intermediate_import_2 =
+ blaze_util::JoinPath(workspace_, "intermediate_import_2");
+ ASSERT_TRUE(blaze_util::MakeDirectories(
+ blaze_util::Dirname(intermediate_import_2), 0755));
+ ASSERT_TRUE(blaze_util::WriteFile("import " + dual_imported_rc_path,
+ intermediate_import_2, 0755));
+
+ // Import the custom location twice.
+ std::string workspace_rc;
+ ASSERT_TRUE(SetUpWorkspaceRcFile(
+ "import " + intermediate_import_1 + "\n"
+ "import " + intermediate_import_2 + "\n",
+ &workspace_rc));
+
+ const std::vector<std::string> args = {"bazel", "build"};
+ ParseOptionsAndCheckOutput(args, blaze_exit_code::SUCCESS, "",
+ "WARNING: Duplicate rc file: "
+ ".*dual_imported.bazelrc is imported multiple "
+ "times from .*workspace.*bazelrc\n");
+}
+
+// TODO(b/112908763): C:/ and c:\\ paths don't compare well, making this fail on
+// Windows. Adding all the conversions necessary to get around this is probably
+// not the right approach either, so leaving this coverage out for Windows. We
+// should not be using string equality for path comparisons, but have a path
+// type which handles these differences for the correct platform. Fix this and
+// enable this test.
+#if !defined(_WIN32) && !defined(__CYGWIN__)
+TEST_F(ParseOptionsTest, ImportingAFileAndPassingItInCausesAWarning) {
+ 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("", imported_rc_path, 0755));
+
+ // Import the custom location, and pass it in by flag.
+ std::string workspace_rc;
+ ASSERT_TRUE(
+ SetUpWorkspaceRcFile("import " + imported_rc_path, &workspace_rc));
+
+ const std::vector<std::string> args = {
+ "bazel", "--bazelrc=" + imported_rc_path, "build"};
+ ParseOptionsAndCheckOutput(
+ args, blaze_exit_code::SUCCESS, "",
+ "WARNING: Duplicate rc file: .*myimportedbazelrc is read multiple times, "
+ "it is a standard rc file location but must have been unnecessarilly "
+ "imported earlier.\n");
+}
+
+TEST_F(ParseOptionsTest, ImportingAPreviouslyLoadedStandardRcCausesAWarning) {
+ std::string system_rc;
+ ASSERT_TRUE(SetUpSystemRcFile("", &system_rc));
+
+ // Import the system_rc extraneously.
+ std::string workspace_rc;
+ ASSERT_TRUE(SetUpWorkspaceRcFile("import " + system_rc, &workspace_rc));
+
+ const std::vector<std::string> args = {"bazel", "build"};
+ std::string output;
+
+ ParseOptionsAndCheckOutput(
+ args, blaze_exit_code::SUCCESS, "",
+ "WARNING: Duplicate rc file: .*bazel.bazelrc is read multiple "
+ "times, most recently imported from .*workspace.*bazelrc\n");
+}
+#endif // !defined(_WIN32) && !defined(__CYGWIN__)
+
+TEST_F(ParseOptionsTest, ImportingStandardRcBeforeItIsLoadedCausesAWarning) {
+ std::string workspace_rc;
+ ASSERT_TRUE(SetUpWorkspaceRcFile("", &workspace_rc));
+
+ // Import the workspace_rc extraneously.
+ std::string system_rc;
+ ASSERT_TRUE(SetUpSystemRcFile("import " + workspace_rc, &system_rc));
+
+ const std::vector<std::string> args = {"bazel", "build"};
+ std::string output;
+ ParseOptionsAndCheckOutput(
+ args, blaze_exit_code::SUCCESS, "",
+ "WARNING: Duplicate rc file: .*workspace.*bazelrc is read multiple "
+ "times, it is a standard rc file location but must have been "
+ "unnecessarilly imported earlier.\n");
+}
+
} // namespace blaze
