Repository navigation
Support supplied Clippy source repositories - #2132
EricLiu2000 wants to merge 5 commits into
Conversation
|
|
|
The Linux cargo-dylint job failed in The corresponding macOS job and the separate Linux cargo-dylint-ci job passed. Could a maintainer rerun the failed job? This remains an unresolved CI failure until the rerun or investigation is complete. |
12beada to
7884914
Compare
EricLiu2000
changed the title
c5bdd31 to
3377f4b
Compare
smoelius
left a comment
smoelius
left a comment
There was a problem hiding this comment.
@EricLiu2000 A left a few comments below, including a couple of questions. This is not a complete review; I will need to look at this again.
Also, I noticed a couple of changes that appear unnecessary. If there are others that I have not yet noticed, please remove them.
| const I_DAY: usize = 2; | ||
|
|
||
| pub fn build() -> Result<()> { | ||
| println!("cargo:rerun-if-env-changed={}", env::DYLINT_CLIPPY_REPO); |
There was a problem hiding this comment.
| println!("cargo:rerun-if-env-changed={}", env::DYLINT_CLIPPY_REPO); |
I don't think this should be necessary. We should be able to consider any Clippy clone as good as another. Granted, they may differ in how current they are and what is checked out, but I don't think the repo's path is a good indication of that.
There was a problem hiding this comment.
Different clones can be checked out to different commits, as you mention. I'd guess that changing the clone used is not going to be a very common operation at all. In that case, keeping this rerun-if-env-changed is a cheap safety guard that only costs anything in this uncommon scenario. Could we keep it?
There was a problem hiding this comment.
Like in my response to #2132 (comment), I worry this will cause confusion. I would prefer that it be removed.
|
|
||
| pub fn build() -> Result<()> { | ||
| println!("cargo:rerun-if-env-changed={}", env::DYLINT_CLIPPY_REPO); | ||
| println!("cargo:rerun-if-env-changed={}", env::RUSTUP_TOOLCHAIN); |
There was a problem hiding this comment.
Can you tell me what your reasoning is for declaring RUSTUP_TOOLCHAIN rerun-if-env-changed?
There was a problem hiding this comment.
The script uses the toolchain name for revision selection and compatibility checks. Cargo already handles compiler changes, but different names can refer to the same compiler. This is arguably a really niche edge case, but similar to the above - if it's a really niche edge case, then this is just a cheap safety guard.
There was a problem hiding this comment.
different names can refer to the same compiler
That's true, but that would cause additional recompilations; it wouldn't prevent necessary recompilations.
I think this is unnecessary and likely to cause confusion. Please remove it.
48093bf to
272df98
Compare
smoelius
left a comment
smoelius
left a comment
There was a problem hiding this comment.
@EricLiu2000 I still have to review the tests. But I would like to please do that after you have had a chance to consider these comments.
| const I_DAY: usize = 2; | ||
|
|
||
| pub fn build() -> Result<()> { | ||
| println!("cargo:rerun-if-env-changed={}", env::DYLINT_CLIPPY_REPO); |
There was a problem hiding this comment.
Like in my response to #2132 (comment), I worry this will cause confusion. I would prefer that it be removed.
|
|
||
| pub fn build() -> Result<()> { | ||
| println!("cargo:rerun-if-env-changed={}", env::DYLINT_CLIPPY_REPO); | ||
| println!("cargo:rerun-if-env-changed={}", env::RUSTUP_TOOLCHAIN); |
There was a problem hiding this comment.
different names can refer to the same compiler
That's true, but that would cause additional recompilations; it wouldn't prevent necessary recompilations.
I think this is unnecessary and likely to cause confusion. Please remove it.
| write(&path_buf, "const EXTRA_SYMBOLS: &[&str] = &[];") | ||
| .with_context(|| format!("`write` failed for `{}`", path_buf.display()))?; | ||
| return Ok(()); | ||
| let sym_rs = if let Some(path) = clippy_repository_path()? { |
There was a problem hiding this comment.
I think this check is unnecessary. If it fails (i.e., DYLINT_CLIPPY_REPO is unset), the check is performed again in clippy_utils_sym_rs via its call to clippy_repository.
It should be possible to keep the old control flow and check DYLINT_CLIPPY_REPO in just clippy_repository.
So, unless you see a mistake in what I've said, please use the old control flow and remove supplied_clippy_sym_rs.
5c2b3d9 to
94ef57e
Compare
Fixes #2131.
Add
DYLINT_CLIPPY_REPOso Dylint can use a supplied local Clippy Git repository instead of cloning from GitHub. Operations use a temporary local clone, with existing revision selection and driver symbol generation unchanged. The supplied repository must contain the history needed by the existing lookup.Verification: internal tests, formatting, and Clippy pass with network access denied. A local integration with the override backported to Dylint 6.0.1 generates the exact symbol table from its matching
clippy_utilsand passes the existing lint checks using unmodified driver sources. This does not test a complete upgrade to 6.2.0.