From b267ec85cf67af4df84ac074f2c8125f0ee4a089 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Fri, 13 Feb 2026 01:29:19 -0800 Subject: [PATCH] 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 --- toolchain/install/busybox_info.cpp | 25 +++++++- toolchain/install/busybox_info_test.cpp | 81 +++++++++++++++++++++---- 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/toolchain/install/busybox_info.cpp b/toolchain/install/busybox_info.cpp index 5b4f9efcf2b4..ed183e05156e 100644 --- a/toolchain/install/busybox_info.cpp +++ b/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 { // Need storage due to `unsetenv` affecting `getenv` lifetime; using `path` // for `GetMode`. @@ -73,9 +89,14 @@ auto GetBusyboxInfo(const char* argv0) -> ErrorOr { 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) { diff --git a/toolchain/install/busybox_info_test.cpp b/toolchain/install/busybox_info_test.cpp index 23de45d336da..faf3760c483a 100644 --- a/toolchain/install/busybox_info_test.cpp +++ b/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) {