Repository: bazelbuild/bazel
Issue: Bazel shows bogus WARNING for the rc files, if they are symlinks.
Labels: untriaged
team-Bazel

### Description of the problem / feature request:

Bazel shows bogus `WARNING` for the rc files, if they are symlinks:

```
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/tmp/workspace/.bazelrc
```

But the rc file is correctly processed, as seen in the reproduction below.

Note that this is new in `0.19rc6` and there is no warning in `0.18`.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

```
mkdir -p /tmp/workspace/tools
cd /tmp/workspace
touch WORKSPACE
touch BUILD
echo "build:test --copt=-DTEST" > tools/bazel.rc
ln -s tools/bazel.rc .bazelrc
bazel build --config=test :all
```
```
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/tmp/workspace/.bazelrc
Starting local Bazel server and connecting to it...
INFO: Analysed 0 targets (3 packages loaded, 0 targets configured).
INFO: Found 0 targets...
INFO: Elapsed time: 1.298s, Critical Path: 0.02s, Remote (0.00% of the time): [queue: 0.00%, setup: 0.00%, process: 0.00%]
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
```

### What operating system are you running Bazel on?

Linux.

### What's the output of `bazel info release`?

```
release 0.19.0rc6
```

### Any other information, logs, or outputs that you want to share?

Backstory (mostly irrelevant to the actual issue):

The `.bazelrc` symlink to `tools/bazel.rc` was added to ease the migration instead of `import %workspace%/tools/bazel.rc` suggested in #6319, since we have projects (e.g. https://github.com/envoyproxy/envoy-filter-example) importing the main project (https://github.com/envoyproxy/envoy) as a submodule and linking to its [`tools/bazel.rc`](https://github.com/envoyproxy/envoy/blob/master/tools/bazel.rc), so `import` breaks, because workspace-relative location changes.

Issue comments:
cc @katre, since this is new in `0.19` (#6116), but probably not a release blocker.
This is a small enough use-case that I don't want to risk a breakage due to cherrypick. We should try and address it for 0.20, however.
I'm getting the same error message for **non-symlinks** on Windows with Bazel 0.19.0:
```
D:\src\codebase>bazel build //...
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
d:\src\codebase/.bazelrc
```
Same as @baskus. Can somebody please confirm this is not the intended behaviour?
Assigning to @dslomov to decide if this is a blocker for 0.20
/sub, this is causing confusing issues on the Mac too. The system_rc is `/etc/bazel.bazelrc` on the Mac, but `/etc` is a symlink to `/private/etc`. This results in confusing warning messages like:

```
$ cat /etc/bazel.bazelrc
build --jobs=4
$ bazel version
WARNING: The following rc files are no longer being read, please transfer
their contents or import their path into one of the standard rc files:
/etc/bazel.bazelrc
```
Thanks @excitoon for linking to #6138! That might indeed be the same issue.

~Someone with a Mac at hand, could you please Bazel built at https://github.com/bazelbuild/bazel/commit/e9a908560133770614eca89ef64681bdf3f04b3e and see if that commit fixes this issue?~ Oh wait, never mind, it happens on Linux too I see.
Confirming that https://github.com/bazelbuild/bazel/commit/e9a908560133770614eca89ef64681bdf3f04b3e fixes this issue too, so I'll say this is a duplicate of https://github.com/bazelbuild/bazel/issues/6138.

Pull request review comments:
nit: not that this is very important, but can you maybe get away with `$TEST_TMPDIR` instead? It's kinda confusing because `$HOME` usually means, well, the home directory of some user.
nit: DedupeBazelRcPaths? (it's not a getter)

Also, why "Existing"? It doesn't check existence, does it?
On Linux we do the same: https://github.com/bazelbuild/bazel/blob/420b9533534951b82c1e13e6d83fa67390e95778/src/main/cpp/blaze_util_posix.cc#L205

Bazel actually sets a mock $HOME for the test.
The canonical path can only be computed of existing paths, so the paths indeed exist.

Renamed the method to its old name.

Patch that resolved the issue:
diff --git a/src/main/cpp/blaze_util.cc b/src/main/cpp/blaze_util.cc
index d8ec359515f779..858ac4e73ad1e5 100644
--- a/src/main/cpp/blaze_util.cc
+++ b/src/main/cpp/blaze_util.cc
@@ -181,6 +181,10 @@ void SetDebugLog(bool enabled) {
   }
 }
 
+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 b9f5a518e6b4c9..b0b55385e78542 100644
--- a/src/main/cpp/blaze_util.h
+++ b/src/main/cpp/blaze_util.h
@@ -104,6 +104,10 @@ std::string ToString(const T &value) {
 // 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 3319faf37e3420..1483dfb9b76a22 100644
--- a/src/main/cpp/blaze_util_windows.cc
+++ b/src/main/cpp/blaze_util_windows.cc
@@ -406,6 +406,12 @@ string GetOutputRoot() {
 }
 
 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 b25db9c92996c6..72fab91392edb5 100644
--- a/src/main/cpp/option_processor-internal.h
+++ b/src/main/cpp/option_processor-internal.h
@@ -26,8 +26,10 @@ namespace blaze {
 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
-// omitted.
+// preserving the original order.
+// All paths in the result have been 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 1298ea1f359abe..b1621956bfe6ac 100644
--- a/src/main/cpp/option_processor.cc
+++ b/src/main/cpp/option_processor.cc
@@ -185,18 +185,22 @@ std::set<std::string> GetOldRcPaths(
         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 @@ void WarnAboutDuplicateRcFiles(const std::set<std::string>& read_files,
   }
 }
 
+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 @@ blaze_exit_code::ExitCode OptionProcessor::GetRcFiles(
   // 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,9 @@ blaze_exit_code::ExitCode OptionProcessor::GetRcFiles(
     }
 
     // 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 +412,8 @@ blaze_exit_code::ExitCode OptionProcessor::GetRcFiles(
       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 +635,7 @@ std::vector<std::string> OptionProcessor::GetBlazercAndEnvCommandArgs(
   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 e356ca690d161d..1a0f8429952d93 100644
--- a/src/main/cpp/rc_file.cc
+++ b/src/main/cpp/rc_file.cc
@@ -63,10 +63,10 @@ RcFile::ParseError RcFile::ParseFile(const string& filename,
   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 0d462dcc4bca7a..5f1eee7ceaf9a4 100644
--- a/src/main/cpp/rc_file.h
+++ b/src/main/cpp/rc_file.h
@@ -42,7 +42,9 @@ class RcFile {
       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 @@ class RcFile {
   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 12673846210f14..c343db94d12c94 100644
--- a/src/main/cpp/startup_options.cc
+++ b/src/main/cpp/startup_options.cc
@@ -98,8 +98,7 @@ StartupOptions::StartupOptions(const string &product_name,
       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 '"


Test patch:
diff --git a/src/test/cpp/rc_file_test.cc b/src/test/cpp/rc_file_test.cc
index 01a4d5aecd37f4..a358ba712f2c0b 100644
--- a/src/test/cpp/rc_file_test.cc
+++ b/src/test/cpp/rc_file_test.cc
@@ -173,8 +173,8 @@ TEST_F(GetRcFileTest, GetRcFilesLoadsAllDefaultBazelrcs) {
   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 +195,7 @@ TEST_F(GetRcFileTest, GetRcFilesRespectsNoSystemRc) {
 
   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 +216,7 @@ TEST_F(GetRcFileTest, GetRcFilesRespectsNoWorkspaceRc) {
 
   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 +317,8 @@ TEST_F(GetRcFileTest, GetRcFilesReadsCommandLineRc) {
   // 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 +338,7 @@ TEST_F(GetRcFileTest, GetRcFilesAcceptsNullCommandLineRc) {
   // 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 {