Port exit-code run-make test to use rust · Pull Request #121884 · rust-lang/rust

ghost · 2024-03-02T03:26:19Z

As part of #121876

~~As draft because formatting will fail because x fmt isn't working for me for some reason, I'll debug that later, just opening this now for review, will mark as ready when formatting is fixed~~ (misleading message from x fmt)

cc @jieyouxu

rustbot · 2024-03-02T03:26:26Z

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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

jieyouxu · 2024-03-02T10:58:33Z

src/tools/run-make-support/src/rustdoc.rs

+    fn new() -> Self {
+        let cmd = setup_common_build_cmd();
+        Self { cmd }
+    }


Maybe missing a -L $TARGET_RPATH_ENV?

see

rust/tests/run-make/tools.mk

Line 12 in 4cdd205

RUSTDOC := $(BARE_RUSTDOC) -L $(TARGET_RPATH_DIR)

Done. It also seems to set up $(HOST_RPATH_ENV) but I don't see that done in the rustc setup, so I assume it's not needed here?

Done. It also seems to set up $(HOST_RPATH_ENV) but I don't see that done in the rustc setup, so I assume it's not needed here?

I think it might be required if host/target differs, so it's safer if we make sure HOST_RPATH_ENV is available to RUSTDOC here.

HOST_RPATH_ENV = \ $(LD_LIB_PATH_ENVVAR)="$(TMPDIR):$(HOST_RPATH_DIR):$($(LD_LIB_PATH_ENVVAR))"

rustc setup not having that is an oversight. If you add a common HOST_RPATH_ENV fn to set up the env var for rustdoc, can you also add it for rustc?

okay, done, added add_host_rpath_env which i think does what that line of code does?

TARGET_RPATH_ENV isn't implemented but as far as i can see that's not needed yet(?)

src/tools/run-make-support/src/rustdoc.rs

workingjubilee · 2024-03-02T20:14:04Z

?
@bors ping

bors · 2024-03-02T20:14:07Z

😪 I'm awake I'm awake

workingjubilee · 2024-03-02T20:14:34Z

@bors delegate=@jieyouxu

bors · 2024-03-02T20:14:36Z

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

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

ghost · 2024-03-09T14:19:44Z

@rustbot label -A-testsuite -T-infra

(was added when i modified the CI yaml to test test-various and x86_64-msvc in 0e8bd06)

tests/run-make/exit-code/rmake.rs

bors · 2024-03-12T02:03:41Z

☔ The latest upstream changes (presumably #122036) made this pull request unmergeable. Please resolve the merge conflicts.

jieyouxu · 2024-03-13T17:46:39Z

Also while we're at it, could you please add a comment documenting the intent of the test?

Mark-Simulacrum · 2024-03-16T15:02:11Z

src/tools/run-make-support/src/lib.rs

+    let temp = env::var("TMPDIR").unwrap();
+    let host_rpath_dir = env::var("HOST_RPATH_DIR").unwrap();
+
+    cmd.env(ld_lib_path_envvar, format!("{temp}:{host_rpath_dir}:{ld_lib_path_value}"));


Should probably be using std::env::join_paths here.

Yeah, "{temp}:{host_rpath_dir}:{ld_lib_path_value}" won't work on Windows where it uses ; for path separators.

Mark-Simulacrum · 2024-03-16T15:02:36Z

src/tools/run-make-support/src/lib.rs

+    let ld_lib_path_value = env::var(&ld_lib_path_envvar).unwrap();
+
+    let temp = env::var("TMPDIR").unwrap();
+    let host_rpath_dir = env::var("HOST_RPATH_DIR").unwrap();


These should all probably use var_os, in combination with join_paths that shouldn't be too hard.

src/tools/run-make-support/src/lib.rs

Mark-Simulacrum · 2024-03-16T15:04:21Z

src/tools/run-make-support/src/lib.rs

+    let temp = env::var("TMPDIR").unwrap();
+    let host_rpath_dir = env::var("HOST_RPATH_DIR").unwrap();
+
+    cmd.env(ld_lib_path_envvar, format!("{temp}:{host_rpath_dir}:{ld_lib_path_value}"));


AFAIK, including publicly-writable directories (like /tmp) into PATH-like environment variables can be dangerous. I think for test purposes it's probably not too bad, but do we actually need to do that? Can we avoid putting things into /tmp?

(Perhaps TMPDIR here is set by bootstrap in a way that isn't /tmp?)

$TMPDIR isn't set by run-make (it's set in bootstrap, I need to go back to check where it's being set and what it's being set to), so this seems to just preserve whatever tools.mk was doing.

Yeah, $TMPDIR here isn't /tmp, it's a build output directory

rust/src/tools/compiletest/src/runtest.rs

Lines 3730 to 3734 in 9023f90

let tmpdir = cwd.join(self.output_base_name());

if tmpdir.exists() {

self.aggressive_rm_rf(&tmpdir).unwrap();

}

create_dir_all(&tmpdir).unwrap();

rustbot · 2024-03-20T11:54:53Z

The run-make-support library was changed

cc @jieyouxu

Some changes occurred in run-make tests.

cc @jieyouxu

ghost · 2024-03-20T11:57:16Z

... was that meant to ping? I rebased to keep up to date with master.

Anyways, did the changes now (just var_os and path changes, also made the RUSTC read in setup_common_rustc_build_cmd use var_os (but tbh i doubt anyone's going to be running this test suite with non-utf8 paths for RUSTC))

The tempdir stuff doesn't seem like it needs any changes, as far as I can see, it's indeed not /tmp.

jieyouxu · 2024-03-20T14:25:50Z

... was that meant to ping? I rebased to keep up to date with master.

(Yes, I added myself to be mentioned when someone changes run-make/support lib.)

bors · 2024-03-22T18:55:46Z

☔ The latest upstream changes (presumably #122580) made this pull request unmergeable. Please resolve the merge conflicts.

ghost · 2024-03-23T18:18:50Z

@rustbot ready

as far as i know all the issues are fixed

jieyouxu

Two minor nits, but otherwise LGTM. Might have some racing with #122840 and #122460, depends on who gets merged first lol

src/tools/run-make-support/src/rustdoc.rs

jieyouxu · 2024-03-23T19:47:05Z

@bors r=@workingjubilee

bors · 2024-03-23T19:47:06Z

📌 Commit 1b53730 has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2024-03-24T04:31:56Z

☔ The latest upstream changes (presumably #122966) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-03-27T11:35:07Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout rmake-exit-code (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self rmake-exit-code --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Removing tests/run-make/exit-code/Makefile
Auto-merging src/tools/run-make-support/src/lib.rs
CONFLICT (content): Merge conflict in src/tools/run-make-support/src/lib.rs
Automatic merge failed; fix conflicts and then commit the result.

ghost · 2024-03-27T13:33:49Z

rebased - I don't know if you were planning to make significant changes to the test harness (making it more of a pain to merge a "don't set out_dir" for rustdoc, so I went with the less invasive change of just deleting the file as needed in the test.

jieyouxu · 2024-03-27T13:55:24Z

rebased - I don't know if you were planning to make significant changes to the test harness (making it more of a pain to merge a "don't set out_dir" for rustdoc, so I went with the less invasive change of just deleting the file as needed in the test.

#122460 (comment) (the rework PR hasn't merged yet, but in that PR I removed setting --out-dir on Rustdoc, and instead let the other rustdoc test set out-dir in its recipe)

bors · 2024-03-27T19:58:49Z

☔ The latest upstream changes (presumably #122460) made this pull request unmergeable. Please resolve the merge conflicts.

ghost · 2024-04-09T18:12:09Z

@rustbot ready

jieyouxu

One minor nit for Rustdoc's API, then r=me after CI is green

src/tools/run-make-support/src/rustdoc.rs

tests/run-make/exit-code/rmake.rs

jieyouxu · 2024-04-09T20:49:29Z

Thanks for the PR!
@bors r+ rollup

bors · 2024-04-09T20:49:32Z

📌 Commit de79a6c has been approved by jieyouxu

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#121884 (Port exit-code run-make test to use rust) - rust-lang#122200 (Unconditionally show update nightly hint on ICE) - rust-lang#123568 (Clean up tests/ui by removing `does-nothing.rs`) - rust-lang#123609 (Don't use bytepos offsets when computing semicolon span for removal) - rust-lang#123612 (Set target-abi module flag for RISC-V targets) - rust-lang#123633 (Store all args in the unsupported Command implementation) - rust-lang#123668 (async closure coroutine by move body MirPass refactoring) Failed merges: - rust-lang#123701 (Only assert for child/parent projection compatibility AFTER checking that theyre coming from the same place) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#121884 - 5225225:rmake-exit-code, r=jieyouxu Port exit-code run-make test to use rust As part of rust-lang#121876 ~~As draft because formatting will fail because `x fmt` isn't working for me for some reason, I'll debug that later, just opening this now for review, will mark as ready when formatting is fixed~~ (misleading message from x fmt) cc `@jieyouxu`

rustbot assigned Mark-Simulacrum Mar 2, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2024

This comment has been minimized.

Sign in to view

jieyouxu reviewed Mar 2, 2024

View reviewed changes

src/tools/run-make-support/src/rustdoc.rs Outdated Show resolved Hide resolved

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 9, 2024

ghost marked this pull request as ready for review March 9, 2024 14:18

rustbot removed A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 9, 2024

jieyouxu reviewed Mar 9, 2024

View reviewed changes

tests/run-make/exit-code/rmake.rs Outdated Show resolved Hide resolved

Mark-Simulacrum reviewed Mar 16, 2024

View reviewed changes

Mark-Simulacrum 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 Mar 16, 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 Mar 23, 2024

jieyouxu reviewed Mar 23, 2024

View reviewed changes

src/tools/run-make-support/src/rustdoc.rs Outdated Show resolved Hide resolved

src/tools/run-make-support/src/rustdoc.rs Outdated Show resolved Hide resolved

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 Mar 23, 2024

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 24, 2024

move exit-code to rmake

65df93b

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 8, 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 Apr 9, 2024

jieyouxu approved these changes Apr 9, 2024

View reviewed changes

src/tools/run-make-support/src/rustdoc.rs Outdated Show resolved Hide resolved

tests/run-make/exit-code/rmake.rs Outdated Show resolved Hide resolved

run-make: make arg take AsRef<OsStr> instead of str

de79a6c

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 Apr 9, 2024

matthiaskrgr mentioned this pull request Apr 10, 2024

Rollup of 7 pull requests #123708

Merged

bors merged commit a79b243 into rust-lang:master Apr 10, 2024

rustbot added this to the 1.79.0 milestone Apr 10, 2024

ghost deleted the rmake-exit-code branch April 10, 2024 08:45

jieyouxu mentioned this pull request Jun 9, 2024

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

Closed

	let tmpdir = cwd.join(self.output_base_name());
	if tmpdir.exists() {
	self.aggressive_rm_rf(&tmpdir).unwrap();
	}
	create_dir_all(&tmpdir).unwrap();

Uh oh!

Conversation

ghost commented Mar 2, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 2, 2024

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

workingjubilee commented Mar 2, 2024

Uh oh!

bors commented Mar 2, 2024

Uh oh!

workingjubilee commented Mar 2, 2024

Uh oh!

bors commented Mar 2, 2024

Uh oh!

ghost commented Mar 9, 2024

Uh oh!

Uh oh!

bors commented Mar 12, 2024

Uh oh!

jieyouxu commented Mar 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Mar 20, 2024

Uh oh!

ghost commented Mar 20, 2024

Uh oh!

jieyouxu commented Mar 20, 2024

Uh oh!

bors commented Mar 22, 2024

Uh oh!

ghost commented Mar 23, 2024

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jieyouxu commented Mar 23, 2024

Uh oh!

bors commented Mar 23, 2024

Uh oh!

bors commented Mar 24, 2024

Uh oh!

bors commented Mar 27, 2024

Uh oh!

ghost commented Mar 27, 2024

Uh oh!

jieyouxu commented Mar 27, 2024

Uh oh!

bors commented Mar 27, 2024

Uh oh!

ghost commented Apr 9, 2024

Uh oh!

ghost commented Mar 2, 2024 •

edited by ghost

Loading

jieyouxu Mar 9, 2024 •

edited

Loading

jieyouxu Mar 16, 2024 •

edited

Loading