Skip to content

Migrate emit-shared-files and emit-path-unhashed run-make tests to rmake #127335

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

Merged
merged 2 commits into from
Jul 7, 2024
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
34 changes: 31 additions & 3 deletions src/tools/run-make-support/src/diff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,7 @@ impl Diff {
self
}

#[track_caller]
pub fn run(&mut self) {
self.drop_bomb.defuse();
fn run_common(&self) -> (&str, &str, String, String) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be better to return a struct with named field instead of a tuple here.

Copy link
Member

Choose a reason for hiding this comment

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

Hum although it's a private function, so not really a big issue either...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was also about to comment on that but I also realized that's private lol

let expected = self.expected.as_ref().expect("expected text not set");
let mut actual = self.actual.as_ref().expect("actual text not set").to_string();
let expected_name = self.expected_name.as_ref().unwrap();
Expand All @@ -104,6 +102,14 @@ impl Diff {
.header(expected_name, actual_name)
.to_string();

(expected_name, actual_name, output, actual)
}

#[track_caller]
pub fn run(&mut self) {
self.drop_bomb.defuse();
let (expected_name, actual_name, output, actual) = self.run_common();

if !output.is_empty() {
// If we can bless (meaning we have a file to write into and the `RUSTC_BLESS_TEST`
// environment variable set), then we write into the file and return.
Expand All @@ -120,4 +126,26 @@ impl Diff {
)
}
}

#[track_caller]
pub fn run_fail(&mut self) {
self.drop_bomb.defuse();
let (expected_name, actual_name, output, actual) = self.run_common();

if output.is_empty() {
// If we can bless (meaning we have a file to write into and the `RUSTC_BLESS_TEST`
// environment variable set), then we write into the file and return.
if let Some(ref expected_file) = self.expected_file {
if std::env::var("RUSTC_BLESS_TEST").is_ok() {
println!("Blessing `{}`", expected_file.display());
fs_wrapper::write(expected_file, actual);
return;
}
}
panic!(
"test failed: `{}` is not different from `{}`\n\n{}",
expected_name, actual_name, output
)
}
}
}
2 changes: 0 additions & 2 deletions src/tools/tidy/src/allowed_run_make_makefiles.txt
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ run-make/dep-info-spaces/Makefile
run-make/dep-info/Makefile
run-make/dump-ice-to-disk/Makefile
run-make/dump-mono-stats/Makefile
run-make/emit-path-unhashed/Makefile
run-make/emit-shared-files/Makefile
run-make/emit-to-stdout/Makefile
run-make/env-dep-info/Makefile
run-make/export-executable-symbols/Makefile
Expand Down
37 changes: 0 additions & 37 deletions tests/run-make/emit-path-unhashed/Makefile

This file was deleted.

34 changes: 34 additions & 0 deletions tests/run-make/emit-path-unhashed/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Specifying how rustc outputs a file can be done in different ways, such as
// the output flag or the KIND=NAME syntax. However, some of these methods used
// to result in different hashes on output files even though they yielded the
// exact same result otherwise. This was fixed in #86045, and this test checks
// that the hash is only modified when the output is made different, such as by
// adding a new output type (in this test, metadata).
// See https://github.com/rust-lang/rust/issues/86044
Comment on lines +1 to +7
Copy link
Member

Choose a reason for hiding this comment

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

Remark: very helpful comment 👍


use run_make_support::{diff, fs_wrapper, rustc};

fn main() {
fs_wrapper::create_dir("emit");
fs_wrapper::create_dir("emit/a");
fs_wrapper::create_dir("emit/b");
fs_wrapper::create_dir("emit/c");
// The default output name.
rustc().emit("link").input("foo.rs").run();
// The output is named with the output flag.
rustc().emit("link").output("emit/a/libfoo.rlib").input("foo.rs").run();
// The output is named with link=NAME.
rustc().emit("link=emit/b/libfoo.rlib").input("foo.rs").run();
// The output is named with link=NAME, with an additional kind tacked on.
rustc().emit("link=emit/c/libfoo.rlib,metadata").input("foo.rs").run();

let base = rustc().arg("-Zls=root").input("libfoo.rlib").run().stdout_utf8();
let a = rustc().arg("-Zls=root").input("emit/a/libfoo.rlib").run().stdout_utf8();
let b = rustc().arg("-Zls=root").input("emit/b/libfoo.rlib").run().stdout_utf8();
let c = rustc().arg("-Zls=root").input("emit/c/libfoo.rlib").run().stdout_utf8();
// Both the output flag and link=NAME methods do not modify the hash of the output file.
diff().expected_text("base", &base).actual_text("a", a).run();
diff().expected_text("base", &base).actual_text("b", b).run();
// However, having multiple types of outputs does modify the hash.
diff().expected_text("base", &base).actual_text("c", c).run_fail();
}
46 changes: 0 additions & 46 deletions tests/run-make/emit-shared-files/Makefile

This file was deleted.

102 changes: 102 additions & 0 deletions tests/run-make/emit-shared-files/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// This test checks the functionality of one of rustdoc's unstable options,
// the ability to specify emit restrictions with `--emit`.
// `invocation-only` should only emit crate-specific files.
// `toolchain-only` should only emit toolchain-specific files.
// `all-shared` should only emit files that can be shared between crates.
// See https://github.com/rust-lang/rust/pull/83478

use run_make_support::{has_extension, has_prefix, rustdoc, shallow_find_files};
use std::path::Path;

fn main() {
rustdoc()
.arg("-Zunstable-options")
.arg("--emit=invocation-specific")
.output("invocation-only")
.arg("--resource-suffix=-xxx")
.args(&["--theme", "y.css"])
.args(&["--extend-css", "z.css"])
.input("x.rs")
.run();
assert!(Path::new("invocation-only/search-index-xxx.js").exists());
assert!(Path::new("invocation-only/settings.html").exists());
assert!(Path::new("invocation-only/x/all.html").exists());
assert!(Path::new("invocation-only/x/index.html").exists());
assert!(Path::new("invocation-only/theme-xxx.css").exists()); // generated from z.css
assert!(!Path::new("invocation-only/storage-xxx.js").exists());
assert!(!Path::new("invocation-only/SourceSerif4-It.ttf.woff2").exists());
// FIXME: this probably shouldn't have a suffix
assert!(Path::new("invocation-only/y-xxx.css").exists());
// FIXME: this is technically incorrect (see `write_shared`)
assert!(!Path::new("invocation-only/main-xxx.js").exists());
Comment on lines +28 to +31
Copy link
Member

Choose a reason for hiding this comment

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

Discussion: I know this is pre-existing, but what is happening here lol

Copy link
Member

@jieyouxu jieyouxu Jul 6, 2024

Choose a reason for hiding this comment

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

cc @GuillaumeGomez sorry for the ping, but you reviewed the original PR #83478: do you happen to know what's the status for --emit=toolchain-shared-resources --emit=invocation-specific or what this test is trying to exercise here? In particular, I don't quite understand the FIXME even though I tried to read the original PR that introduced this test.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe #83784 is relevant?

Copy link
Member

Choose a reason for hiding this comment

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

Ok I see. So normally, static files (like main.js) are generated with the hash of the file itself. I suppose the --resource-suffix option flag overrides this. Not sure if it's a good or bad though... Better to leave it as is.


rustdoc()
.arg("-Zunstable-options")
.arg("--emit=toolchain-shared-resources")
.output("toolchain-only")
.arg("--resource-suffix=-xxx")
.args(&["--extend-css", "z.css"])
.input("x.rs")
.run();
assert_eq!(
shallow_find_files("toolchain-only/static.files", |path| {
has_prefix(path, "storage-") && has_extension(path, "js")
})
.len(),
1
);
assert_eq!(
shallow_find_files("toolchain-only/static.files", |path| {
has_prefix(path, "SourceSerif4-It-") && has_extension(path, "woff2")
})
.len(),
1
);
assert_eq!(
shallow_find_files("toolchain-only/static.files", |path| {
has_prefix(path, "main-") && has_extension(path, "js")
})
.len(),
1
);
assert!(!Path::new("toolchain-only/search-index-xxx.js").exists());
assert!(!Path::new("toolchain-only/x/index.html").exists());
assert!(!Path::new("toolchain-only/theme.css").exists());
assert!(!Path::new("toolchain-only/y-xxx.css").exists());

rustdoc()
.arg("-Zunstable-options")
.arg("--emit=toolchain-shared-resources,unversioned-shared-resources")
.output("all-shared")
.arg("--resource-suffix=-xxx")
.args(&["--extend-css", "z.css"])
.input("x.rs")
.run();
assert_eq!(
shallow_find_files("all-shared/static.files", |path| {
has_prefix(path, "storage-") && has_extension(path, "js")
})
.len(),
1
);
assert_eq!(
shallow_find_files("all-shared/static.files", |path| {
has_prefix(path, "SourceSerif4-It-") && has_extension(path, "woff2")
})
.len(),
1
);
assert!(!Path::new("all-shared/search-index-xxx.js").exists());
assert!(!Path::new("all-shared/settings.html").exists());
assert!(!Path::new("all-shared/x").exists());
assert!(!Path::new("all-shared/src").exists());
assert!(!Path::new("all-shared/theme.css").exists());
assert_eq!(
shallow_find_files("all-shared/static.files", |path| {
has_prefix(path, "main-") && has_extension(path, "js")
})
.len(),
1
);
assert!(!Path::new("all-shared/y-xxx.css").exists());
}
Loading