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

improve filesearch::get_or_default_sysroot #103660

Merged
merged 1 commit into from
Nov 5, 2022
Merged

Conversation

onur-ozkan
Copy link
Member

fn get_or_default_sysroot is now improved and used in miri and clippy, and tests are still passing as they should. So we no longer need to implement custom workarounds/hacks to find sysroot in tools like miri/clippy.

Resolves #98832

re-opened from #103581

@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2022

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

Please see the contribution instructions for more information.

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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 Oct 27, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

@jyn514
Copy link
Member

jyn514 commented Oct 28, 2022

r? @jyn514

@ozkanonur if you remove the last commit ("merge branch master") we can merge this:)

@onur-ozkan
Copy link
Member Author

r? @jyn514

@ozkanonur if you remove the last commit ("merge branch master") we can merge this:)

It was conflict resolving commit. Rollbacked it.

@jyn514
Copy link
Member

jyn514 commented Oct 28, 2022

Try using https://rustc-dev-guide.rust-lang.org/git.html#rebasing-and-conflicts to resolve the conflicts.

@jyn514
Copy link
Member

jyn514 commented Oct 28, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 28, 2022

📌 Commit ae0232e has been approved by jyn514

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 Oct 28, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 28, 2022
improve `filesearch::get_or_default_sysroot`

`fn get_or_default_sysroot` is now improved and used in `miri` and `clippy`, and tests are still passing as they should. So we no longer need to implement custom workarounds/hacks to find sysroot in tools like miri/clippy.

Resolves rust-lang#98832

re-opened from rust-lang#103581
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Nov 3, 2022
improve `filesearch::get_or_default_sysroot`

`fn get_or_default_sysroot` is now improved and used in `miri` and `clippy`, and tests are still passing as they should. So we no longer need to implement custom workarounds/hacks to find sysroot in tools like miri/clippy.

Resolves rust-lang#98832

re-opened from rust-lang#103581
@onur-ozkan
Copy link
Member Author

@bors r+

A reminder, PR popped-off from the merge queue after windows compatibility fix(I tested it).

@jyn514
Copy link
Member

jyn514 commented Nov 4, 2022

Hmm, I don't actually see any changes since the failed rollup? both the force pushes say "no changes to compare"

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Nov 4, 2022

Hmm, I don't actually see any changes since the failed rollup? both the force pushes say "no changes to compare"

Here is the change for win compatibility fix f1b11b39940bda3e37d2c3a588f1baba27e6bb59

and 9e06962018506edc185ddc48eb30cc95dd81aa3f for fixing fmt error.

At the end, commits squashed and pushed with improve filesearch::get_or_default_sysroot message

@jyn514
Copy link
Member

jyn514 commented Nov 4, 2022

Thanks :)

@bors r+

oxalica added a commit to oxalica/rust-overlay that referenced this pull request Nov 8, 2022
@flip1995
Copy link
Member

flip1995 commented Nov 19, 2022

This no longer takes the SYSROOT env var into account. At least not for clippy-driver, and with that makes Clippy CI fail: https://github.com/rust-lang/rust-clippy/actions/runs/3505195043/jobs/5871355206#step:12:22

This no longer works to set the SYSROOT:

$ SYSROOT=/path/to/sysroot ./target/debug/clippy-driver --print sysroot

Is this intentional? @ozkanonur @jyn514 (Sorry for reporting this so late. The sync was blocked for so long, partly because I didn't have time to deal with it)

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Nov 19, 2022

This no longer takes the SYSROOT env var into account. At least not for clippy-driver, and with that makes Clippy CI fail: https://github.com/rust-lang/rust-clippy/actions/runs/3505195043/jobs/5871355206#step:12:22

This no longer works to set the SYSROOT:

$ SYSROOT=/path/to/sysroot ./target/debug/clippy-driver --print sysroot

Is this intentional? @ozkanonur @jyn514 (Sorry for reporting this so late. The sync was blocked for so long, partly because I didn't have time to deal with it)

I think I should have leave it supported(like miri's MIRI_SYSROOT). So, it might be unintentional. But it's better to wait Joshua's answer since I am pretty newbie on this project. :)

I can open the fix PR if needed(depending on the answer from Joshua).

@RalfJung
Copy link
Member

Miri still has this logic for that purpose -- maybe the equivalent in clippy got accidentally deleted in this PR. (A bit surprising that rustc CI would pass then though since it checks clippy?)

@onur-ozkan
Copy link
Member Author

Miri still has this logic for that purpose -- maybe the equivalent in clippy got accidentally deleted in this PR. (A bit surprising that rustc CI would pass then though since it checks clippy?)

I guess we should also implement a test that checks this case then

@flip1995
Copy link
Member

Ah ok, yes this should also be implemented in Clippy. I will do that in the Sync PR on the Clippy side and then sync it back.

The CI didn't fail, because in the Rust repo some tests aren't run. One of them is our clippy-driver test. This is because changes like this should be done in the Clippy repo, rather than the Rust repo. In the Rust repo only fallout fixes should be done to the subtrees.

@RalfJung
Copy link
Member

In the Rust repo only fallout fixes should be done to the subtrees.

This might be true for clippy but not all subtrees -- for Miri we run pretty much all tests and I do full feature development in the rutsc repo when needed (because rustc changes are required).

flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 21, 2022
improve `filesearch::get_or_default_sysroot`

`fn get_or_default_sysroot` is now improved and used in `miri` and `clippy`, and tests are still passing as they should. So we no longer need to implement custom workarounds/hacks to find sysroot in tools like miri/clippy.

Resolves rust-lang#98832

re-opened from rust-lang#103581
@jyn514
Copy link
Member

jyn514 commented Nov 26, 2022

@flip1995 @RalfJung can you please document both those policies? any of the following seem like good places:

  • (ideally) the ping message that shows up when the subtree is modified
  • the dev-guide
  • the README for your tool

bkchr added a commit to bkchr/nixpkgs-mozilla that referenced this pull request Nov 30, 2022
Rust changed the way the SYSROOT is determined in: rust-lang/rust#103660

Before this change the SYSROOT was determined based on the rustc
executable. Now it is determined based on the librustc_driver-*.so file.
This pr fixes the detection by copying the library to the derivation as
we have done it for the rustc executable.
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
improve `filesearch::get_or_default_sysroot`

`fn get_or_default_sysroot` is now improved and used in `miri` and `clippy`, and tests are still passing as they should. So we no longer need to implement custom workarounds/hacks to find sysroot in tools like miri/clippy.

Resolves rust-lang#98832

re-opened from rust-lang#103581
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#103621 (Correctly resolve Inherent Associated Types)
 - rust-lang#103660 (improve `filesearch::get_or_default_sysroot`)
 - rust-lang#103866 (Remove some return-type diagnostic booleans from `FnCtxt`)
 - rust-lang#103867 (Remove `has_errors` from `FnCtxt`)
 - rust-lang#103994 (Specify that `break` cannot be used outside of loop *or* labeled block)
 - rust-lang#103995 (Small round of typo fixes)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
nbp pushed a commit to mozilla/nixpkgs-mozilla that referenced this pull request Jan 26, 2023
Rust changed the way the SYSROOT is determined in: rust-lang/rust#103660

Before this change the SYSROOT was determined based on the rustc
executable. Now it is determined based on the librustc_driver-*.so file.
This pr fixes the detection by copying the library to the derivation as
we have done it for the rustc executable.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2023
… r=ozkanonur

compiler/rustc_session: fix sysroot detection logic

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 rust-lang#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!
nbdd0121 added a commit to Rust-for-Linux/klint that referenced this pull request Apr 20, 2023
rust-lang/rust#103660 makes this unnecessary. Closes #2
jyn514 added a commit to jyn514/ubrustc that referenced this pull request Jun 20, 2023
- Remove sysroot handling. Since rust-lang/rust#103660 this is detected automatically!
- Update to a newer toolchain; in particular `install_ice_hook` has changed its signature.
- Remove the wrapper code; it only matters for cargo plugins, but ubrustc only has an equivalent of clippy-driver, not cargo-clippy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the default sysroot configurable by rustc_private tools
9 participants