run-make-support: add wrapper for `fs` operations by Oneirical · Pull Request #125736 · rust-lang/rust

Oneirical · 2024-05-29T19:36:33Z

Suggested by #125728.

The point of this wrapper is to stop silent fails caused by forgetting to unwrap fs functions. However, functions like fs::read which return something and get stored in a variable should cause a failure on their own if they are not unwrapped (as the Result will be stored in the variable, and something will be done on that Result that should have been done to its contents). Is it still pertinent to wrap fs::read_to_string, fs::metadata and so on?

Closes: #125728

try-job: x86_64-msvc
try-job: i686-mingw

rustbot · 2024-05-29T19:36:41Z

r? @jieyouxu

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-29T19:36:42Z

The run-make-support library was changed

cc @jieyouxu

Some changes occurred in run-make tests.

cc @jieyouxu

bors · 2024-05-30T03:57:58Z

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

jieyouxu

Good changes, I think tests are less busy to read without all the .unwrap()s.

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

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

Oneirical · 2024-06-09T01:45:35Z

@Kobzol, in my defense, all I did was follow orders.

I'm joking!! It's my bad, I copypasted the block of commands and didn't check the git diff.

At this step: git fetch origin master I should have synced with upstream first.

Sorry, once again, to anyone wrongfully pinged.

EDIT: Alright, the spillage looks contained. Just need to cleave off the irrelevant tags now!

Kobzol · 2024-06-09T07:45:03Z

LGTM! Although the first two commits of this PR should be removed. If you're still brave enough to trust my git command suggestions, you can do it e.g. like this:

$ git rebase -i HEAD~4
(delete the first two lines starting with "pick")
(check that the git history is ok and contains just two commits that against the base branch)
$ git push --force-with-lease

Oneirical · 2024-06-09T17:35:03Z

LGTM! Although the first two commits of this PR should be removed. If you're still brave enough to trust my git command suggestions, you can do it e.g. like this:
$ git rebase -i HEAD~4
(delete the first two lines starting with "pick")
(check that the git history is ok and contains just two commits that against the base branch)
$ git push --force-with-lease

Hehe, thankfully I've had to do a bunch of rebases like this to keep the commit count low in my other PRs.

@rustbot review

jieyouxu · 2024-06-09T17:42:00Z

Oh I was a bit confused for a moment, because the commit messages look as if they had nothing to do with the PR title lol. Ah, the first commit also has conflict markers... maybe split the changes into one commit for the library and another for updating tests, or squash them together?

jieyouxu

Thank you for working on this, the tests do look a lot cleaner! I left some review comments, but looks good to me overall.

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

jieyouxu · 2024-06-09T17:51:13Z

tests/run-make/reset-codegen-1/rmake.rs

    for (output_file, emit) in flags {
-        fs::remove_file(output_file).unwrap_or_default();
-        compile(output_file, emit);
-        fs::remove_file(output_file);
+        compile(&bin_name(output_file), emit);
    }


Concern: I would like to see the remove_files remain because they also check the output filenames with no --emit, single-artifact --emit and multi-artifact --emits. See 8d182e3 (#126197).

This was my change, sorry. The previous code was highly non-obvious (I suspect that the removes were there because previously the code wasn't run in a temporary directory). I'd suggest to do this:

Explicitly add the expected paths as a third tuple item in the flags array

Add something like assert!(is_file(expected_filename)) for each of the expected filenames. That makes it much more obvious than removing the file.

Thanks, yeah, that's a much better way to doing this.

@Oneirical could we instead check for the expected files (have -o's filename include the expected extensions apart from the multioutput case, and check that an output artifact is in fact produced for the respective --emit?

jieyouxu · 2024-06-09T17:54:07Z

@rustbot author

Oneirical · 2024-06-09T18:55:58Z

Oh I was a bit confused for a moment, because the commit messages look as if they had nothing to do with the PR title lol. Ah, the first commit also has conflict markers... maybe split the changes into one commit for the library and another for updating tests, or squash them together?

Right, a side-effect of the little commit spillage. I put everything in one commit with a reasonable name and changed the PR title too.

All requested changes applied.

@rustbot review

jieyouxu · 2024-06-09T19:05:18Z

Ah, just a suggestion for checking if we have expected output file instead of trying to remove files as a proxy (#125736 (comment)), then r=me.

jieyouxu · 2024-06-10T23:00:35Z

@rustbot blocked (waiting on #125752 to merge)

Kobzol · 2024-06-11T13:40:10Z

#125752 was merged, so this needs a rebase.

jieyouxu · 2024-06-11T13:42:54Z

@rustbot author (no longer blocked)

jieyouxu

Just one small problem with an intra-doc link, r=me after fixing that because it might fail in CI when ./x doc src/tools/run-make-support and after PR CI is green.

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

jieyouxu · 2024-06-11T14:08:57Z

The current rollup contains some rmake.rs that uses std::fs, but we should land this PR ASAP to prevent more bitrotting or conflicts (I will fixup the rolled up tests after this PR).

jieyouxu · 2024-06-11T14:54:47Z

@bors r+ rollup=never p=1 (some other run-make PRs are blocked on this)

bors · 2024-06-11T14:54:50Z

📌 Commit c84afee has been approved by jieyouxu

It is now in the queue for this repository.

bors · 2024-06-11T15:50:29Z

⌛ Testing commit c84afee with merge 3ea5e23...

bors · 2024-06-11T18:15:11Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 3ea5e23 to master...

rust-timer · 2024-06-11T19:33:00Z

Finished benchmarking commit (3ea5e23): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 3.3%)

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)	3.3%	[3.3%, 3.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.3%	[3.3%, 3.3%]	1

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: 679.328s -> 676.994s (-0.34%)
Artifact size: 320.10 MiB -> 320.03 MiB (-0.02%)

rustbot assigned jieyouxu May 29, 2024

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