8000 Enable incremental rustfmt adoption by anp · Pull Request #65939 · rust-lang/rust · GitHub
[go: up one dir, main page]

Skip to content

Enable incremental rustfmt adoption #65939

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.

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

Merged
merged 7 commits into from
Dec 22, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Implement rustfmt running manually using ignore crate
This replaces cargo-fmt with rustfmt with --skip-children which should
allow us to format code without running into rust-lang/rustfmt#3930.

This also bumps up the version of rustfmt used to a more recent one.
  • Loading branch information
Mark-Simulacrum committed Dec 22, 2019
commit dddd8724271a2a38ce552f5b1062a7ea070ec126
5 changes: 3 additions & 2 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ dependencies = [
"cmake",
"filetime",
"getopts",
"ignore",
"lazy_static 1.3.0",
"libc",
"num_cpus",
Expand Down Expand Up @@ -1525,9 +1526,9 @@ checksum = "c3360c7b59e5ffa2653671fb74b4741a5d343c03f331c0a4aeda42b5c2b0ec7d"

[[package]]
name = "ignore"
version = "0.4.7"
version = "0.4.10"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8dc57fa12805f367736a38541ac1a9fc6a52812a0ca959b1d4d4b640a89eb002"
checksum = "0ec16832258409d571aaef8273f3c3cc5b060d784e159d1a0f3b0017308f84a7"
dependencies = [
"crossbeam-channel",
"globset",
Expand Down
1 change: 1 addition & 0 deletions rustfmt.toml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ ignore = [
"src/rustllvm/",
"src/test/",
"src/tools/",
"src/etc",

# do not format submodules
"src/doc/book",
Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ serde_json = "1.0.2"
toml = "0.5"
lazy_static = "1.3.0"
time = "0.1"
ignore = "0.4.10"

[dev-dependencies]
pretty_assertions = "0.5"
61 changes: 46 additions & 15 deletions src/bootstrap/format.rs
6D40
Original file line number Diff line number Diff line change
@@ -1,28 +1,59 @@
//! Runs rustfmt on the repository.

use crate::{util, Build};
use crate::Build;
use std::process::Command;
use ignore::WalkBuilder;
use std::path::Path;
use build_helper::t;

pub fn format(build: &Build, check: bool) {
let target = &build.build;
fn rustfmt(build: &Build, path: &Path, check: bool) {
let rustfmt_path = build.config.initial_rustfmt.as_ref().unwrap_or_else(|| {
eprintln!("./x.py fmt is not supported on this channel");
std::process::exit(1);
}).clone();
let cargo_fmt_path = rustfmt_path.with_file_name(util::exe("cargo-fmt", &target));
assert!(cargo_fmt_path.is_file(), "{} not a file", cargo_fmt_path.display());

let mut cmd = Command::new(&cargo_fmt_path);
// cargo-fmt calls rustfmt as a bare command, so we need it to only find the correct one
cmd.env("PATH", cargo_fmt_path.parent().unwrap());
cmd.current_dir(&build.src);
cmd.arg("fmt");
});

let mut cmd = Command::new(&rustfmt_path);
// avoid the submodule config paths from coming into play,
// we only allow a single global config for the workspace for now
cmd.arg("--config-path").arg(&build.src.canonicalize().unwrap());
cmd.arg("--unstable-features");
cmd.arg("--skip-children");
if check {
cmd.arg("--");
cmd.arg("--check");
}
cmd.arg(&path);
let cmd_debug = format!("{:?}", cmd);
let status = cmd.status().expect("executing rustfmt");
assert!(status.success(), "running {} successful", cmd_debug);
Comment on lines +26 to +27
Copy link
Member
@eddyb eddyb Mar 22, 2020

Choose a reason for hiding this comment

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

One rustfmt --check failure should not prevent running rustfmt --check on other files, otherwise you only get one error at a time.

Copy link
Member Author

Choose a reason for hiding this comment

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

When that bit was originally written we were running rustfmt once for the whole repository and it showed all errors. Is that no longer how it's running?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like @Mark-Simulacrum pushed commits to this PR to run rustfmt on individual files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha - maybe file a bug so someone can pick up a fix?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, rustfmt on the whole repository was leading to errors in rustfmt (IIRC, it had some problems with ignoring files or so, I don't recall the specifics now).

This should be fixable though, like we do in tidy etc by keeping track of whether we errored and only stopping at the end.

}

#[derive(serde::Deserialize)]
struct RustfmtConfig {
ignore: Vec<String>,
}

pub fn format(build: &Build, check: bool) {
let mut builder = ignore::types::TypesBuilder::new();
builder.add_defaults();
builder.select("rust");
let matcher = builder.build().unwrap();

let rustfmt_config = t!(std::fs::read_to_string(build.src.join("rustfmt.toml")));
let rustfmt_config: RustfmtConfig = t!(toml::from_str(&rustfmt_config));
let mut ignore_fmt = ignore::overrides::OverrideBuilder::new(&build.src);
for ignore in rustfmt_config.ignore {
ignore_fmt.add(&format!("!{}", ignore)).expect(&ignore);
}
let ignore_fmt = ignore_fmt.build().unwrap();

let status = cmd.status().expect("executing cargo-fmt");
assert!(status.success(), "cargo-fmt errored with status {:?}", status);
let walker = WalkBuilder::new(&build.src)
.types(matcher)
.overrides(ignore_fmt)
.build();
for entry in walker {
let entry = t!(entry);
if entry.file_type().map_or(false, |t| t.is_file()) {
rustfmt(build, &entry.path(), check);
}
}
}
2 changes: 1 addition & 1 deletion src/stage0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ cargo: beta
# We use a nightly rustfmt to format the source because it solves some bootstrapping
# issues with use of new syntax in this repo. If you're looking at the beta/stable branch, this key should be omitted,
# as we don't want to depend on rustfmt from nightly there.
rustfmt: nightly-2019-11-05
rustfmt: nightly-2019-12-18

# When making a stable release the process currently looks like:
#
Expand Down
0