Refactor InstallPaths API and comments a little. (#4341)

Stemming from #4331, trying to break apart InstallPaths class comments
into three parts:

- Construction semantics, staying in the class comment
  - Trying to refer to methods with more detailed  documentation.
- Install prefix contents, now on `prefix_`
- Install structure, consolidating on `install_dirs`

For code refactoring, `driver()` and `prefix()` were only used by the
install paths test. Rather than having a comment not to use `prefix()`,
this instead extracts it out to a TestPeer model (which we have
elsewhere with `TypedNodesTestPeer`, thus my choice in approaches).

toolchain/install/BUILD

@@ -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
@@ -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(

toolchain/install/install_paths.cpp

@@ -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.

toolchain/install/install_paths.h

@@ -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
@@ -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`
+  //
+  // 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_;
 };
 

toolchain/install/install_paths_test.cpp

@@ -16,6 +16,14 @@
 #include "tools/cpp/runfiles/runfiles.h"
 
 namespace Carbon {
+
+class InstallPathsTestPeer {
+ public:
+  static auto GetPrefix(const InstallPaths& paths) -> llvm::StringRef {
+    return paths.prefix_;
+  }
+};
+
 namespace {
 
 using ::bazel::tools::cpp::runfiles::Runfiles;
@@ -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;
@@ -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.