Bazel client: fix reporting of ignored RC files
(This commit is a roll-forward of commit e9a908560133770614eca89ef64681bdf3f04b3e
that was rolled back by commit 37335187bf31dac48155e77f404bd11008d68ea7. The culprit
of the breakage is fixed in the Google-internal
depot.)
Compute the difference between the read RC files
and the ignored ones based on their canonical
paths, not on the textual value of the path.
This aids for proper reporting of ignored RC
files, while reporting them with a path familiar
to the user (e.g. paths the user passed with flags
rather than the potentially different canonical
version of the same path).
Also: respect $HOME on Windows when Bazel is
running inside of a test, to avoid picking up the
real RC file from the user's home directory. This
fixes //src/test/cpp:rc_file_test on Windows.
Fixes https://github.com/bazelbuild/bazel/issues/6138
Fixes https://github.com/bazelbuild/bazel/issues/6469
Closes #6910.
PiperOrigin-RevId: 225832065
diff --git a/src/main/cpp/blaze_util.cc b/src/main/cpp/blaze_util.cc
index d8ec359..b35af9b 100644
--- a/src/main/cpp/blaze_util.cc
+++ b/src/main/cpp/blaze_util.cc
@@ -181,6 +181,8 @@
}
}
+bool IsRunningWithinTest() { return !GetEnv("TEST_TMPDIR").empty(); }
+
void WithEnvVars::SetEnvVars(const map<string, EnvVarValue>& vars) {
for (const auto& var : vars) {
switch (var.second.action) {
diff --git a/src/main/cpp/blaze_util.h b/src/main/cpp/blaze_util.h
index b9f5a51..b0b5538 100644
--- a/src/main/cpp/blaze_util.h
+++ b/src/main/cpp/blaze_util.h
@@ -104,6 +104,10 @@
// Revisit once client logging is fixed (b/32939567).
void SetDebugLog(bool enabled);
+// Returns true if this Bazel instance is running inside of a Bazel test.
+// This method observes the TEST_TMPDIR envvar.
+bool IsRunningWithinTest();
+
// What WithEnvVar should do with an environment variable
enum EnvVarAction { UNSET, SET };
diff --git a/src/main/cpp/blaze_util_windows.cc b/src/main/cpp/blaze_util_windows.cc
index 3319faf..1483dfb 100644
--- a/src/main/cpp/blaze_util_windows.cc
+++ b/src/main/cpp/blaze_util_windows.cc
@@ -406,6 +406,12 @@
}
string GetHomeDir() {
+ if (IsRunningWithinTest()) {
+ // Bazel is running inside of a test. Respect $HOME that the test setup has
+ // set instead of using the actual home directory of the current user.
+ return GetEnv("HOME");
+ }
+
PWSTR wpath;
// Look up the user's home directory. The default value of "FOLDERID_Profile"
// is the same as %USERPROFILE%, but it does not require the envvar to be set.
diff --git a/src/main/cpp/option_processor-internal.h b/src/main/cpp/option_processor-internal.h
index b25db9c..e1b9e7d 100644
--- a/src/main/cpp/option_processor-internal.h
+++ b/src/main/cpp/option_processor-internal.h
@@ -26,7 +26,9 @@
namespace internal {
// Returns the deduped set of bazelrc paths (with respect to its canonical form)
-// preserving the original order. The paths that cannot be resolved are
+// preserving the original order.
+// All paths in the result were verified to exist (otherwise their canonical
+// form couldn't have been computed). The paths that cannot be resolved are
// omitted.
std::vector<std::string> DedupeBlazercPaths(
const std::vector<std::string>& paths);
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc
index 1298ea1..181afd2 100644
--- a/src/main/cpp/option_processor.cc
+++ b/src/main/cpp/option_processor.cc
@@ -185,18 +185,22 @@
internal::FindRcAlongsideBinary(cwd, path_to_binary);
candidate_bazelrc_paths = {workspace_rc, binary_rc, system_bazelrc_path};
}
- const std::vector<std::string> deduped_blazerc_paths =
- internal::DedupeBlazercPaths(candidate_bazelrc_paths);
- std::set<std::string> old_rc_paths(deduped_blazerc_paths.begin(),
- deduped_blazerc_paths.end());
string user_bazelrc_path = internal::FindLegacyUserBazelrc(
SearchUnaryOption(startup_args, "--bazelrc"), workspace);
if (!user_bazelrc_path.empty()) {
- old_rc_paths.insert(user_bazelrc_path);
+ candidate_bazelrc_paths.push_back(user_bazelrc_path);
}
- return old_rc_paths;
+ // DedupeBlazercPaths returns paths whose canonical path could be computed,
+ // therefore these paths must exist.
+ const std::vector<std::string> deduped_existing_blazerc_paths =
+ internal::DedupeBlazercPaths(candidate_bazelrc_paths);
+ return std::set<std::string>(deduped_existing_blazerc_paths.begin(),
+ deduped_existing_blazerc_paths.end());
}
+// Deduplicates the given paths based on their canonical form.
+// Computes the canonical form using blaze_util::MakeCanonical.
+// Returns the unique paths in their original form (not the canonical one).
std::vector<std::string> DedupeBlazercPaths(
const std::vector<std::string>& paths) {
std::set<std::string> canonical_paths;
@@ -291,6 +295,20 @@
}
}
+std::vector<std::string> GetLostFiles(
+ const std::set<std::string>& old_files,
+ const std::set<std::string>& read_files_canon) {
+ std::vector<std::string> result;
+ for (const auto& old : old_files) {
+ std::string old_canon = blaze_util::MakeCanonical(old.c_str());
+ if (!old_canon.empty() &&
+ read_files_canon.find(old_canon) == read_files_canon.end()) {
+ result.push_back(old);
+ }
+ }
+ return result;
+}
+
} // namespace internal
// TODO(#4502) Consider simplifying result_rc_files to a vector of RcFiles, no
@@ -366,7 +384,7 @@
// that don't point to real files.
rc_files = internal::DedupeBlazercPaths(rc_files);
- std::set<std::string> read_files;
+ std::set<std::string> read_files_canonical_paths;
// Parse these potential files, in priority order;
for (const std::string& top_level_bazelrc_path : rc_files) {
std::unique_ptr<RcFile> parsed_rc;
@@ -377,9 +395,10 @@
}
// 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());
+ const std::deque<std::string>& sources =
+ parsed_rc->canonical_source_paths();
+ internal::WarnAboutDuplicateRcFiles(read_files_canonical_paths, sources);
+ read_files_canonical_paths.insert(sources.begin(), sources.end());
result_rc_files->push_back(std::move(parsed_rc));
}
@@ -394,16 +413,8 @@
workspace_layout, workspace, cwd, cmd_line->path_to_binary,
cmd_line->startup_args, internal::FindSystemWideRc(system_bazelrc_path_));
- // std::vector<std::string> old_files = internal::GetOldRcPathsInOrder(
- // workspace_layout, workspace, cwd, cmd_line->path_to_binary,
- // cmd_line->startup_args);
- //
- // std::sort(old_files.begin(), old_files.end());
- std::vector<std::string> lost_files(old_files.size());
- std::vector<std::string>::iterator end_iter = std::set_difference(
- old_files.begin(), old_files.end(), read_files.begin(), read_files.end(),
- lost_files.begin());
- lost_files.resize(end_iter - lost_files.begin());
+ std::vector<std::string> lost_files =
+ internal::GetLostFiles(old_files, read_files_canonical_paths);
if (!lost_files.empty()) {
std::string joined_lost_rcs;
blaze_util::JoinStrings(lost_files, '\n', &joined_lost_rcs);
@@ -625,7 +636,7 @@
int cur_index = 1;
std::map<std::string, int> rcfile_indexes;
for (const auto& blazerc : blazercs) {
- for (const std::string& source_path : blazerc->sources()) {
+ for (const std::string& source_path : blazerc->canonical_source_paths()) {
// Deduplicate the rc_source list because the same file might be included
// from multiple places.
if (rcfile_indexes.find(source_path) != rcfile_indexes.end()) continue;
diff --git a/src/main/cpp/rc_file.cc b/src/main/cpp/rc_file.cc
index e356ca6..1a0f842 100644
--- a/src/main/cpp/rc_file.cc
+++ b/src/main/cpp/rc_file.cc
@@ -63,10 +63,10 @@
const std::string canonical_filename =
blaze_util::MakeCanonical(filename.c_str());
- rcfile_paths_.push_back(canonical_filename);
- // Keep a pointer to the canonical_filename string in rcfile_paths_ for the
- // RcOptions.
- string* filename_ptr = &rcfile_paths_.back();
+ canonical_rcfile_paths_.push_back(canonical_filename);
+ // Keep a pointer to the canonical_filename string in canonical_rcfile_paths_
+ // for the RcOptions.
+ string* filename_ptr = &canonical_rcfile_paths_.back();
// A '\' at the end of a line continues the line.
blaze_util::Replace("\\\r\n", "", &contents);
diff --git a/src/main/cpp/rc_file.h b/src/main/cpp/rc_file.h
index 0d462dc..5f1eee7 100644
--- a/src/main/cpp/rc_file.h
+++ b/src/main/cpp/rc_file.h
@@ -42,7 +42,9 @@
std::string workspace, ParseError* error, std::string* error_text);
// Returns all relevant rc sources for this file (including itself).
- const std::deque<std::string>& sources() const { return rcfile_paths_; }
+ const std::deque<std::string>& canonical_source_paths() const {
+ return canonical_rcfile_paths_;
+ }
// Command -> all options for that command (in order of appearance).
using OptionMap = std::unordered_map<std::string, std::vector<RcOption>>;
@@ -68,9 +70,12 @@
const std::string workspace_;
// Full closure of rcfile paths imported from this file (including itself).
+ // These are all canonical paths, created with blaze_util::MakeCanonical.
+ // This also means all of these paths should exist.
+ //
// The RcOption structs point to the strings in here so they need to be stored
// in a container that offers stable pointers, like a deque (and not vector).
- std::deque<std::string> rcfile_paths_;
+ std::deque<std::string> canonical_rcfile_paths_;
// All options parsed from the file.
OptionMap options_;
};
diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc
index 1267384..c343db9 100644
--- a/src/main/cpp/startup_options.cc
+++ b/src/main/cpp/startup_options.cc
@@ -98,8 +98,7 @@
idle_server_tasks(true),
original_startup_options_(std::vector<RcStartupFlag>()),
unlimit_coredumps(false) {
- bool testing = !blaze::GetEnv("TEST_TMPDIR").empty();
- if (testing) {
+ if (blaze::IsRunningWithinTest()) {
output_root = blaze_util::MakeAbsolute(blaze::GetEnv("TEST_TMPDIR"));
max_idle_secs = 15;
BAZEL_LOG(USER) << "$TEST_TMPDIR defined: output root default is '"
diff --git a/src/test/cpp/rc_file_test.cc b/src/test/cpp/rc_file_test.cc
index 01a4d5a..25baaaa 100644
--- a/src/test/cpp/rc_file_test.cc
+++ b/src/test/cpp/rc_file_test.cc
@@ -173,8 +173,10 @@
ASSERT_EQ(2, parsed_rcs.size());
const std::deque<std::string> expected_system_rc_que = {system_rc};
const std::deque<std::string> expected_workspace_rc_que = {workspace_rc};
- EXPECT_EQ(expected_system_rc_que, parsed_rcs[0].get()->sources());
- EXPECT_EQ(expected_workspace_rc_que, parsed_rcs[1].get()->sources());
+ EXPECT_EQ(expected_system_rc_que,
+ parsed_rcs[0].get()->canonical_source_paths());
+ EXPECT_EQ(expected_workspace_rc_que,
+ parsed_rcs[1].get()->canonical_source_paths());
}
TEST_F(GetRcFileTest, GetRcFilesRespectsNoSystemRc) {
@@ -195,7 +197,8 @@
ASSERT_EQ(1, parsed_rcs.size());
const std::deque<std::string> expected_workspace_rc_que = {workspace_rc};
- EXPECT_EQ(expected_workspace_rc_que, parsed_rcs[0].get()->sources());
+ EXPECT_EQ(expected_workspace_rc_que,
+ parsed_rcs[0].get()->canonical_source_paths());
}
TEST_F(GetRcFileTest, GetRcFilesRespectsNoWorkspaceRc) {
@@ -216,7 +219,8 @@
ASSERT_EQ(1, parsed_rcs.size());
const std::deque<std::string> expected_system_rc_que = {system_rc};
- EXPECT_EQ(expected_system_rc_que, parsed_rcs[0].get()->sources());
+ EXPECT_EQ(expected_system_rc_que,
+ parsed_rcs[0].get()->canonical_source_paths());
}
TEST_F(GetRcFileTest, GetRcFilesRespectsNoWorkspaceRcAndNoSystemCombined) {
@@ -317,8 +321,9 @@
// Because of the variety of path representations in windows, this
// equality test does not attempt to check the entire path.
ASSERT_EQ(1, parsed_rcs.size());
- ASSERT_EQ(1, parsed_rcs[0].get()->sources().size());
- EXPECT_THAT(parsed_rcs[0].get()->sources().front(), HasSubstr("mybazelrc"));
+ ASSERT_EQ(1, parsed_rcs[0].get()->canonical_source_paths().size());
+ EXPECT_THAT(parsed_rcs[0].get()->canonical_source_paths().front(),
+ HasSubstr("mybazelrc"));
}
TEST_F(GetRcFileTest, GetRcFilesAcceptsNullCommandLineRc) {
@@ -338,7 +343,7 @@
// but it does technically count as a file
ASSERT_EQ(1, parsed_rcs.size());
const std::deque<std::string> expected_rc_que = {kNullDevice};
- EXPECT_EQ(expected_rc_que, parsed_rcs[0].get()->sources());
+ EXPECT_EQ(expected_rc_que, parsed_rcs[0].get()->canonical_source_paths());
}
class ParseOptionsTest : public RcFileTest {
