Update Clippy dependencies without patch versions by smoelius · Pull Request #88...
source link: https://github.com/rust-lang/rust/pull/88517
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Some changes occurred in src/tools/clippy.
(rust-highfive has picked a reviewer for you, use r? to override)
This comment has been hidden.
This will add additional versions for cargo_metadata and compiletest_rs.
This will add additional versions for cargo_metadata and compiletest_rs.
Is that an impediment?
There is some movement to keep number of (duplicated) dependencies as low as possible, in other case we always can do cargo update, yes?
For cargo_metadata this will be 3rd version for no particular reason, as for compiletest_rs, it used only in clippy and miri, so it can be bumped in miri repo too, to remove duplicate.
I prefer this non-patch version over the patch version. It only changes the Cargo.lock file half of what the patch version alternative does.
tar = "0.4"
toml = "0.5"
ureq = "2.2"
walkdir = "2.3"
This removes clap and adds walkdir?
Removal of clap was me fat-fingering the patch by hand.
walkdir was added here: rust-lang/rust-clippy@997ddbb
This version of clippy does yet not seem to include that commit.
Both problems were fixed with my most recent push.
Outdated
@@ -429,6 +447,19 @@ dependencies = [
"serde_json",
]
[[package]]
name = "cargo_metadata"
We'll probably have to do a coordinated dep update for cargo_metadata and compiletest_rs.
Old version for cargo_metadata (with bunch of other stuff) can be dropped when rustfmt will be updated in rust repo (dependencies already updated in rustfmt repo).
cargo_metadata in rustfmt seems to still use version 0.12 instead of 0.14. Is this intentional?
With rustfmt updated, we'd also need to update the tidy script and RLS.
It was updated to 0.12 as it was most common version in workspace and required less changes, in case if it will updated to newer version.
Should I close #88515 and remove the "do not merge" status from this PR?
Separately, all of this PR's changes, besides those to cargo_common_metadata.rs, could be generated with a script. Would it be helpful if I were to record that somehow? For example, I could suggest a change to Clippy's CONTRIBUTING.md file, say, around the Syncing changes section.
Should I close #88515 and remove the "do not merge" status from this PR?
Separately, all of this PR's changes, besides those to
cargo_common_metadata.rs, could be generated with a script. Would it be helpful if I were to record that somehow? For example, I could suggest a change to Clippy'sCONTRIBUTING.mdfile, say, around the Syncing changes section.
I dunno. Updates should be evaluated one by one anyways so I probably wouldn't use a script. 
changed the title Do not merge: Update Clippy dependencies without patch versions
Update Clippy dependencies without patch versions
Done.
I dunno. Updates should be evaluated one by one anyways so I probably wouldn't use a script.
From what I can tell, there is currently no Clippy "Dependabot," i.e., something to tell you when updates are possible. A script would at least provide this information, and one could always ignore the script's results.
Anyway, it's here: https://gist.github.com/smoelius/02c63fa0aa33d1f2bc913c8008890d45
Please let me know if anything else is needed for this PR.
From what I can tell, there is currently no Clippy "Dependabot,"
That's because we also have to look out for the Rust repo a bit to not duplicate deps.
To prevent duplicate deps, I suggest to keep cargo_metadata at 0.12 for now and do a coordinated update with Miri for compiletest_rs.
To prevent duplicate deps, I suggest to keep
cargo_metadataat0.12for now
This means that semver will have to be held back as well.
Also, PathBuf does not implement AsRef<str>, but it does implement AsRef<OsStr> (as does Utf8PathBuf). So I changed the cargo_common_metadata lint to use AsRef<OsStr> instead of AsRef<str>. This way, when cargo_metadata is eventually upgraded to 0.14, the cargo_common_metadata lint should compile without changes. (I tested this in the clippy repo and it seemed to work.)
Also, I interpreted your comment @flip1995 to mean compiletest_rs should not be downgraded. Please let me know if this is not correct.
Some NITs I noticed.
To do the compiletest-rs update with MIRI, I suggest, that you open a PR against MIRI updating that dep. Once that gets merged you can do a submodule update for MIRI here in the Rust repo. To that submodule update, you can attach a commit updating the Clippy deps. In the last commit you can do the Cargo.lock update with cargo update -p miri -p clippy.
Outdated
# begin automatic update
clippy_lints = { version = "0.1.50", path = "clippy_lints" }
clippy_lints = { version = "0.1", path = "clippy_lints" }
# end automatic update
Can you remove those comments, while you're at it. This is definitely not automatically updated 
src/tools/clippy/clippy_dummy/Cargo.toml
Outdated Show resolved
src/tools/clippy/clippy_utils/Cargo.toml
Outdated Show resolved
To do the compiletest-rs update with MIRI, I suggest, that you open a PR against MIRI updating that dep. Once that gets merged you can do a submodule update for MIRI here in the Rust repo. To that submodule update, you can attach a commit updating the Clippy deps. In the last commit you can do the
Cargo.lockupdate withcargo update -p miri -p clippy.
I think this is done now.
Regex update can be perf sensitive, plus 7 commits kinda a lot, squash a little.
Regex update can be perf sensitive, plus 7 commits kinda a lot, squash a little.
How many do you think would be appropriate?
It looks like a few of the commits amend or reverse parts of the first commit so those can be squashed.
Better?
Should I now request a review from a rust maintainer?
I can't unresolve conversations here and also can't comment again on the same line where a resolved comment already is.... So here are links to the two comments I still have:
Apologies. I think it's fixed now.
Thanks! Waiting for CI to pass.
@bors r+ rollup=iffy (Cargo.lock update)
Commit bd4b17a has been approved by flip1995
Testing commit 
Test failed - checks-actions
I'm afraid the error doesn't mean much to me.
@bors retry
Windows image update has broken things.
Test successful - checks-actions
Approved by: flip1995
Pushing c6f32f3 to master...
Finished benchmarking commit (c6f32f3): comparison url.
Summary: This benchmark run did not return any relevant changes.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
@rustbot label: -perf-regression
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
No one assigned
None yet
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK