Skip to content

driver: print rustc --version --verbose on "clippy-driver rustc --version" #5166

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
Closed
wants to merge 1 commit into from
Closed

driver: print rustc --version --verbose on "clippy-driver rustc --version" #5166

wants to merge 1 commit into from

Conversation

matthiaskrgr
Copy link
Member

Fixes #5159

changelog: make "clippy-driver rustc --version" print "rustc --version --verbose" output.
@matthiaskrgr matthiaskrgr force-pushed the driver_rustc_version branch 4 times, most recently from fb4d447 to bb576e9 Compare February 12, 2020 20:35
@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 13, 2020
@flip1995
Copy link
Member

Looking at #5167, I think we should rethink the output of clippy, when called with --version/-V/.... I think adding a rustc --version doesn't really solve this problem.

Maybe we could add clippy-driver rustc ... which just forwards every flag to rustc (maybe even disables clippy completely and just runs plain rustc on the code)
@matthiaskrgr
Copy link
Member Author

Yeah that sounds like a good solution.
jsgf
jsgf reviewed Feb 13, 2020
View reviewed changes
src/driver.rs
fn print_rustc_version(args: &[String]) {
// make "clippy-driver rustc --version" print rustc --version output
if args.len() > 2 && args[1..2] == ["rustc".to_string(), "--version".to_string()] {
let rustc_version = Command::new("rustc")
Copy link
Contributor

Choose a reason for hiding this comment

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

No, there's no guarantee that there's a rustc in the path, or that if there is, it has anything to do with the version of clippy-driver we're running.
@jsgf
Copy link
Contributor

jsgf commented Feb 13, 2020

@flip1995 Yes, I think making clippy-driver rustc ... pass all options through to the linked-in rustc makes sense, but it should still keep the clippy functionality. clippy-driver is already a functionally complete rustc, aside from the handful of options it intercepts early.

If we could go back in time, I'd suggest that all clippy-specific options should be of the form --clippy-X - --clippy-version etc - so that all other options can be passed directly through to rustc. But using clippy-driver rustc as a way of quoting them seems like a reasonable workaround now.
@flip1995
Copy link
Member

but it should still keep the clippy functionality.

Yeah, I also think this would be the better behavior. Just in case this should be way harder to implement, I'd also be good with just having clippy-driver rustc == rustc-linked-with-clippy.

I'd suggest that all clippy-specific options should be of the form --clippy-X

I think I would have disagreed with this, but let's not bikeshed something we would have to go back in time to change 😄
@jsgf
Copy link
Contributor

jsgf commented Feb 14, 2020

Yeah, I also think this would be the better behavior. Just in case this should be way harder to implement, I'd also be good with just having clippy-driver rustc == rustc-linked-with-clippy.

In addition, if filename(argv[0]) == "rustc", then behave as rustc (so you can either execve it as rustc, or symlink rustc -> clippy-driver).
@flip1995
Copy link
Member

ping @matthiaskrgr Are you interested in implementing this? Otherwise I'd close this PR, because just adding rustc --version doesn't really adds much IMHO, since something similar can already be achieved by just -Vv (accidentally).
@matthiaskrgr
Copy link
Member Author

I posted a different attempt where clippy-driver rustc --foo just execs rustc --foo: #5178
@jsgf
Copy link
Contributor

jsgf commented Feb 15, 2020

@matthiaskrgr That's basically what this PR does, and it has the same problem (I commented on your PR as well as here).
@flip1995
Copy link
Member

Closing in favor of #5178
@flip1995 flip1995 closed this Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

4 participants

@matthiaskrgr @flip1995 @jsgf @JohnTitor