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