Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor InstallPaths API and comments a little. #4341

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Refactor InstallPaths API and comments a little. #4341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion toolchain/install/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ package(default_visibility = ["//visibility:public"])
# Build rules supporting the install data tree for the Carbon toolchain.
#
# This populates a synthetic Carbon toolchain installation under the
# `prefix_root` directory. For details on its layout, see `install_paths.h`.
# `prefix_root` directory. For details on its layout, see `install_dirs` below.

# A library for computing install paths for the toolchain. Note that this
# library does *not* include the data itself, as that would form a dependency
Expand Down Expand Up @@ -76,6 +76,15 @@ lld_aliases = [
"wasm-ld",
]

# Given a root `prefix_root`, the hierarchy looks like:
#
# - prefix_root/bin: Binaries intended for direct use.
# - prefix_root/lib/carbon: Private data and files.
# - prefix_root/lib/carbon/core: The `Core` package files.
# - prefix_root/lib/carbon/llvm/bin: LLVM binaries.
#
# This will be how installs are provided on Unix-y platforms, and is loosely
# based on the FHS (Filesystem Hierarchy Standard).
install_dirs = {
"bin": [
install_target(
Expand Down
7 changes: 0 additions & 7 deletions toolchain/install/install_paths.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,13 +146,6 @@ auto InstallPaths::CheckMarkerFile() -> void {
}
}

auto InstallPaths::driver() const -> std::string {
llvm::SmallString<256> path(prefix_);
// TODO: Adjust this to work equally well on Windows.
llvm::sys::path::append(path, llvm::sys::path::Style::posix, "bin/carbon");
return path.str().str();
}

auto InstallPaths::core_package() const -> std::string {
llvm::SmallString<256> path(prefix_);
// TODO: Adjust this to work equally well on Windows.
Expand Down
95 changes: 43 additions & 52 deletions toolchain/install/install_paths.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,48 +14,23 @@ namespace Carbon {

// Locates the toolchain installation and provides paths to various components.
//
// The Carbon toolchain expects to be installed into some install prefix. For
// example, this is expected to be similar to the CMake install prefix:
// The Carbon toolchain expects to be installed into some install prefix; see
// `prefix_` for details. When locating an install, we verify it with
// `CheckMarkerFile`. When errors occur, `SetError` makes `error()`
// available for diagnostics and clears the install prefix (leaving things
// minimally functional).
//
// - `C:/Program Files/Carbon` or similar on Windows.
// - `/usr` or `/usr/local` on Linux and most BSDs.
// - `/opt/homebrew` or similar on macOS with Homebrew.
// - `bazel-bin/some/bazel/target.runfiles/_main/toolchain/install/prefix_root`
// for unit tests and just-built binaries during development.
// The factory methods locate the install prefix based on their use-case:
//
// See https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html
// for more details. While we don't build the toolchain with CMake, we expect
// our installation to behave in a similar and compatible way.
//
// There are multiple ways of locating an install's prefix:
// - MakeExeRelative for command line tools in an install.
// - 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.
//
// Within this prefix, we expect a hierarchy on Unix-y platforms:
//
// - `prefix_root/bin/carbon` - the main CLI driver
// - `prefix_root/lib/carbon/carbon_install.txt` - a marker for the install
// - `prefix_root/lib/carbon/...` - private data & binaries
//
// This is loosely based on the FHS (Filesystem Hierarchy Standard).
// - `MakeExeRelative` for command line tools in an install.
// - `MakeForBazelRunfiles` for locating through Bazel's runfile tree.
// - `Make` for an explicit path, for example in tests.
//
// An instance of this class provides methods that query for specific paths
// within the install. Note that we want to abstract away any platform
// differences in the installation layout, and so while there are some broad
// paths available here, like the `prefix` method, those should primarily be
// used for logging or debugging. When a specific part of the install is needed,
// a dedicated accessor should be added that computes the path for that
// component.
//
// Path accessor methods on the class return `llvm::StringRef` for any paths
// that are stored in the class, and a `std::string` for any that are computed
// on demand.
// differences in the installation layout. When a specific part of the install
// is needed, a dedicated accessor should be added that computes the path for
// that component.
//
// TODO: Need to check the installation structure of LLVM on Windows and figure
// out what Carbon's should be within a Windows prefix and how much of the
Expand Down Expand Up @@ -103,35 +78,51 @@ class InstallPaths {
return error_;
}

// The computed installation prefix. This should correspond to the
// `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_; }

auto driver() const -> std::string;
// The directory containing the `Core` package. Computed on demand.
auto core_package() const -> std::string;

// The directory containing LLVM install binaries. Computed on demand.
auto llvm_install_bin() const -> std::string;

private:
friend class InstallPathsTestPeer;

InstallPaths() { SetError("No prefix provided!"); }
explicit InstallPaths(llvm::StringRef prefix) : prefix_(prefix) {}

// Set an error message on the install paths and reset the prefix to empty,
// which should use the current working directory.
auto SetError(llvm::Twine message) -> void;

// Check that the install paths have a marker file at the expected location,
// and if not calls `SetError` with the relevant error message.
// Check that the install paths have a marker file at
// `prefix()/lib/carbon/carbon_install.txt". If not, calls `SetError` with the
// relevant error message.
auto CheckMarkerFile() -> void;

// The computed installation prefix. 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.
//
// When run from bazel (for example, in unit tests or development binaries)
// this will look like:
// `bazel-bin/some/bazel/target.runfiles/_main/toolchain/install/prefix_root`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prefix_root at the end looks a little suspicious to me. Is that right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use an explicit prefix_root for all the things installed in the package to keep them distinct from the code and other things in the install package for managing an installation.

//
// When installed, it's expected to be similar to the CMake install prefix:
//
// - `C:/Program Files/Carbon` or similar on Windows.
// - `/usr` or `/usr/local` on Linux and most BSDs.
// - `/opt/homebrew` or similar on macOS with Homebrew.
//
// See https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html
// for more details. While we don't build the toolchain with CMake, we expect
// our installation to behave in a similar and compatible way.
//
// The hierarchy of files beneath the install prefix can be found in the
// BUILD's `install_dirs`.
llvm::SmallString<256> prefix_;

std::optional<std::string> error_;
};

Expand Down
23 changes: 17 additions & 6 deletions toolchain/install/install_paths_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@
#include "tools/cpp/runfiles/runfiles.h"

namespace Carbon {

class InstallPathsTestPeer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not for this PR, maybe for a follow-up)

Is "test peer" a well established term? I couldn't find it when searching, and it was new to me. And the term "peer" is specific enough that I somewhat expect it to have a fairly specific meaning.

If its not that established, I'm still perfectly happy with the pattern itself to allow testing -- but maybe ...TestHelper as a name to be a little more generic? But as I said, not this PR, a follow-up to maybe rename both if folks like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Peer specifically comes from https://abseil.io/tips/135

Copy link
Contributor Author

@jonmeow jonmeow Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, left to my own devices, I'd just make InstallPathsTest itself the friend... (I would mainly expect peers/helpers for something we use in multiple places, not just once)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm merging, expecting we'll come up with some unified solution for InstallPathsTestPeer, TypedNodesTestPeer, and InstTestHelper)

public:
static auto GetPrefix(const InstallPaths& paths) -> llvm::StringRef {
return paths.prefix_;
}
};

namespace {

using ::bazel::tools::cpp::runfiles::Runfiles;
Expand All @@ -37,17 +45,20 @@ class InstallPathsTest : public ::testing::Test {
// check that the path accessors point to the right kind of file or
// directory.
auto TestInstallPaths(const InstallPaths& paths) -> void {
SCOPED_TRACE(llvm::formatv("Install prefix: '{0}'", paths.prefix()));
auto prefix = InstallPathsTestPeer::GetPrefix(paths);

SCOPED_TRACE(llvm::formatv("Install prefix: '{0}'", prefix));

// Grab a the prefix into a string to make it easier to use in the test.
std::string prefix = paths.prefix().str();
EXPECT_TRUE(llvm::sys::fs::exists(prefix));
EXPECT_TRUE(llvm::sys::fs::is_directory(prefix));

// Now check that all the expected parts of the toolchain's install are in
// fact found using the API.
std::string driver_path = paths.driver();
ASSERT_THAT(driver_path, StartsWith(prefix));
llvm::SmallString<256> driver_path(prefix);
// TODO: Adjust this to work equally well on Windows.
llvm::sys::path::append(driver_path, llvm::sys::path::Style::posix,
"bin/carbon");
EXPECT_TRUE(llvm::sys::fs::exists(driver_path)) << "path: " << driver_path;
EXPECT_TRUE(llvm::sys::fs::can_execute(driver_path))
<< "path: " << driver_path;
Expand Down Expand Up @@ -120,11 +131,11 @@ TEST_F(InstallPathsTest, BinaryRunfiles) {
TEST_F(InstallPathsTest, Errors) {
auto paths = InstallPaths::Make("/foo/bar/baz");
EXPECT_THAT(paths.error(), Optional(HasSubstr("foo/bar/baz")));
EXPECT_THAT(paths.prefix(), Eq(""));
EXPECT_THAT(InstallPathsTestPeer::GetPrefix(paths), Eq(""));

paths = InstallPaths::MakeExeRelative("foo/bar/baz");
EXPECT_THAT(paths.error(), Optional(HasSubstr("foo/bar/baz")));
EXPECT_THAT(paths.prefix(), Eq(""));
EXPECT_THAT(InstallPathsTestPeer::GetPrefix(paths), Eq(""));

// Note that we can't test the runfiles code path from within a test because
// it succeeds some of the time even with a bogus executable name.
Expand Down
Loading