From 9546535e7f4f3348b4003462564199e4b09f42b7 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 17 May 2023 04:11:18 -0700 Subject: [PATCH] Strip rust-toolchain.toml from current directory of build script runs Summary: See the explanation on `create_cwd`. This fixes the incompatibility with proc-macro2 1.0.57 observed in D45864167. Alternatives considered: - `http_archive` has an `excludes` mechanism which could work here, but it is hideous because it is based on regex, unlike everything else in BUCK which use globs. `excludes = ["^proc-macro2-1\\.0\\.57/rust-toolchain(\\.toml)?$"]`. The approach in this diff avoids adding any noise into the reindeer-generated targets. - I considered implementing a workaround in cargo_buildscript.bzl instead of in buildscript_run.py, basically by making a `filegroup` that overlays `rust-toolchain/.go_away_rustup` + `rust-toolchain.toml/.go_away_rustup` on top of the crate's sources. Having rust-toolchain.toml be a directory causes rustup's file read of that path to fail, which it treats the same as if the path does not exist. I prefer the more straightforward workaround in this diff. Reviewed By: themarwhal Differential Revision: D45942826 fbshipit-source-id: 730f2e0568645825f76f4f4cfec600a2ebd4a444 --- prelude/rust/cargo_buildscript.bzl | 15 ++++-- prelude/rust/tools/buildscript_run.py | 66 +++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/prelude/rust/cargo_buildscript.bzl b/prelude/rust/cargo_buildscript.bzl index 3ea42376..0b88941b 100644 --- a/prelude/rust/cargo_buildscript.bzl +++ b/prelude/rust/cargo_buildscript.bzl @@ -27,7 +27,7 @@ load("@prelude//decls/toolchains_common.bzl", "toolchains_common") def _cargo_buildscript_impl(ctx: "context") -> ["provider"]: toolchain_info = ctx.attrs._rust_toolchain[RustToolchainInfo] - manifest_dir = ctx.attrs.manifest_dir[DefaultInfo].default_outputs[0] + cwd = ctx.actions.declare_output("cwd", dir = True) out_dir = ctx.actions.declare_output("OUT_DIR", dir = True) rustc_flags = ctx.actions.declare_output("rustc_flags") @@ -35,6 +35,8 @@ def _cargo_buildscript_impl(ctx: "context") -> ["provider"]: ctx.attrs.runner[RunInfo], cmd_args("--buildscript=", ctx.attrs.buildscript[RunInfo], delimiter = ""), cmd_args("--rustc-cfg=", ctx.attrs.rustc_cfg[DefaultInfo].default_outputs[0], delimiter = ""), + cmd_args("--manifest-dir=", ctx.attrs.manifest_dir[DefaultInfo].default_outputs[0], delimiter = ""), + cmd_args("--create-cwd=", cwd.as_output(), delimiter = ""), cmd_args("--outfile=", rustc_flags.as_output(), delimiter = ""), ) @@ -42,11 +44,10 @@ def _cargo_buildscript_impl(ctx: "context") -> ["provider"]: env = {} env["CARGO"] = "/bin/false" - env["CARGO_MANIFEST_DIR"] = manifest_dir env["CARGO_PKG_NAME"] = ctx.attrs.package_name env["CARGO_PKG_VERSION"] = ctx.attrs.version env["OUT_DIR"] = out_dir.as_output() - env["RUSTC"] = cmd_args(toolchain_info.compiler).relative_to(manifest_dir) + env["RUSTC"] = cmd_args(toolchain_info.compiler).relative_to(cwd) env["RUSTC_LINKER"] = "/bin/false" env["RUST_BACKTRACE"] = "1" env["TARGET"] = toolchain_info.rustc_target_triple @@ -62,7 +63,7 @@ def _cargo_buildscript_impl(ctx: "context") -> ["provider"]: # Environment variables specified in the target's attributes get priority # over all the above. for k, v in ctx.attrs.env.items(): - env[k] = cmd_args(v).relative_to(manifest_dir) + env[k] = cmd_args(v).relative_to(cwd) ctx.actions.run( cmd, @@ -119,7 +120,11 @@ def buildscript_run( path.removeprefix(prefix_with_trailing_slash): path for path in glob( include = ["{}/**".format(prefix)], - exclude = ["{}/METADATA.bzl".format(prefix)], + exclude = [ + "{}/METADATA.bzl".format(prefix), + "{}/rust-toolchain".format(prefix), + "{}/rust-toolchain.toml".format(prefix), + ], ) }, copy = False, diff --git a/prelude/rust/tools/buildscript_run.py b/prelude/rust/tools/buildscript_run.py index b272eb4a..21513177 100755 --- a/prelude/rust/tools/buildscript_run.py +++ b/prelude/rust/tools/buildscript_run.py @@ -42,6 +42,62 @@ def cfg_env(rustc_cfg: Path) -> Dict[str, str]: return cfgs +def create_cwd(path: Path, manifest_dir: Path) -> Path: + """Create a directory with most of the same contents as manifest_dir, but + excluding Rustup's rust-toolchain.toml configuration file. + + Keeping rust-toolchain.toml goes wrong in the situation that all of the + following happen: + + 1. toolchains//:rust uses compiler = "rustc", like the + system_rust_toolchain. + + 2. The rustc in $PATH is rustup's rustc shim. + + 3. A third-party dependency has both a rust-toolchain.toml and a build.rs + that runs "rustc" or env::var_os("RUSTC"), such as to inspect `rustc + --version` or to compile autocfg-style probe code. + + Cargo defines that build scripts run using the package's manifest directory + as the current directory, so the rustc subprocess spawned from build.rs + would also run in that manifest directory. But other rustc invocations + performed by Buck run from the repo root. + + Rustup only looks at one rust-toolchain.toml file, using the nearest one + present in any parent directory. The file can set `channel` to control which + installed version of rustc to run. + + It is bad if it's possible for the rustc run by a build script vs rustc run + by the rest of the build to be different toolchains. In order to configure + their crate appropriately, build scripts rely on using the same rustc that + their crate will be later compiled by. + + This problem doesn't happen during Cargo-based builds because rustup + installs both a cargo shim and a rustc shim. When you run a rustup-managed + Cargo, one of the first things it does is define a RUSTUP_TOOLCHAIN + environment variable pointing to the rustup channel id of the currently + selected cargo. Subsequent invocations of the rustup cargo shim or rustc + shim with this variable in the environment no longer pay attention to any + rust-toolchain.toml file. + + We cannot follow the same approach because there is no API in rustup for + finding out a suitable RUSTUP_TOOLCHAIN value consistent with which + toolchain "rustc" currently refers to, and even if there were, it isn't + guaranteed that "rustc" refers to a rustup-managed toolchain in the first + place. + """ + + path.mkdir(exist_ok=True) + + for dir_entry in manifest_dir.iterdir(): + if dir_entry.name not in ["rust-toolchain", "rust-toolchain.toml"]: + link = path.joinpath(dir_entry.name) + link.unlink(missing_ok=True) + link.symlink_to(os.path.relpath(dir_entry, path)) + + return path + + def run_buildscript(buildscript: str, env: Dict[str, str], cwd: str) -> str: try: proc = subprocess.run( @@ -65,6 +121,8 @@ def run_buildscript(buildscript: str, env: Dict[str, str], cwd: str) -> str: class Args(NamedTuple): buildscript: str rustc_cfg: Path + manifest_dir: Path + create_cwd: Path outfile: IO[str] @@ -72,6 +130,8 @@ def arg_parse() -> Args: parser = argparse.ArgumentParser(description="Run Rust build script") parser.add_argument("--buildscript", type=str, required=True) parser.add_argument("--rustc-cfg", type=Path, required=True) + parser.add_argument("--manifest-dir", type=Path, required=True) + parser.add_argument("--create-cwd", type=Path, required=True) parser.add_argument("--outfile", type=argparse.FileType("w"), required=True) return Args(**vars(parser.parse_args())) @@ -86,11 +146,11 @@ def main() -> None: # noqa: C901 os.makedirs(out_dir, exist_ok=True) env["OUT_DIR"] = os.path.abspath(out_dir) - manifest_dir = os.getenv("CARGO_MANIFEST_DIR") - env["CARGO_MANIFEST_DIR"] = os.path.abspath(manifest_dir) + cwd = create_cwd(args.create_cwd, args.manifest_dir) + env["CARGO_MANIFEST_DIR"] = os.path.abspath(cwd) env = dict(os.environ, **env) - script_output = run_buildscript(args.buildscript, env, cwd=manifest_dir) + script_output = run_buildscript(args.buildscript, env=env, cwd=cwd) cargo_rustc_cfg_pattern = re.compile("^cargo:rustc-cfg=(.*)") flags = ""