blaze_util::ConvertPath should not make paths absolute.

It does not claim to, and this was already true for posix platforms. Windows platforms, however, always made the path absolute, which was a hard-to-diagnose difference between the two.

Similarly, MakeAbsolute was relying on this to be correct for windows, so this change splits the implementation and keeps the behavior consistent. While we're here, also remove the empty-string behavior from MakeAbsolute, and instead make it clear at all sites that this behavior is present and affects accepted flag syntax. We may want to remove this later.

RELNOTES: None.
PiperOrigin-RevId: 199663395

9 files changed, 53 insertions, 29 deletions

diff --git a/src/main/cpp/blaze_util.cc b/src/main/cpp/blaze_util.cc
index 10c4e1a38f..d8ec359515 100644
--- a/src/main/cpp/blaze_util.cc
+++ b/src/main/cpp/blaze_util.cc
@@ -28,6 +28,7 @@
 #include "src/main/cpp/util/file.h"
 #include "src/main/cpp/util/logging.h"
 #include "src/main/cpp/util/numbers.h"
+#include "src/main/cpp/util/path_platform.h"
 #include "src/main/cpp/util/port.h"
 #include "src/main/cpp/util/strings.h"
 
@@ -118,6 +119,14 @@ bool IsArg(const string& arg) {
       && (arg != "-help") && (arg != "-h");
 }
 
+std::string AbsolutePathFromFlag(const std::string& value) {
+  if (value.empty()) {
+    return blaze_util::GetCwd();
+  } else {
+    return blaze_util::MakeAbsolute(value);
+  }
+}
+
 void LogWait(unsigned int elapsed_seconds, unsigned int wait_seconds) {
   SigPrintf("WARNING: Waiting for server process to terminate "
             "(waited %d seconds, waiting at most %d)\n",
diff --git a/src/main/cpp/blaze_util.h b/src/main/cpp/blaze_util.h
index 084e8d6653..b9f5a518e6 100644
--- a/src/main/cpp/blaze_util.h
+++ b/src/main/cpp/blaze_util.h
@@ -61,6 +61,12 @@ bool SearchNullaryOption(const std::vector<std::string>& args,
 // Returns true iff arg is a valid command line argument for bazel.
 bool IsArg(const std::string& arg);
 
+// Returns the flag value as an absolute path. For legacy reasons, it accepts
+// the empty string as cwd.
+// TODO(b/109874628): Assess if removing the empty string case would break
+// legitimate uses, and if not, remove it.
+std::string AbsolutePathFromFlag(const std::string& value);
+
 // Wait to see if the server process terminates. Checks the server's status
 // immediately, and repeats the check every 100ms until approximately
 // wait_seconds elapses or the server process terminates. Returns true if a
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc
index a1d7f9d976..c2f480f545 100644
--- a/src/main/cpp/option_processor.cc
+++ b/src/main/cpp/option_processor.cc
@@ -138,7 +138,7 @@ blaze_exit_code::ExitCode OptionProcessor::FindUserBlazerc(
       "." + parsed_startup_options_->GetLowercaseProductName() + "rc";
 
   if (cmd_line_rc_file != nullptr) {
-    string rcFile = blaze_util::MakeAbsolute(cmd_line_rc_file);
+    string rcFile = blaze::AbsolutePathFromFlag(cmd_line_rc_file);
     if (!blaze_util::CanReadFile(rcFile)) {
       blaze_util::StringPrintf(error,
           "Error: Unable to read %s file '%s'.", rc_basename.c_str(),
diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc
index 3699d40498..67e0d55add 100644
--- a/src/main/cpp/startup_options.cc
+++ b/src/main/cpp/startup_options.cc
@@ -189,19 +189,19 @@ blaze_exit_code::ExitCode StartupOptions::ProcessArg(
   const char* value = NULL;
 
   if ((value = GetUnaryOption(arg, next_arg, "--output_base")) != NULL) {
-    output_base = blaze_util::MakeAbsolute(value);
+    output_base = blaze::AbsolutePathFromFlag(value);
     option_sources["output_base"] = rcfile;
   } else if ((value = GetUnaryOption(arg, next_arg,
                                      "--install_base")) != NULL) {
-    install_base = blaze_util::MakeAbsolute(value);
+    install_base = blaze::AbsolutePathFromFlag(value);
     option_sources["install_base"] = rcfile;
   } else if ((value = GetUnaryOption(arg, next_arg,
                                      "--output_user_root")) != NULL) {
-    output_user_root = blaze_util::MakeAbsolute(value);
+    output_user_root = blaze::AbsolutePathFromFlag(value);
     option_sources["output_user_root"] = rcfile;
   } else if ((value = GetUnaryOption(arg, next_arg,
                                      "--server_jvm_out")) != NULL) {
-    server_jvm_out = blaze_util::MakeAbsolute(value);
+    server_jvm_out = blaze::AbsolutePathFromFlag(value);
     option_sources["server_jvm_out"] = rcfile;
   } else if (GetNullaryOption(arg, "--deep_execroot")) {
     deep_execroot = true;
@@ -223,7 +223,7 @@ blaze_exit_code::ExitCode StartupOptions::ProcessArg(
                                      "--host_javabase")) != NULL) {
     // TODO(bazel-team): Consider examining the javabase and re-execing in case
     // of architecture mismatch.
-    host_javabase = blaze_util::MakeAbsolute(value);
+    host_javabase = blaze::AbsolutePathFromFlag(value);
     option_sources["host_javabase"] = rcfile;
   } else if ((value = GetUnaryOption(arg, next_arg, "--host_jvm_args")) !=
              NULL) {
diff --git a/src/main/cpp/util/path.cc b/src/main/cpp/util/path.cc
index efa10b81d4..b69a594d5a 100644
--- a/src/main/cpp/util/path.cc
+++ b/src/main/cpp/util/path.cc
@@ -48,17 +48,4 @@ std::string JoinPath(const std::string &path1, const std::string &path2) {
   }
 }
 
-std::string MakeAbsolute(const std::string &path) {
-  std::string converted_path = ConvertPath(path);
-  if (converted_path.empty()) {
-    return GetCwd();
-  }
-  if (IsDevNull(converted_path.c_str()) ||
-      blaze_util::IsAbsolute(converted_path)) {
-    return converted_path;
-  }
-
-  return JoinPath(blaze_util::GetCwd(), converted_path);
-}
-
 }  // namespace blaze_util
diff --git a/src/main/cpp/util/path.h b/src/main/cpp/util/path.h
index 38e735b7af..23edd831ca 100644
--- a/src/main/cpp/util/path.h
+++ b/src/main/cpp/util/path.h
@@ -29,15 +29,6 @@ std::string Basename(const std::string &path);
 
 std::string JoinPath(const std::string &path1, const std::string &path2);
 
-// Returns the given path in absolute form.  Does not change paths that are
-// already absolute.
-//
-// If called from working directory "/bar":
-//   MakeAbsolute("foo") --> "/bar/foo"
-//   MakeAbsolute("/foo") ---> "/foo"
-//   MakeAbsolute("C:/foo") ---> "C:/foo"
-std::string MakeAbsolute(const std::string &path);
-
 }  // namespace blaze_util
 
 #endif  // BAZEL_SRC_MAIN_CPP_UTIL_PATH_H_
diff --git a/src/main/cpp/util/path_platform.h b/src/main/cpp/util/path_platform.h
index abb25dcb75..1f14bd6ab0 100644
--- a/src/main/cpp/util/path_platform.h
+++ b/src/main/cpp/util/path_platform.h
@@ -45,6 +45,15 @@ bool IsRootDirectory(const std::string &path);
 // Returns true if `path` is absolute.
 bool IsAbsolute(const std::string &path);
 
+// Returns the given path in absolute form.  Does not change paths that are
+// already absolute.
+//
+// If called from working directory "/bar":
+//   MakeAbsolute("foo") --> "/bar/foo"
+//   MakeAbsolute("/foo") ---> "/foo"
+//   MakeAbsolute("C:/foo") ---> "C:/foo"
+std::string MakeAbsolute(const std::string &path);
+
 // TODO(bazel-team) consider changing the path(_platform) header split to be a
 // path.h and path_windows.h split, which would make it clearer what functions
 // are included by an import statement. The downside to this gain in clarity
diff --git a/src/main/cpp/util/path_posix.cc b/src/main/cpp/util/path_posix.cc
index bbeffca30c..267dc22931 100644
--- a/src/main/cpp/util/path_posix.cc
+++ b/src/main/cpp/util/path_posix.cc
@@ -20,7 +20,9 @@
 #include <unistd.h>  // access, open, close, fsync
 #include "src/main/cpp/util/errors.h"
 #include "src/main/cpp/util/exit_code.h"
+#include "src/main/cpp/util/file_platform.h"
 #include "src/main/cpp/util/logging.h"
+#include "src/main/cpp/util/path.h"
 
 namespace blaze_util {
 
@@ -57,4 +59,11 @@ bool IsAbsolute(const std::string &path) {
   return !path.empty() && path[0] == '/';
 }
 
+std::string MakeAbsolute(const std::string &path) {
+  if (blaze_util::IsAbsolute(path) || path.empty()) {
+    return path;
+  }
+
+  return JoinPath(blaze_util::GetCwd(), path);
+}
 }  // namespace blaze_util
diff --git a/src/main/cpp/util/path_windows.cc b/src/main/cpp/util/path_windows.cc
index d3756c71bf..041a0e847e 100644
--- a/src/main/cpp/util/path_windows.cc
+++ b/src/main/cpp/util/path_windows.cc
@@ -62,11 +62,24 @@ static bool HasDriveSpecifierPrefix(const char_type* ch) {
 
 std::string ConvertPath(const std::string& path) {
   // The path may not be Windows-style and may not be normalized, so convert it.
+  std::string converted_path;
+  std::string error;
+  if (!blaze_util::AsWindowsPath(path, &converted_path, &error)) {
+    BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
+        << "ConvertPath(" << path << "): AsWindowsPath failed: " << error;
+  }
+  std::transform(converted_path.begin(), converted_path.end(),
+                 converted_path.begin(), ::towlower);
+  return converted_path;
+}
+
+std::string MakeAbsolute(const std::string& path) {
+  // The path may not be Windows-style and may not be normalized, so convert it.
   std::wstring wpath;
   std::string error;
   if (!AsAbsoluteWindowsPath(path, &wpath, &error)) {
     BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
-        << "ConvertPath(" << path
+        << "MakeAbsolute(" << path
         << "): AsAbsoluteWindowsPath failed: " << error;
   }
   std::transform(wpath.begin(), wpath.end(), wpath.begin(), ::towlower);