Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update run-make/used to use any_symbol_contains #128603

Merged
merged 2 commits into from
Aug 3, 2024
Merged

Update run-make/used to use any_symbol_contains #128603

merged 2 commits into from
Aug 3, 2024

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Aug 3, 2024 •
edited
Loading

This makes it so we don't need nm or llvm-nm.

I also tested that BAR is removed. I'm not sure if this is wanted though.

@rustbot
Copy link
Collaborator

rustbot commented Aug 3, 2024

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 rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 3, 2024

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

@ChrisDenton
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
@bors
Auto merge of rust-lang#128603 - ChrisDenton:used, r=<try>
eb844db 
Update run-make/used to use `any_symbol_contains`

This makes it so we don't need `nm` or `llvm-nm`.

I also tested that `BAR` is removed. I'm not sure if this is wanted though.

try-job: x86_64-msvc
try-job: i686-msvc
@bors
Copy link
Contributor

bors commented Aug 3, 2024

⌛ Trying commit b564b70 with merge eb844db...

@ChrisDenton ChrisDenton mentioned this pull request Aug 3, 2024
Open
28 tasks
@jieyouxu
Copy link
Member

jieyouxu commented Aug 3, 2024

I'm inclined to say we probably don't care if BAR is removed here, since AFAICT we only care about FOO being present in the final binary (that it survives linking).

@ChrisDenton
Copy link
Member Author

Fair. I do wonder why we have BAR in the lib then.

@jieyouxu
Copy link
Member

jieyouxu commented Aug 3, 2024

Fair. I do wonder why we have BAR in the lib then.

It's possible that it's due to any_symbol_contains underlying object-based implementation, but the original Makefile also just used plain nm no flags so...

@ChrisDenton
Copy link
Member Author

I mean we could just delete this line in used.rs and it won't make any difference to the test:

rust/tests/run-make/used/used.rs

Line 6 in a604303

static BAR: u32 = 0;

@jieyouxu
Copy link
Member

jieyouxu commented Aug 3, 2024

Yeah, that seems reasonable to me 👍

jieyouxu
jieyouxu approved these changes Aug 3, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, r=me if try jobs come back green 👍

@ChrisDenton
Copy link
Member Author

@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Aug 3, 2024

📌 Commit eb45146 has been approved by jieyouxu

It is now in the queue for this repository.

@bors 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 Aug 3, 2024
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 3, 2024
@tgross35
Rollup merge of rust-lang#128603 - ChrisDenton:used, r=jieyouxu
74cc27f 
Update run-make/used to use `any_symbol_contains`

This makes it so we don't need `nm` or `llvm-nm`.

I also tested that `BAR` is removed. I'm not sure if this is wanted though.
@tgross35 tgross35 mentioned this pull request Aug 3, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
@bors
Auto merge of rust-lang#128608 - tgross35:rollup-kibt0o5, r=tgross35
cacd19d 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#127095 (Migrate `reproducible-build-2` and `stable-symbol-names` `run-make` tests to rmake)
 - rust-lang#127921 (Stabilize unsafe extern blocks (RFC 3484))
 - rust-lang#128466 (Update the stdarch submodule)
 - rust-lang#128530 (Implement `UncheckedIterator` directly for `RepeatN`)
 - rust-lang#128581 (Assert that all attributes are actually checked via `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes)
 - rust-lang#128603 (Update run-make/used to use `any_symbol_contains`)

Failed merges:

 - rust-lang#128410 (Migrate `remap-path-prefix-dwarf` `run-make` test to rmake)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 3, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
@bors
Auto merge of rust-lang#128614 - matthiaskrgr:rollup-d2fextz, r=matth…
64ebd39 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#127921 (Stabilize unsafe extern blocks (RFC 3484))
 - rust-lang#128283 (bootstrap: fix bug preventing the use of custom targets)
 - rust-lang#128530 (Implement `UncheckedIterator` directly for `RepeatN`)
 - rust-lang#128551 (chore: refactor backtrace style in panic)
 - rust-lang#128573 (Simplify `body` usage in rustdoc)
 - rust-lang#128581 (Assert that all attributes are actually checked via `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes)
 - rust-lang#128603 (Update run-make/used to use `any_symbol_contains`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0655ed2 into rust-lang:master Aug 3, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
@rust-timer
Unrolled build for rust-lang#128603
5a5615f 
Rollup merge of rust-lang#128603 - ChrisDenton:used, r=jieyouxu

Update run-make/used to use `any_symbol_contains`

This makes it so we don't need `nm` or `llvm-nm`.

I also tested that `BAR` is removed. I'm not sure if this is wanted though.
@rustbot rustbot added this to the 1.82.0 milestone Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
Oxidizing run-make tests
Status: Done
Milestone
1.82.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ChrisDenton @rustbot @bors @jieyouxu