Migrate `symbol-visibility` `run-make` test to rmake #127060

Oneirical · 2024-06-27T20:39:48Z

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

Pretty scary!

The expected number of symbols on each check has been changed slightly to reflect the differences between llvm_readobj and nm, as I think the former will print hidden symbols once and visible symbols twice, while the latter will only print visible symbols.
The original test ran the same exact checks on cdylib twice, for seemingly no reason. I have removed it.
This may be possible to optimize some more? llvm_readobj could get called only once for each library type, and the regex could avoid being created repeatedly. I am not sure if these kinds of considerations are important for a run-make test.

Demands a Windows try-job.

try-job: x86_64-mingw

rustbot · 2024-06-27T20:39:56Z

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-06-27T20:39:58Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Kobzol · 2024-06-27T21:22:41Z

@bors try

Migrate `symbol-visibility` `run-make` test to rmake 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). Pretty scary! - The expected number of symbols on each check has been changed slightly to reflect the differences between `llvm_readobj` and `nm`, as I think the former will print hidden symbols once and visible symbols twice, while the latter will only print visible symbols. - The original test ran the same exact checks on `cdylib` twice, for seemingly no reason. I have removed it. - This may be possible to optimize some more? `llvm_readobj` could get called only once for each library type, and the regex could avoid being created repeatedly. I am not sure if these kinds of considerations are important for a `run-make` test. Demands a Windows try-job. try-job: x86_64-msvc

bors · 2024-06-27T21:23:51Z

⌛ Trying commit e825910 with merge 7cd66cf...

bors · 2024-06-27T23:03:43Z

💔 Test failed - checks-actions

Oneirical · 2024-06-28T13:54:05Z

What was even the point of those .a extensions in the original test... I'd like a second try job: @rustbot review

Kobzol · 2024-06-28T13:55:18Z

@bors try

bors · 2024-06-28T13:56:28Z

⌛ Trying commit d2ec57d with merge 49774b5...

Migrate `symbol-visibility` `run-make` test to rmake 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). Pretty scary! - The expected number of symbols on each check has been changed slightly to reflect the differences between `llvm_readobj` and `nm`, as I think the former will print hidden symbols once and visible symbols twice, while the latter will only print visible symbols. - The original test ran the same exact checks on `cdylib` twice, for seemingly no reason. I have removed it. - This may be possible to optimize some more? `llvm_readobj` could get called only once for each library type, and the regex could avoid being created repeatedly. I am not sure if these kinds of considerations are important for a `run-make` test. Demands a Windows try-job. try-job: x86_64-msvc

bors · 2024-06-28T15:31:09Z

💔 Test failed - checks-actions

Oneirical · 2024-06-28T15:40:46Z

What was even the point of those .a extensions in the original test...

Okay, I get it, now. This test is beyond broken on MSVC, so the ignore should be restored, but it might work on Windows-gnu.

Please run try on mingw next, I already changed the description.

lqd · 2024-06-28T16:01:24Z

@bors try

bors · 2024-06-28T16:02:34Z

⌛ Trying commit b75711a with merge 90c22c3...

Oneirical · 2024-07-30T17:17:11Z

@bors try

I am using llvm_nm now. I do notice the similarity with the test being ported in #128314, so when @lolbinarycat's custom pure Rust symbol reader is stable, it could be an interesting alternative for this series of "find this symbol in the object file" type of tests.

Migrate `symbol-visibility` `run-make` test to rmake 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). Pretty scary! - The expected number of symbols on each check has been changed slightly to reflect the differences between `llvm_readobj` and `nm`, as I think the former will print hidden symbols once and visible symbols twice, while the latter will only print visible symbols. - The original test ran the same exact checks on `cdylib` twice, for seemingly no reason. I have removed it. - This may be possible to optimize some more? `llvm_readobj` could get called only once for each library type, and the regex could avoid being created repeatedly. I am not sure if these kinds of considerations are important for a `run-make` test. Demands a Windows try-job. try-job: x86_64-mingw

bors · 2024-07-30T17:18:23Z

⌛ Trying commit 83b80a6 with merge 8cc0310...

bors · 2024-07-30T19:06:29Z

☀️ Try build successful - checks-actions
Build commit: 8cc0310 (8cc0310a769f2764a20a3ed5dbfa0e569d9155bf)

Oneirical · 2024-07-30T19:18:39Z

@rustbot review

jieyouxu

Thanks, this looks good to me, just a tiny nit about the #[no_mangle] attribute in comments becoming //[no_mangle]. r=me after that!

jieyouxu · 2024-07-30T22:27:37Z

tests/run-make/symbol-visibility/rmake.rs

+
+    // Check the combined case, where we generate a cdylib and an rlib in the same
+    // compilation session:
+    // Check that a cdylib exports its public //[no_mangle] functions


Nit: #[no_mangle], looks like search and replace mistake. Same with other instances of //[no_mangle]

jieyouxu · 2024-07-30T22:29:09Z

r=me with the //[no_mangle] in comments fixed
@bors delegate+

bors · 2024-07-30T22:29:12Z

✌️ @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-07-30T22:29:31Z

@bors rollup=iffy (highly platform specific symbol checking logic)

Oneirical · 2024-07-31T14:39:26Z

@bors r=@jieyouxu

bors · 2024-07-31T14:39:29Z

📌 Commit ea04b0a has been approved by jieyouxu

It is now in the queue for this repository.

Migrate `symbol-visibility` `run-make` test to rmake 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). Pretty scary! - The expected number of symbols on each check has been changed slightly to reflect the differences between `llvm_readobj` and `nm`, as I think the former will print hidden symbols once and visible symbols twice, while the latter will only print visible symbols. - The original test ran the same exact checks on `cdylib` twice, for seemingly no reason. I have removed it. - This may be possible to optimize some more? `llvm_readobj` could get called only once for each library type, and the regex could avoid being created repeatedly. I am not sure if these kinds of considerations are important for a `run-make` test. Demands a Windows try-job. try-job: x86_64-mingw

Rollup of 7 pull requests Successful merges: - rust-lang#117468 (Stabilize Wasm relaxed SIMD) - rust-lang#123813 (Add `REDUNDANT_IMPORTS` lint for new redundant import detection) - rust-lang#127060 (Migrate `symbol-visibility` `run-make` test to rmake) - rust-lang#127159 (match lowering: Hide `Candidate` from outside the lowering algorithm) - rust-lang#128296 (Update target-spec metadata for loongarch64 targets) - rust-lang#128416 (android: Remove libstd hacks for unsupported Android APIs) - rust-lang#128431 (Add myself as VxWorks target maintainer for reference) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-08-01T05:42:12Z

⌛ Testing commit ea04b0a with merge 70591dc...

bors · 2024-08-01T08:08:23Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 70591dc to master...

rust-timer · 2024-08-02T02:21:41Z

Finished benchmarking commit (70591dc): 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 5.7%)

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)	5.7%	[2.0%, 9.3%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.7%	[2.0%, 9.3%]	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: 759.166s -> 757.358s (-0.24%)
Artifact size: 336.88 MiB -> 336.90 MiB (0.01%)

rustbot assigned jieyouxu Jun 27, 2024

Kobzol mentioned this pull request Jun 27, 2024

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

Open

This comment has been minimized.

Sign in to view

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2024

Oneirical force-pushed the testificate branch from e825910 to d2ec57d Compare June 28, 2024 13:52

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 Jun 28, 2024

This comment has been minimized.

Sign in to view

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2024

Oneirical force-pushed the testificate branch from d2ec57d to b75711a Compare June 28, 2024 15:42

Oneirical added 2 commits July 30, 2024 13:07

rewrite symbol-visibility to rmake

689bb11

invalid stdout_utf8 handling in run_make_support

dcaa17a

Oneirical force-pushed the testificate branch from 5558cd1 to 83b80a6 Compare July 30, 2024 17:15

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 Jul 30, 2024

jieyouxu approved these changes Jul 30, 2024

View reviewed changes

use llvm-nm in symbol-visibility rmake test

ea04b0a

Oneirical force-pushed the testificate branch from 83b80a6 to ea04b0a Compare July 31, 2024 14:39

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 Jul 31, 2024

tgross35 mentioned this pull request Jul 31, 2024

Rollup of 7 pull requests #128454

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 1, 2024

bors merged commit 70591dc into rust-lang:master Aug 1, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 1, 2024

bors mentioned this pull request Aug 1, 2024

[EXPERIMENTAL] symbol-visibility: x86_64-mingw, what did you see?! #128393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `symbol-visibility` `run-make` test to rmake #127060

Migrate `symbol-visibility` `run-make` test to rmake #127060

Oneirical commented Jun 27, 2024 •

edited

Loading

rustbot commented Jun 27, 2024

rustbot commented Jun 27, 2024

Kobzol commented Jun 27, 2024

bors commented Jun 27, 2024

This comment has been minimized.

bors commented Jun 27, 2024

Oneirical commented Jun 28, 2024

Kobzol commented Jun 28, 2024

bors commented Jun 28, 2024

This comment has been minimized.

bors commented Jun 28, 2024

Oneirical commented Jun 28, 2024 •

edited

Loading

lqd commented Jun 28, 2024

bors commented Jun 28, 2024

Oneirical commented Jul 30, 2024 •

edited

Loading

bors commented Jul 30, 2024

bors commented Jul 30, 2024

Oneirical commented Jul 30, 2024

jieyouxu left a comment

jieyouxu Jul 30, 2024

jieyouxu commented Jul 30, 2024

bors commented Jul 30, 2024

jieyouxu commented Jul 30, 2024

Oneirical commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Aug 1, 2024

bors commented Aug 1, 2024

rust-timer commented Aug 2, 2024

Migrate symbol-visibility run-make test to rmake #127060

Migrate symbol-visibility run-make test to rmake #127060

Conversation

Oneirical commented Jun 27, 2024 • edited Loading

rustbot commented Jun 27, 2024

rustbot commented Jun 27, 2024

Kobzol commented Jun 27, 2024

bors commented Jun 27, 2024

This comment has been minimized.

bors commented Jun 27, 2024

Oneirical commented Jun 28, 2024

Kobzol commented Jun 28, 2024

bors commented Jun 28, 2024

This comment has been minimized.

bors commented Jun 28, 2024

Oneirical commented Jun 28, 2024 • edited Loading

lqd commented Jun 28, 2024

bors commented Jun 28, 2024

Oneirical commented Jul 30, 2024 • edited Loading

bors commented Jul 30, 2024

bors commented Jul 30, 2024

Oneirical commented Jul 30, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Jul 30, 2024

Choose a reason for hiding this comment

jieyouxu commented Jul 30, 2024

bors commented Jul 30, 2024

jieyouxu commented Jul 30, 2024

Oneirical commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Aug 1, 2024

bors commented Aug 1, 2024

rust-timer commented Aug 2, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Migrate `symbol-visibility` `run-make` test to rmake #127060

Migrate `symbol-visibility` `run-make` test to rmake #127060

Oneirical commented Jun 27, 2024 •

edited

Loading

Oneirical commented Jun 28, 2024 •

edited

Loading

Oneirical commented Jul 30, 2024 •

edited

Loading