Skip to content

build_helper::compiletest module factored out of compiletest for use by bootstrap #135653

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
68 changes: 37 additions & 31 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::ffi::{OsStr, OsString};
use std::path::{Path, PathBuf};
use std::{env, fs, iter};

use build_helper::compiletest::Mode as CtMode;
use clap_complete::shells;

use crate::core::build_steps::compile::run_cargo;
Expand Down Expand Up @@ -915,7 +916,7 @@ impl Step for RustdocJSNotStd {
builder.ensure(Compiletest {
compiler: self.compiler,
target: self.target,
mode: "rustdoc-js",
mode: CtMode::RustdocJs,
suite: "rustdoc-js",
path: "tests/rustdoc-js",
compare_mode: None,
Expand Down Expand Up @@ -1347,68 +1348,68 @@ impl Step for CrateBuildHelper {
}
}

test!(Ui { path: "tests/ui", mode: "ui", suite: "ui", default: true });
test!(Ui { path: "tests/ui", mode: CtMode::Ui, suite: "ui", default: true });

test!(Crashes { path: "tests/crashes", mode: "crashes", suite: "crashes", default: true });
test!(Crashes { path: "tests/crashes", mode: CtMode::Crashes, suite: "crashes", default: true });

test!(Codegen { path: "tests/codegen", mode: "codegen", suite: "codegen", default: true });
test!(Codegen { path: "tests/codegen", mode: CtMode::Codegen, suite: "codegen", default: true });

test!(CodegenUnits {
path: "tests/codegen-units",
mode: "codegen-units",
mode: CtMode::CodegenUnits,
suite: "codegen-units",
default: true,
});

test!(Incremental {
path: "tests/incremental",
mode: "incremental",
mode: CtMode::Incremental,
suite: "incremental",
default: true,
});

test!(Debuginfo {
path: "tests/debuginfo",
mode: "debuginfo",
mode: CtMode::DebugInfo,
suite: "debuginfo",
default: true,
compare_mode: Some("split-dwarf"),
});

test!(UiFullDeps {
path: "tests/ui-fulldeps",
mode: "ui",
mode: CtMode::Ui,
suite: "ui-fulldeps",
default: true,
only_hosts: true,
});

test!(Rustdoc {
path: "tests/rustdoc",
mode: "rustdoc",
mode: CtMode::Rustdoc,
suite: "rustdoc",
default: true,
only_hosts: true,
});
test!(RustdocUi {
path: "tests/rustdoc-ui",
mode: "ui",
mode: CtMode::Ui,
suite: "rustdoc-ui",
default: true,
only_hosts: true,
});

test!(RustdocJson {
path: "tests/rustdoc-json",
mode: "rustdoc-json",
mode: CtMode::RustdocJson,
suite: "rustdoc-json",
default: true,
only_hosts: true,
});

test!(Pretty {
path: "tests/pretty",
mode: "pretty",
mode: CtMode::Pretty,
suite: "pretty",
default: true,
only_hosts: true,
Expand Down Expand Up @@ -1441,15 +1442,20 @@ impl Step for RunMake {
builder.ensure(Compiletest {
compiler: self.compiler,
target: self.target,
mode: "run-make",
mode: CtMode::RunMake,
suite: "run-make",
path: "tests/run-make",
compare_mode: None,
});
}
}

test!(Assembly { path: "tests/assembly", mode: "assembly", suite: "assembly", default: true });
test!(Assembly {
path: "tests/assembly",
mode: CtMode::Assembly,
suite: "assembly",
default: true
});

/// Runs the coverage test suite at `tests/coverage` in some or all of the
/// coverage test modes.
Expand Down Expand Up @@ -1536,7 +1542,7 @@ impl Step for Coverage {
builder.ensure(Compiletest {
compiler,
target,
mode,
mode: mode.parse().unwrap(),
suite: Self::SUITE,
path: Self::PATH,
compare_mode: None,
Expand All @@ -1546,7 +1552,7 @@ impl Step for Coverage {

test!(CoverageRunRustdoc {
path: "tests/coverage-run-rustdoc",
mode: "coverage-run",
mode: CtMode::CoverageRun,
suite: "coverage-run-rustdoc",
default: true,
only_hosts: true,
Expand Down Expand Up @@ -1578,7 +1584,7 @@ impl Step for MirOpt {
builder.ensure(Compiletest {
compiler: self.compiler,
target,
mode: "mir-opt",
mode: CtMode::MirOpt,
suite: "mir-opt",
path: "tests/mir-opt",
compare_mode: None,
Expand Down Expand Up @@ -1615,7 +1621,7 @@ impl Step for MirOpt {
struct Compiletest {
compiler: Compiler,
target: TargetSelection,
mode: &'static str,
mode: CtMode,
suite: &'static str,
path: &'static str,
compare_mode: Option<&'static str>,
Expand Down Expand Up @@ -1729,7 +1735,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the

let is_rustdoc = suite == "rustdoc-ui" || suite == "rustdoc-js";

if mode == "run-make" {
if mode == CtMode::RunMake {
let cargo_path = if builder.top_stage == 0 {
// If we're using `--stage 0`, we should provide the bootstrap cargo.
builder.initial_cargo.clone()
Expand All @@ -1752,17 +1758,17 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
}

// Avoid depending on rustdoc when we don't need it.
if mode == "rustdoc"
|| mode == "run-make"
|| (mode == "ui" && is_rustdoc)
|| mode == "rustdoc-js"
|| mode == "rustdoc-json"
if mode == CtMode::Rustdoc
|| mode == CtMode::RunMake
|| (mode == CtMode::Ui && is_rustdoc)
|| mode == CtMode::RustdocJs
|| mode == CtMode::RustdocJson
|| suite == "coverage-run-rustdoc"
{
cmd.arg("--rustdoc-path").arg(builder.rustdoc(compiler));
}

if mode == "rustdoc-json" {
if mode == CtMode::RustdocJson {
// Use the beta compiler for jsondocck
let json_compiler = compiler.with_stage(0);
cmd.arg("--jsondocck-path")
Expand All @@ -1771,7 +1777,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
.arg(builder.ensure(tool::JsonDocLint { compiler: json_compiler, target }));
}

if matches!(mode, "coverage-map" | "coverage-run") {
if matches!(mode, CtMode::CoverageMap | CtMode::CoverageRun) {
let coverage_dump = builder.tool_exe(Tool::CoverageDump);
cmd.arg("--coverage-dump-path").arg(coverage_dump);
}
Expand All @@ -1789,7 +1795,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
cmd.arg("--sysroot-base").arg(sysroot);
cmd.arg("--stage-id").arg(stage_id);
cmd.arg("--suite").arg(suite);
cmd.arg("--mode").arg(mode);
cmd.arg("--mode").arg(mode.to_str());
cmd.arg("--target").arg(target.rustc_target_arg());
cmd.arg("--host").arg(&*compiler.host.triple);
cmd.arg("--llvm-filecheck").arg(builder.llvm_filecheck(builder.config.build));
Expand Down Expand Up @@ -1827,7 +1833,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the

if let Some(ref nodejs) = builder.config.nodejs {
cmd.arg("--nodejs").arg(nodejs);
} else if mode == "rustdoc-js" {
} else if mode == CtMode::RustdocJs {
panic!("need nodejs to run rustdoc-js suite");
}
if let Some(ref npm) = builder.config.npm {
Expand Down Expand Up @@ -1985,7 +1991,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
cmd.env("RUSTFLAGS", rustflags);
}

if !builder.config.dry_run() && matches!(mode, "run-make" | "coverage-run") {
if !builder.config.dry_run() && matches!(mode, CtMode::RunMake | CtMode::CoverageRun) {
// The llvm/bin directory contains many useful cross-platform
// tools. Pass the path to run-make tests so they can use them.
// (The coverage-run tests also need these tools to process
Expand All @@ -1997,7 +2003,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
cmd.arg("--llvm-bin-dir").arg(llvm_bin_path);
}

if !builder.config.dry_run() && mode == "run-make" {
if !builder.config.dry_run() && mode == CtMode::RunMake {
// If LLD is available, add it to the PATH
if builder.config.lld_enabled {
let lld_install_root =
Expand All @@ -2017,7 +2023,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the

// Only pass correct values for these flags for the `run-make` suite as it
// requires that a C++ compiler was configured which isn't always the case.
if !builder.config.dry_run() && mode == "run-make" {
if !builder.config.dry_run() && mode == CtMode::RunMake {
cmd.arg("--cc")
.arg(builder.cc(target))
.arg("--cxx")
Expand Down
130 changes: 130 additions & 0 deletions src/build_helper/src/compiletest.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
//! Types representing arguments to compiletest.

use std::fmt;
use std::str::FromStr;

pub use self::Mode::*;

macro_rules! string_enum {
($(#[$meta:meta])* $vis:vis enum $name:ident { $($variant:ident => $repr:expr,)* }) => {
$(#[$meta])*
$vis enum $name {
$($variant,)*
}

impl $name {
$vis const VARIANTS: &'static [Self] = &[$(Self::$variant,)*];
$vis const STR_VARIANTS: &'static [&'static str] = &[$(Self::$variant.to_str(),)*];

$vis const fn to_str(&self) -> &'static str {
match self {
$(Self::$variant => $repr,)*
}
}
}

impl fmt::Display for $name {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Display::fmt(self.to_str(), f)
}
}

impl FromStr for $name {
type Err = String;

fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
$($repr => Ok(Self::$variant),)*
_ => Err(format!(concat!("unknown `", stringify!($name), "` variant: `{}`"), s)),
}
}
}
}
}

// Make the macro visible outside of this module, for tests.
#[cfg(test)]
pub(crate) use string_enum;

string_enum! {
#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
pub enum Mode {
Copy link
Member

Choose a reason for hiding this comment

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

Please call this TestMode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why? the command line flag is called --mode, no? so why shouldn't the struct match?

Copy link
Member

@jieyouxu jieyouxu Jan 18, 2025

Choose a reason for hiding this comment

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

Because build_helpers is a shared dep between bootstrap, compiletest, and some other bootstrap tools. bootstrap itself already has a Mode, not to mention Mode is a very generic term.

--mode for compiletest is fine because it's very clear from context as its a flag of the compiletest binary itself (only matters when bootstrap is actually trying to run compiletest binary).

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 would think its position in the compiletest module would be adequate context.

i can if you really want me to, it's just gonna take a bit since it's refactoring across several crates.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I would prefer if this is called TestMode and not just Mode.

Pretty => "pretty",
DebugInfo => "debuginfo",
Codegen => "codegen",
Rustdoc => "rustdoc",
RustdocJson => "rustdoc-json",
CodegenUnits => "codegen-units",
Incremental => "incremental",
RunMake => "run-make",
Ui => "ui",
RustdocJs => "rustdoc-js",
MirOpt => "mir-opt",
Assembly => "assembly",
CoverageMap => "coverage-map",
CoverageRun => "coverage-run",
Crashes => "crashes",
}
}

impl Default for Mode {
fn default() -> Self {
Mode::Ui
}
}

impl Mode {
pub fn aux_dir_disambiguator(self) -> &'static str {
Copy link
Member

Choose a reason for hiding this comment

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

I don't feel like these two helpers belong in build_helpers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would require turning them into free functions. do you want me to do that?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please do move them as free functions into compiletest, this is an impl detail that doesn't feel like they belong here

// Pretty-printing tests could run concurrently, and if they do,
// they need to keep their output segregated.
match self {
Pretty => ".pretty",
_ => "",
}
}

pub fn output_dir_disambiguator(self) -> &'static str {
// Coverage tests use the same test files for multiple test modes,
// so each mode should have a separate output directory.
match self {
CoverageMap | CoverageRun => self.to_str(),
_ => "",
}
}
}

string_enum! {
#[derive(Clone, Copy, PartialEq, Debug, Hash)]
pub enum PassMode {
Check => "check",
Build => "build",
Run => "run",
}
}

#[derive(Copy, Clone, Debug, PartialEq, PartialOrd)]
pub enum FailMode {
Check,
Build,
Run,
}

string_enum! {
#[derive(Clone, Debug, PartialEq)]
pub enum CompareMode {
Polonius => "polonius",
NextSolver => "next-solver",
NextSolverCoherence => "next-solver-coherence",
SplitDwarf => "split-dwarf",
SplitDwarfSingle => "split-dwarf-single",
}
}

string_enum! {
#[derive(Clone, Copy, Debug, PartialEq)]
pub enum Debugger {
Cdb => "cdb",
Gdb => "gdb",
Lldb => "lldb",
}
}
3 changes: 3 additions & 0 deletions src/build_helper/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
//! Types and functions shared across tools in this workspace.

pub mod ci;
pub mod compiletest;
pub mod drop_bomb;
pub mod fs;
pub mod git;
pub mod metrics;
pub mod stage0_parser;
#[cfg(test)]
mod tests;
pub mod util;

/// The default set of crates for opt-dist to collect LLVM profiles.
Expand Down
Loading
Loading