Don't use static crt by default when build proc-macro #69519
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @eddyb cc @petrochenkov |
|
r? @alexcrichton (I don't know much about linking or |
|
Sorry while I'm familiar with the bits here I wouldn't be comfortable signing off and approving this. Everything dealing with crt-static I feel has become very hairy and hard to follow, and I no longer fully understand all the ways it interacts with rustc. Can another reviewer be assigned? |
|
Let's try r? @nagisa |
|
I’m not the most familiar with this code either, but a 5 minute look around the relevant portions of the code-base makes me feel that this is wrong and is just trying to work around an issue elsewhere. In particular this appears to now ignore the |
|
This PR is based on the idea from #40174 (comment) suggested by rust-lang/cargo#7564 (comment) I have updated the code and test it locally. I don’t know the progress of bootstraping rustc on Redox OS , but this PR also affect the Redox target(if they have built a runable rustc on Redox) since relibc have the same behavior like musl. |
|
So the idea is probably in the right direction, but I think twiddling with how the Please also add a test (probably will need to resort to a |
12101111
force-pushed
the
remove-proc-macro-check
branch
from
d207195
to
7ca1b2f
Compare
12101111
changed the title
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
smaeul
reviewed
Mar 8, 2020
| // on musl target | ||
|
|
||
| // build-pass | ||
| // only-musl |
There was a problem hiding this comment.
This test doesn't depend on any musl-specific functionality, so I would expect it to run everywhere.
There was a problem hiding this comment.
It depend on musl target but CI don't have it.
2020-03-03T09:46:02.3188089Z error[E0463]: can't find crate for `std`
2020-03-03T09:46:02.3188535Z |
2020-03-03T09:46:02.3189112Z = note: the `x86_64-unknown-linux-musl` target may not be installed
2020-03-03T09:46:02.3189589Z error: aborting due to previous error
2020-03-03T09:46:02.3189910Z
2020-03-03T09:46:02.3190426Z For more information about this error, try `rustc --explain E0463`.
There was a problem hiding this comment.
Which CI job was it? This is a problem with the CI configuration, not something that needs to be worked around in the test. Specifically, the musl targets built by CI are missing the dynamic version of libstd. To fix this, they need to be built with crt_static=false (the config.toml option, not the rustc flag), like the dist-x86_64-musl job does:
There was a problem hiding this comment.
@smaeul it failed on x86_64-gnu-llvm-7.
There was a problem hiding this comment.
The x86_64-gnu-llvm-7 job only builds for x86_64-unknown-linux-gnu. It doesn't look like it does anything with musl at all. So where did "--target=x86_64-unknown-linux-musl" in the rustc arguments for this test come from?
There was a problem hiding this comment.
Oh, I see, the old version of the test had compile-flags: --target=x86_64-unknown-linux-musl in it. That was the problem. After removing that line, the test should work on all targets.
So since the behavior tested here is not musl-specific, I would suggest 1) removing only-musl and 2) renaming the test to mention crt-static instead of musl.
|
I fairly confident your test would pass regardless of changes in this PR. |
|
It seems a hack like |
|
@bors r+ Thanks for putting up with us :) |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
Sorry for the inconvenience. Could you try this again? @nagisa |
|
@bors r=nagisa |
|
📌 Commit afd374f has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit afd374f with merge 04cc8af4d0b1e758c7f9a34146848ccc557c7063... |
|
@bors retry yielding |
|
@bors p=20 |
|
☀️ Test successful - checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Don't check value of
crt-staticwhen build proc-macro crates, since they are always built dynamically.For more information, see rust-lang/cargo#7563 (comment)
I hope this will fix issues about compiling
proc_macrocrates on musl host without bring more issues.Fix rust-lang/cargo#7563