Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add clippy and fix commands to x.py #56595

Merged
merged 1 commit into from
May 26, 2019
Merged

Add clippy and fix commands to x.py #56595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 17 additions & 8 deletions src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,8 @@ impl<'a> ShouldRun<'a> {
pub enum Kind {
Build,
Check,
Clippy,
Fix,
Test,
Bench,
Dist,
Expand Down Expand Up @@ -359,7 +361,7 @@ impl<'a> Builder<'a> {
tool::Miri,
native::Lld
),
Kind::Check => describe!(
Kind::Check | Kind::Clippy | Kind::Fix => describe!(
check::Std,
check::Test,
check::Rustc,
Expand Down Expand Up @@ -520,6 +522,8 @@ impl<'a> Builder<'a> {
let (kind, paths) = match build.config.cmd {
Subcommand::Build { ref paths } => (Kind::Build, &paths[..]),
Subcommand::Check { ref paths } => (Kind::Check, &paths[..]),
Subcommand::Clippy { ref paths } => (Kind::Clippy, &paths[..]),
Subcommand::Fix { ref paths } => (Kind::Fix, &paths[..]),
Subcommand::Doc { ref paths } => (Kind::Doc, &paths[..]),
Subcommand::Test { ref paths, .. } => (Kind::Test, &paths[..]),
Subcommand::Bench { ref paths, .. } => (Kind::Bench, &paths[..]),
Expand Down Expand Up @@ -757,17 +761,17 @@ impl<'a> Builder<'a> {
};

let libstd_stamp = match cmd {
"check" => check::libstd_stamp(self, cmp, target),
"check" | "clippy" | "fix" => check::libstd_stamp(self, cmp, target),
_ => compile::libstd_stamp(self, cmp, target),
};

let libtest_stamp = match cmd {
"check" => check::libtest_stamp(self, cmp, target),
"check" | "clippy" | "fix" => check::libtest_stamp(self, cmp, target),
_ => compile::libstd_stamp(self, cmp, target),
};

let librustc_stamp = match cmd {
"check" => check::librustc_stamp(self, cmp, target),
"check" | "clippy" | "fix" => check::librustc_stamp(self, cmp, target),
_ => compile::librustc_stamp(self, cmp, target),
};

Expand Down Expand Up @@ -831,9 +835,9 @@ impl<'a> Builder<'a> {
assert_eq!(target, compiler.host);
}

// Set a flag for `check` so that certain build scripts can do less work
// (e.g., not building/requiring LLVM).
if cmd == "check" {
// Set a flag for `check`/`clippy`/`fix`, so that certain build
// scripts can do less work (e.g. not building/requiring LLVM).
if cmd == "check" || cmd == "clippy" || cmd == "fix" {
cargo.env("RUST_CHECK", "1");
}

Expand Down Expand Up @@ -898,6 +902,11 @@ impl<'a> Builder<'a> {
extra_args.push_str(&s);
}

if cmd == "clippy" {
extra_args.push_str("-Zforce-unstable-if-unmarked -Zunstable-options \
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tested that this prevents the rebuilds? I would have assumed this to still cause build thrashing. If this works, then I'm happy with it, if not, you should be able to add them in the args function you created below by putting them after a -- argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked it - there are no rebuilds. The only thing is that after throwing its warnings (a lot of stuff found in libcore) ./x.py clippy is complaining that it can't find crate for std and I no longer remember if this was an acceptable step forward or not ^^. I remember having trouble making it respect the build order.

Copy link
Contributor

Choose a reason for hiding this comment

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

While not great, we can solve this in follow-ups. I think this may happen whenever one tries to run clippy on a target without libstd, so it's likely that it's a clippy issue.

--json-rendered=termcolor");
}

if !extra_args.is_empty() {
cargo.env(
"RUSTFLAGS",
Expand Down Expand Up @@ -966,7 +975,7 @@ impl<'a> Builder<'a> {
if let Some(ref error_format) = self.config.rustc_error_format {
cargo.env("RUSTC_ERROR_FORMAT", error_format);
}
if cmd != "build" && cmd != "check" && cmd != "rustc" && want_rustdoc {
if !(["build", "check", "clippy", "fix", "rustc"].contains(&cmd)) && want_rustdoc {
cargo.env("RUSTDOC_LIBDIR", self.rustc_libdir(compiler));
}

Expand Down
37 changes: 30 additions & 7 deletions src/bootstrap/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
//! Implementation of compiling the compiler and standard library, in "check" mode.
//! Implementation of compiling the compiler and standard library, in "check"-based modes.

use crate::compile::{run_cargo, std_cargo, test_cargo, rustc_cargo, rustc_cargo_env,
add_to_sysroot};
use crate::builder::{RunConfig, Builder, ShouldRun, Step};
use crate::builder::{RunConfig, Builder, Kind, ShouldRun, Step};
use crate::tool::{prepare_tool_cargo, SourceType};
use crate::{Compiler, Mode};
use crate::cache::{INTERNER, Interned};
Expand All @@ -13,6 +13,22 @@ pub struct Std {
pub target: Interned<String>,
}

fn args(kind: Kind) -> Vec<String> {
match kind {
Kind::Clippy => vec!["--".to_owned(), "--cap-lints".to_owned(), "warn".to_owned()],
_ => Vec::new()
}
}

fn cargo_subcommand(kind: Kind) -> &'static str {
match kind {
Kind::Check => "check",
Kind::Clippy => "clippy",
Kind::Fix => "fix",
_ => unreachable!()
}
}

impl Step for Std {
type Output = ();
const DEFAULT: bool = true;
Expand All @@ -31,13 +47,14 @@ impl Step for Std {
let target = self.target;
let compiler = builder.compiler(0, builder.config.build);

let mut cargo = builder.cargo(compiler, Mode::Std, target, "check");
let mut cargo = builder.cargo(compiler, Mode::Std, target, cargo_subcommand(builder.kind));
std_cargo(builder, &compiler, target, &mut cargo);

let _folder = builder.fold_output(|| format!("stage{}-std", compiler.stage));
builder.info(&format!("Checking std artifacts ({} -> {})", &compiler.host, target));
run_cargo(builder,
&mut cargo,
args(builder.kind),
&libstd_stamp(builder, compiler, target),
true);

Expand Down Expand Up @@ -78,13 +95,15 @@ impl Step for Rustc {

builder.ensure(Test { target });

let mut cargo = builder.cargo(compiler, Mode::Rustc, target, "check");
let mut cargo = builder.cargo(compiler, Mode::Rustc, target,
cargo_subcommand(builder.kind));
rustc_cargo(builder, &mut cargo);

let _folder = builder.fold_output(|| format!("stage{}-rustc", compiler.stage));
builder.info(&format!("Checking compiler artifacts ({} -> {})", &compiler.host, target));
run_cargo(builder,
&mut cargo,
args(builder.kind),
&librustc_stamp(builder, compiler, target),
true);

Expand Down Expand Up @@ -127,7 +146,8 @@ impl Step for CodegenBackend {

builder.ensure(Rustc { target });

let mut cargo = builder.cargo(compiler, Mode::Codegen, target, "check");
let mut cargo = builder.cargo(compiler, Mode::Codegen, target,
cargo_subcommand(builder.kind));
cargo.arg("--manifest-path").arg(builder.src.join("src/librustc_codegen_llvm/Cargo.toml"));
rustc_cargo_env(builder, &mut cargo);

Expand All @@ -136,6 +156,7 @@ impl Step for CodegenBackend {
let _folder = builder.fold_output(|| format!("stage{}-rustc_codegen_llvm", compiler.stage));
run_cargo(builder,
&mut cargo,
args(builder.kind),
&codegen_backend_stamp(builder, compiler, target, backend),
true);
}
Expand Down Expand Up @@ -166,13 +187,14 @@ impl Step for Test {

builder.ensure(Std { target });

let mut cargo = builder.cargo(compiler, Mode::Test, target, "check");
let mut cargo = builder.cargo(compiler, Mode::Test, target, cargo_subcommand(builder.kind));
test_cargo(builder, &compiler, target, &mut cargo);

let _folder = builder.fold_output(|| format!("stage{}-test", compiler.stage));
builder.info(&format!("Checking test artifacts ({} -> {})", &compiler.host, target));
run_cargo(builder,
&mut cargo,
args(builder.kind),
&libtest_stamp(builder, compiler, target),
true);

Expand Down Expand Up @@ -212,7 +234,7 @@ impl Step for Rustdoc {
compiler,
Mode::ToolRustc,
target,
"check",
cargo_subcommand(builder.kind),
"src/tools/rustdoc",
SourceType::InTree,
&[]);
Expand All @@ -221,6 +243,7 @@ impl Step for Rustdoc {
println!("Checking rustdoc artifacts ({} -> {})", &compiler.host, target);
run_cargo(builder,
&mut cargo,
args(builder.kind),
&rustdoc_stamp(builder, compiler, target),
true);

Expand Down
24 changes: 23 additions & 1 deletion src/bootstrap/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ impl Step for Std {
&compiler.host, target));
run_cargo(builder,
&mut cargo,
vec![],
&libstd_stamp(builder, compiler, target),
false);

Expand Down Expand Up @@ -425,6 +426,7 @@ impl Step for Test {
&compiler.host, target));
run_cargo(builder,
&mut cargo,
vec![],
&libtest_stamp(builder, compiler, target),
false);

Expand Down Expand Up @@ -556,6 +558,7 @@ impl Step for Rustc {
compiler.stage, &compiler.host, target));
run_cargo(builder,
&mut cargo,
vec![],
&librustc_stamp(builder, compiler, target),
false);

Expand Down Expand Up @@ -707,6 +710,7 @@ impl Step for CodegenBackend {
let _folder = builder.fold_output(|| format!("stage{}-rustc_codegen_llvm", compiler.stage));
let files = run_cargo(builder,
cargo.arg("--features").arg(features),
vec![],
&tmp_stamp,
false);
if builder.config.dry_run {
Expand Down Expand Up @@ -1077,6 +1081,7 @@ pub fn add_to_sysroot(

pub fn run_cargo(builder: &Builder<'_>,
cargo: &mut Command,
tail_args: Vec<String>,
stamp: &Path,
is_check: bool)
-> Vec<PathBuf>
Expand All @@ -1099,7 +1104,7 @@ pub fn run_cargo(builder: &Builder<'_>,
// files we need to probe for later.
let mut deps = Vec::new();
let mut toplevel = Vec::new();
let ok = stream_cargo(builder, cargo, &mut |msg| {
let ok = stream_cargo(builder, cargo, tail_args, &mut |msg| {
let (filenames, crate_types) = match msg {
CargoMessage::CompilerArtifact {
filenames,
Expand All @@ -1108,6 +1113,10 @@ pub fn run_cargo(builder: &Builder<'_>,
},
..
} => (filenames, crate_types),
CargoMessage::CompilerMessage { message } => {
eprintln!("{}", message.rendered);
return;
}
_ => return,
};
for filename in filenames {
Expand Down Expand Up @@ -1235,6 +1244,7 @@ pub fn run_cargo(builder: &Builder<'_>,
pub fn stream_cargo(
builder: &Builder<'_>,
cargo: &mut Command,
tail_args: Vec<String>,
cb: &mut dyn FnMut(CargoMessage<'_>),
) -> bool {
if builder.config.dry_run {
Expand All @@ -1245,6 +1255,10 @@ pub fn stream_cargo(
cargo.arg("--message-format").arg("json")
.stdout(Stdio::piped());

for arg in tail_args {
cargo.arg(arg);
}

builder.verbose(&format!("running: {:?}", cargo));
let mut child = match cargo.spawn() {
Ok(child) => child,
Expand Down Expand Up @@ -1291,5 +1305,13 @@ pub enum CargoMessage<'a> {
},
BuildScriptExecuted {
package_id: Cow<'a, str>,
},
CompilerMessage {
message: ClippyMessage<'a>
}
}

#[derive(Deserialize)]
pub struct ClippyMessage<'a> {
rendered: Cow<'a, str>,
}
34 changes: 34 additions & 0 deletions src/bootstrap/flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ pub enum Subcommand {
Check {
paths: Vec<PathBuf>,
},
Clippy {
paths: Vec<PathBuf>,
},
Fix {
paths: Vec<PathBuf>,
},
Doc {
paths: Vec<PathBuf>,
},
Expand Down Expand Up @@ -90,6 +96,8 @@ Usage: x.py <subcommand> [options] [<paths>...]
Subcommands:
build Compile either the compiler or libraries
check Compile either the compiler or libraries, using cargo check
clippy Run clippy
fix Run cargo fix
test Build and run some test suites
bench Build and run some benchmarks
doc Build documentation
Expand Down Expand Up @@ -146,6 +154,8 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`"
let subcommand = args.iter().find(|&s| {
(s == "build")
|| (s == "check")
|| (s == "clippy")
|| (s == "fix")
|| (s == "test")
|| (s == "bench")
|| (s == "doc")
Expand Down Expand Up @@ -281,6 +291,28 @@ Arguments:
the compiler.",
);
}
"clippy" => {
subcommand_help.push_str(
"\n
Arguments:
This subcommand accepts a number of paths to directories to the crates
and/or artifacts to run clippy against. For example:

./x.py clippy src/libcore
./x.py clippy src/libcore src/libproc_macro",
);
}
"fix" => {
subcommand_help.push_str(
"\n
Arguments:
This subcommand accepts a number of paths to directories to the crates
and/or artifacts to run `cargo fix` against. For example:

./x.py fix src/libcore
./x.py fix src/libcore src/libproc_macro",
);
}
"test" => {
subcommand_help.push_str(
"\n
Expand Down Expand Up @@ -363,6 +395,8 @@ Arguments:
let cmd = match subcommand.as_str() {
"build" => Subcommand::Build { paths },
"check" => Subcommand::Check { paths },
"clippy" => Subcommand::Clippy { paths },
"fix" => Subcommand::Fix { paths },
"test" => Subcommand::Test {
paths,
bless: matches.opt_present("bless"),
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/tool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ impl Step for ToolBuild {
let _folder = builder.fold_output(|| format!("stage{}-{}", compiler.stage, tool));
builder.info(&format!("Building stage{} tool {} ({})", compiler.stage, tool, target));
let mut duplicates = Vec::new();
let is_expected = compile::stream_cargo(builder, &mut cargo, &mut |msg| {
let is_expected = compile::stream_cargo(builder, &mut cargo, vec![], &mut |msg| {
// Only care about big things like the RLS/Cargo for now
match tool {
| "rls"
Expand Down