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

compiler/rustc_session: fix sysroot detection logic #108376

Merged
merged 1 commit into from
Mar 1, 2023
Merged

compiler/rustc_session: fix sysroot detection logic #108376

merged 1 commit into from
Mar 1, 2023

Conversation

liushuyu
Copy link
Contributor

This pull request fixes the sysroot detection logic on systems where /usr/lib contains a multi-arch structure (e.g. installs rustc_driver into /usr/lib/x86_64-linux-gnu folder).

This fixes a regression for various Linux systems introduced in #103660. On Debian and Ubuntu systems, the logic in the pull request, as mentioned earlier, will resolve the sysroot to /usr/lib, making rustc --print target-libdir to return /usr/lib/lib/rustlib/x86_64-unknown-linux-gnu/lib (notice the extra lib at the beginning).

The fix is not very "clean" according to the standard. If you have any suggestions on improving the logic, you are more than welcome to comment below!

@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 23, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@liushuyu liushuyu force-pushed the fix-sysroot-infer-103660 branch from 7ad3a2b to e53f6f5 Compare February 23, 2023 00:47
@liushuyu liushuyu marked this pull request as ready for review February 23, 2023 04:33
@Noratrieb
Copy link
Member

Why is this fix Linux specific? This shouldn't not work on Windows, right?
cc @jyn514

@liushuyu
Copy link
Contributor Author

Sorry for the late reply.

Why is this fix Linux specific? This shouldn't not work on Windows, right?

Yes, although I think #103660 removed this logic to fix certain issues on the Windows platform.
I am gating this logic to Linux-only to avoid reverting/breaking the Windows platform again.

@petrochenkov petrochenkov 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 Feb 27, 2023
@petrochenkov
Copy link
Contributor

cc @ozkanonur @jyn514
If someone familiar with this area and #103660 can review this instead of me, that would be great.

@onur-ozkan
Copy link
Member

onur-ozkan commented Feb 27, 2023 •
edited
Loading

Yes, although I think #103660 removed this logic to fix certain issues on the Windows platform.
I am gating this logic to Linux-only to avoid reverting/breaking the Windows platform again.

Which logic? Do you mean fn from_current_exe? It was the primary goal replacing it. This brings back the problem to tools like miri, clippy finding wrong sysroots. It's nothing to do with Windows compatibility.

@onur-ozkan
Copy link
Member

If someone familiar with this area and #103660 can review this instead of me, that would be great.

Sure, will take it

r? @ozkanonur

@rustbot rustbot assigned onur-ozkan and unassigned petrochenkov Feb 27, 2023
@liushuyu liushuyu force-pushed the fix-sysroot-infer-103660 branch from e53f6f5 to 45df5a5 Compare February 27, 2023 20:32
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@liushuyu liushuyu force-pushed the fix-sysroot-infer-103660 branch 2 times, most recently from 4ee8694 to a6245cc Compare February 27, 2023 22:28
@liushuyu liushuyu requested review from onur-ozkan and bjorn3 and removed request for onur-ozkan and bjorn3 February 27, 2023 22:29
@liushuyu liushuyu requested review from onur-ozkan and removed request for bjorn3 February 27, 2023 22:29
@liushuyu liushuyu force-pushed the fix-sysroot-infer-103660 branch from a6245cc to 0da3064 Compare February 27, 2023 23:14
@liushuyu liushuyu requested a review from onur-ozkan February 27, 2023 23:43
@onur-ozkan
Copy link
Member

Thank you for the fixes. This looks good to me.

I am not sure why one of the pipeline is stuck 🤔 Can you force push to trigger pipelines again? @liushuyu

@liushuyu
compiler/rustc_session: fix sysroot detection logic ...
2186358 
... on systems where /usr/lib contains a multi-arch structure
@liushuyu liushuyu force-pushed the fix-sysroot-infer-103660 branch from 0da3064 to 2186358 Compare February 28, 2023 17:03
@liushuyu
Copy link
Contributor Author

I am not sure why one of the pipeline is stuck thinking Can you force push to trigger pipelines again? @liushuyu

Done. Rebased and force-pushed.

onur-ozkan
onur-ozkan approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

lgtm

@onur-ozkan
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 28, 2023

📌 Commit 2186358 has been approved by ozkanonur

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 1, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2023
@bors
Auto merge of rust-lang#108587 - matthiaskrgr:rollup-rw6po59, r=matth…
bcb610d 
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#108376 (compiler/rustc_session: fix sysroot detection logic)
 - rust-lang#108400 (add llvm cgu instructions stats to perf)
 - rust-lang#108496 (fix rust-lang#108495, postfix decrement and prefix decrement has no warning)
 - rust-lang#108505 (Further unify validity intrinsics)
 - rust-lang#108520 (Small cleanup to `one_bound_for_assoc_type`)
 - rust-lang#108560 (Some `infer/mod.rs` cleanups)
 - rust-lang#108563 (Make mailmap more correct)
 - rust-lang#108564 (Fix `x clean` with specific paths)
 - rust-lang#108571 (Add contains_key to SortedIndexMultiMap)
 - rust-lang#108578 (Update Fuchsia platform team members)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0dfbce1 into rust-lang:master Mar 1, 2023
@rustbot rustbot added this to the 1.69.0 milestone Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@onur-ozkan onur-ozkan onur-ozkan approved these changes

Assignees

@onur-ozkan onur-ozkan

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
8 participants
@liushuyu @rustbot @rust-log-analyzer @Noratrieb @petrochenkov @onur-ozkan @bors @bjorn3