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

Handle use of RUSTC_WORKSPACE_WRAPPER #1280

Merged
merged 3 commits into from
Sep 15, 2022
Merged

Conversation

tofay
Copy link
Contributor

@tofay tofay commented Aug 11, 2022

Fixes #1274

Handle the scenario where cargo is being used with sccache as a RUSTC_WRAPPER and another tool is being used as a
RUSTC_WORKSPACE_WRAPPER, by inspecting the first argument.

In this case rustc will be the first argument passed to the workspace wrapper, and the rustc version check will need to also do <wrapper> rustc -vV rather than <wrapper> -vV.

As a precaution against inadvertently identifying a c compiler as a rustc workspace wrapper, I also checked for the presence of the CARGO env which cargo sets for all processes it runs.

@amousset
Copy link

Works as expected with cargo-auditable.

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2022

Codecov Report

Merging #1280 (3d85426) into main (c8d0cbe) will increase coverage by 0.07%.
The diff coverage is 35.16%.

❗ Current head 3d85426 differs from pull request most recent head b24fff7. Consider uploading reports for the commit b24fff7 to get more accurate results

@@            Coverage Diff             @@
##             main    #1280      +/-   ##
==========================================
+ Coverage   29.60%   29.67%   +0.07%     
==========================================
  Files          49       49              
  Lines       17246    17321      +75     
  Branches     8304     8333      +29     
==========================================
+ Hits         5105     5140      +35     
- Misses       6798     6808      +10     
- Partials     5343     5373      +30     
Impacted Files Coverage Δ
src/commands.rs 14.97% <0.00%> (-0.05%) ⬇️
src/compiler/compiler.rs 35.78% <34.11%> (+0.61%) ⬆️
src/server.rs 30.80% <75.00%> (-0.01%) ⬇️
src/util.rs 35.03% <0.00%> (-0.32%) ⬇️
src/compiler/rust.rs 32.75% <0.00%> (-0.27%) ⬇️
src/lib.rs 12.13% <0.00%> (-0.06%) ⬇️
src/compiler/c.rs 38.79% <0.00%> (+0.23%) ⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/compiler/compiler.rs Outdated Show resolved Hide resolved
src/compiler/compiler.rs Outdated Show resolved Hide resolved
src/compiler/compiler.rs Outdated Show resolved Hide resolved
@tofay tofay requested a review from drahnr August 25, 2022 10:13
Handle the scenario where cargo is being used with sccache as a
RUSTC_WRAPPER and another tool is being used as a
RUSTC_WORKSPACE_WRAPPER, by looking at whether rustc is the first
argument.

Signed-off-by: Tom Fay <[email protected]>
the wrapped rustc may not just be "rustc", it may have an extension
or more path components
Copy link
Collaborator

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

Looks almost ready to be merged!

Your contribution is very much appreciated!

It'd be great if you could address the few remaining miniscule nits.

src/compiler/compiler.rs Outdated Show resolved Hide resolved
src/compiler/compiler.rs Show resolved Hide resolved
src/compiler/compiler.rs Outdated Show resolved Hide resolved
src/compiler/compiler.rs Show resolved Hide resolved
@tofay
Copy link
Contributor Author

tofay commented Sep 6, 2022

Thanks for the review - I've addressed the nits now.

@tofay tofay requested a review from drahnr September 6, 2022 13:49
@drahnr
Copy link
Collaborator

drahnr commented Sep 6, 2022

If CI is passed, we're good to go! Thank you!

@Shnatsel
Copy link

Shnatsel commented Sep 6, 2022

CI is all green 🎉

@Shnatsel
Copy link

If CI is passed, we're good to go! Thank you!

We're just waiting on someone to push the "merge" button, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails to detect the compiler if RUSTC_WORKSPACE_WRAPPER is set
5 participants