Rewrite `emit`, `mixing-formats` and `bare-outfile` `run-make` tests in `rmake.rs` format #125383

Oneirical · 2024-05-21T19:54:40Z

Part of #121876 and the associated Google Summer of Code project.

try-job: x86_64-msvc

rustbot · 2024-05-21T19:54:48Z

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-05-21T19:54:50Z

Some changes occurred in run-make tests.

cc @jieyouxu

jieyouxu · 2024-05-21T21:41:34Z

tests/run-make/bare-outfile/rmake.rs

+    env::set_current_dir(tmp_dir());
+    rustc().output("foo").input("foo.rs");


Suggestion: I think this is cleaner if it's written with -o foo + --out-dir tmp_dir()

The output filename can be set with the -o flag. A suffix may be added to the filename with the -C extra-filename flag. The files are written to the current directory unless the --out-dir flag is used.

That way, we don't need the explicit copy of foo.rs at all.

i.e. rustc().out_dir("foo").output("foo"), depending on what you think about the name of output -- should it be renamed output_filename?

I just realized that rustc() will always pass through the function I copied below (I think you wrote it). So, logically, we wouldn't need to specify the --out-dir tmp_dir(), as it's already set?

I'm not too sure I understand your suggestion. In rustc().out_dir("foo").output("foo"), .out_dir("foo") is setting the file name foo as the output directory, but it's a filename, not a directory. I could certainly rename output to output_filename, but that would require replacing every instance of it in already ported tests.

fn setup_common() -> Command { let rustc = env::var("RUSTC").unwrap(); let mut cmd = Command::new(rustc); set_host_rpath(&mut cmd); cmd.arg("--out-dir").arg(tmp_dir()).arg("-L").arg(tmp_dir()); cmd }

My bad, I forgot by default the Rustc instance returned by rustc() we already set --out-dir to tmp_dir(), so env::set_current_dir and fs::copy etc. wasn't needed in the first place I think?

And yeah, ignore the output_filename comment, I was just thinking if output() and out_dir() can become confusing.

My bad, I forgot by default the Rustc instance returned by rustc() we already set --out-dir to tmp_dir(), so env::set_current_dir and fs::copy etc. wasn't needed in the first place I think?

I will remove those 2 commands and see if that causes CI to fail.

jieyouxu · 2024-05-21T21:43:48Z

tests/run-make/emit/rmake.rs

+// in the same rustc call. A fix was created in #30452. This test checks that
+// the fix did not accidentally break compilation.


Nit: probably more straightforward to say "the test checks that rustc still compiles a source file successfully when emission of multiple output artifacts are requested"? What do you think?

That is true, and it's something that came up a few times, too. A lot of these tests come from "person fixes non-critical but annoying bug in rustc, maintainer asks for a test to make sure the fix did not break anything". It's important to be specific.

jieyouxu · 2024-05-21T21:47:10Z

tests/run-make/mixing-formats/rmake.rs

+    rustc().crate_type("rlib").input("foo.rs").run();
+    rustc().crate_type("dylib").input("bar1.rs").arg("-Cprefer-dynamic").run();
+    rustc().crate_type("dylib,rlib").input("baz.rs").arg("-Cprefer-dynamic").run();
+    rustc().crate_type("bin").input("baz.rs").run();
+    fs::remove_dir_all(tmp_dir()).unwrap();
+    fs::create_dir(tmp_dir()).unwrap();


Suggestion: it might be cleaner to create a helper to abstract the "clean tmp_dir() functionality, i.e.

// helper fn test_with_teardown(f: Fn()) { f(); fs::remove_dir_all(tmp_dir()).unwrap(); fs::create_dir(tmp_dir()).unwrap(); } // callsite fn main() { test_with_teardown(|| { // Building just baz rustc().crate_type("rlib").input("foo.rs").run(); rustc().crate_type("dylib").input("bar1.rs").arg("-Cprefer-dynamic").run(); rustc().crate_type("dylib,rlib").input("baz.rs").arg("-Cprefer-dynamic").run(); rustc().crate_type("bin").input("baz.rs").run(); }); }

You're right, it's a lot cleaner! I wanted to abstract over this test a lot initially, making a Flags struct with fields crate_type, prefers_dynamic and should_fail, but I think writing everything out is clearer... with the slight improvement of your helper teardown function. Added.

jieyouxu · 2024-05-21T22:27:04Z

@rustbot author

Oneirical · 2024-05-22T17:34:57Z

@rustbot review

Oneirical · 2024-05-22T22:06:54Z

so env::set_current_dir and fs::copy etc. wasn't needed in the first place I think?

error: couldn't create a temp dir: Read-only file system (os error 30) at path "/checkout/tests/run-make/bare-outfile/rmetaSu8yRp"

The error message seems to indicate that they were needed. I am restoring them now.

jieyouxu · 2024-05-22T22:13:37Z

tests/run-make/mixing-formats/rmake.rs

+use run_make_support::{rustc, tmp_dir};
+use std::fs;
+
+fn test_with_teardown(rustc_calls: &dyn Fn()) {


Nit: can't this just be f: Fn(), it doesn't need to be &dyn Fn() AFAICT?

I tried it that way initially, but this triggers the following error message:

2024-05-22T18:08:36.1683386Z --- stderr ------------------------------- 2024-05-22T18:08:36.1684109Z error[E0277]: the size for values of type `(dyn Fn() + 'static)` cannot be known at compilation time 2024-05-22T18:08:36.1703969Z ##[error] --> /checkout/tests/run-make/mixing-formats/rmake.rs:18:23 2024-05-22T18:08:36.1711694Z | 2024-05-22T18:08:36.1712020Z 18 | fn test_with_teardown(rustc_calls: Fn()) { 2024-05-22T18:08:36.1712621Z | ^^^^^^^^^^^ doesn't have a size known at compile-time 2024-05-22T18:08:36.1713013Z | 2024-05-22T18:08:36.1713456Z = help: the trait `Sized` is not implemented for `(dyn Fn() + 'static)` 2024-05-22T18:08:36.1714031Z = help: unsized fn params are gated as an unstable feature 2024-05-22T18:08:36.1714497Z help: you can use `impl Trait` as the argument type 2024-05-22T18:08:36.1714858Z | 2024-05-22T18:08:36.1715140Z 18 | fn test_with_teardown(rustc_calls: impl Fn()) { 2024-05-22T18:08:36.1715523Z | ++++ 2024-05-22T18:08:36.1716122Z help: function arguments must have a statically known size, borrowed types always have a known size 2024-05-22T18:08:36.1716913Z | 2024-05-22T18:08:36.1717197Z 18 | fn test_with_teardown(rustc_calls: &dyn Fn()) { 2024-05-22T18:08:36.1717580Z | ++++ 2024-05-22T18:08:36.1717817Z

Full log

I'm dumb and forgor the impl lol (sorry for misleading)

fn foo(f: impl Fn()) { f() } fn main() { foo(|| { println!("hello world") }) }

jieyouxu · 2024-05-23T00:16:54Z

Thanks, r=me after CI is green.
@bors delegate+

bors · 2024-05-23T00:16:58Z

✌️ @Oneirical, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

jieyouxu · 2024-05-23T00:18:05Z

@bors rollup

jieyouxu · 2024-06-03T11:03:05Z

@bors try

bors · 2024-06-03T11:04:17Z

⌛ Trying commit 6e120cf with merge b0a563a...

Rewrite `emit`, `mixing-formats` and `bare-outfile` `run-make` tests in `rmake.rs` format Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). try-job: x86_64-msvc

bors · 2024-06-03T12:40:47Z

☀️ Try build successful - checks-actions
Build commit: b0a563a (b0a563ae952ac6fd4c4f9ea5dd00aa42e0880562)

jieyouxu · 2024-06-03T12:56:41Z

Thanks! @bors r+ rollup=iffy

bors · 2024-06-03T12:56:44Z

📌 Commit 6e120cf has been approved by jieyouxu

It is now in the queue for this repository.

bors · 2024-06-03T14:14:17Z

⌛ Testing commit 6e120cf with merge def4665...

Rewrite `emit`, `mixing-formats` and `bare-outfile` `run-make` tests in `rmake.rs` format Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). try-job: x86_64-msvc

rust-log-analyzer · 2024-06-03T15:41:14Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2024-06-03T15:42:00Z

💔 Test failed - checks-actions

Oneirical · 2024-06-03T16:00:55Z

It's blank?

@bors r=@jieyouxu rollup=iffy

bors · 2024-06-03T16:00:57Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: Opt-in to FulfillmentError generation to avoid doing extra work in the new solver #125864

bors · 2024-06-03T16:00:58Z

📌 Commit 6e120cf has been approved by jieyouxu

It is now in the queue for this repository.

bors · 2024-06-03T18:35:59Z

⌛ Testing commit 6e120cf with merge 7c52d2d...

bors · 2024-06-03T20:50:48Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 7c52d2d to master...

rust-timer · 2024-06-03T22:33:08Z

Finished benchmarking commit (7c52d2d): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Max RSS (memory usage)

Results (primary -3.0%, secondary -3.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-3.9%, -2.0%]	2
Improvements ✅ (secondary)	-3.6%	[-5.5%, -2.2%]	3
All ❌✅ (primary)	-3.0%	[-3.9%, -2.0%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.681s -> 671.059s (-0.24%)
Artifact size: 318.96 MiB -> 319.05 MiB (0.03%)

rustbot assigned jieyouxu May 21, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2024

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label May 21, 2024

This comment has been minimized.

Sign in to view

jieyouxu reviewed May 21, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2024

This comment has been minimized.

Sign in to view

Oneirical force-pushed the bundle-them-up branch from 70ccce5 to cd347a8 Compare May 22, 2024 19:30

This comment has been minimized.

Sign in to view

Oneirical force-pushed the bundle-them-up branch from cd347a8 to 5ff7803 Compare May 22, 2024 20:19

This comment has been minimized.

Sign in to view

Oneirical force-pushed the bundle-them-up branch from ceb113a to 5ff7803 Compare May 22, 2024 22:07

jieyouxu reviewed May 22, 2024

View reviewed changes

Oneirical force-pushed the bundle-them-up branch from 5ff7803 to 4e37db5 Compare May 22, 2024 23:55

jieyouxu approved these changes May 23, 2024

View reviewed changes

Oneirical force-pushed the bundle-them-up branch from e6f8a83 to 6e120cf Compare June 3, 2024 01:59

jieyouxu approved these changes Jun 3, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2024

bors merged commit 7c52d2d into rust-lang:master Jun 3, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone Jun 3, 2024

Oneirical deleted the bundle-them-up branch June 3, 2024 20:54

bors mentioned this pull request Jun 3, 2024

Create run-make env_var and env_var_os helpers #125917

Merged

jieyouxu mentioned this pull request Jun 9, 2024

Tracking Issue for porting run-make tests to use Rust #121876

Open

		env::set_current_dir(tmp_dir());
		rustc().output("foo").input("foo.rs");

		// in the same rustc call. A fix was created in #30452. This test checks that
		// the fix did not accidentally break compilation.

Rewrite emit, mixing-formats and bare-outfile run-make tests in rmake.rs format #125383

Rewrite emit, mixing-formats and bare-outfile run-make tests in rmake.rs format #125383

Conversation

Oneirical commented May 21, 2024 • edited Loading

rustbot commented May 21, 2024

rustbot commented May 21, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Oneirical May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented May 21, 2024

Oneirical commented May 22, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Oneirical commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented May 23, 2024

bors commented May 23, 2024

jieyouxu commented May 23, 2024

jieyouxu commented Jun 3, 2024

bors commented Jun 3, 2024

bors commented Jun 3, 2024

jieyouxu commented Jun 3, 2024

bors commented Jun 3, 2024

bors commented Jun 3, 2024

rust-log-analyzer commented Jun 3, 2024

bors commented Jun 3, 2024

Oneirical commented Jun 3, 2024

bors commented Jun 3, 2024

bors commented Jun 3, 2024

bors commented Jun 3, 2024

bors commented Jun 3, 2024

rust-timer commented Jun 3, 2024

Overall result: ❌ regressions - no action needed

Rewrite `emit`, `mixing-formats` and `bare-outfile` `run-make` tests in `rmake.rs` format #125383

Rewrite `emit`, `mixing-formats` and `bare-outfile` `run-make` tests in `rmake.rs` format #125383

Oneirical commented May 21, 2024 •

edited

Loading

Oneirical May 22, 2024 •

edited

Loading