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

Detect rustc(drivers) more precisely #1897

Closed
wants to merge 2 commits into from
Closed

Detect rustc(drivers) more precisely #1897

wants to merge 2 commits into from

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Oct 7, 2023 •
edited
Loading

If we are not sure it's rustc like and we check -vV output to accept valid rustc drivers.

Fixes #861

@sagudev sagudev marked this pull request as ready for review October 7, 2023 06:42
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2023 •
edited
Loading

Codecov Report

Attention: 125 lines in your changes are missing coverage. Please review.

Comparison is base (abbd88e) 31.00% compared to head (0bf8ee2) 30.55%.

❗ Current head 0bf8ee2 differs from pull request most recent head 1a58db1. Consider uploading reports for the commit 1a58db1 to get more accurate results

Files Patch % Lines
src/compiler/compiler.rs 28.07% 17 Missing and 106 partials ⚠️
src/test/tests.rs 33.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1897      +/-   ##
==========================================
- Coverage   31.00%   30.55%   -0.46%     
==========================================
  Files          52       51       -1     
  Lines       19753    19254     -499     
  Branches     9506     9238     -268     
==========================================
- Hits         6125     5883     -242     
+ Misses       7925     7731     -194     
+ Partials     5703     5640      -63

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sagudev
Copy link
Contributor Author

sagudev commented Oct 7, 2023

Aarch fails on unrelated error (installation of musl).

@Xuanwo
Copy link
Collaborator

Xuanwo commented Oct 7, 2023

Aarch fails on unrelated error (installation of musl).

Yep. It's unrelated. I'm working on the fix. Please ignore it for now. Thanks for the patience.

@sagudev sagudev mentioned this pull request Oct 9, 2023
Merged
3 tasks
sylvestre
sylvestre reviewed Oct 10, 2023
View reviewed changes
src/compiler/compiler.rs Outdated Show resolved Hide resolved
sylvestre
sylvestre reviewed Oct 11, 2023
View reviewed changes
src/compiler/compiler.rs Outdated Show resolved Hide resolved
sylvestre
sylvestre reviewed Oct 11, 2023
View reviewed changes
src/compiler/compiler.rs Outdated Show resolved Hide resolved
@sagudev sagudev requested a review from sylvestre October 17, 2023 04:28
@sylvestre sylvestre requested a review from drahnr October 17, 2023 08:04
drahnr
drahnr reviewed Oct 17, 2023
View reviewed changes
src/compiler/compiler.rs Outdated Show resolved Hide resolved
@drahnr
Copy link
Collaborator

drahnr commented Oct 17, 2023

I have yet to do a full review.

@sagudev
Copy link
Contributor Author

sagudev commented Oct 18, 2023

I squashed all commits as they were not meant to be reviewed separately. I also updated verify_rust_compiler to not depend on must_be_rust and moved this logic back to detect_compiler

drahnr
drahnr reviewed Oct 18, 2023
View reviewed changes
src/compiler/compiler.rs Outdated Show resolved Hide resolved
@sagudev
Copy link
Contributor Author

sagudev commented Nov 27, 2023

rebased

@sagudev sagudev requested a review from drahnr November 27, 2023 16:31
@sagudev sagudev mentioned this pull request Dec 12, 2023
Closed
@drahnr
Copy link
Collaborator

drahnr commented Jan 23, 2024

I'll do a full review as soon as all CI steps went green

@sagudev
Copy link
Contributor Author

sagudev commented Jan 25, 2024

Checks are green in my fork: https://github.com/sagudev/sccache/actions/runs/7656273169 & https://github.com/sagudev/sccache/actions/runs/7656273174

drahnr
drahnr reviewed Jan 25, 2024
View reviewed changes
src/compiler/compiler.rs Outdated Show resolved Hide resolved
drahnr
drahnr reviewed Jan 25, 2024
View reviewed changes
src/compiler/compiler.rs
if stdout.starts_with("rustc ") {
return Ok(stdout);
}
if !must_be_rust && is_like_c_compiler(&rustc_executable) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the need for the must_be_rust. I do not understand why we go back to mutable state if we can use scoped Option<PathBuf> if there is a rustc-like or None, and use that to determine the full compiler path or fallback to c_compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to be compatible with current behavior. If name of compiler is rustc and we cannot detect it we fast fail (we do not try it for c compiler).

Copy link
Collaborator

Choose a reason for hiding this comment

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

That may be, but it can be expressed more concisely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, should I just change behavior and simply code or express it differently (currently have no idea how)?

drahnr
drahnr reviewed Jan 25, 2024
View reviewed changes
src/compiler/compiler.rs
"",
"gcc: error: unrecognized command-line option '-vV'; did you mean '-v'?",
)),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Iirc, the result will be cached, so the impact of another resolution call is very small, even for small projects. Correct me if I am wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this comment to #1897 (comment)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a principled question, if we incur much overhead for the compiler resolution with this changeset.

Copy link
Contributor Author

@sagudev sagudev Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure, to be honest. But if the results are cached then it should be ok.

drahnr
drahnr requested changes Jan 25, 2024
View reviewed changes
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.

I don't particularly like the use mutable bool variables in deciding the compiler. There is an outstanding question on the efficiency of test runs and ifwe can assume -vV is unique to rust.

@sagudev
Copy link
Contributor Author

sagudev commented Jan 25, 2024

if we can assume -vV is unique to rust.

Even if it is not we try to parse the result and if it does not contain rustc(-drivers) specific output we know it is not rusty.

@drahnr drahnr mentioned this pull request Feb 12, 2024
Merged
@sylvestre
Copy link
Collaborator

sylvestre commented Feb 20, 2024 •
edited by drahnr
Loading

moved into a different PR #1897

@sylvestre sylvestre closed this Feb 20, 2024
sylvestre pushed a commit that referenced this pull request Feb 23, 2024
@drahnr @sagudev
rustc driver detection - includes #1897 (#2085)
0cc0c62 
* compiler: Add an additional fallback attempt if no vanilla rustc or known cc was found

Fixes #861


Co-authored-by: sagudev <16504129+sagudev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drahnr drahnr drahnr requested changes

@sylvestre sylvestre Awaiting requested review from sylvestre

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support arbitrary rustc drivers
5 participants
@sagudev @codecov-commenter @Xuanwo @drahnr @sylvestre