Reduce `../` traversal in busybox logic (#6721)

This removes support for strange symlink structures _within_ an
install-shaped tree, but AFAIK, that is not one of the (frustratingly
many) cases where we need them. Avoiding this significantly shortens and
reduces repetition in the commandline formed by the busybox, and also
appears to work better when running the busybox from inside a Bazel
checkout.

The motivation here is to fix issues that arose when more heavily using
the installed toolchain with the example Bazel project. As more of that
functionality lands, this should also be tested there.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

toolchain/install/busybox_info.cpp

@@ -22,6 +22,22 @@ static auto GetMode(const std::filesystem::path& argv0)
   return std::nullopt;
 }
 
+// Try to walk up the path using `.parent_path()` if we can to avoid extra
+// components to resolve. However, if the path is relative to the current
+// working directory and we run out of parent components, walk up by appending
+// `../` components instead.
+static auto WalkUp(std::filesystem::path p) -> std::filesystem::path {
+  // Remove `./` components.
+  while (p.filename() == ".") {
+    p = p.parent_path();
+  }
+  if (!p.is_absolute() && (p.empty() || p.filename() == "..")) {
+    return p / "..";
+  } else {
+    return p.parent_path();
+  }
+}
+
 auto GetBusyboxInfo(const char* argv0) -> ErrorOr<BusyboxInfo> {
   // Need storage due to `unsetenv` affecting `getenv` lifetime; using `path`
   // for `GetMode`.
@@ -73,9 +89,14 @@ auto GetBusyboxInfo(const char* argv0) -> ErrorOr<BusyboxInfo> {
       parent_path = parent_path.parent_path();
     }
     if (parent_path.filename() == "bin") {
+      // Note that we use a specialized approach to walking up rather than
+      // always appending `../` components. While largely equivalent, this helps
+      // keep paths shorter and avoids redundant work. We also don't expect to
+      // need to respect _internally_ strange symlinking structures that would
+      // need to use appended `../` components.
       auto lib_path = info.bin_path.filename() == "carbon"
-                          ? parent_path / ".." / "lib" / "carbon"
-                          : parent_path / ".." / "..";
+                          ? WalkUp(std::move(parent_path)) / "lib" / "carbon"
+                          : WalkUp(WalkUp(std::move(parent_path)));
       auto busybox_path = lib_path / "carbon-busybox";
       if (auto access = Filesystem::Cwd().Access(busybox_path);
           access.ok() && *access) {

toolchain/install/busybox_info_test.cpp

@@ -54,6 +54,13 @@ class BusyboxInfoTest : public ::testing::Test {
     return path_ / prefix;
   }
 
+  // Helper function to change the working directory and check for an error.
+  auto ChangeWorkingDir(std::filesystem::path p) -> void {
+    std::error_code ec;
+    std::filesystem::current_path(p, ec);
+    CARBON_CHECK(!ec, "Error changing working directory: {0}", ec.message());
+  }
+
   // The path to the running binary, `busybox_info_test`. This is provided
   // because `GetExecutablePath` can fall back to it.
   std::string running_binary_;
@@ -193,22 +200,75 @@ TEST_F(BusyboxInfoTest, LayerSymlinksInstallTree) {
 
   auto info = GetBusyboxInfo((prefix / "bin/carbon").c_str());
   ASSERT_TRUE(info.ok()) << info.error();
-  EXPECT_THAT(info->bin_path, Eq(prefix / "bin/../lib/carbon/carbon-busybox"));
+  EXPECT_THAT(info->bin_path, Eq(prefix / "lib/carbon/carbon-busybox"));
   EXPECT_THAT(info->mode, Eq(std::nullopt));
 
   info = GetBusyboxInfo((prefix / "lib/carbon/llvm/bin/clang").c_str());
   ASSERT_TRUE(info.ok()) << info.error();
-  EXPECT_THAT(info->bin_path,
-              Eq(prefix / "lib/carbon/llvm/bin/../../carbon-busybox"));
+  EXPECT_THAT(info->bin_path, Eq(prefix / "lib/carbon/carbon-busybox"));
   EXPECT_THAT(info->mode, Eq("clang"));
 
   info = GetBusyboxInfo((prefix / "lib/carbon/llvm/bin/clang++").c_str());
   ASSERT_TRUE(info.ok()) << info.error();
-  EXPECT_THAT(info->bin_path,
-              Eq(prefix / "lib/carbon/llvm/bin/../../carbon-busybox"));
+  EXPECT_THAT(info->bin_path, Eq(prefix / "lib/carbon/carbon-busybox"));
   EXPECT_THAT(info->mode, Eq("clang++"));
 }
 
+TEST_F(BusyboxInfoTest, RunWithinInstallTree) {
+  dir_.Symlink("actual-busybox", running_binary_).Check();
+
+  // Create a facsimile of the install prefix with even the busybox as a
+  // symlink. Also include potential relative sibling symlinks like `clang++` to
+  // `clang`.
+  auto prefix = MakeInstallTree("test_prefix", (path_ / "actual-busybox"));
+
+  std::error_code ec;
+  auto orig_cwd = std::filesystem::current_path(ec);
+  CARBON_CHECK(!ec, "Error getting working directory: {0}", ec.message());
+  auto restore_cwd = llvm::scope_exit([&] {
+    std::filesystem::current_path(orig_cwd, ec);
+    CARBON_CHECK(!ec, "Error changing working directory: {0}", ec.message());
+  });
+
+  // First test using a working-directory relative `./bin/carbon` path.
+  ChangeWorkingDir(prefix);
+
+  auto info = GetBusyboxInfo("./bin/carbon");
+  ASSERT_TRUE(info.ok()) << info.error();
+  EXPECT_THAT(info->bin_path, Eq("./lib/carbon/carbon-busybox"));
+  EXPECT_THAT(info->mode, Eq(std::nullopt));
+
+  // Also test using a working-directory relative `./bin/clang` path.
+  ChangeWorkingDir(prefix / "lib/carbon/llvm");
+
+  info = GetBusyboxInfo("./bin/clang");
+  ASSERT_TRUE(info.ok()) << info.error();
+  EXPECT_THAT(info->bin_path, Eq("../carbon-busybox"));
+  EXPECT_THAT(info->mode, Eq("clang"));
+
+  // Include redundant `./` components that should be stripped before we give up
+  // and use `../` components.
+  info = GetBusyboxInfo("./././bin/clang");
+  ASSERT_TRUE(info.ok()) << info.error();
+  EXPECT_THAT(info->bin_path, Eq("../carbon-busybox"));
+  EXPECT_THAT(info->mode, Eq("clang"));
+
+  // Also test using a working-directory relative `./llvm/bin/clang` path.
+  ChangeWorkingDir(prefix / "lib/carbon");
+
+  info = GetBusyboxInfo("./llvm/bin/clang");
+  ASSERT_TRUE(info.ok()) << info.error();
+  EXPECT_THAT(info->bin_path, Eq("./carbon-busybox"));
+  EXPECT_THAT(info->mode, Eq("clang"));
+
+  // Include redundant `./` components at  multiple levels, only one of which we
+  // end up needing to strip off.
+  info = GetBusyboxInfo("././llvm/././bin/clang");
+  ASSERT_TRUE(info.ok()) << info.error();
+  EXPECT_THAT(info->bin_path, Eq("././carbon-busybox"));
+  EXPECT_THAT(info->mode, Eq("clang"));
+}
+
 TEST_F(BusyboxInfoTest, StopSearchAtFirstSymlinkWithRelativeBusybox) {
   // Some install of Carbon under `opt`.
   std::filesystem::path opt_prefix = MakeInstallTree("opt");
@@ -229,11 +289,10 @@ TEST_F(BusyboxInfoTest, StopSearchAtFirstSymlinkWithRelativeBusybox) {
   // traversing the symlink further.
   auto info = GetBusyboxInfo((path_ / "bin/carbon").c_str());
   ASSERT_TRUE(info.ok()) << info.error();
-  EXPECT_THAT(info->bin_path, Eq(path_ / "bin/../lib/carbon/carbon-busybox"));
+  EXPECT_THAT(info->bin_path, Eq(path_ / "lib/carbon/carbon-busybox"));
   info = GetBusyboxInfo((path_ / "lib/carbon/llvm/bin/clang").c_str());
   ASSERT_TRUE(info.ok()) << info.error();
-  EXPECT_THAT(info->bin_path,
-              Eq(path_ / "lib/carbon/llvm/bin/../../carbon-busybox"));
+  EXPECT_THAT(info->bin_path, Eq(path_ / "lib/carbon/carbon-busybox"));
 }
 
 TEST_F(BusyboxInfoTest, RejectSymlinkInUnrelatedInstall) {
@@ -255,8 +314,7 @@ TEST_F(BusyboxInfoTest, RejectSymlinkInUnrelatedInstall) {
   // walks the symlink to find the correct installation.
   auto info = GetBusyboxInfo((usr_local / "carbon").c_str());
   ASSERT_TRUE(info.ok()) << info.error();
-  EXPECT_THAT(info->bin_path,
-              Eq(usr_local / "bin/../lib/carbon/carbon-busybox"));
+  EXPECT_THAT(info->bin_path, Eq(usr_local / "lib/carbon/carbon-busybox"));
 
   // Ensure this works even with intervening `.` directory components.
   usr_local_dir.Symlink("carbon2", "bin/././carbon").Check();
@@ -265,8 +323,7 @@ TEST_F(BusyboxInfoTest, RejectSymlinkInUnrelatedInstall) {
   // walks the symlink to find the correct installation.
   info = GetBusyboxInfo((usr_local / "carbon2").c_str());
   ASSERT_TRUE(info.ok()) << info.error();
-  EXPECT_THAT(info->bin_path,
-              Eq(usr_local / "bin/../lib/carbon/carbon-busybox"));
+  EXPECT_THAT(info->bin_path, Eq(usr_local / "lib/carbon/carbon-busybox"));
 }
 
 TEST_F(BusyboxInfoTest, EnvBinaryPathOverride) {