Refactored the Rustfmt tool to work cross-platform (#1375)
* Refactored rustfmt tool to work cross-platform
* Update tools/rustfmt/srcs/lib.rs
Co-authored-by: Daniel Wagner-Hall <dawagner@gmail.com>
* Assume `RUST_DEFAULT_EDITION` is always set.
* Unified edition querys
* Silenced rustfmt error in generated sources
Co-authored-by: Daniel Wagner-Hall <dawagner@gmail.com>
diff --git a/rust/private/BUILD.bazel b/rust/private/BUILD.bazel
index 9debaa3..40f4a69 100644
--- a/rust/private/BUILD.bazel
+++ b/rust/private/BUILD.bazel
@@ -1,5 +1,6 @@
load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
load("//rust/private:rust_analyzer.bzl", "rust_analyzer_detect_sysroot")
+load("//rust/private:rustfmt.bzl", "rustfmt_workspace_name")
load("//rust/private:stamp.bzl", "stamp_build_setting")
bzl_library(
@@ -24,3 +25,8 @@
name = "rust_analyzer_detect_sysroot",
visibility = ["//visibility:public"],
)
+
+rustfmt_workspace_name(
+ name = "rustfmt_workspace_name",
+ visibility = ["//visibility:public"],
+)
diff --git a/rust/private/rustfmt.bzl b/rust/private/rustfmt.bzl
index 9c59f48..ca7b7ae 100644
--- a/rust/private/rustfmt.bzl
+++ b/rust/private/rustfmt.bzl
@@ -138,9 +138,10 @@
# The executable of a test target must be the output of an action in
# the rule implementation. This file is simply a symlink to the real
# rustfmt test runner.
+ is_windows = ctx.executable._runner.extension == ".exe"
runner = ctx.actions.declare_file("{}{}".format(
ctx.label.name,
- ctx.executable._runner.extension,
+ ".exe" if is_windows else "",
))
ctx.actions.symlink(
@@ -149,22 +150,33 @@
is_executable = True,
)
- manifests = [target[OutputGroupInfo].rustfmt_manifest for target in ctx.attr.targets]
+ manifests = depset(transitive = [target[OutputGroupInfo].rustfmt_manifest for target in ctx.attr.targets])
srcs = [depset(_find_rustfmtable_srcs(target)) for target in ctx.attr.targets]
runfiles = ctx.runfiles(
- transitive_files = depset(transitive = manifests + srcs),
+ transitive_files = depset(transitive = srcs + [manifests]),
)
runfiles = runfiles.merge(
ctx.attr._runner[DefaultInfo].default_runfiles,
)
- return [DefaultInfo(
- files = depset([runner]),
- runfiles = runfiles,
- executable = runner,
- )]
+ path_env_sep = ";" if is_windows else ":"
+
+ return [
+ DefaultInfo(
+ files = depset([runner]),
+ runfiles = runfiles,
+ executable = runner,
+ ),
+ testing.TestEnvironment({
+ "RUSTFMT_MANIFESTS": path_env_sep.join([
+ manifest.short_path
+ for manifest in sorted(manifests.to_list())
+ ]),
+ "RUST_BACKTRACE": "1",
+ }),
+ ]
rustfmt_test = rule(
implementation = _rustfmt_test_impl,
@@ -184,3 +196,22 @@
},
test = True,
)
+
+def _rustfmt_workspace_name_impl(ctx):
+ output = ctx.actions.declare_file(ctx.label.name)
+
+ ctx.actions.write(
+ output = output,
+ content = "RUSTFMT_WORKSPACE={}".format(
+ ctx.workspace_name,
+ ),
+ )
+
+ return [DefaultInfo(
+ files = depset([output]),
+ )]
+
+rustfmt_workspace_name = rule(
+ implementation = _rustfmt_workspace_name_impl,
+ doc = "A rule for detecting the workspace name for Rustfmt runfiles.",
+)
diff --git a/test/generated_inputs/BUILD.bazel b/test/generated_inputs/BUILD.bazel
index 3ed32d9..2820282 100644
--- a/test/generated_inputs/BUILD.bazel
+++ b/test/generated_inputs/BUILD.bazel
@@ -28,6 +28,7 @@
":src.rs",
],
edition = "2018",
+ tags = ["norustfmt"],
)
rust_library(
@@ -38,6 +39,7 @@
],
crate_root = "lib.rs",
edition = "2018",
+ tags = ["norustfmt"],
)
rust_library(
@@ -50,6 +52,7 @@
crate_root = ":src.rs",
edition = "2018",
rustc_flags = ["--cfg=generated_file_as_root"],
+ tags = ["norustfmt"],
)
# When no lib.rs, main.rs file exists, we try to use the file that carries
@@ -62,12 +65,14 @@
],
edition = "2018",
rustc_flags = ["--cfg=generated_file_as_root"],
+ tags = ["norustfmt"],
)
rust_test(
name = "generated_src_test",
crate = ":use_generated_src_with_crate_root_defined",
edition = "2018",
+ tags = ["norustfmt"],
)
rust_binary(
@@ -77,6 +82,7 @@
":src.rs",
],
edition = "2018",
+ tags = ["norustfmt"],
)
input_from_different_cfg(
@@ -93,12 +99,14 @@
name = "generated_src_in_different_cfg_as_root",
srcs = [":input_from_different_cfg"],
edition = "2018",
+ tags = ["norustfmt"],
)
rust_test(
name = "generated_src_in_different_cfg_as_root_test",
crate = "generated_src_in_different_cfg_as_root",
edition = "2018",
+ tags = ["norustfmt"],
)
rust_library(
@@ -109,12 +117,14 @@
],
crate_root = "root.rs",
edition = "2018",
+ tags = ["norustfmt"],
)
rust_test(
name = "generated_src_in_different_cfg_test",
crate = "generated_src_in_different_cfg",
edition = "2018",
+ tags = ["norustfmt"],
)
bool_flag(
diff --git a/test/generated_inputs/lib_for_src.rs b/test/generated_inputs/lib_for_src.rs
index 697bc9d..6d2a037 100644
--- a/test/generated_inputs/lib_for_src.rs
+++ b/test/generated_inputs/lib_for_src.rs
@@ -1 +1,3 @@
-pub fn forty_two() -> i32 { 42 }
+pub fn forty_two() -> i32 {
+ 42
+}
diff --git a/tools/rustfmt/BUILD.bazel b/tools/rustfmt/BUILD.bazel
index db9faa6..b1e2e38 100644
--- a/tools/rustfmt/BUILD.bazel
+++ b/tools/rustfmt/BUILD.bazel
@@ -23,6 +23,12 @@
"RUSTFMT": "$(rootpath //rust/toolchain:current_exec_rustfmt_files)",
"RUSTFMT_CONFIG": "$(rootpath //:rustfmt.toml)",
},
+ rustc_env_files = [
+ "@rules_rust//rust/private:rustfmt_workspace_name",
+ ],
+ deps = [
+ "//tools/runfiles",
+ ],
)
rust_binary(
@@ -36,7 +42,11 @@
edition = "2018",
rustc_env = {
"ASPECT_REPOSITORY": aspect_repository(),
+ "RUST_DEFAULT_EDITION": "$(RUST_DEFAULT_EDITION)",
},
+ toolchains = [
+ "@rules_rust//rust/toolchain:current_rust_toolchain",
+ ],
deps = [
":rustfmt_lib",
"//util/label",
diff --git a/tools/rustfmt/srcs/lib.rs b/tools/rustfmt/srcs/lib.rs
index ad2e86a..2d8accc 100644
--- a/tools/rustfmt/srcs/lib.rs
+++ b/tools/rustfmt/srcs/lib.rs
@@ -5,18 +5,6 @@
/// The expected extension of rustfmt manifest files generated by `rustfmt_aspect`.
pub const RUSTFMT_MANIFEST_EXTENSION: &str = "rustfmt";
-/// Generate an absolute path to a file without resolving symlinks
-fn absolutify_existing<T: AsRef<Path>>(path: &T) -> std::io::Result<PathBuf> {
- let absolute_path = if path.as_ref().is_absolute() {
- path.as_ref().to_owned()
- } else {
- std::env::current_dir()
- .expect("Failed to get working directory")
- .join(path)
- };
- std::fs::metadata(&absolute_path).map(|_| absolute_path)
-}
-
/// A struct containing details used for executing rustfmt.
#[derive(Debug)]
pub struct RustfmtConfig {
@@ -31,11 +19,26 @@
/// Parse command line arguments and environment variables to
/// produce config data for running rustfmt.
pub fn parse_rustfmt_config() -> RustfmtConfig {
- RustfmtConfig {
- rustfmt: absolutify_existing(&env!("RUSTFMT")).expect("Unable to find rustfmt binary"),
- config: absolutify_existing(&env!("RUSTFMT_CONFIG"))
- .expect("Unable to find rustfmt config file"),
+ let runfiles = runfiles::Runfiles::create().unwrap();
+
+ let rustfmt = runfiles.rlocation(concat!(env!("RUSTFMT_WORKSPACE"), "/", env!("RUSTFMT")));
+ if !rustfmt.exists() {
+ panic!("rustfmt does not exist at: {}", rustfmt.display());
}
+
+ let config = runfiles.rlocation(concat!(
+ env!("RUSTFMT_WORKSPACE"),
+ "/",
+ env!("RUSTFMT_CONFIG")
+ ));
+ if !config.exists() {
+ panic!(
+ "rustfmt config file does not exist at: {}",
+ config.display()
+ );
+ }
+
+ RustfmtConfig { rustfmt, config }
}
/// A struct of target specific information for use in running `rustfmt`.
@@ -45,7 +48,7 @@
pub edition: String,
/// A list of all (non-generated) source files for formatting.
- pub sources: Vec<String>,
+ pub sources: Vec<PathBuf>,
}
/// Parse rustfmt flags from a manifest generated by builds using `rustfmt_aspect`.
@@ -67,8 +70,33 @@
.parse::<i32>()
.expect("The edition should be a numeric value. eg `2018`.");
+ let runfiles = runfiles::Runfiles::create().unwrap();
+
RustfmtManifest {
edition,
- sources: lines,
+ sources: lines
+ .into_iter()
+ .map(|src| runfiles.rlocation(format!("{}/{}", env!("RUSTFMT_WORKSPACE"), src)))
+ .collect(),
}
}
+
+#[cfg(target_family = "windows")]
+const PATH_ENV_SEP: &str = ";";
+
+#[cfg(target_family = "unix")]
+const PATH_ENV_SEP: &str = ":";
+
+/// Parse the runfiles of the current executable for manifests generated
+/// by the `rustfmt_aspect` aspect.
+pub fn find_manifests() -> Vec<PathBuf> {
+ let runfiles = runfiles::Runfiles::create().unwrap();
+
+ std::env::var("RUSTFMT_MANIFESTS")
+ .map(|var| {
+ var.split(PATH_ENV_SEP)
+ .map(|path| runfiles.rlocation(format!("{}/{}", env!("RUSTFMT_WORKSPACE"), path)))
+ .collect()
+ })
+ .unwrap_or_default()
+}
diff --git a/tools/rustfmt/srcs/main.rs b/tools/rustfmt/srcs/main.rs
index 4e4419f..dc29968 100644
--- a/tools/rustfmt/srcs/main.rs
+++ b/tools/rustfmt/srcs/main.rs
@@ -1,8 +1,12 @@
+//! A tool for querying Rust source files wired into Bazel and running Rustfmt on them.
+
+use std::collections::HashMap;
use std::env;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
use std::process::{Command, Stdio};
use std::str;
+/// The Bazel Rustfmt tool entry point
fn main() {
// Gather all command line and environment settings
let options = parse_args();
@@ -14,52 +18,40 @@
apply_rustfmt(&options, &targets);
}
-/// Perform a `bazel` query to determine a list of Bazel targets which are to be formatted.
-fn query_rustfmt_targets(options: &Config) -> Vec<String> {
- // Determine what packages to query
- let scope = match options.packages.is_empty() {
- true => "//...:all".to_owned(),
- false => {
- // Check to see if all the provided packages are actually targets
- let is_all_targets = options
- .packages
- .iter()
- .all(|pkg| match label::analyze(pkg) {
- Ok(tgt) => tgt.name != "all",
- Err(_) => false,
- });
+/// The edition to use in cases where the default edition is unspecified by Bazel
+const FALLBACK_EDITION: &str = "2018";
- // Early return if a list of targets and not packages were provided
- if is_all_targets {
- return options.packages.clone();
- }
+/// Determine the Rust edition to use in cases where a target has not explicitly
+/// specified the edition via an `edition` attribute.
+fn get_default_edition() -> &'static str {
+ if !env!("RUST_DEFAULT_EDITION").is_empty() {
+ env!("RUST_DEFAULT_EDITION")
+ } else {
+ FALLBACK_EDITION
+ }
+}
- options.packages.join(" + ")
- }
- };
+/// Get a list of all editions to run formatting for
+fn get_editions() -> Vec<String> {
+ vec!["2015".to_owned(), "2018".to_owned(), "2021".to_owned()]
+}
- let query_args = vec![
- "query".to_owned(),
- format!(
- r#"kind('{types}', {scope}) except attr(tags, 'norustfmt', kind('{types}', {scope}))"#,
- types = "^rust_",
- scope = scope
- ),
- ];
-
- let child = Command::new(&options.bazel)
- .current_dir(&options.workspace)
- .args(query_args)
+/// Run a bazel command, capturing stdout while streaming stderr to surface errors
+fn bazel_command(bazel_bin: &Path, args: &[String], current_dir: &Path) -> Vec<String> {
+ let child = Command::new(bazel_bin)
+ .current_dir(current_dir)
+ .args(args)
.stdout(Stdio::piped())
.stderr(Stdio::inherit())
.spawn()
- .expect("Failed to spawn bazel query command");
+ .expect("Failed to spawn bazel command");
let output = child
.wait_with_output()
.expect("Failed to wait on spawned command");
if !output.status.success() {
+ eprintln!("Failed to perform `bazel query` command.");
std::process::exit(output.status.code().unwrap_or(1));
}
@@ -71,74 +63,92 @@
.collect()
}
-/// Build a list of Bazel targets using the `rustfmt_aspect` to produce the
-/// arguments to use when formatting the sources of those targets.
-fn generate_rustfmt_target_manifests(options: &Config, targets: &[String]) {
- let build_args = vec![
- "build".to_owned(),
+/// The regex representation of an empty `edition` attribute
+const EMPTY_EDITION: &str = "^$";
+
+/// Query for all `*.rs` files in a workspace that are dependencies of targets with the requested edition.
+fn edition_query(bazel_bin: &Path, edition: &str, scope: &str, current_dir: &Path) -> Vec<String> {
+ let query_args = vec![
+ "query".to_owned(),
+ // Query explanation:
+ // Filter all local targets ending in `*.rs`.
+ // Get all source files.
+ // Get direct dependencies.
+ // Get all targets with the specified `edition` attribute.
+ // Except for targets tagged with `norustfmt`.
+ // And except for targets with a populated `crate` attribute since `crate` defines edition for this target
format!(
- "--aspects={}//rust:defs.bzl%rustfmt_aspect",
- env!("ASPECT_REPOSITORY")
+ r#"let scope = set({scope}) in filter("^//.*\.rs$", kind("source file", deps(attr(edition, "{edition}", $scope) except attr(tags, "(^\[|, )norustfmt(, |\]$)", $scope) + attr(crate, ".*", $scope), 1)))"#,
+ edition = edition,
+ scope = scope,
),
- "--output_groups=rustfmt_manifest".to_owned(),
+ "--keep_going".to_owned(),
+ "--noimplicit_deps".to_owned(),
];
- let child = Command::new(&options.bazel)
- .current_dir(&options.workspace)
- .args(build_args)
- .args(targets)
- .stdout(Stdio::piped())
- .stderr(Stdio::inherit())
- .spawn()
- .expect("Failed to spawn command");
+ bazel_command(bazel_bin, &query_args, current_dir)
+}
- let output = child
- .wait_with_output()
- .expect("Failed to wait on spawned command");
+/// Perform a `bazel` query to determine all source files which are to be
+/// formatted for particular Rust editions.
+fn query_rustfmt_targets(options: &Config) -> HashMap<String, Vec<String>> {
+ let scope = options
+ .packages
+ .clone()
+ .into_iter()
+ .reduce(|acc, item| acc + " " + &item)
+ .unwrap_or_else(|| "//...:all".to_owned());
- if !output.status.success() {
- std::process::exit(output.status.code().unwrap_or(1));
- }
+ let editions = get_editions();
+ let default_edition = get_default_edition();
+
+ editions
+ .into_iter()
+ .map(|edition| {
+ let mut targets = edition_query(&options.bazel, &edition, &scope, &options.workspace);
+
+ // For all targets relying on the toolchain for it's edition,
+ // query anything with an unset edition
+ if edition == default_edition {
+ targets.extend(edition_query(
+ &options.bazel,
+ EMPTY_EDITION,
+ &scope,
+ &options.workspace,
+ ))
+ }
+
+ (edition, targets)
+ })
+ .collect()
}
/// Run rustfmt on a set of Bazel targets
-fn apply_rustfmt(options: &Config, targets: &[String]) {
- // Ensure the targets are first built and a manifest containing `rustfmt`
- // arguments are generated before formatting source files.
- generate_rustfmt_target_manifests(options, targets);
+fn apply_rustfmt(options: &Config, editions_and_targets: &HashMap<String, Vec<String>>) {
+ // There is no work to do if the list of targets is empty
+ if editions_and_targets.is_empty() {
+ return;
+ }
- for target in targets.iter() {
- // Replace any `:` characters and strip leading slashes
- let target_path = target.replace(':', "/").trim_start_matches('/').to_owned();
-
- // Find a manifest for the current target. Not all targets will have one
- let manifest = options.workspace.join("bazel-bin").join(format!(
- "{}.{}",
- &target_path,
- rustfmt_lib::RUSTFMT_MANIFEST_EXTENSION,
- ));
-
- if !manifest.exists() {
+ for (edition, targets) in editions_and_targets.iter() {
+ if targets.is_empty() {
continue;
}
- // Load the manifest containing rustfmt arguments
- let rustfmt_config = rustfmt_lib::parse_rustfmt_manifest(&manifest);
-
- // Ignore any targets which do not have source files. This can
- // occur in cases where all source files are generated.
- if rustfmt_config.sources.is_empty() {
- continue;
- }
+ // Get paths to all formattable sources
+ let sources: Vec<String> = targets
+ .iter()
+ .map(|target| target.replace(':', "/").trim_start_matches('/').to_owned())
+ .collect();
// Run rustfmt
let status = Command::new(&options.rustfmt_config.rustfmt)
.current_dir(&options.workspace)
.arg("--edition")
- .arg(rustfmt_config.edition)
+ .arg(edition)
.arg("--config-path")
.arg(&options.rustfmt_config.config)
- .args(rustfmt_config.sources)
+ .args(sources)
.status()
.expect("Failed to run rustfmt");
diff --git a/tools/rustfmt/srcs/test_main.rs b/tools/rustfmt/srcs/test_main.rs
index ea2adba..57cd203 100644
--- a/tools/rustfmt/srcs/test_main.rs
+++ b/tools/rustfmt/srcs/test_main.rs
@@ -1,5 +1,4 @@
-use std::fs;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
use std::process::Command;
fn main() {
@@ -55,31 +54,9 @@
pub manifests: Vec<rustfmt_lib::RustfmtManifest>,
}
-/// Parse the runfiles of the current executable for manifests generated
-/// but the `rustfmt_aspect` aspect.
-fn find_manifests(dir: &Path, manifests: &mut Vec<PathBuf>) {
- if dir.is_dir() {
- for entry in fs::read_dir(dir).expect("Failed to read directory contents") {
- let entry = entry.expect("Failed to read directory entry");
- let path = entry.path();
- if path.is_dir() {
- find_manifests(&path, manifests);
- } else if let Some(ext) = path.extension() {
- if ext == rustfmt_lib::RUSTFMT_MANIFEST_EXTENSION {
- manifests.extend(vec![path]);
- }
- }
- }
- }
-}
-
/// Parse settings from the environment into a config struct
fn parse_args() -> Config {
- let mut manifests: Vec<PathBuf> = Vec::new();
- find_manifests(
- &runfiles::find_runfiles_dir().expect("Failed to find runfiles directory"),
- &mut manifests,
- );
+ let manifests: Vec<PathBuf> = rustfmt_lib::find_manifests();
if manifests.is_empty() {
panic!("No manifests were found");
