improved `fn get_or_default_sysroot` #103581

onur-ozkan · 2022-10-26T15:08:50Z

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

rustbot · 2022-10-26T15:08:54Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

rust-highfive · 2022-10-26T15:08:55Z

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

Please see the contribution instructions for more information.

jyn514 · 2022-10-26T15:15:43Z

r? @RalfJung

RalfJung · 2022-10-26T16:44:13Z

src/tools/miri/src/bin/miri.rs

-        }
+
+        args.push(sysroot_flag.to_owned());
+        args.push(default_sysroot);


This is still using the --sysroot arg-passing hack to set the sysroot, so I don't see how that's better than the status quo.

All this seems to do is extract some common code from Miri and clippy, but we could have that more easily by moving that code to a helper crate. The actual problem of rustc hard-coding the default sysroot does not get resolved by this.

This is still using the --sysroot arg-passing hack to set the sysroot, so I don't see how that's better than the status quo.

I suppose I misunderstand the goal here then. I guess all sysroot calculation/value passing logic has to be excluded from miri and it should still keep working as before. If so, I will work on that.

compiler/rustc_session/src/filesearch.rs

bors · 2022-10-27T00:39:03Z

☔ The latest upstream changes (presumably #103571) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_session/Cargo.toml

src/tools/miri/src/bin/miri.rs

RalfJung · 2022-10-27T19:07:57Z

The Miri change looks great. :) I just hope it will work in all the same cases as before, which are not all covered by our CI...

I don't know all that code in rustc_session so I can't comment on it.

jyn514 · 2022-10-27T19:46:11Z

r? @jyn514

jyn514

This looks good to me :)

@RalfJung I know we don't run clippy tests in CI - do we run miri tests? Would be nice to see whether this works at all before a full rollup.

@ozkanonur can you run x test src/tools/clippy in the meantime and see if that passes? It will take a long time to compile unfortunately.

compiler/rustc_session/src/filesearch.rs

jyn514 · 2022-10-27T21:55:54Z

Amazing, thanks! Can you please squash the commits? r=me with that done.

RalfJung · 2022-10-27T22:47:44Z

I know we don't run clippy tests in CI - do we run miri tests? Would be nice to see whether this works at all before a full rollup.

The 'tools' builder (the third PR builder) tests both clippy and miri. But the things touched by this PR are most relevant for out-of-tree builds so it is unclear whether CI is even very informative.

I will locally test Miri building.

RalfJung · 2022-10-27T23:00:55Z

Miri is looking good :)

rustbot · 2022-10-27T23:08:17Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

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

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 26, 2022

rust-highfive assigned estebank Oct 26, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2022

rust-highfive assigned RalfJung and unassigned estebank Oct 26, 2022

jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-miri Area: The miri tool A-clippy Area: Clippy labels Oct 26, 2022

RalfJung reviewed Oct 26, 2022

View reviewed changes

compiler/rustc_session/src/filesearch.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Oct 26, 2022

View reviewed changes

compiler/rustc_session/src/filesearch.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Oct 26, 2022

View reviewed changes

compiler/rustc_session/src/filesearch.rs Outdated Show resolved Hide resolved

onur-ozkan changed the title ~~improved fn get_or_default_sysroot~~ WIP: improved fn get_or_default_sysroot Oct 26, 2022

onur-ozkan changed the title ~~WIP: improved fn get_or_default_sysroot~~ improved fn get_or_default_sysroot Oct 27, 2022

onur-ozkan requested review from RalfJung and bjorn3 and removed request for RalfJung and bjorn3 October 27, 2022 18:25

bjorn3 reviewed Oct 27, 2022

View reviewed changes

compiler/rustc_session/Cargo.toml Outdated Show resolved Hide resolved

RalfJung reviewed Oct 27, 2022

View reviewed changes

src/tools/miri/src/bin/miri.rs Outdated Show resolved Hide resolved

rustbot assigned jyn514 and unassigned RalfJung Oct 27, 2022

jyn514 approved these changes Oct 27, 2022

View reviewed changes

compiler/rustc_session/src/filesearch.rs Outdated Show resolved Hide resolved

compiler/rustc_session/src/filesearch.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

onur-ozkan requested a review from jyn514 October 27, 2022 21:48

jyn514 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 Oct 27, 2022

onur-ozkan force-pushed the master branch from 09b2745 to d47a7b0 Compare October 27, 2022 22:46

onur-ozkan force-pushed the master branch from d47a7b0 to 09b2745 Compare October 27, 2022 22:52

onur-ozkan force-pushed the master branch from 09b2745 to ce06989 Compare October 27, 2022 23:08

onur-ozkan force-pushed the master branch 3 times, most recently from 8d622d6 to 01459f6 Compare October 27, 2022 23:26

onur-ozkan closed this Oct 27, 2022

onur-ozkan force-pushed the master branch from 01459f6 to 0da281b Compare October 27, 2022 23:40

onur-ozkan mentioned this pull request Oct 27, 2022

improve filesearch::get_or_default_sysroot #103660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improved `fn get_or_default_sysroot` #103581

improved `fn get_or_default_sysroot` #103581

onur-ozkan commented Oct 26, 2022

rustbot commented Oct 26, 2022

rust-highfive commented Oct 26, 2022

jyn514 commented Oct 26, 2022

RalfJung Oct 26, 2022

onur-ozkan Oct 26, 2022 •

edited

Loading

bors commented Oct 27, 2022

RalfJung commented Oct 27, 2022

jyn514 commented Oct 27, 2022

jyn514 left a comment

This comment has been minimized.

This comment has been minimized.

jyn514 commented Oct 27, 2022

RalfJung commented Oct 27, 2022 •

edited

Loading

RalfJung commented Oct 27, 2022

rustbot commented Oct 27, 2022

improved fn get_or_default_sysroot #103581

improved fn get_or_default_sysroot #103581

Conversation

onur-ozkan commented Oct 26, 2022

rustbot commented Oct 26, 2022

rust-highfive commented Oct 26, 2022

jyn514 commented Oct 26, 2022

RalfJung Oct 26, 2022

Choose a reason for hiding this comment

onur-ozkan Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

bors commented Oct 27, 2022

RalfJung commented Oct 27, 2022

jyn514 commented Oct 27, 2022

jyn514 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

jyn514 commented Oct 27, 2022

RalfJung commented Oct 27, 2022 • edited Loading

RalfJung commented Oct 27, 2022

rustbot commented Oct 27, 2022

improved `fn get_or_default_sysroot` #103581

improved `fn get_or_default_sysroot` #103581

onur-ozkan Oct 26, 2022 •

edited

Loading

RalfJung commented Oct 27, 2022 •

edited

Loading