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.

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

Create "suggested tests" tool in rustbuild #106249

Merged
merged 2 commits into from
Apr 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
13 changes: 11 additions & 2 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3459,9 +3459,9 @@ dependencies = [

[[package]]
name = "once_cell"
version = "1.16.0"
version = "1.17.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "86f0b0d4bf799edbc74508c1e8bf170ff5f41238e5f8225603ca7caaae2b7860"
checksum = "b7e5500299e16ebb147ae15a00a942af264cf3688f47923b8fc2cd5858f23ad3"

[[package]]
name = "opener"
Expand Down Expand Up @@ -6097,6 +6097,15 @@ version = "2.4.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6bdef32e8150c2a081110b42772ffe7d7c9032b606bc226c8260fd97e0976601"

[[package]]
name = "suggest-tests"
version = "0.1.0"
dependencies = [
"build_helper",
"glob",
"once_cell",
]

[[package]]
name = "syn"
version = "1.0.102"
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ members = [
"src/tools/lld-wrapper",
"src/tools/collect-license-metadata",
"src/tools/generate-copyright",
"src/tools/suggest-tests",
]

exclude = [
Expand Down
22 changes: 21 additions & 1 deletion src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,7 @@ pub enum Kind {
Install,
Run,
Setup,
Suggest,
}

impl Kind {
Expand All @@ -610,6 +611,7 @@ impl Kind {
"install" => Kind::Install,
"run" | "r" => Kind::Run,
"setup" => Kind::Setup,
"suggest" => Kind::Suggest,
_ => return None,
})
}
Expand All @@ -629,6 +631,7 @@ impl Kind {
Kind::Install => "install",
Kind::Run => "run",
Kind::Setup => "setup",
Kind::Suggest => "suggest",
}
}
}
Expand Down Expand Up @@ -709,6 +712,7 @@ impl<'a> Builder<'a> {
test::CrateRustdoc,
test::CrateRustdocJsonTypes,
test::CrateJsonDocLint,
test::SuggestTestsCrate,
test::Linkcheck,
test::TierCheck,
test::ReplacePlaceholderTest,
Expand Down Expand Up @@ -827,7 +831,7 @@ impl<'a> Builder<'a> {
Kind::Setup => describe!(setup::Profile, setup::Hook, setup::Link, setup::Vscode),
Kind::Clean => describe!(clean::CleanAll, clean::Rustc, clean::Std),
// special-cased in Build::build()
Kind::Format => vec![],
Kind::Format | Kind::Suggest => vec![],
}
}

Expand Down Expand Up @@ -891,6 +895,7 @@ impl<'a> Builder<'a> {
Subcommand::Run { ref paths, .. } => (Kind::Run, &paths[..]),
Subcommand::Clean { ref paths, .. } => (Kind::Clean, &paths[..]),
Subcommand::Format { .. } => (Kind::Format, &[][..]),
Subcommand::Suggest { .. } => (Kind::Suggest, &[][..]),
Subcommand::Setup { profile: ref path } => (
Kind::Setup,
path.as_ref().map_or([].as_slice(), |path| std::slice::from_ref(path)),
Expand All @@ -900,6 +905,21 @@ impl<'a> Builder<'a> {
Self::new_internal(build, kind, paths.to_owned())
}

/// Creates a new standalone builder for use outside of the normal process
pub fn new_standalone(
build: &mut Build,
kind: Kind,
paths: Vec<PathBuf>,
stage: Option<u32>,
) -> Builder<'_> {
// FIXME: don't mutate `build`
if let Some(stage) = stage {
build.config.stage = stage;
}

Self::new_internal(build, kind, paths.to_owned())
Comment on lines +908 to +920
Copy link
Member

Choose a reason for hiding this comment

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

I think you can avoid needing this new function by modifying build.config.cmd and using Builder::new instead. You could change sug! to generate the command for you so it's not too much of a pain:

(test, $stage:literal, $paths:expr) => Suggestion { stage: $stage, cmd: Subcommand::Test { paths: $paths } },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the idea, I think I also need to modify build.config.stage. Sorry I'm on holiday right now so the progress is slow.

Copy link
Member

Choose a reason for hiding this comment

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

No need to apologize! You're responding quite fast actually 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, using serde_json means that we can't directly create Subcommands. Honestly, the solution is good enough as is for now.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the problem, sorry - how is serde_json related here?

Copy link
Member

Choose a reason for hiding this comment

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

@Ezrashaw I don't think I ever got an answer here - how is serde related?

Copy link
Contributor Author

@Ezrashaw Ezrashaw Apr 6, 2023

Choose a reason for hiding this comment

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

Hmm, using serde_json means that we can't directly create Subcommands. Honestly, the solution is good enough as is for now.

Sorry about that. I have no idea what I was thinking lol. I think that this isn't that relevant anymore since suggestions are implemented in code.

}

pub fn execute_cli(&self) {
self.run_step_descriptions(&Builder::get_step_descriptions(self.kind), &self.paths);
}
Expand Down
24 changes: 10 additions & 14 deletions src/bootstrap/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ pub enum DryRun {
/// filled out from the decoded forms of the structs below. For documentation
/// each field, see the corresponding fields in
/// `config.example.toml`.
#[derive(Default)]
#[cfg_attr(test, derive(Clone))]
#[derive(Default, Clone)]
pub struct Config {
pub changelog_seen: Option<usize>,
pub ccache: Option<String>,
Expand Down Expand Up @@ -240,32 +239,28 @@ pub struct Config {
pub initial_rustfmt: RefCell<RustfmtState>,
}

#[derive(Default, Deserialize)]
#[cfg_attr(test, derive(Clone))]
#[derive(Default, Deserialize, Clone)]
pub struct Stage0Metadata {
pub compiler: CompilerMetadata,
pub config: Stage0Config,
pub checksums_sha256: HashMap<String, String>,
pub rustfmt: Option<RustfmtMetadata>,
}
#[derive(Default, Deserialize)]
#[cfg_attr(test, derive(Clone))]
#[derive(Default, Deserialize, Clone)]
pub struct CompilerMetadata {
pub date: String,
pub version: String,
}

#[derive(Default, Deserialize)]
#[cfg_attr(test, derive(Clone))]
#[derive(Default, Deserialize, Clone)]
pub struct Stage0Config {
pub dist_server: String,
pub artifacts_server: String,
pub artifacts_with_llvm_assertions_server: String,
pub git_merge_commit_email: String,
pub nightly_branch: String,
}
#[derive(Default, Deserialize)]
#[cfg_attr(test, derive(Clone))]
#[derive(Default, Deserialize, Clone)]
pub struct RustfmtMetadata {
pub date: String,
pub version: String,
Expand Down Expand Up @@ -443,8 +438,7 @@ impl PartialEq<&str> for TargetSelection {
}

/// Per-target configuration stored in the global configuration structure.
#[derive(Default)]
#[cfg_attr(test, derive(Clone))]
#[derive(Default, Clone)]
pub struct Target {
/// Some(path to llvm-config) if using an external LLVM.
pub llvm_config: Option<PathBuf>,
Expand Down Expand Up @@ -1396,7 +1390,8 @@ impl Config {
| Subcommand::Fix { .. }
| Subcommand::Run { .. }
| Subcommand::Setup { .. }
| Subcommand::Format { .. } => flags.stage.unwrap_or(0),
| Subcommand::Format { .. }
| Subcommand::Suggest { .. } => flags.stage.unwrap_or(0),
};

// CI should always run stage 2 builds, unless it specifically states otherwise
Expand All @@ -1421,7 +1416,8 @@ impl Config {
| Subcommand::Fix { .. }
| Subcommand::Run { .. }
| Subcommand::Setup { .. }
| Subcommand::Format { .. } => {}
| Subcommand::Format { .. }
| Subcommand::Suggest { .. } => {}
}
}

Expand Down
14 changes: 11 additions & 3 deletions src/bootstrap/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ pub struct Flags {
pub free_args: Option<Vec<String>>,
}

#[derive(Debug)]
#[cfg_attr(test, derive(Clone))]
#[derive(Debug, Clone)]
pub enum Subcommand {
Build {
paths: Vec<PathBuf>,
Expand Down Expand Up @@ -149,6 +148,9 @@ pub enum Subcommand {
Setup {
profile: Option<PathBuf>,
},
Suggest {
run: bool,
},
}

impl Default for Subcommand {
Expand Down Expand Up @@ -183,6 +185,7 @@ Subcommands:
install Install distribution artifacts
run, r Run tools contained in this repository
setup Create a config.toml (making it easier to use `x.py` itself)
suggest Suggest a subset of tests to run, based on modified files

To learn more about a subcommand, run `./x.py <subcommand> -h`",
);
Expand Down Expand Up @@ -349,6 +352,9 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`",
Kind::Run => {
opts.optmulti("", "args", "arguments for the tool", "ARGS");
}
Kind::Suggest => {
opts.optflag("", "run", "run suggested tests");
}
_ => {}
};

Expand Down Expand Up @@ -565,7 +571,7 @@ Arguments:
Profile::all_for_help(" ").trim_end()
));
}
Kind::Bench | Kind::Clean | Kind::Dist | Kind::Install => {}
Kind::Bench | Kind::Clean | Kind::Dist | Kind::Install | Kind::Suggest => {}
};
// Get any optional paths which occur after the subcommand
let mut paths = matches.free[1..].iter().map(|p| p.into()).collect::<Vec<PathBuf>>();
Expand Down Expand Up @@ -626,6 +632,7 @@ Arguments:
Kind::Format => Subcommand::Format { check: matches.opt_present("check"), paths },
Kind::Dist => Subcommand::Dist { paths },
Kind::Install => Subcommand::Install { paths },
Kind::Suggest => Subcommand::Suggest { run: matches.opt_present("run") },
Kind::Run => {
if paths.is_empty() {
println!("\nrun requires at least a path!\n");
Expand Down Expand Up @@ -734,6 +741,7 @@ impl Subcommand {
Subcommand::Install { .. } => Kind::Install,
Subcommand::Run { .. } => Kind::Run,
Subcommand::Setup { .. } => Kind::Setup,
Subcommand::Suggest { .. } => Kind::Suggest,
}
}

Expand Down
19 changes: 14 additions & 5 deletions src/bootstrap/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ mod render_tests;
mod run;
mod sanity;
mod setup;
mod suggest;
mod tarball;
mod test;
mod tool;
Expand Down Expand Up @@ -189,6 +190,7 @@ pub enum GitRepo {
/// although most functions are implemented as free functions rather than
/// methods specifically on this structure itself (to make it easier to
/// organize).
#[cfg_attr(not(feature = "build-metrics"), derive(Clone))]
pub struct Build {
/// User-specified configuration from `config.toml`.
config: Config,
Expand Down Expand Up @@ -242,7 +244,7 @@ pub struct Build {
metrics: metrics::BuildMetrics,
}

#[derive(Debug)]
#[derive(Debug, Clone)]
struct Crate {
name: Interned<String>,
deps: HashSet<Interned<String>>,
Expand Down Expand Up @@ -656,13 +658,20 @@ impl Build {
job::setup(self);
}

if let Subcommand::Format { check, paths } = &self.config.cmd {
return format::format(&builder::Builder::new(&self), *check, &paths);
}

// Download rustfmt early so that it can be used in rust-analyzer configs.
let _ = &builder::Builder::new(&self).initial_rustfmt();

// hardcoded subcommands
match &self.config.cmd {
Subcommand::Format { check, paths } => {
return format::format(&builder::Builder::new(&self), *check, &paths);
}
Subcommand::Suggest { run } => {
return suggest::suggest(&builder::Builder::new(&self), *run);
Ezrashaw marked this conversation as resolved.
Show resolved Hide resolved
}
_ => (),
}

{
let builder = builder::Builder::new(&self);
if let Some(path) = builder.paths.get(0) {
Expand Down
80 changes: 80 additions & 0 deletions src/bootstrap/suggest.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
#![cfg_attr(feature = "build-metrics", allow(unused))]

use std::str::FromStr;

use std::path::PathBuf;

use crate::{
builder::{Builder, Kind},
tool::Tool,
};

#[cfg(feature = "build-metrics")]
pub fn suggest(builder: &Builder<'_>, run: bool) {
panic!("`x suggest` is not supported with `build-metrics`")
}

/// Suggests a list of possible `x.py` commands to run based on modified files in branch.
#[cfg(not(feature = "build-metrics"))]
pub fn suggest(builder: &Builder<'_>, run: bool) {
let suggestions =
builder.tool_cmd(Tool::SuggestTests).output().expect("failed to run `suggest-tests` tool");

if !suggestions.status.success() {
println!("failed to run `suggest-tests` tool ({})", suggestions.status);
println!(
"`suggest_tests` stdout:\n{}`suggest_tests` stderr:\n{}",
String::from_utf8(suggestions.stdout).unwrap(),
String::from_utf8(suggestions.stderr).unwrap()
);
panic!("failed to run `suggest-tests`");
}

let suggestions = String::from_utf8(suggestions.stdout).unwrap();
let suggestions = suggestions
.lines()
.map(|line| {
let mut sections = line.split_ascii_whitespace();

// this code expects one suggestion per line in the following format:
// <x_subcommand> {some number of flags} [optional stage number]
let cmd = sections.next().unwrap();
let stage = sections.next_back().map(|s| str::parse(s).ok()).flatten();
let paths: Vec<PathBuf> = sections.map(|p| PathBuf::from_str(p).unwrap()).collect();
Ezrashaw marked this conversation as resolved.
Show resolved Hide resolved

(cmd, stage, paths)
})
.collect::<Vec<_>>();

if !suggestions.is_empty() {
println!("==== SUGGESTIONS ====");
for sug in &suggestions {
print!("x {} ", sug.0);
if let Some(stage) = sug.1 {
print!("--stage {stage} ");
}

for path in &sug.2 {
print!("{} ", path.display());
}
println!();
}
println!("=====================");
} else {
println!("No suggestions found!");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
println!("No suggestions found!");
println!("No suggestions found!");
return;

Add an early return here to avoid printing the hint about --run

Copy link
Member

Choose a reason for hiding this comment

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

@Ezrashaw looks like this comment was never addressed?

Copy link
Contributor Author

@Ezrashaw Ezrashaw Apr 6, 2023

Choose a reason for hiding this comment

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

I believe it was? Line 57? It's underneath the box, kind of hidden lol.

return;
}

if run {
for sug in suggestions {
let mut build = builder.build.clone();

let builder =
Builder::new_standalone(&mut build, Kind::parse(&sug.0).unwrap(), sug.2, sug.1);

builder.execute_cli()
}
} else {
println!("help: consider using the `--run` flag to automatically run suggested tests");
}
}
Loading