Skip to content

Convert all Makefile tests to rmake tests with legacy-makefile-test #131598

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

Closed
wants to merge 1 commit into from
Closed
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
1 change: 1 addition & 0 deletions src/tools/compiletest/src/command-list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"ignore-x86_64-unknown-linux-gnu",
"incremental",
"known-bug",
"legacy-makefile-test",
"llvm-cov-flags",
"min-cdb-version",
"min-gdb-version",
Expand Down
16 changes: 12 additions & 4 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,9 @@ pub struct TestProps {
pub no_auto_check_cfg: bool,
/// Run tests which require enzyme being build
pub has_enzyme: bool,
/// This `run-make` test should run a legacy Makefile, instead of building
/// and running `rmake.rs`.
pub legacy_makefile_test: bool,
}

mod directives {
Expand Down Expand Up @@ -242,6 +245,7 @@ mod directives {
pub const LLVM_COV_FLAGS: &'static str = "llvm-cov-flags";
pub const FILECHECK_FLAGS: &'static str = "filecheck-flags";
pub const NO_AUTO_CHECK_CFG: &'static str = "no-auto-check-cfg";
pub const LEGACY_MAKEFILE_TEST: &'static str = "legacy-makefile-test";
// This isn't a real directive, just one that is probably mistyped often
pub const INCORRECT_COMPILER_FLAGS: &'static str = "compiler-flags";
}
Expand Down Expand Up @@ -299,6 +303,7 @@ impl TestProps {
filecheck_flags: vec![],
no_auto_check_cfg: false,
has_enzyme: false,
legacy_makefile_test: false,
}
}

Expand Down Expand Up @@ -563,6 +568,12 @@ impl TestProps {
}

config.set_name_directive(ln, NO_AUTO_CHECK_CFG, &mut self.no_auto_check_cfg);

config.set_name_directive(
ln,
LEGACY_MAKEFILE_TEST,
&mut self.legacy_makefile_test,
);
},
);

Expand Down Expand Up @@ -828,9 +839,6 @@ fn iter_header(
}
}

// NOTE(jieyouxu): once we get rid of `Makefile`s we can unconditionally check for `//@`.
let comment = if testfile.extension().is_some_and(|e| e == "rs") { "//@" } else { "#" };

let mut rdr = BufReader::with_capacity(1024, rdr);
let mut ln = String::new();
let mut line_number = 0;
Expand All @@ -856,7 +864,7 @@ fn iter_header(
return;
}

let Some((header_revision, non_revisioned_directive_line)) = line_directive(comment, ln)
let Some((header_revision, non_revisioned_directive_line)) = line_directive("//@", ln)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that line_directive is also called by debugger tests to parse its own “directives” with a // prefix. But I hope to be able to eventually split that off to use its own separate parsing function.

else {
continue;
};
Expand Down
20 changes: 2 additions & 18 deletions src/tools/compiletest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -704,13 +704,7 @@ fn collect_tests_from_dir(
}

if config.mode == Mode::RunMake {
if dir.join("Makefile").exists() && dir.join("rmake.rs").exists() {
return Err(io::Error::other(
"run-make tests cannot have both `Makefile` and `rmake.rs`",
));
}

if dir.join("Makefile").exists() || dir.join("rmake.rs").exists() {
if dir.join("rmake.rs").exists() {
let paths = TestPaths {
file: dir.to_path_buf(),
relative_dir: relative_dir_path.parent().unwrap().to_path_buf(),
Expand Down Expand Up @@ -789,17 +783,7 @@ fn make_test(
poisoned: &mut bool,
) -> Vec<test::TestDescAndFn> {
let test_path = if config.mode == Mode::RunMake {
if testpaths.file.join("rmake.rs").exists() && testpaths.file.join("Makefile").exists() {
panic!("run-make tests cannot have both `rmake.rs` and `Makefile`");
}

if testpaths.file.join("rmake.rs").exists() {
// Parse directives in rmake.rs.
testpaths.file.join("rmake.rs")
} else {
// Parse directives in the Makefile.
testpaths.file.join("Makefile")
}
testpaths.file.join("rmake.rs")
} else {
PathBuf::from(&testpaths.file)
};
Expand Down
10 changes: 8 additions & 2 deletions src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use tracing::*;

use crate::common::{
Assembly, Codegen, CodegenUnits, CompareMode, Config, CoverageMap, CoverageRun, Crashes,
DebugInfo, Debugger, FailMode, Incremental, JsDocTest, MirOpt, PassMode, Pretty, RunMake,
DebugInfo, Debugger, FailMode, Incremental, JsDocTest, MirOpt, Mode, PassMode, Pretty, RunMake,
Rustdoc, RustdocJson, TestPaths, UI_EXTENSIONS, UI_FIXED, UI_RUN_STDERR, UI_RUN_STDOUT,
UI_STDERR, UI_STDOUT, UI_SVG, UI_WINDOWS_SVG, Ui, expected_output_path, incremental_dir,
output_base_dir, output_base_name, output_testname_unique,
Expand Down Expand Up @@ -128,7 +128,13 @@ pub fn run(config: Arc<Config>, testpaths: &TestPaths, revision: Option<&str>) {
print!("\n\n");
}
debug!("running {:?}", testpaths.file.display());
let mut props = TestProps::from_file(&testpaths.file, revision, &config);
let mut props = {
let file = match config.mode {
Mode::RunMake => &testpaths.file.join("rmake.rs"),
_ => &testpaths.file,
};
TestProps::from_file(file, revision, &config)
};
Comment on lines +131 to +137
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future I'd like to experiment with making the canonical name of a run-make test be the path of its rmake.rs, rather than the path of the enclosing directory. That should help eliminate a bunch of special cases for dealing with Mode::RunMake.

But when I tried, there were enough little complications that I didn't want to do so immediately in this PR.

Copy link
Member

@jieyouxu jieyouxu Oct 12, 2024

Choose a reason for hiding this comment

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

I have found working with the Testpaths abstraction to be tricky, among other things.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable to me. Yeah, there are some quirks with a specific primary test file vs directory, but I haven't done an extensive survey on that yet.


// For non-incremental (i.e. regular UI) tests, the incremental directory
// takes into account the revision name, since the revisions are independent
Expand Down
11 changes: 6 additions & 5 deletions src/tools/compiletest/src/runtest/run_make.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,18 @@ use crate::util::{copy_dir_all, dylib_env_var};

impl TestCx<'_> {
pub(super) fn run_rmake_test(&self) {
let test_dir = &self.testpaths.file;
if test_dir.join("rmake.rs").exists() {
self.run_rmake_v2_test();
} else if test_dir.join("Makefile").exists() {
if self.props.legacy_makefile_test {
self.run_rmake_legacy_test();
} else {
self.fatal("failed to find either `rmake.rs` or `Makefile`")
self.run_rmake_v2_test();
}
}

fn run_rmake_legacy_test(&self) {
if !self.testpaths.file.join("Makefile").exists() {
self.fatal("failed to find `Makefile` for legacy makefile test");
}

let cwd = env::current_dir().unwrap();
let src_root = self.config.src_base.parent().unwrap().parent().unwrap();
let src_root = cwd.join(&src_root);
Expand Down
9 changes: 9 additions & 0 deletions tests/run-make/branch-protection-check-IBT/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//@ legacy-makefile-test

//@ only-x86_64

//@ ignore-test
// FIXME(jieyouxu): This test never runs because the `ifeq` check in the Makefile
// compares `x86` to `x86_64`, which always evaluates to false.
// When the test does run, the compilation does not include `.note.gnu.property`.
// See https://github.com/rust-lang/rust/pull/126720 for more information.
1 change: 1 addition & 0 deletions tests/run-make/cat-and-grep-sanity-check/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
//@ legacy-makefile-test
4 changes: 4 additions & 0 deletions tests/run-make/extern-fn-reachable/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
//@ legacy-makefile-test

//@ ignore-cross-compile
//@ ignore-windows-msvc
7 changes: 7 additions & 0 deletions tests/run-make/incr-add-rust-src-component/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//@ legacy-makefile-test

//@ ignore-cross-compile

// This test uses `ln -s` rather than copying to save testing time, but its
// usage doesn't work on windows. So ignore windows.
//@ ignore-windows
3 changes: 3 additions & 0 deletions tests/run-make/issue-84395-lto-embed-bitcode/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
//@ legacy-makefile-test

//@ needs-force-clang-based-tests
4 changes: 4 additions & 0 deletions tests/run-make/jobserver-error/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
//@ legacy-makefile-test

//@ only-linux
//@ ignore-cross-compile
4 changes: 4 additions & 0 deletions tests/run-make/libs-through-symlinks/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
//@ legacy-makefile-test

//@ ignore-cross-compile
//@ ignore-windows
4 changes: 4 additions & 0 deletions tests/run-make/split-debuginfo/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
//@ legacy-makefile-test

//@ ignore-cross-compile
//@ ignore-riscv64 On this platform only `-Csplit-debuginfo=off` is supported, see #120518
5 changes: 5 additions & 0 deletions tests/run-make/symbol-mangling-hashed/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
//@ legacy-makefile-test

//@ ignore-cross-compile
//@ only-linux
//@ only-x86_64
5 changes: 5 additions & 0 deletions tests/run-make/translation/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
//@ legacy-makefile-test

// This test uses `ln -s` rather than copying to save testing time, but its
// usage doesn't work on Windows.
//@ ignore-windows
Loading