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

Start making clippy easier to invoke in non-cargo contexts #3665

Merged
merged 6 commits into from
Feb 6, 2019

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented Jan 15, 2019

Clippy (clippy-driver) currently has a couple of strong but unnecessary couplings with cargo. This series:

  1. makes detection of check builds more robust, and
  2. make clippy-driver use the --sysroot specified on the command line as its internal sysroot.

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 15, 2019
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

@jsgf
Copy link
Contributor Author

jsgf commented Jan 17, 2019

@oli-obk Clippy's advice for https://travis-ci.com/rust-lang/rust-clippy/jobs/170625109#L1309 is wrong and would not compile - this is an explicit while because it also advances the iterator within the body. Should I just suppress that particular clippy warning for the function?

@Manishearth
Copy link
Member

Yes, please do. And file a bug for that false positive.

@jsgf jsgf force-pushed the master branch 2 times, most recently from 132c086 to 7d9799f Compare January 18, 2019 20:51
@phansch
Copy link
Member

phansch commented Jan 27, 2019

retriggering CI

@phansch phansch closed this Jan 27, 2019
@phansch phansch reopened this Jan 27, 2019
@phansch
Copy link
Member

phansch commented Jan 27, 2019

Looks like it just needs a small rustfmt fix: https://travis-ci.com/rust-lang/rust-clippy/jobs/173272226#L1397

@jsgf jsgf force-pushed the master branch 3 times, most recently from 400079c to 41100d8 Compare January 28, 2019 01:09
@jsgf
Copy link
Contributor Author

jsgf commented Jan 28, 2019

I fixed the formatting, but the tests seem to be failing for unrelated reasons which is preventing me from making sure the tests I added are actually working.

@phansch
Copy link
Member

phansch commented Feb 1, 2019

huh that's a weird CI failure indeed. Some UI tests seem to have no stderr output?

I will trigger a new build and see if it still happens.

@phansch phansch closed this Feb 1, 2019
@phansch phansch reopened this Feb 1, 2019
@bors
Copy link
Contributor

bors commented Feb 2, 2019

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

@phansch
Copy link
Member

phansch commented Feb 2, 2019

The weird UI test failures seem to have gone away now. I'm not sure where they came from, tbh.

It looks like the output of the cstring.stderr is different now: https://travis-ci.com/rust-lang/rust-clippy/jobs/174594861#L1040

jsgf and others added 5 commits February 2, 2019 11:43
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
If the user explicitly sets sysroot on the command line, then use that
value.

Issue rust-lang#3663
CARGO_MANIFEST_DIR if it isn't set. If CARGO_MANIFEST_DIR isn't set, fall back
"." rather than panicing.

Issue rust-lang#3663
It saves on having to pair `cd <path> && think && cd ..`.
@jsgf
Copy link
Contributor Author

jsgf commented Feb 2, 2019

@phansch It hasn't changed, but clippy is being invoked differently I think. I'm still trying to understand how all the ui tests actually get invoked.

@oli-obk oli-obk closed this Feb 6, 2019
@oli-obk oli-obk reopened this Feb 6, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Feb 6, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Feb 6, 2019

📌 Commit f0131fb has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Feb 6, 2019

⌛ Testing commit f0131fb with merge e176324...

bors added a commit that referenced this pull request Feb 6, 2019
Start making clippy easier to invoke in non-cargo contexts

Clippy (clippy-driver) currently has a couple of strong but unnecessary couplings with cargo. This series:
1. makes detection of check builds more robust, and
2. make clippy-driver use the --sysroot specified on the command line as its internal sysroot.
@bors
Copy link
Contributor

bors commented Feb 6, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing e176324 to master...

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants