user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Rollup merge of #147506 - Zalathar:isolate, r=jieyouxu

Browse source 
compiletest: Isolate public APIs and minimize public surface area

As part of my ongoing efforts to improve directive parsing, I would like to be able to make internal changes without worrying about whether they're going to break the rustdoc-gui-test tool. That tool currently uses compiletest as a dependency to help with directive parsing.

This PR therefore isolates all of compiletest's public APIs into two dedicated modules, one used by rustdoc-gui-test, and one used by the compiletest binary in `compiletest/src/bin/main.rs`.

All other modules (and crate-root items) are then made non-`pub` to achieve the API isolation. Doing so reveals some unused items, which have been removed.

(To reduce the amount of immediate textual churn, this PR does not comprehensively replace `pub` with `pub(crate)` throughout the whole crate; that could be done in a follow-up PR.)

---

Ideally, rustdoc-gui-test would not depend on compiletest at all, but properly fixing that is out of scope for this PR.
- rust-lang/rust#143827

r? jieyouxu
This commit is contained in:
Stuart Cook 2025-10-09 18:43:27 +11:00 • committed by GitHub
commit 76d4f37c40
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 190 additions and 165 deletions
Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock
View file

@ -4805,7 +4805,6 @@ name = "rustdoc-gui-test"
version = "0.1.0"
dependencies = [
"build_helper",
"camino",
"compiletest",
"getopts",
"walkdir",

3
src/bootstrap/src/core/build_steps/test.rs
View file

@ -1974,9 +1974,6 @@ HELP: You can add it into `bootstrap.toml` in `rust.codegen-backends = [{name:?}
} else if mode == "rustdoc-js" {
panic!("need nodejs to run rustdoc-js suite");
}
if let Some(ref npm) = builder.config.npm {
cmd.arg("--npm").arg(npm);
}
if builder.config.rust_optimize_tests {
cmd.arg("--optimize-tests");
}

22
src/tools/compiletest/src/bin/main.rs
View file

@ -1,23 +1,3 @@
use std::env;
use std::io::IsTerminal;
use std::sync::Arc;
use compiletest::{early_config_check, parse_config, run_tests};
fn main() {
tracing_subscriber::fmt::init();
// colored checks stdout by default, but for some reason only stderr is a terminal.
// compiletest *does* print many things to stdout, but it doesn't really matter.
if std::io::stderr().is_terminal()
&& matches!(std::env::var("NO_COLOR").as_deref(), Err(_) | Ok("0"))
{
colored::control::set_override(true);
}
let config = Arc::new(parse_config(env::args().collect()));
early_config_check(&config);
run_tests(config);
compiletest::cli::main();
}

26
src/tools/compiletest/src/cli.rs Normal file
View file

@ -0,0 +1,26 @@
//! Isolates the APIs used by `bin/main.rs`, to help minimize the surface area
//! of public exports from the compiletest library crate.
use std::env;
use std::io::IsTerminal;
use std::sync::Arc;
use crate::{early_config_check, parse_config, run_tests};
pub fn main() {
tracing_subscriber::fmt::init();
// colored checks stdout by default, but for some reason only stderr is a terminal.
// compiletest *does* print many things to stdout, but it doesn't really matter.
if std::io::stderr().is_terminal()
&& matches!(std::env::var("NO_COLOR").as_deref(), Err(_) | Ok("0"))
{
colored::control::set_override(true);
}
let config = Arc::new(parse_config(env::args().collect()));
early_config_check(&config);
run_tests(config);
}

109
src/tools/compiletest/src/common.rs
View file

@ -631,8 +631,6 @@ pub struct Config {
/// Path to a NodeJS executable. Used for JS doctests, emscripten and WASM tests.
pub nodejs: Option<String>,
/// Path to a npm executable. Used for rustdoc GUI tests.
pub npm: Option<String>,
/// Whether to rerun tests even if the inputs are unchanged.
pub force_rerun: bool,
@ -686,110 +684,6 @@ pub struct Config {
}
impl Config {
/// Incomplete config intended for `src/tools/rustdoc-gui-test` **only** as
/// `src/tools/rustdoc-gui-test` wants to reuse `compiletest`'s directive -> test property
/// handling for `//@ {compile,run}-flags`, do not use for any other purpose.
///
/// FIXME(#143827): this setup feels very hacky. It so happens that `tests/rustdoc-gui/`
/// **only** uses `//@ {compile,run}-flags` for now and not any directives that actually rely on
/// info that is assumed available in a fully populated [`Config`].
pub fn incomplete_for_rustdoc_gui_test() -> Config {
// FIXME(#143827): spelling this out intentionally, because this is questionable.
//
// For instance, `//@ ignore-stage1` will not work at all.
Config {
mode: TestMode::Rustdoc,
// E.g. this has no sensible default tbh.
suite: TestSuite::Ui,
// Dummy values.
edition: Default::default(),
bless: Default::default(),
fail_fast: Default::default(),
compile_lib_path: Utf8PathBuf::default(),
run_lib_path: Utf8PathBuf::default(),
rustc_path: Utf8PathBuf::default(),
cargo_path: Default::default(),
stage0_rustc_path: Default::default(),
query_rustc_path: Default::default(),
rustdoc_path: Default::default(),
coverage_dump_path: Default::default(),
python: Default::default(),
jsondocck_path: Default::default(),
jsondoclint_path: Default::default(),
llvm_filecheck: Default::default(),
llvm_bin_dir: Default::default(),
run_clang_based_tests_with: Default::default(),
src_root: Utf8PathBuf::default(),
src_test_suite_root: Utf8PathBuf::default(),
build_root: Utf8PathBuf::default(),
build_test_suite_root: Utf8PathBuf::default(),
sysroot_base: Utf8PathBuf::default(),
stage: Default::default(),
stage_id: String::default(),
debugger: Default::default(),
run_ignored: Default::default(),
with_rustc_debug_assertions: Default::default(),
with_std_debug_assertions: Default::default(),
filters: Default::default(),
skip: Default::default(),
filter_exact: Default::default(),
force_pass_mode: Default::default(),
run: Default::default(),
runner: Default::default(),
host_rustcflags: Default::default(),
target_rustcflags: Default::default(),
rust_randomized_layout: Default::default(),
optimize_tests: Default::default(),
target: Default::default(),
host: Default::default(),
cdb: Default::default(),
cdb_version: Default::default(),
gdb: Default::default(),
gdb_version: Default::default(),
lldb_version: Default::default(),
llvm_version: Default::default(),
system_llvm: Default::default(),
android_cross_path: Default::default(),
adb_path: Default::default(),
adb_test_dir: Default::default(),
adb_device_status: Default::default(),
lldb_python_dir: Default::default(),
verbose: Default::default(),
color: Default::default(),
remote_test_client: Default::default(),
compare_mode: Default::default(),
rustfix_coverage: Default::default(),
has_html_tidy: Default::default(),
has_enzyme: Default::default(),
channel: Default::default(),
git_hash: Default::default(),
cc: Default::default(),
cxx: Default::default(),
cflags: Default::default(),
cxxflags: Default::default(),
ar: Default::default(),
target_linker: Default::default(),
host_linker: Default::default(),
llvm_components: Default::default(),
nodejs: Default::default(),
npm: Default::default(),
force_rerun: Default::default(),
only_modified: Default::default(),
target_cfgs: Default::default(),
builtin_cfg_names: Default::default(),
supported_crate_types: Default::default(),
nocapture: Default::default(),
nightly_branch: Default::default(),
git_merge_commit_email: Default::default(),
profiler_runtime: Default::default(),
diff_command: Default::default(),
minicore_path: Default::default(),
default_codegen_backend: CodegenBackend::Llvm,
override_codegen_backend: None,
}
}
/// FIXME: this run scheme is... confusing.
pub fn run_enabled(&self) -> bool {
self.run.unwrap_or_else(|| {
@ -825,7 +719,8 @@ impl Config {
self.target_cfg().abi == abi
}
pub fn matches_family(&self, family: &str) -> bool {
#[cfg_attr(not(test), expect(dead_code, reason = "only used by tests for `ignore-{family}`"))]
pub(crate) fn matches_family(&self, family: &str) -> bool {
self.target_cfg().families.iter().any(|f| f == family)
}

3
src/tools/compiletest/src/directives.rs
View file

@ -198,8 +198,6 @@ pub struct TestProps {
pub filecheck_flags: Vec<String>,
/// Don't automatically insert any `--check-cfg` args
pub no_auto_check_cfg: bool,
/// Run tests which require enzyme being build
pub has_enzyme: bool,
/// Build and use `minicore` as `core` stub for `no_core` tests in cross-compilation scenarios
/// that don't otherwise want/need `-Z build-std`.
pub add_core_stubs: bool,
@ -314,7 +312,6 @@ impl TestProps {
llvm_cov_flags: vec![],
filecheck_flags: vec![],
no_auto_check_cfg: false,
has_enzyme: false,
add_core_stubs: false,
core_stubs_compile_flags: vec![],
dont_require_annotations: Default::default(),

36
src/tools/compiletest/src/lib.rs
View file

@ -3,20 +3,22 @@
#[cfg(test)]
mod tests;
pub mod common;
pub mod cli;
mod common;
mod debuggers;
pub mod diagnostics;
pub mod directives;
pub mod edition;
pub mod errors;
mod diagnostics;
mod directives;
mod edition;
mod errors;
mod executor;
mod json;
mod output_capture;
mod panic_hook;
mod raise_fd_limit;
mod read2;
pub mod runtest;
pub mod util;
mod runtest;
pub mod rustdoc_gui_test;
mod util;
use core::panic;
use std::collections::HashSet;
@ -48,7 +50,7 @@ use crate::executor::{CollectedTest, ColorConfig};
/// The config mostly reflects command-line arguments, but there might also be
/// some code here that inspects environment variables or even runs executables
/// (e.g. when discovering debugger versions).
pub fn parse_config(args: Vec<String>) -> Config {
fn parse_config(args: Vec<String>) -> Config {
let mut opts = Options::new();
opts.reqopt("", "compile-lib-path", "path to host shared libraries", "PATH")
.reqopt("", "run-lib-path", "path to target shared libraries", "PATH")
@ -462,7 +464,6 @@ pub fn parse_config(args: Vec<String>) -> Config {
host_linker: matches.opt_str("host-linker"),
llvm_components: matches.opt_str("llvm-components").unwrap(),
nodejs: matches.opt_str("nodejs"),
npm: matches.opt_str("npm"),
force_rerun: matches.opt_present("force-rerun"),
@ -486,14 +487,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
}
}
pub fn opt_str(maybestr: &Option<String>) -> &str {
match *maybestr {
None => "(none)",
Some(ref s) => s,
}
}
pub fn opt_str2(maybestr: Option<String>) -> String {
fn opt_str2(maybestr: Option<String>) -> String {
match maybestr {
None => "(none)".to_owned(),
Some(s) => s,
@ -501,7 +495,7 @@ pub fn opt_str2(maybestr: Option<String>) -> String {
}
/// Called by `main` after the config has been parsed.
pub fn run_tests(config: Arc<Config>) {
fn run_tests(config: Arc<Config>) {
debug!(?config, "run_tests");
panic_hook::install_panic_hook();
@ -639,7 +633,7 @@ impl TestCollector {
/// FIXME(Zalathar): Now that we no longer rely on libtest, try to overhaul
/// test discovery to take into account the filters/tests specified on the
/// command-line, instead of having to enumerate everything.
pub(crate) fn collect_and_make_tests(config: Arc<Config>) -> Vec<CollectedTest> {
fn collect_and_make_tests(config: Arc<Config>) -> Vec<CollectedTest> {
debug!("making tests from {}", config.src_test_suite_root);
let common_inputs_stamp = common_inputs_stamp(&config);
let modified_tests =
@ -851,7 +845,7 @@ fn collect_tests_from_dir(
}
/// Returns true if `file_name` looks like a proper test file name.
pub fn is_test(file_name: &str) -> bool {
fn is_test(file_name: &str) -> bool {
if !file_name.ends_with(".rs") {
return false;
}
@ -1131,7 +1125,7 @@ fn check_for_overlapping_test_paths(found_path_stems: &HashSet<Utf8PathBuf>) {
}
}
pub fn early_config_check(config: &Config) {
fn early_config_check(config: &Config) {
if !config.has_html_tidy && config.mode == TestMode::Rustdoc {
warning!("`tidy` (html-tidy.org) is not installed; diffs will not be generated");
}

142
src/tools/compiletest/src/rustdoc_gui_test.rs Normal file
View file

@ -0,0 +1,142 @@
//! This module isolates the compiletest APIs used by the rustdoc-gui-test tool.
//!
//! Thanks to this isolation layer, changes to compiletest directive parsing
//! might require changes to the items in this module, but shouldn't require
//! changes to rustdoc-gui-test itself.
//!
//! The current relationship between compiletest and rustdoc-gui-test is
//! awkward. Ideally, rustdoc-gui-test should either split off its own
//! directive parser and become fully independent, or be incorporated into
//! compiletest as another test mode.
//!
//! See <https://github.com/rust-lang/rust/issues/143827> for more context.
use std::path::Path;
use camino::{Utf8Path, Utf8PathBuf};
use crate::common::{CodegenBackend, Config, TestMode, TestSuite};
use crate::directives::TestProps;
/// Subset of compiletest directive values that are actually used by
/// rustdoc-gui-test.
#[derive(Debug)]
pub struct RustdocGuiTestProps {
pub compile_flags: Vec<String>,
pub run_flags: Vec<String>,
}
impl RustdocGuiTestProps {
pub fn from_file(test_file_path: &Path) -> Self {
let test_file_path = Utf8Path::from_path(test_file_path).unwrap();
let config = incomplete_config_for_rustdoc_gui_test();
let props = TestProps::from_file(test_file_path, None, &config);
let TestProps { compile_flags, run_flags, .. } = props;
Self { compile_flags, run_flags }
}
}
/// Incomplete config intended for `src/tools/rustdoc-gui-test` **only** as
/// `src/tools/rustdoc-gui-test` wants to reuse `compiletest`'s directive -> test property
/// handling for `//@ {compile,run}-flags`, do not use for any other purpose.
///
/// FIXME(#143827): this setup feels very hacky. It so happens that `tests/rustdoc-gui/`
/// **only** uses `//@ {compile,run}-flags` for now and not any directives that actually rely on
/// info that is assumed available in a fully populated [`Config`].
fn incomplete_config_for_rustdoc_gui_test() -> Config {
// FIXME(#143827): spelling this out intentionally, because this is questionable.
//
// For instance, `//@ ignore-stage1` will not work at all.
Config {
mode: TestMode::Rustdoc,
// E.g. this has no sensible default tbh.
suite: TestSuite::Ui,
// Dummy values.
edition: Default::default(),
bless: Default::default(),
fail_fast: Default::default(),
compile_lib_path: Utf8PathBuf::default(),
run_lib_path: Utf8PathBuf::default(),
rustc_path: Utf8PathBuf::default(),
cargo_path: Default::default(),
stage0_rustc_path: Default::default(),
query_rustc_path: Default::default(),
rustdoc_path: Default::default(),
coverage_dump_path: Default::default(),
python: Default::default(),
jsondocck_path: Default::default(),
jsondoclint_path: Default::default(),
llvm_filecheck: Default::default(),
llvm_bin_dir: Default::default(),
run_clang_based_tests_with: Default::default(),
src_root: Utf8PathBuf::default(),
src_test_suite_root: Utf8PathBuf::default(),
build_root: Utf8PathBuf::default(),
build_test_suite_root: Utf8PathBuf::default(),
sysroot_base: Utf8PathBuf::default(),
stage: Default::default(),
stage_id: String::default(),
debugger: Default::default(),
run_ignored: Default::default(),
with_rustc_debug_assertions: Default::default(),
with_std_debug_assertions: Default::default(),
filters: Default::default(),
skip: Default::default(),
filter_exact: Default::default(),
force_pass_mode: Default::default(),
run: Default::default(),
runner: Default::default(),
host_rustcflags: Default::default(),
target_rustcflags: Default::default(),
rust_randomized_layout: Default::default(),
optimize_tests: Default::default(),
target: Default::default(),
host: Default::default(),
cdb: Default::default(),
cdb_version: Default::default(),
gdb: Default::default(),
gdb_version: Default::default(),
lldb_version: Default::default(),
llvm_version: Default::default(),
system_llvm: Default::default(),
android_cross_path: Default::default(),
adb_path: Default::default(),
adb_test_dir: Default::default(),
adb_device_status: Default::default(),
lldb_python_dir: Default::default(),
verbose: Default::default(),
color: Default::default(),
remote_test_client: Default::default(),
compare_mode: Default::default(),
rustfix_coverage: Default::default(),
has_html_tidy: Default::default(),
has_enzyme: Default::default(),
channel: Default::default(),
git_hash: Default::default(),
cc: Default::default(),
cxx: Default::default(),
cflags: Default::default(),
cxxflags: Default::default(),
ar: Default::default(),
target_linker: Default::default(),
host_linker: Default::default(),
llvm_components: Default::default(),
nodejs: Default::default(),
force_rerun: Default::default(),
only_modified: Default::default(),
target_cfgs: Default::default(),
builtin_cfg_names: Default::default(),
supported_crate_types: Default::default(),
nocapture: Default::default(),
nightly_branch: Default::default(),
git_merge_commit_email: Default::default(),
profiler_runtime: Default::default(),
diff_command: Default::default(),
minicore_path: Default::default(),
default_codegen_backend: CodegenBackend::Llvm,
override_codegen_backend: None,
}
}

2
src/tools/compiletest/src/util.rs
View file

@ -102,7 +102,9 @@ macro_rules! string_enum {
}
impl $name {
#[allow(dead_code)]
$vis const VARIANTS: &'static [Self] = &[$(Self::$variant,)*];
#[allow(dead_code)]
$vis const STR_VARIANTS: &'static [&'static str] = &[$(Self::$variant.to_str(),)*];
$vis const fn to_str(&self) -> &'static str {

1
src/tools/rustdoc-gui-test/Cargo.toml
View file

@ -5,7 +5,6 @@ edition = "2021"
[dependencies]
build_helper = { path = "../../build_helper" }
camino = "1"
compiletest = { path = "../compiletest" }
getopts = "0.2"
walkdir = "2"

10
src/tools/rustdoc-gui-test/src/main.rs
View file

@ -5,7 +5,7 @@ use std::sync::Arc;
use build_helper::npm;
use build_helper::util::try_run;
use compiletest::directives::TestProps;
use compiletest::rustdoc_gui_test::RustdocGuiTestProps;
use config::Config;
mod config;
@ -43,13 +43,7 @@ fn main() -> Result<(), ()> {
.current_dir(path);
if let Some(librs) = find_librs(entry.path()) {
let compiletest_c = compiletest::common::Config::incomplete_for_rustdoc_gui_test();
let test_props = TestProps::from_file(
&camino::Utf8PathBuf::try_from(librs).unwrap(),
None,
&compiletest_c,
);
let test_props = RustdocGuiTestProps::from_file(&librs);
if !test_props.compile_flags.is_empty() {
cargo.env("RUSTDOCFLAGS", test_props.compile_flags.join(" "));