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.

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

adapt native-link-modifier-bundle test no not emit LLVM bitcode #98424

Closed
wants to merge 2 commits into from

Conversation

krasimirgg
Copy link
Contributor

No functional changes intended.
This updates the test case to not embed LLVM bitcode.
This fixes a test failure over at the experimental build of rustc with HEAD LLVM:
https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/11144#01814d0f-a46a-4c19-91cf-41e720edb6f9/684-691.

The issue is that this test uses the system nm, which may not be recent
enough to understand the bitcode produced by rustc when compiled against HEAD LLVM.

Similar to what we did for another test in #94023. Note that we did another thing there, replaced nm with llvm-nm. I decided to not replace nm with llvm-nm here as that was a thing specifically changed in the original PR introducing this test: #95818 (comment).

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2022
@krasimirgg krasimirgg marked this pull request as ready for review June 23, 2022 15:14
@Mark-Simulacrum
Copy link
Member

specifically changed in the original PR introducing this test

I don't think this change was actually necessary; we should probably use llvm-nm here.

cc @petrochenkov in case you recall why you made that change, but I suspect it wasn't actually a contributor to fixing the test in that PR

@Mark-Simulacrum 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 Jun 26, 2022
@petrochenkov
Copy link
Contributor

we should probably use llvm-nm here

I don't have a strong opinion here.
On one hand this is the only test that used llvm-nm instead of platform-provided nm.
On another hand maybe we should instead migrate all nm tests to llvm-nm if we are building it anyway.

If it passes through CI then it's fine, I guess.

cc @petrochenkov in case you recall why you made that change

I've seen llvm-nm producing an IO error when writing to a pipe where regular nm didn't produce it, but it may be entirely unrelated to CI failures in #95818.

@krasimirgg
Copy link
Contributor Author

I'll create an alternative of this PR that uses llvm-nm and let it go through CI.

@krasimirgg krasimirgg closed this Jun 28, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 4, 2022
adapt native-link-modifier-bundle test to use llvm-nm

No functional changes intended.

This updates the test case to use `llvm-nm` as an alternative to rust-lang#98424.

This fixes a test failure over at the experimental build of rustc with HEAD LLVM:
https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/11144#01814d0f-a46a-4c19-91cf-41e720edb6f9/684-691.

The issue is that this test uses the system nm, which may not be recent
enough to understand the bitcode produced by rustc when compiled against HEAD LLVM.

Similar to what we did for another test in rust-lang#94023.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants