Skip to content

clippy-driver has unnecessary couplings with cargo #3663

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

Closed
jsgf opened this issue Jan 15, 2019 · 8 comments
Closed

clippy-driver has unnecessary couplings with cargo #3663

jsgf opened this issue Jan 15, 2019 · 8 comments
Labels
E-needs-test Call for participation: writing tests T-cargo Type: cargo related

Comments

@jsgf
Copy link
Contributor

jsgf commented Jan 15, 2019

I'm trying to use clippy-driver in a non-cargo build system (Buck), by using it as a drop-in replacement for rustc. There are a few ways in which this could be improved:

  1. clippy-driver looks to see if its doing a check build by explicitly looking for the string --emit=dep-info,metadata, which is the string that Cargo uses. This is brittle - for example, Buck's #check builds don't use dep-info. It should just check for the presence of metadata.
  2. It also relies on CARGO_MANIFEST_DIR being set to find .?clippy.toml, and panics if it isn't. It should at least fall back to a default dir like ..
  3. It relies on SYSROOT being set to find the compiler - it would be nice if there a way to derive SYSROOT from the path of the executable and inhibit the search for a rustup installation.
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 15, 2019
Rather than looking for a fixed --emit arg set, just check to see
if we're emitting metadata at all. This makes it more robust to
being invoked by tools other than cargo (or if cargo changes its
invocation).

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 15, 2019
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 15, 2019
Rather than looking for a fixed --emit arg set, just check to see
if we're emitting metadata at all. This makes it more robust to
being invoked by tools other than cargo (or if cargo changes its
invocation).

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 15, 2019
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
@oli-obk oli-obk added T-cargo Type: cargo related E-needs-test Call for participation: writing tests labels Jan 17, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2019

I think we should setup a somewhat realistic test scenario for non-cargo uses. Mostly this is covered by our compiletest suite but that one is very fine tuned to replicate some of cargo's quicks. We should just add a test script to travis that runs clippy-driver directly on a few files

@jsgf
Copy link
Contributor Author

jsgf commented Jan 17, 2019

Thanks for the feedback - yeah I'm specifically looking for advice on how to test this. And I also want to actually deploy it locally to make sure these changes are necessary and sufficient.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 18, 2019

For testing I'd create a script (to run locally and on CI) that runs cargo-clippy on a few files (e.g. in the tests/ui directory) and checks whether it gives the expected result.

To really test a non-rustup system we'd probably need to compile a cargo-clippy without any of the env vars set so it goes into the fallback. We probably need to create an entirely separate travis job to test that in a fully honest manner. As an intermediate solution you could touch src/driver.rs, clear the problematic env vars, and cargo build again. The resulting clippy-driver should then be fairly vanilla

@jsgf
Copy link
Contributor Author

jsgf commented Jan 18, 2019

we'd probably need to compile a cargo-clippy without any of the env vars set

It did strike me as odd that setting compile-time env vars overrides all the runtime ways of setting things. The normal convention is to take these things in order of preference:

  1. command-line arg
  2. env var
  3. built-in default (from env or whatever other build-time config)

(that is, more specific to a particular invocation to least)

Is there a specific reason for clippy-driver's order?

jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 18, 2019
Rather than looking for a fixed --emit arg set, just check to see
if we're emitting metadata at all. This makes it more robust to
being invoked by tools other than cargo (or if cargo changes its
invocation).

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 18, 2019
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 18, 2019
Rather than looking for a fixed --emit arg set, just check to see
if we're emitting metadata at all. This makes it more robust to
being invoked by tools other than cargo (or if cargo changes its
invocation).

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 18, 2019
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
@oli-obk
Copy link
Contributor

oli-obk commented Jan 23, 2019

Is there a specific reason for clippy-driver's order?

Historically homegrown spaghetti code

I think it would be totally fine to invert the order.

jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 27, 2019
Rather than looking for a fixed --emit arg set, just check to see
if we're emitting metadata at all. This makes it more robust to
being invoked by tools other than cargo (or if cargo changes its
invocation).

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 27, 2019
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 27, 2019
CARGO_MANIFEST_DIR if it isn't set. If CARGO_MANIFEST_DIR isn't set, fall back
"." rather than panicing.

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 27, 2019
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 27, 2019
CARGO_MANIFEST_DIR if it isn't set. If CARGO_MANIFEST_DIR isn't set, fall back
"." rather than panicing.

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 27, 2019
Rather than looking for a fixed --emit arg set, just check to see
if we're emitting metadata at all. This makes it more robust to
being invoked by tools other than cargo (or if cargo changes its
invocation).

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 27, 2019
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Jan 27, 2019
CARGO_MANIFEST_DIR if it isn't set. If CARGO_MANIFEST_DIR isn't set, fall back
"." rather than panicing.

Issue rust-lang#3663
@jsgf
Copy link
Contributor Author

jsgf commented Jan 30, 2019

@oli-obk OK, I tried adding a test but the last travis run fell over on unrelated problems.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 31, 2019

our CI works again (for now 😆 )

jsgf added a commit to jsgf/rust-clippy that referenced this issue Feb 2, 2019
Rather than looking for a fixed --emit arg set, just check to see
if we're emitting metadata at all. This makes it more robust to
being invoked by tools other than cargo (or if cargo changes its
invocation).

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Feb 2, 2019
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
jsgf added a commit to jsgf/rust-clippy that referenced this issue Feb 2, 2019
CARGO_MANIFEST_DIR if it isn't set. If CARGO_MANIFEST_DIR isn't set, fall back
"." rather than panicing.

Issue rust-lang#3663
@oli-obk
Copy link
Contributor

oli-obk commented Feb 15, 2019

@jsgf I think you resolved all the 3 points in the main post of this issue. Thus I'm closing the issue. Please open new issues with any problems in this direction that you encounter

@oli-obk oli-obk closed this as completed Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-needs-test Call for participation: writing tests T-cargo Type: cargo related
Projects
None yet
Development

No branches or pull requests

2 participants