From 6311552fcc3259d33b7c64dd85f94299a98ecb06 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 10 Sep 2024 13:59:49 -0700 Subject: [PATCH] Generate and use a manifest for prelude files. (#4291) This removes the directory crawl because bazel doesn't remove files from execroot when the rule generating them would no longer generate them. Fixes #4288 --- core/BUILD | 23 +++++++- core/manifest.bzl | 25 +++++++++ toolchain/driver/driver.cpp | 2 +- toolchain/install/install_paths.cpp | 53 ++++++++++--------- toolchain/install/install_paths.h | 6 +-- .../install/install_paths_test_helpers.cpp | 2 +- toolchain/install/symlink_filegroup.bzl | 10 ++-- toolchain/testing/file_test.cpp | 7 +-- 8 files changed, 85 insertions(+), 43 deletions(-) create mode 100644 core/manifest.bzl diff --git a/core/BUILD b/core/BUILD index 8cbc4a3ba923..c951b06b3538 100644 --- a/core/BUILD +++ b/core/BUILD @@ -2,9 +2,28 @@ # Exceptions. See /LICENSE for license information. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -# The prelude, and all of its dependencies. +load("manifest.bzl", "manifest") + +# Raw prelude files. +filegroup( + name = "prelude_files", + srcs = ["prelude.carbon"] + glob(["prelude/**/*.carbon"]), +) + +# A list of prelude inputs. +# This is consumed by //toolchain/install:install_paths. +manifest( + name = "prelude_manifest", + srcs = [":prelude_files"], + out = "prelude_manifest.txt", +) + +# All files for the toolchain install. filegroup( name = "prelude", - srcs = ["prelude.carbon"] + glob(["prelude/**/*.carbon"]), + srcs = [ + ":prelude_files", + ":prelude_manifest.txt", + ], visibility = ["//visibility:public"], ) diff --git a/core/manifest.bzl b/core/manifest.bzl new file mode 100644 index 000000000000..592e276b5bf8 --- /dev/null +++ b/core/manifest.bzl @@ -0,0 +1,25 @@ +# 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 + +"""Rule for producing a manifest for a filegroup.""" + +def _manifest(ctx): + dir = ctx.build_file_path.removesuffix("BUILD") + content = [f.path.removeprefix(dir) for f in ctx.files.srcs] + ctx.actions.write(ctx.outputs.out, "\n".join(content) + "\n") + + return [ + DefaultInfo( + files = depset(direct = [ctx.outputs.out]), + default_runfiles = ctx.runfiles(files = [ctx.outputs.out]), + ), + ] + +manifest = rule( + implementation = _manifest, + attrs = { + "out": attr.output(mandatory = True), + "srcs": attr.label_list(mandatory = True), + }, +) diff --git a/toolchain/driver/driver.cpp b/toolchain/driver/driver.cpp index 5b3873c3ed12..b3b5e7183396 100644 --- a/toolchain/driver/driver.cpp +++ b/toolchain/driver/driver.cpp @@ -829,7 +829,7 @@ auto Driver::Compile(const CompileOptions& options, // package-specific search path based on the library name. llvm::SmallVector prelude; if (options.prelude_import && options.phase >= CompileOptions::Phase::Check) { - if (auto find = installation_->FindPreludeFiles(); find.ok()) { + if (auto find = installation_->ReadPreludeManifest(); find.ok()) { prelude = std::move(*find); } else { error_stream_ << "ERROR: " << find.error() << "\n"; diff --git a/toolchain/install/install_paths.cpp b/toolchain/install/install_paths.cpp index 197598060203..820d4d164625 100644 --- a/toolchain/install/install_paths.cpp +++ b/toolchain/install/install_paths.cpp @@ -7,9 +7,11 @@ #include #include "common/check.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Path.h" +#include "llvm/Support/VirtualFileSystem.h" #include "tools/cpp/runfiles/runfiles.h" namespace Carbon { @@ -76,43 +78,42 @@ auto InstallPaths::Make(llvm::StringRef install_prefix) -> InstallPaths { return paths; } -auto InstallPaths::FindPreludeFiles() const +auto InstallPaths::ReadPreludeManifest() const -> ErrorOr> { // This is structured to avoid a vector copy on success. ErrorOr> result = llvm::SmallVector(); - std::string dir = core_package(); + llvm::SmallString<256> manifest; + llvm::sys::path::append(manifest, llvm::sys::path::Style::posix, + core_package(), "prelude_manifest.txt"); - // Include /core/prelude.carbon, which is the entry point into the - // prelude. - { - llvm::SmallString<256> prelude_file(dir); - llvm::sys::path::append(prelude_file, llvm::sys::path::Style::posix, - "prelude.carbon"); - result->push_back(prelude_file.str().str()); + auto fs = llvm::vfs::getRealFileSystem(); + llvm::ErrorOr> file = + fs->getBufferForFile(manifest); + if (!file) { + result = ErrorBuilder() << "Loading prelude manifest `" << manifest + << "`: " << file.getError().message(); + return result; } - // Glob for /core/prelude/**/*.carbon and add all the files we find. - llvm::SmallString<256> prelude_dir(dir); - llvm::sys::path::append(prelude_dir, llvm::sys::path::Style::posix, - "prelude"); - std::error_code ec; - for (llvm::sys::fs::recursive_directory_iterator prelude_files_it( - prelude_dir, ec, /*follow_symlinks=*/false); - prelude_files_it != llvm::sys::fs::recursive_directory_iterator(); - prelude_files_it.increment(ec)) { - if (ec) { - result = ErrorBuilder() << "Could not find prelude: " << ec.message(); - return result; - } - - auto prelude_file = prelude_files_it->path(); - if (llvm::sys::path::extension(prelude_file) == ".carbon") { - result->push_back(prelude_file); + // The manifest should have one file per line. + llvm::StringRef buffer = file.get()->getBuffer(); + while (true) { + auto [token, remainder] = llvm::getToken(buffer, "\n"); + if (token.empty()) { + break; } + llvm::SmallString<256> path; + llvm::sys::path::append(path, llvm::sys::path::Style::posix, core_package(), + token); + result->push_back(path.str().str()); + buffer = remainder; } + if (result->empty()) { + result = ErrorBuilder() << "Prelude manifest `" << manifest << "` is empty"; + } return result; } diff --git a/toolchain/install/install_paths.h b/toolchain/install/install_paths.h index bb29171904db..3297f25f7af9 100644 --- a/toolchain/install/install_paths.h +++ b/toolchain/install/install_paths.h @@ -87,9 +87,9 @@ class InstallPaths { // using Carbon in an environment with an unusual path to the installed files. static auto Make(llvm::StringRef install_prefix) -> InstallPaths; - // Finds the source files that define the prelude and returns a list of their - // filenames. The list always includes at least one file. - auto FindPreludeFiles() const -> ErrorOr>; + // Returns the contents of the prelude manifest file. This is the list of + // files that define the prelude, and will always be non-empty on success. + auto ReadPreludeManifest() const -> ErrorOr>; // Check for an error detecting the install paths correctly. // diff --git a/toolchain/install/install_paths_test_helpers.cpp b/toolchain/install/install_paths_test_helpers.cpp index c0738224da99..acdda8a6050e 100644 --- a/toolchain/install/install_paths_test_helpers.cpp +++ b/toolchain/install/install_paths_test_helpers.cpp @@ -14,7 +14,7 @@ auto AddPreludeFilesToVfs(InstallPaths install_paths, llvm::vfs::InMemoryFileSystem* vfs) -> void { // Load the prelude into the test VFS. auto real_fs = llvm::vfs::getRealFileSystem(); - auto prelude = install_paths.FindPreludeFiles(); + auto prelude = install_paths.ReadPreludeManifest(); CARBON_CHECK(prelude.ok()) << prelude.error(); for (const auto& path : *prelude) { diff --git a/toolchain/install/symlink_filegroup.bzl b/toolchain/install/symlink_filegroup.bzl index 63747e506e54..89eee0f2c93a 100644 --- a/toolchain/install/symlink_filegroup.bzl +++ b/toolchain/install/symlink_filegroup.bzl @@ -6,13 +6,10 @@ def _symlink_filegroup_impl(ctx): prefix = ctx.attr.out_prefix - remove_prefix = ctx.attr.remove_prefix outputs = [] for f in ctx.files.srcs: - out = ctx.actions.declare_file( - prefix + f.short_path.removeprefix(remove_prefix), - ) + out = ctx.actions.declare_file(prefix + f.short_path) outputs.append(out) ctx.actions.symlink(output = out, target_file = f) @@ -21,8 +18,8 @@ def _symlink_filegroup_impl(ctx): return [ DefaultInfo( - files = depset(outputs), - runfiles = ctx.runfiles(files = outputs), + files = depset(direct = outputs), + default_runfiles = ctx.runfiles(files = outputs), ), ] @@ -30,7 +27,6 @@ symlink_filegroup = rule( implementation = _symlink_filegroup_impl, attrs = { "out_prefix": attr.string(mandatory = True), - "remove_prefix": attr.string(default = ""), "srcs": attr.label_list(mandatory = True), }, ) diff --git a/toolchain/testing/file_test.cpp b/toolchain/testing/file_test.cpp index f69b08492e10..55cd36e4fccc 100644 --- a/toolchain/testing/file_test.cpp +++ b/toolchain/testing/file_test.cpp @@ -36,7 +36,7 @@ class ToolchainFileTest : public FileTestBase { auto Run(const llvm::SmallVector& test_args, llvm::vfs::InMemoryFileSystem& fs, llvm::raw_pwrite_stream& stdout, llvm::raw_pwrite_stream& stderr) -> ErrorOr override { - CARBON_ASSIGN_OR_RETURN(auto prelude, installation_.FindPreludeFiles()); + CARBON_ASSIGN_OR_RETURN(auto prelude, installation_.ReadPreludeManifest()); for (const auto& file : prelude) { CARBON_RETURN_IF_ERROR(AddFile(fs, file)); } @@ -136,10 +136,11 @@ class ToolchainFileTest : public FileTestBase { llvm::ErrorOr> file = llvm::MemoryBuffer::getFile(path); if (file.getError()) { - return ErrorBuilder() << file.getError().message(); + return ErrorBuilder() + << "Getting `" << path << "`: " << file.getError().message(); } if (!fs.addFile(path, /*ModificationTime=*/0, std::move(*file))) { - return ErrorBuilder() << "Duplicate file: " << path; + return ErrorBuilder() << "Duplicate file: `" << path << "`"; } return Success(); }