Fix: bootstrap failed when enable both 'llvm'/'gcc' codegen backends#152620
Fix: bootstrap failed when enable both 'llvm'/'gcc' codegen backends#152620zhanghe9702 wants to merge 1 commit intorust-lang:mainfrom
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Please also write a proper PR description / commit message (not just a link to an issue). That helps future readers and reviewers understand the context of the change (e.g., why you think this is the right change, any references that are useful such as docs from gcc, ...).
| // DESTDIR during the install phase,and the rustc bootstrap process itself | ||
| // may also specify DESTDIR to set the toolchain installation path, it is | ||
| // necessary to eliminate potential installation path errors for libgccjit.so | ||
| // caused by DESTDIR during make install. |
There was a problem hiding this comment.
IIRC, there are other similar variables that might get set and we should override here (e.g., for docs and such). Is that accurate? Should we strip those as well?
There was a problem hiding this comment.
it looks like only DESTDIR would disturb make install , others won't cause panic
There was a problem hiding this comment.
Panic doesn't seem like the only concern? Does gcc's make install not respect other related variables? I think it'd be a good idea to remove it from configure and make as well, just in case.
Also, it seems likely that we should have a test for this. I see that we do enable both LLVM and GCC in the CI (src/ci/docker/host-x86_64/x86_64-gnu-gcc/Dockerfile) -- but maybe that's working because there's no DESTDIR that happens to be configured there? Can we add one (should be harmless since it's not a dist build) to continuously verify this works?
I see you haven't updated the PR description as well, as I requested in my last review. Please do that before sending this PR back to me. Thanks!
There was a problem hiding this comment.
You're absolutely right. However, since the recommended way to use the GCC backend currently is to compile it separately and then specify the path of libgccjit.so, I’ll look into whether there's a better way to handle this."
|
Reminder, once the PR becomes ready for a review, use |
…c, this commit fix it. Fix: rust-lang#152619
| // DESTDIR during the install phase,and the rustc bootstrap process itself | ||
| // may also specify DESTDIR to set the toolchain installation path, it is | ||
| // necessary to eliminate potential installation path errors for libgccjit.so | ||
| // caused by DESTDIR during make install. |
There was a problem hiding this comment.
Panic doesn't seem like the only concern? Does gcc's make install not respect other related variables? I think it'd be a good idea to remove it from configure and make as well, just in case.
Also, it seems likely that we should have a test for this. I see that we do enable both LLVM and GCC in the CI (src/ci/docker/host-x86_64/x86_64-gnu-gcc/Dockerfile) -- but maybe that's working because there's no DESTDIR that happens to be configured there? Can we add one (should be harmless since it's not a dist build) to continuously verify this works?
I see you haven't updated the PR description as well, as I requested in my last review. Please do that before sending this PR back to me. Thanks!
No description provided.