From c7057eff8918cd37b6dd45cd5987cd6fa910d6aa Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Mon, 23 Sep 2024 15:26:53 -0700 Subject: [PATCH] Improve install_paths handling for relative paths. (#4331) Because install_paths is not presently validated, and it's resolved after the `SetWorkingDirForBazel` call, if a relative path is used with bazel then it would fail silently. This starts making the driver share install path errors, and starts changing how `//toolchain` launches `carbon`. Note the implementation is still brittle and will break with symlinks. That's something I plan to address as part of busyboxing. --- toolchain/BUILD | 2 +- toolchain/driver/driver_main.cpp | 8 ++++++-- toolchain/install/BUILD | 8 ++++++++ toolchain/install/install_paths.cpp | 14 ++++++++++++++ toolchain/install/install_paths.h | 18 ++++++++++++------ toolchain/install/install_paths_test.cpp | 2 +- toolchain/install/run_tool.bzl | 17 +++++++++++++++++ toolchain/install/run_tool.py | 19 +++++++++++++++++++ 8 files changed, 78 insertions(+), 10 deletions(-) create mode 100644 toolchain/install/run_tool.bzl create mode 100644 toolchain/install/run_tool.py diff --git a/toolchain/BUILD b/toolchain/BUILD index 24e73e5e65ca..a6a22fe04c41 100644 --- a/toolchain/BUILD +++ b/toolchain/BUILD @@ -6,5 +6,5 @@ # available. alias( name = "toolchain", - actual = "//toolchain/install:prefix_root/bin/carbon", + actual = "//toolchain/install:run_carbon", ) diff --git a/toolchain/driver/driver_main.cpp b/toolchain/driver/driver_main.cpp index bddf0617887a..af0a908967b8 100644 --- a/toolchain/driver/driver_main.cpp +++ b/toolchain/driver/driver_main.cpp @@ -19,15 +19,19 @@ auto main(int argc, char** argv) -> int { return EXIT_FAILURE; } + // Resolve paths before calling SetWorkingDirForBazel. std::string exe_path = Carbon::FindExecutablePath(argv[0]); + const auto install_paths = Carbon::InstallPaths::MakeExeRelative(exe_path); + if (install_paths.error()) { + llvm::errs() << "error: " << *install_paths.error(); + return EXIT_FAILURE; + } Carbon::SetWorkingDirForBazel(); llvm::SmallVector args(argv + 1, argv + argc); auto fs = llvm::vfs::getRealFileSystem(); - const auto install_paths = Carbon::InstallPaths::MakeExeRelative(exe_path); - Carbon::Driver driver(*fs, &install_paths, llvm::outs(), llvm::errs()); bool success = driver.RunCommand(args).success; return success ? EXIT_SUCCESS : EXIT_FAILURE; diff --git a/toolchain/install/BUILD b/toolchain/install/BUILD index 6ae9f1d84749..773b1436843e 100644 --- a/toolchain/install/BUILD +++ b/toolchain/install/BUILD @@ -7,6 +7,7 @@ load("@llvm-project//llvm:binary_alias.bzl", "binary_alias") load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library", "cc_test") load("@rules_pkg//pkg:mappings.bzl", "pkg_attributes", "pkg_filegroup", "pkg_files", "pkg_mklink", "strip_prefix") load("pkg_helpers.bzl", "pkg_naming_variables", "pkg_tar_and_test") +load("run_tool.bzl", "run_tool") load("symlink_filegroup.bzl", "symlink_filegroup") package(default_visibility = ["//visibility:public"]) @@ -230,3 +231,10 @@ pkg_tar_and_test( ], test_install_marker = ":install_marker", ) + +# Support `bazel run` on specific binaries. +run_tool( + name = "run_carbon", + data = [":install_data"], + tool = "prefix_root/bin/carbon", +) diff --git a/toolchain/install/install_paths.cpp b/toolchain/install/install_paths.cpp index 7e8e88dc87a3..f752036c5bf3 100644 --- a/toolchain/install/install_paths.cpp +++ b/toolchain/install/install_paths.cpp @@ -44,6 +44,11 @@ auto InstallPaths::MakeExeRelative(llvm::StringRef exe_path) -> InstallPaths { llvm::sys::path::remove_filename(paths.prefix_); llvm::sys::path::append(paths.prefix_, llvm::sys::path::Style::posix, "../"); + if (auto error = llvm::sys::fs::make_absolute(paths.prefix_)) { + paths.SetError(error.message()); + return paths; + } + paths.CheckMarkerFile(); return paths; } @@ -67,6 +72,11 @@ auto InstallPaths::MakeForBazelRunfiles(llvm::StringRef exe_path) llvm::sys::path::append(paths.prefix_, llvm::sys::path::Style::posix, "../../"); + if (auto error = llvm::sys::fs::make_absolute(paths.prefix_)) { + paths.SetError(error.message()); + return paths; + } + paths.CheckMarkerFile(); CARBON_CHECK(!paths.error(), "{0}", *paths.error()); return paths; @@ -125,6 +135,10 @@ auto InstallPaths::SetError(llvm::Twine message) -> void { } auto InstallPaths::CheckMarkerFile() -> void { + if (!llvm::sys::path::is_absolute(prefix_)) { + SetError(llvm::Twine("Not an absolute path: ") + prefix_); + } + llvm::SmallString<256> path(prefix_); llvm::sys::path::append(path, llvm::sys::path::Style::posix, MarkerPath); if (!llvm::sys::fs::exists(path)) { diff --git a/toolchain/install/install_paths.h b/toolchain/install/install_paths.h index 3297f25f7af9..f579ca708376 100644 --- a/toolchain/install/install_paths.h +++ b/toolchain/install/install_paths.h @@ -32,10 +32,10 @@ namespace Carbon { // - MakeForBazelRunfiles for locating through Bazel's runfile tree. // - Make for an explicit path, for example in tests. // -// When locating an install, we verify it by -// looking for the `carbon_install.txt` marker file at a specific location -// below. When errors occur, the install prefix is made empty, and error() can -// be used for diagnostics; InstallPaths remains minimally functional. +// When locating an install, we verify it by looking for the +// `carbon_install.txt` marker file at a specific location below. When errors +// occur, the install prefix is made empty, and error() can be used for +// diagnostics; InstallPaths remains minimally functional. // // Within this prefix, we expect a hierarchy on Unix-y platforms: // @@ -83,8 +83,9 @@ class InstallPaths { // fails for any reason, it will `CARBON_CHECK` fail with the error message. static auto MakeForBazelRunfiles(llvm::StringRef exe_path) -> InstallPaths; - // Provide an explicit install paths prefix. This is useful for testing or for - // using Carbon in an environment with an unusual path to the installed files. + // Provide an explicit install paths prefix, which must be absolute. This is + // useful for testing or for using Carbon in an environment with an unusual + // path to the installed files. static auto Make(llvm::StringRef install_prefix) -> InstallPaths; // Returns the contents of the prelude manifest file. This is the list of @@ -106,6 +107,11 @@ class InstallPaths { // `prefix_root` directory in Bazel's output, or to some prefix the toolchain // is installed into on a system such as `/usr/local` or `/home/$USER`. // + // This will be an absolute path. We keep an absolute path for when the + // command line uses a relative path (`./bin/carbon`) and the working + // directory changes after initialization (for example, to Bazel's working + // directory). + // // In the event of an error, this will be the empty string. auto prefix() const -> llvm::StringRef { return prefix_; } diff --git a/toolchain/install/install_paths_test.cpp b/toolchain/install/install_paths_test.cpp index 79668fae0489..df9fe10a1c5c 100644 --- a/toolchain/install/install_paths_test.cpp +++ b/toolchain/install/install_paths_test.cpp @@ -118,7 +118,7 @@ TEST_F(InstallPathsTest, BinaryRunfiles) { } TEST_F(InstallPathsTest, Errors) { - auto paths = InstallPaths::Make("foo/bar/baz"); + auto paths = InstallPaths::Make("/foo/bar/baz"); EXPECT_THAT(paths.error(), Optional(HasSubstr("foo/bar/baz"))); EXPECT_THAT(paths.prefix(), Eq("")); diff --git a/toolchain/install/run_tool.bzl b/toolchain/install/run_tool.bzl new file mode 100644 index 000000000000..3b65723ce06f --- /dev/null +++ b/toolchain/install/run_tool.bzl @@ -0,0 +1,17 @@ +# Part of the Carbon Language project, under the Apache License v2.0 with LLVM +# Exceptions. See /LICENSE for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +"""Supports running a tool from the install filegroup.""" + +load("@rules_python//python:defs.bzl", "py_binary") + +def run_tool(name, tool, data): + # TODO: Fix the driver file discovery in order to allow symlinks. + py_binary( + name = name, + main = "run_tool.py", + srcs = ["run_tool.py"], + args = ["$(location {})".format(tool)], + data = [tool] + data, + ) diff --git a/toolchain/install/run_tool.py b/toolchain/install/run_tool.py new file mode 100644 index 000000000000..732c673b25a1 --- /dev/null +++ b/toolchain/install/run_tool.py @@ -0,0 +1,19 @@ +#!/usr/bin/env python3 + +"""Runs the tool specified in argv. + +This script is essentially just a bounce-through to get an appropriate arg0. +See the TODO in run_tool.bzl. +""" + +__copyright__ = """ +Part of the Carbon Language project, under the Apache License v2.0 with LLVM +Exceptions. See /LICENSE for license information. +SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +""" + +import os +import sys + +if __name__ == "__main__": + os.execv(sys.argv[1], sys.argv[1:])