Open
Conversation
2bndy5
added
enhancement
2bndy5
force-pushed
the
clang-tools-downloader
branch
12 times, most recently
from
89de56d to
f005df3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
In order of precedence, this patch allows installing clang tools from 1. PyPI (downloaded wheels are verified using SHA256 or Bake2b256) 2. system package manager(s) (if available) 3. static binaries (verified with accompanied SHA512 checksum) Also upgrade cargo/rust dependencies.
disables any API that is specific to building a binary. Thus CLI-specific API is not exposed in the library API. Also reviewed docs
also check cache before (re)populating it
2bndy5
force-pushed
the
clang-tools-downloader
branch
from
f005df3 to
6b5a77b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
- Coverage 96.90% 94.45% -2.46%
==========================================
Files 14 26 +12
Lines 3167 4201 +1034
==========================================
+ Hits 3069 3968 +899
- Misses 98 233 +135 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2bndy5
force-pushed
the
clang-tools-downloader
branch
from
3c9fa8a to
068e76c
Compare
and conditionally use `sudo`
2bndy5
force-pushed
the
clang-tools-downloader
branch
from
068e76c to
a84fa70
Compare
Contributor
WalkthroughIntroduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp-linter/src/rest_api/mod.rs (1)
146-165:⚠️ Potential issue | 🔴 CriticalBug: Version fields are swapped between format and tidy comments.
The code passes the wrong version to each comment function:
- Line 152:
make_format_commentreceivestidy_versionbut should receiveformat_version- Line 162:
make_tidy_commentreceivesformat_versionbut should receivetidy_versionThe inline comments also have it backwards, confirming this is a copy-paste error. This will cause the generated PR comments to display incorrect tool versions.
🐛 Proposed fix
if format_checks_failed > 0 { make_format_comment( files, &mut comment, format_checks_failed, - // tidy_version should be `Some()` value at this point. - &clang_versions.tidy_version.as_ref().unwrap().to_string(), + // format_version should be `Some()` value at this point. + &clang_versions.format_version.as_ref().unwrap().to_string(), &mut remaining_length, ); } if tidy_checks_failed > 0 { make_tidy_comment( files, &mut comment, tidy_checks_failed, - // format_version should be `Some()` value at this point. - &clang_versions.format_version.as_ref().unwrap().to_string(), + // tidy_version should be `Some()` value at this point. + &clang_versions.tidy_version.as_ref().unwrap().to_string(), &mut remaining_length, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp-linter/src/rest_api/mod.rs` around lines 146 - 165, The make_format_comment and make_tidy_comment calls are passing the swapped version fields; change the arguments so make_format_comment receives clang_versions.format_version.as_ref().unwrap().to_string() and make_tidy_comment receives clang_versions.tidy_version.as_ref().unwrap().to_string(), keeping the other parameters (files, &mut comment, *_checks_failed, &mut remaining_length) unchanged so the correct tool versions are shown.cpp-linter/src/cli/structs.rs (1)
246-289:⚠️ Potential issue | 🔴 CriticalGate the remaining Clap-based tests behind
bin.
display_lines_changed_only_enum()anddisplay_thread_comments_enum()callValueEnum::from_str(), but those trait implementations only exist under#[cfg(feature = "bin")]. The unconditionalCliimport on line 251 has the same problem—Cliis only defined when the feature is enabled. Runningcargo testwithout--features binwill fail to compile.Suggested fix
#[cfg(test)] mod test { #[cfg(feature = "bin")] use clap::{Parser, ValueEnum}; - use super::{Cli, LinesChangedOnly, ThreadComments}; + #[cfg(feature = "bin")] + use super::Cli; + use super::{LinesChangedOnly, ThreadComments}; #[test] #[cfg(feature = "bin")] fn parse_positional() { let cli = Cli::parse_from(["cpp-linter", "file1.c", "file2.h"]); @@ } #[test] + #[cfg(feature = "bin")] fn display_lines_changed_only_enum() { let input = "Diff"; assert_eq!( LinesChangedOnly::from_str(&input, true).unwrap(), LinesChangedOnly::Diff @@ } #[test] + #[cfg(feature = "bin")] fn display_thread_comments_enum() { let input = "Update"; assert_eq!( ThreadComments::from_str(input, true).unwrap(), ThreadComments::Update🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp-linter/src/cli/structs.rs` around lines 246 - 289, The failing compile is caused by using Clap-only symbols when the "bin" feature is disabled; wrap the Clap-dependent import and tests with the feature gate: move or annotate use super::{Cli, LinesChangedOnly, ThreadComments} so that Cli is only imported under #[cfg(feature = "bin")] and add #[cfg(feature = "bin")] to the parse_positional, display_lines_changed_only_enum, and display_thread_comments_enum tests (or alternatively gate the entire test module with #[cfg(feature = "bin")]) so that LinesChangedOnly::from_str and ThreadComments::from_str are only referenced when the "bin" feature is enabled.
🟡 Minor comments (2)
cpp-linter/src/logger.rs-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorAdd feature gate to logger module export in lib.rs.
The logger module is gated with
#![cfg(feature = "bin")]in its own file, but line 14 of lib.rs exports it unconditionally withpub mod logger;. This will cause compilation failure when thebinfeature is disabled. The export must be gated:#![cfg(feature = "bin")] pub mod logger;.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp-linter/src/logger.rs` at line 2, The lib export for the logger module is unconditional while the module file itself is gated with #![cfg(feature = "bin")]; update the module export so it is also feature-gated by changing the `pub mod logger;` export to be annotated with the same cfg (i.e., add `#[cfg(feature = "bin")]` before the `pub mod logger;`) so the compiler only exports the logger when the `bin` feature is enabled..github/workflows/run-dev-tests.yml-107-117 (1)
107-117:⚠️ Potential issue | 🟡 MinorPrevent redundant
eval_versiontest execution.The
warmupprofile runseval_versiontests, but theciprofile (used for all non-max versions) also includes them via its default-filter:default-filter = "kind(test) + test(#*use_extra_args) + test(#*eval_version)"This causes
eval_versiontests to run twice per clang version (except the max version). Update theciprofile filter to excludeeval_versiontests:default-filter = "kind(test) + test(#*use_extra_args) - test(#*eval_version)"This aligns with the
allprofile which intentionally excludes them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/run-dev-tests.yml around lines 107 - 117, The CI is running eval_version tests twice because the "ci" profile's default-filter still includes eval_version; update the "ci" profile's default-filter (the default-filter setting for the ci profile) to exclude eval_version tests by replacing the inclusion of test(#*eval_version) with an exclusion (- test(#*eval_version)), matching how the "all" profile intentionally excludes them so warmup + ci do not duplicate eval_version runs.
🧹 Nitpick comments (7)
clang-installer/README.md (1)
5-7: Tighten wording for API scope clarity.This is accurate, but the sentence reads a bit awkwardly. Consider: “This utility is used by the cpp-linter CLI, and its API is internal to the
cpp-lintercrate.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang-installer/README.md` around lines 5 - 7, Replace the awkward sentence "This is a utility for cpp-linter CLI, thus its API is considered internal to cpp-linter crate." with a tighter phrasing such as: "This utility is used by the cpp-linter CLI, and its API is internal to the `cpp-linter` crate." Update the README.md sentence to match that suggested wording for clarity and grammar.clang-installer/src/tool.rs (1)
185-191: Symlink validation only confirms readability, not target validity.
fs::read_link()verifies the symlink itself is readable but doesn't confirm the target file exists or is executable. If the target was deleted, this would still succeed but the symlink would be broken.Consider adding
symlink.exists()check or usingfs::metadata(symlink_path)to verify the final target is accessible.💡 Optional enhancement to validate symlink target
// check if the symlink is valid and points to an existing file. let symlink = fs::read_link(symlink_path)?; + if !symlink_path.exists() { + return Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("Symlink target does not exist: {}", symlink.to_string_lossy()), + )); + } log::info!(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang-installer/src/tool.rs` around lines 185 - 191, The current code uses fs::read_link(symlink_path) and logs the link but doesn't verify the link's target exists or is accessible; update the logic around symlink_path and the symlink variable to resolve the final target (e.g., join symlink_path.parent() with symlink) and then call fs::metadata or Path::exists on that resolved target to ensure the file exists (and optionally is executable) before proceeding; keep the existing log but add an error path that handles broken symlinks when fs::metadata(target) fails.clang-installer/Cargo.toml (1)
21-38: Consider documenting why crypto crates are pinned locally.
blake2(0.10.6) andsha2(0.10.9) are pinned directly rather than viaworkspace.dependencies. This is reasonable for cryptographic stability, but a brief comment explaining the rationale would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang-installer/Cargo.toml` around lines 21 - 38, Add an inline comment in Cargo.toml next to the blake2 = "0.10.6" and sha2 = "0.10.9" dependency entries explaining that these crypto crates are intentionally pinned locally (not in workspace.dependencies) for cryptographic stability/security and to avoid accidental upgrades that could change hashing outputs or break binary compatibility; reference the blake2 and sha2 lines so maintainers understand the deliberate pinning and know not to move them to workspace.dependencies without review.clang-installer/src/downloader/caching.rs (1)
29-43: Consider:StaticDistDownloaderbypassesis_cache_valid.Based on
clang-installer/src/downloader/static_dist.rs(lines 145, 159),StaticDistDownloaderdirectly checks.exists()instead of usingis_cache_valid. While this is functionally equivalent whenmax_age = None, it creates an inconsistent pattern. If cache eviction policies change in the future,StaticDistDownloaderwon't benefit.This is optional since the current behavior is correct—just noting for future maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang-installer/src/downloader/caching.rs` around lines 29 - 43, StaticDistDownloader currently bypasses the shared cache validation logic by directly calling .exists() on the cache file; update StaticDistDownloader (the code in StaticDistDownloader::... where it checks cache_file.exists()) to call the shared helper is_cache_valid(cache_file, max_age) instead so it honors the same max_age/eviction semantics as is_cache_valid; ensure you pass the same max_age used elsewhere and remove the direct .exists() check so future cache policy changes apply consistently..github/workflows/run-dev-tests.yml (1)
34-34: Minor: Inconsistent quoting in matrix.
"windows-latest"is quoted whileubuntu-latestis not. While YAML handles both, consistent quoting improves readability.- os: ["windows-latest", ubuntu-latest] + os: [windows-latest, ubuntu-latest]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/run-dev-tests.yml at line 34, The matrix OS entries under the "os" key are inconsistently quoted; update the matrix so both entries use the same quoting style (either both quoted or both unquoted) for consistency and readability—locate the "os" matrix line in the workflow (os: ["windows-latest", ubuntu-latest]) and make the two entries uniform, e.g., change ubuntu-latest to "ubuntu-latest" to match "windows-latest".clang-installer/tests/pypi.rs (1)
45-48: Consider extracting assertions into the test function.The
setup()function performs both setup and assertions, making it harder to understand test structure. The test functiondownload_clang_formatjust callssetup().awaitwith no additional assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang-installer/tests/pypi.rs` around lines 45 - 48, The test currently hides assertions inside setup(), so update the code so setup() only performs initialization and move the assertions into the test function download_clang_format; locate the async fn setup() and remove any assert!/assert_eq!/expect calls from it, then call setup().await inside download_clang_format and add the relevant assertions there (or split setup into setup() and verify_* helpers if preferred) so the test function explicitly contains the verification logic.clang-installer/src/main.rs (1)
120-122: Validate--directoryin the CLI layer.Passing an existing file path here will currently fail later in the downloader with a generic IO error. A custom clap value parser that accepts missing paths and existing directories, but rejects existing files, would keep this concern in the CLI.
Based on learnings: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang-installer/src/main.rs` around lines 120 - 122, Add a clap value parser for the `directory` argument that accepts non-existent paths and existing directories but rejects existing files; implement a function like `validate_install_dir` (signature returning Result<PathBuf, String> or implementing TypedValueParser) which converts the input to a PathBuf, returns Ok for paths that don't exist or that are directories, and returns Err with a clear message when the path exists and is a file, then attach it to the `pub directory: Option<PathBuf>` arg via the `#[arg(value_parser = validate_install_dir)]` attribute so validation happens in the CLI layer instead of later in downloader/tool lookup code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/node/src/lib.rs`:
- Around line 6-13: The exported N-API function main was made synchronous (pub
fn main) but the TypeScript declaration and tests still expect a Promise and a
numeric return, and the tokio runtime build uses .unwrap() which can panic; fix
by either reverting main to an async function (pub async fn main) so napi-rs
will return a Promise and preserve the existing index.d.ts/test expectations, or
if you intend main to be synchronous keep pub fn main but regenerate
bindings/node/index.d.ts to remove the Promise and adjust the test to expect the
synchronous void/number contract; in both cases replace the .unwrap() on
tokio::runtime::Builder::new_multi_thread().build() with proper error handling
(map the build error into Error::new(Status::GenericFailure, ... ) or propagate
it) and ensure run_main(args) is invoked consistently (await when async,
block_on when keeping sync) so the Rust signature, JS declaration, and tests are
aligned.
In `@bindings/python/Cargo.toml`:
- Line 20: The pyo3 dependency declaration currently sets features =
["abi3-py39"] which overrides workspace defaults and drops the required
extension-module feature; update the pyo3 entry in bindings/python/Cargo.toml so
its features include both "abi3-py39" and "extension-module" (i.e., add
"extension-module" alongside the existing "abi3-py39") to ensure the
extension-module workspace feature is not lost.
In `@Cargo.toml`:
- Line 31: The workspace currently builds reqwest without any TLS backend but
publicly exports the rest_api module which requires reqwest::Client for HTTPS;
fix this by adding an explicit library TLS feature and gating rest_api behind
it: add a new feature (e.g., "tls") in Cargo.toml that enables reqwest with
native-tls (adjust the reqwest dependency entry to include feature =
["native-tls", "http2", "charset"] under that feature), remove implicit reliance
on the existing "bin" feature for TLS, and wrap the public export of the
rest_api module (e.g., the pub mod rest_api declaration in lib.rs) with
#[cfg(feature = "tls")] so consumers must opt in to TLS support; alternatively,
if you prefer TLS always available, make the library default feature include
"tls" instead of tying it to "bin".
In `@clang-installer/src/downloader/hashing.rs`:
- Around line 37-42: The read loops currently use "while let Ok(bytes_read) =
file_reader.read(&mut buf)" which swallows I/O errors and leads to partial-hash
finalization; change each loop (the three branches using file_reader.read +
hasher.update + hasher.finalize) to explicitly propagate read errors instead of
silently breaking — e.g. replace the while-let with a loop that calls
file_reader.read(&mut buf) and uses the ? operator (or maps Err to
DownloadError::IoError) to return on error, breaking only on bytes_read == 0,
then call hasher.update for the bytes read; apply this change to all three
branches that compute the hash so real I/O errors are returned rather than
producing HashMismatch.
In `@clang-installer/src/downloader/mod.rs`:
- Around line 42-49: The code currently opens and truncates cache_path
(cache_file) before the download is verified, risking partial/corrupt cache
entries; change the logic to create and write to a temporary file in the same
directory (using cache_parent and a unique temp name, e.g.,
cache_path.with_extension(".partial") or using a random suffix), write the
downloaded body into that temp file (use OpenOptions as before), flush/fsync the
temp file and its parent directory, then atomically rename (std::fs::rename) the
temp file to cache_path only on successful completion; ensure error paths remove
the temp file and do not touch cache_path. Use the existing cache_parent,
cache_path, and cache_file identifiers to locate the change.
In `@clang-installer/src/downloader/native_packages/mod.rs`:
- Around line 90-97: The code currently trusts
mgr.is_installed_package(&pkg_name, Some(min_version)) and returns the found
ClangVersion immediately; instead, after calling tool.get_exe_path(...) and
tool.capture_version(...) you must validate that the captured version satisfies
the original RequestedVersion/version_req (not just min_version) before
returning. In practice, in the block containing is_installed_package, call
capture_version, check the captured version against version_req (or the
exact/upper-bound requirement represented by RequestedVersion::Requirement), and
only construct and return ClangVersion when that check passes; if it does not,
log a mismatch (including tool, mgr, version and version_req) and continue so
the installer can try other resolution paths.
In `@clang-installer/src/downloader/native_packages/unix.rs`:
- Around line 115-139: The package install logic incorrectly runs Homebrew under
sudo and uses pacman's -y flag; update the UnixPackageManager match so PacMan
uses ["-S", "--noconfirm"] instead of ["-S", "-y"], and change the Command
creation so Homebrew is never prefixed with sudo (i.e., if self ==
UnixPackageManager::Homebrew call Command::new(self.as_str()) regardless of
Self::has_sudo()). Keep the existing has_sudo() behavior for other managers and
ensure package_id and args are passed the same way.
- Around line 176-179: The check for DNF packages in UnixPackageManager::Dnf
uses `dnf list <package>` which returns success for packages available in
repositories (not only installed); update the logic in the
is_installed_package() branch handling UnixPackageManager::Dnf to run `dnf list
installed <package>` (i.e., add the "installed" subcommand) and base the success
decision on that command's exit status so only packages present in the RPMDB are
considered installed.
In `@clang-installer/src/downloader/native_packages/windows.rs`:
- Around line 71-83: The package table parsing uses split(' ') which preserves
empty fields from padded columns and breaks lookups; in the Chocolatey parsing
change the iterator creation (the variable l produced by line.split(' ')) to use
split_whitespace() instead, and in the Winget parsing replace split(' ').skip(1)
with split_whitespace().skip(1) so tokenization ignores padding when matching
package_name and extracting version strings (refer to the iterator variable l
and the Version::parse call to verify correct token positions).
In `@clang-installer/src/downloader/pypi.rs`:
- Around line 423-448: The extracted binary is being cached/placed only by major
version (bin_name uses ver.major) while the wheel cache uses the full ver, which
can cause incorrect reuse; change the extracted path/name so it includes the
full version (use ver or ver.to_string() instead of ver.major) when building
bin_name and extracted_bin before calling Self::extract_bin(clang_tool,
&cached_wheel, &extracted_bin) so each full-version wheel produces a distinct
extracted executable; if you still want a major-version alias (e.g.,
clang-format-17) create or update that separate symlink/alias after extracting
the fully-versioned binary rather than replacing the extracted file.
- Around line 435-438: The code currently skips integrity checks when
info.digests.first() is None; change the logic in the function that handles PyPI
metadata (the block using info.digests.first() and digest.verify(&cached_wheel))
to fail closed: if info.digests.first() returns None, return an error (or
propagate a VerificationMissing error) instead of proceeding, and only call
digest.verify(&cached_wheel) when a digest is present; update any surrounding
error types/context to provide a clear message that no supported digest was
present for the wheel.
- Around line 171-189: The macOS branch in is_compatible_with_system currently
requires exact arch string matches and thus rejects universal2 wheels; update
the macos handling in is_compatible_with_system so that when
std::env::consts::OS == "macos" it treats self.arch == "universal2" as
compatible for both sys_arch "x86_64" and "aarch64" (in addition to the existing
exact matches), i.e., accept universal2 for both Intel and Apple Silicon while
preserving the existing exact checks for "x86_64" and "arm64".
In `@clang-installer/src/progress_bar.rs`:
- Line 1: The progress bar checks terminal status using stdin rather than
stdout, so carriage-return redraws can be written to a redirected stdout; update
the terminal check to use stdout().is_terminal() everywhere it's used (e.g.,
where is_interactive is computed and the checks around lines 51-52) so the
progress bar only enables interactive redraws when stdout is a terminal; adjust
any imports/usages from stdin to stdout accordingly and ensure Write/Result
types still compile.
In `@clang-installer/src/version.rs`:
- Around line 301-304: Save the previous CPP_LINTER_CACHE environment value with
std::env::var_os before calling TempDir::new(), set CPP_LINTER_CACHE to
tmp_cache_dir.path() via std::env::set_var, and ensure you restore the original
value (with std::env::set_var if Some or std::env::remove_var if None) in a
drop/guard placed before tmp_cache_dir is dropped so the process-global state is
restored during test teardown; reference the TempDir::new(), std::env::set_var,
and std::env::var_os/remove_var calls to locate where to add the
save-and-restore guard.
- Around line 331-347: The test function eval_version currently runs by default
and can perform global installs; change it to be gated behind an explicit opt-in
by either marking it with #[ignore] or checking a CI-only env var (e.g.,
CLANG_INSTALL_TEST) at the start of eval_version and early-return when not set.
Locate the async test function named eval_version (and the use of
RequestedVersion::Requirement.eval_tool) and add the guard so the potentially
dangerous installation code only executes when the env flag is present or when
the test is run with --ignored in CI.
- Around line 137-168: After a successful download (from
PyPiDownloader::download_tool or StaticDistDownloader::download_tool) capture
the actual version via tool.capture_version(&bin) and verify it satisfies the
original version_req before creating the symlink and returning ClangVersion; if
the captured version does not meet version_req, do not symlink — either attempt
the next fallback (e.g., continue to other download methods or
try_install_package) or return Err(GetToolError::NotFound). Update the logic
around where bin is obtained and before tool.symlink_bin(&bin, &symlink_path,
overwrite_symlink) to perform this check and handle failures accordingly.
In `@clang-installer/tests/pypi.rs`:
- Around line 13-17: The test is unsafely modifying process-global state via
env::set_var("CPP_LINTER_CACHE", ...) which races when tests run in parallel;
instead, remove the unsafe env::set_var usage in tests/pypi.rs (where
tmp_cache_dir is created in the setup()/test) and either (A) change the
downloader API call in the test to accept tmp_cache_dir.path() explicitly (pass
the cache dir into the function that downloads/pulls from PyPI rather than
relying on CPP_LINTER_CACHE), or (B) mark the affected tests to run serially via
nextest test-group config; reference the tmp_cache_dir variable, the
env::set_var call, and the setup()/downloader invocation to locate the change.
In `@clang-installer/tests/static_dist.rs`:
- Around line 12-16: The tests download_clang_format_17 and
download_clang_format_12_0_1 race by mutating the global env var
CPP_LINTER_CACHE via env::set_var around TempDir::new; fix by applying the same
solution used in pypi.rs — either group these tests under a serial test group
(so they don't run in parallel) or change the API to accept a cache directory
parameter and pass tmp_cache_dir.path() directly instead of setting
CPP_LINTER_CACHE; update the test(s) referencing TempDir::new and remove the
unsafe env::set_var usage, or wrap the tests in a serial test annotation to
ensure isolated execution.
In `@cpp-linter/src/run.rs`:
- Line 6: The feature-gate on run.rs prevents integration tests from importing
run::run_main; remove or change it so tests compile: either remove the
crate-level attribute #![cfg(feature = "bin")] from run.rs (so the module and
function run_main are always compiled), or keep the gate and add
required-features = ["bin"] to the test target in Cargo.toml for
tests/comments.rs, or refactor tests to import a non-gated public entrypoint
instead of run::run_main; ensure you reference the run.rs module and the
run_main symbol when applying the chosen fix.
---
Outside diff comments:
In `@cpp-linter/src/cli/structs.rs`:
- Around line 246-289: The failing compile is caused by using Clap-only symbols
when the "bin" feature is disabled; wrap the Clap-dependent import and tests
with the feature gate: move or annotate use super::{Cli, LinesChangedOnly,
ThreadComments} so that Cli is only imported under #[cfg(feature = "bin")] and
add #[cfg(feature = "bin")] to the parse_positional,
display_lines_changed_only_enum, and display_thread_comments_enum tests (or
alternatively gate the entire test module with #[cfg(feature = "bin")]) so that
LinesChangedOnly::from_str and ThreadComments::from_str are only referenced when
the "bin" feature is enabled.
In `@cpp-linter/src/rest_api/mod.rs`:
- Around line 146-165: The make_format_comment and make_tidy_comment calls are
passing the swapped version fields; change the arguments so make_format_comment
receives clang_versions.format_version.as_ref().unwrap().to_string() and
make_tidy_comment receives
clang_versions.tidy_version.as_ref().unwrap().to_string(), keeping the other
parameters (files, &mut comment, *_checks_failed, &mut remaining_length)
unchanged so the correct tool versions are shown.
---
Minor comments:
In @.github/workflows/run-dev-tests.yml:
- Around line 107-117: The CI is running eval_version tests twice because the
"ci" profile's default-filter still includes eval_version; update the "ci"
profile's default-filter (the default-filter setting for the ci profile) to
exclude eval_version tests by replacing the inclusion of test(#*eval_version)
with an exclusion (- test(#*eval_version)), matching how the "all" profile
intentionally excludes them so warmup + ci do not duplicate eval_version runs.
In `@cpp-linter/src/logger.rs`:
- Line 2: The lib export for the logger module is unconditional while the module
file itself is gated with #![cfg(feature = "bin")]; update the module export so
it is also feature-gated by changing the `pub mod logger;` export to be
annotated with the same cfg (i.e., add `#[cfg(feature = "bin")]` before the `pub
mod logger;`) so the compiler only exports the logger when the `bin` feature is
enabled.
---
Nitpick comments:
In @.github/workflows/run-dev-tests.yml:
- Line 34: The matrix OS entries under the "os" key are inconsistently quoted;
update the matrix so both entries use the same quoting style (either both quoted
or both unquoted) for consistency and readability—locate the "os" matrix line in
the workflow (os: ["windows-latest", ubuntu-latest]) and make the two entries
uniform, e.g., change ubuntu-latest to "ubuntu-latest" to match
"windows-latest".
In `@clang-installer/Cargo.toml`:
- Around line 21-38: Add an inline comment in Cargo.toml next to the blake2 =
"0.10.6" and sha2 = "0.10.9" dependency entries explaining that these crypto
crates are intentionally pinned locally (not in workspace.dependencies) for
cryptographic stability/security and to avoid accidental upgrades that could
change hashing outputs or break binary compatibility; reference the blake2 and
sha2 lines so maintainers understand the deliberate pinning and know not to move
them to workspace.dependencies without review.
In `@clang-installer/README.md`:
- Around line 5-7: Replace the awkward sentence "This is a utility for
cpp-linter CLI, thus its API is considered internal to cpp-linter crate." with a
tighter phrasing such as: "This utility is used by the cpp-linter CLI, and its
API is internal to the `cpp-linter` crate." Update the README.md sentence to
match that suggested wording for clarity and grammar.
In `@clang-installer/src/downloader/caching.rs`:
- Around line 29-43: StaticDistDownloader currently bypasses the shared cache
validation logic by directly calling .exists() on the cache file; update
StaticDistDownloader (the code in StaticDistDownloader::... where it checks
cache_file.exists()) to call the shared helper is_cache_valid(cache_file,
max_age) instead so it honors the same max_age/eviction semantics as
is_cache_valid; ensure you pass the same max_age used elsewhere and remove the
direct .exists() check so future cache policy changes apply consistently.
In `@clang-installer/src/main.rs`:
- Around line 120-122: Add a clap value parser for the `directory` argument that
accepts non-existent paths and existing directories but rejects existing files;
implement a function like `validate_install_dir` (signature returning
Result<PathBuf, String> or implementing TypedValueParser) which converts the
input to a PathBuf, returns Ok for paths that don't exist or that are
directories, and returns Err with a clear message when the path exists and is a
file, then attach it to the `pub directory: Option<PathBuf>` arg via the
`#[arg(value_parser = validate_install_dir)]` attribute so validation happens in
the CLI layer instead of later in downloader/tool lookup code.
In `@clang-installer/src/tool.rs`:
- Around line 185-191: The current code uses fs::read_link(symlink_path) and
logs the link but doesn't verify the link's target exists or is accessible;
update the logic around symlink_path and the symlink variable to resolve the
final target (e.g., join symlink_path.parent() with symlink) and then call
fs::metadata or Path::exists on that resolved target to ensure the file exists
(and optionally is executable) before proceeding; keep the existing log but add
an error path that handles broken symlinks when fs::metadata(target) fails.
In `@clang-installer/tests/pypi.rs`:
- Around line 45-48: The test currently hides assertions inside setup(), so
update the code so setup() only performs initialization and move the assertions
into the test function download_clang_format; locate the async fn setup() and
remove any assert!/assert_eq!/expect calls from it, then call setup().await
inside download_clang_format and add the relevant assertions there (or split
setup into setup() and verify_* helpers if preferred) so the test function
explicitly contains the verification logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2faad7e-a64b-42f1-97e3-435fd6ff482f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
.config/nextest.toml.github/workflows/benchmark.yml.github/workflows/binary-builds.yml.github/workflows/run-dev-tests.yml.pre-commit-config.yamlCargo.tomlbindings/node/Cargo.tomlbindings/node/src/lib.rsbindings/python/Cargo.tomlclang-installer/Cargo.tomlclang-installer/README.mdclang-installer/src/downloader/caching.rsclang-installer/src/downloader/hashing.rsclang-installer/src/downloader/mod.rsclang-installer/src/downloader/native_packages/mod.rsclang-installer/src/downloader/native_packages/unix.rsclang-installer/src/downloader/native_packages/windows.rsclang-installer/src/downloader/pypi.rsclang-installer/src/downloader/static_dist.rsclang-installer/src/lib.rsclang-installer/src/main.rsclang-installer/src/progress_bar.rsclang-installer/src/tool.rsclang-installer/src/utils.rsclang-installer/src/version.rsclang-installer/tests/common.rsclang-installer/tests/pypi.rsclang-installer/tests/pypi_clang-format.jsonclang-installer/tests/static_dist.rscpp-linter/Cargo.tomlcpp-linter/README.mdcpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rscpp-linter/src/cli/mod.rscpp-linter/src/cli/structs.rscpp-linter/src/common_fs/file_filter.rscpp-linter/src/common_fs/mod.rscpp-linter/src/logger.rscpp-linter/src/rest_api/github/mod.rscpp-linter/src/rest_api/mod.rscpp-linter/src/run.rscpp-linter/tests/comments.rscpp-linter/tests/demo/.clang-tidycspell.config.ymldocs/Cargo.tomlnurfile
💤 Files with no reviewable changes (2)
- cpp-linter/tests/demo/.clang-tidy
- cpp-linter/README.md
Comment on lines
6
to
13
| #[napi] | ||
| pub async fn main(args: Vec<String>) -> Result<()> { | ||
| run_main(args) | ||
| .await | ||
| pub fn main(args: Vec<String>) -> Result<()> { | ||
| tokio::runtime::Builder::new_multi_thread() | ||
| .enable_all() | ||
| .build() | ||
| .unwrap() | ||
| .block_on(run_main(args)) | ||
| .map_err(|e| Error::new(Status::GenericFailure, e.to_string())) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check current index.d.ts declaration and test expectations
echo "=== TypeScript declaration ==="
cat bindings/node/index.d.ts
echo -e "\n=== Test expectations ==="
cat bindings/node/__test__/index.spec.mjs
echo -e "\n=== CLI usage ==="
cat bindings/node/cli.jsRepository: cpp-linter/cpp-linter-rs
Length of output: 562
🏁 Script executed:
cat -n bindings/node/src/lib.rs | head -20Repository: cpp-linter/cpp-linter-rs
Length of output: 545
🌐 Web query:
NAPI-RS sync function Result return type Promise JavaScript binding
💡 Result:
In NAPI-RS, for synchronous functions marked with #[napi], returning Result<T, E> (where T: ToNapiValue) does not produce a JavaScript Promise. Instead, Result<T, E> implements ToNapiValue, converting directly to a JavaScript value: Ok(T) becomes the JS value of T, and Err(E) (typically napi::Error) throws a JavaScript Error exception automatically when returned from the function. This is synchronous behavior—no Promise is returned or involved. Synchronous #[napi] functions always return plain synchronous JS values (or throw synchronously on error). To return a Promise from JS, use async fn (which auto-wraps in Promise), AsyncTask, ts_return_type = "Promise<...>" on sync fns with spawning, or explicitly return Promise/PromiseRaw types. Examples from docs: - #[napi] pub fn sum(a: u32, b: u32) -> u32 → JS: sum(a: number, b: number): number (sync) - #[napi] pub fn call_function(callback: Function<u32, u32>) -> Result → JS sync return number, throws on Err - No Result in supported sync return types table; it's handled via exception throwing. Result is not listed as a direct return type in the function docs tables because errors throw rather than returning a value.
Citations:
- 1: https://napi.rs/docs/concepts/function
- 2: https://napi.rs/en/docs/concepts/function.en
- 3: https://docs.rs/napi/latest/napi/type.Result.html
- 4: https://napi.rs/docs/concepts/values
- 5: https://docs.rs/napi/latest/napi/bindgen_prelude/trait.ToNapiValue.html
- 6: https://docs.rs/napi/latest/napi/struct.Error.html
Breaking change: Synchronous function but TypeScript declaration still expects Promise.
The function is now synchronous (pub fn, not pub async fn), but bindings/node/index.d.ts (auto-generated) still declares it as returning Promise<void>. NAPI-RS will not wrap a synchronous Result<()> in a Promise—it returns synchronously and throws errors directly.
This breaks:
- The test in
bindings/node/__test__/index.spec.mjswhich usesawait main(...)and expects a return value of0(but the function returns()) - TypeScript consumers expecting a Promise
- The contract between Rust and JavaScript
Additionally, line 11 uses .unwrap() on runtime construction, which will panic without context if it fails.
Required: Either revert the function to pub async fn (simpler) or keep it synchronous and regenerate the TypeScript declaration to reflect the non-Promise return type. The unwrap should also be replaced with proper error handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/node/src/lib.rs` around lines 6 - 13, The exported N-API function
main was made synchronous (pub fn main) but the TypeScript declaration and tests
still expect a Promise and a numeric return, and the tokio runtime build uses
.unwrap() which can panic; fix by either reverting main to an async function
(pub async fn main) so napi-rs will return a Promise and preserve the existing
index.d.ts/test expectations, or if you intend main to be synchronous keep pub
fn main but regenerate bindings/node/index.d.ts to remove the Promise and adjust
the test to expect the synchronous void/number contract; in both cases replace
the .unwrap() on tokio::runtime::Builder::new_multi_thread().build() with proper
error handling (map the build error into Error::new(Status::GenericFailure, ...
) or propagate it) and ensure run_main(args) is invoked consistently (await when
async, block_on when keeping sync) so the Rust signature, JS declaration, and
tests are aligned.
Collaborator
Author
There was a problem hiding this comment.
This was done for a reason. It had something to do with one of the newer types not being Send compatible. So, we need to keep this synchronous from the TS side and update the .unwrap() for better UX.
Contributor
There was a problem hiding this comment.
@2bndy5, understood — thanks for the context! Here's the fix for the .unwrap():
#[napi]
pub fn main(args: Vec<String>) -> Result<()> {
tokio::runtime::Builder::new_multi_thread()
.enable_all()
.build()
.map_err(|e| Error::new(Status::GenericFailure, e.to_string()))?
.block_on(run_main(args))
.map_err(|e| Error::new(Status::GenericFailure, e.to_string()))
}This maps the runtime build failure into a proper napi::Error and propagates it with ? instead of panicking.
Note: Since the function is now synchronous,
bindings/node/index.d.tsandbindings/node/__test__/index.spec.mjswill also need to be updated to remove thePromise<void>return type and theawaitusage respectively — if that hasn't been done already.
✏️ Learnings added
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: bindings/node/src/lib.rs:6-13
Timestamp: 2026-03-30T06:49:52.836Z
Learning: In `bindings/node/src/lib.rs`, the `#[napi] pub fn main` was intentionally changed from `pub async fn` to `pub fn` (synchronous) because a newer type used in the call chain is not `Send`-compatible, requiring a `block_on` approach with an explicit Tokio runtime. The TypeScript binding should reflect a synchronous (non-Promise) return type.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 50
File: .github/workflows/build-docs.yml:56-59
Timestamp: 2024-09-29T18:03:56.584Z
Learning: The `cli_gen` Rust library is used as a binding in the docs Python package.
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/downloader/pypi.rs:435-438
Timestamp: 2026-03-30T06:38:17.216Z
Learning: In `clang-installer/src/downloader/pypi.rs`, the PyPI downloader intentionally uses `info.digests.first()` with an `if let Some(digest)` guard (no fail-closed on missing digest). This is acceptable because the downloader exclusively targets the `clang-format` and `clang-tidy` PyPI packages, which reliably publish digests. Failing closed for a missing digest was explicitly declined by the maintainer (2bndy5).
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/version.rs:301-304
Timestamp: 2026-03-30T06:22:32.553Z
Learning: In this repo’s Rust tests (run via `cargo-nextest` configured by `.config/nextest.toml`), each test runs in its own process. Therefore, `unsafe { std::env::set_var(...) }` used inside tests is safe with respect to cross-test environment variable pollution, and you generally do not need to implement a save-and-restore guard for env vars in test code.
6 tasks
Collaborator
Author
|
Eeesh! Coderabbit came pretty close to wiping the floor with my laziness/negligence. I'll come back to this soon/tomorrow. I need to nurture my broken ego first. 🤣 New additional CLI
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
bindings/node/index.d.ts (1)
3-3:⚠️ Potential issue | 🟡 MinorPlease switch the Node binding test to a no-throw assertion.
Line 3 makes the public contract
void, butbindings/node/__test__/index.spec.mjsstill assertsawait main(...) === 0. That expectation is now stale.Based on learnings: In
bindings/node/src/lib.rs, the#[napi] pub fn mainwas intentionally changed frompub async fntopub fn(synchronous) because a newer type used in the call chain is notSend-compatible, requiring ablock_onapproach with an explicit Tokio runtime. The TypeScript binding should reflect a synchronous (non-Promise) return type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/node/index.d.ts` at line 3, The test expects a numeric return but the TypeScript binding now declares main(args: string[]) => void; update the test in bindings/node/__test__/index.spec.mjs to use a no-throw assertion instead of asserting await main(...) === 0 — call main([...]) synchronously (or without awaiting) and assert it does not throw (e.g., wrap in try/catch or use your test framework’s doesNotThrow/doesNotReject equivalent), keeping the declaration in bindings/node/index.d.ts as export declare function main(args: Array<string>): void.
🧹 Nitpick comments (2)
clang-installer/src/progress_bar.rs (1)
68-72: Add drop-time cleanup so early-return paths don’t leave the progress line unterminated.
finish()at Line 69 is manual. Callers that return early with?can skip it, leaving a partial progress line. A best-effortDropcleanup avoids noisy logs on error paths.Proposed refactor
pub struct ProgressBar { @@ prompt: String, + finished: bool, } @@ Self { @@ prompt: prompt.to_string(), + finished: false, } } @@ pub fn finish(&mut self) -> Result<()> { writeln!(&mut self.stdout_handle)?; // Move to the next line after finishing - self.stdout_handle.flush() + self.finished = true; + self.stdout_handle.flush() } } + +impl Drop for ProgressBar { + fn drop(&mut self) { + if !self.finished { + let _ = writeln!(&mut self.stdout_handle); + let _ = self.stdout_handle.flush(); + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang-installer/src/progress_bar.rs` around lines 68 - 72, The progress line can remain unterminated if callers return early and never call ProgressBar::finish, so add a best-effort Drop implementation for the ProgressBar type that ensures the line is terminated and flushed: add a finished: bool field to the ProgressBar struct, set finished = true inside finish(&mut self) after the writeln!/flush succeed, and implement Drop for ProgressBar to check finished and if false perform a safe writeln! to stdout_handle and a flush, swallowing/ignoring any errors (don't panic) to avoid noisy logs on error paths; reference ProgressBar, finish, stdout_handle, and the new finished flag when making the changes.clang-installer/src/downloader/native_packages/windows.rs (1)
167-168: Redundantcmdvariable shadowing.Line 149 creates
let mut cmd = Command::new(self.as_str()), but line 168 shadows it withlet mut cmd = Command::new("winget"). The firstcmdis unused in the Winget branch. While not a bug, it's cleaner to createcmdinside each match arm.Optional cleanup
async fn install_package( &self, package_name: &str, version: Option<&Version>, ) -> Result<(), PackageManagerError> { - let mut cmd = Command::new(self.as_str()); match self { Self::Chocolatey => { + let mut cmd = Command::new(self.as_str()); cmd.arg("install").arg(package_name).arg("-y");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang-installer/src/downloader/native_packages/windows.rs` around lines 167 - 168, The code currently creates a top-level let mut cmd = Command::new(self.as_str()) and then shadows it with let mut cmd = Command::new("winget") inside the Self::Winget match arm; remove the redundant top-level creation and instead instantiate Command::new(...) inside each match arm (e.g., use Command::new(self.as_str()) in the arms that need it and Command::new("winget") inside the Self::Winget arm) so the variable cmd is only declared once per arm and there is no shadowing of cmd or unused bindings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang-installer/src/downloader/native_packages/windows.rs`:
- Around line 107-124: The winget parsing loop incorrectly skips an extra token
before reading the version: in the for loop where lines are split into l
(split_ascii_whitespace().skip(1)) and you check package_name, remove the extra
l.next() that skips the version; instead after confirming name == package_name
and consuming the id via l.next() for the id check, directly call l.next() once
to get ver_str and parse it with Version::parse (used with
ver_cmp.matches(&ver)); update the logic around ver_cmp.clone() / Version::parse
usage so the version token is read immediately after the id-consumption and
treat missing/invalid version as not installed as before.
In `@clang-installer/src/progress_bar.rs`:
- Around line 63-64: The inc method and the step-calculation code must be
hardened: in ProgressBar::inc use checked_add or saturating_add to update
self.current and then clamp self.current to at most self.total to avoid overflow
and runaway values; when computing ratios (current/total) treat total == 0
specially (e.g., treat total as 1 or mark complete) and compute a bounded ratio
in [0.0,1.0]; compute new_steps = (ratio * max_steps) but then cap new_steps to
a reasonable range (e.g., 0..=max_steps) before entering the render loops so the
loops at the locations that use new_steps (the bodies referenced around the
render loops at Line 91 and Line 106) cannot iterate extremely many times. Apply
these same clamping/guard patterns to all places that use total, current, ratio,
or new_steps so the progress bar cannot drive pathological render loops.
In `@clang-installer/src/version.rs`:
- Around line 117-135: The cache lookup builds cached_bin using
get_min_ver(version_req) which yields a full semver (e.g., "12.0.0") but
PyPI-stored binaries use a major-only name (see pypi.rs around the PyPI download
naming), so the code in the StaticDistDownloader::get_cache_dir() -> join("bin")
path misses PyPI entries; update the lookup in the function that builds
cached_bin to also try the major-only variant (e.g., format
"{tool}-{min_ver.major}{bin_ext}") or normalize both producers to the same
filename scheme, attempt both candidate paths, and when a candidate exists call
tool.capture_version(&candidate) and validate with version_req.matches(&version)
before returning the ClangVersion with the matched path.
---
Duplicate comments:
In `@bindings/node/index.d.ts`:
- Line 3: The test expects a numeric return but the TypeScript binding now
declares main(args: string[]) => void; update the test in
bindings/node/__test__/index.spec.mjs to use a no-throw assertion instead of
asserting await main(...) === 0 — call main([...]) synchronously (or without
awaiting) and assert it does not throw (e.g., wrap in try/catch or use your test
framework’s doesNotThrow/doesNotReject equivalent), keeping the declaration in
bindings/node/index.d.ts as export declare function main(args: Array<string>):
void.
---
Nitpick comments:
In `@clang-installer/src/downloader/native_packages/windows.rs`:
- Around line 167-168: The code currently creates a top-level let mut cmd =
Command::new(self.as_str()) and then shadows it with let mut cmd =
Command::new("winget") inside the Self::Winget match arm; remove the redundant
top-level creation and instead instantiate Command::new(...) inside each match
arm (e.g., use Command::new(self.as_str()) in the arms that need it and
Command::new("winget") inside the Self::Winget arm) so the variable cmd is only
declared once per arm and there is no shadowing of cmd or unused bindings.
In `@clang-installer/src/progress_bar.rs`:
- Around line 68-72: The progress line can remain unterminated if callers return
early and never call ProgressBar::finish, so add a best-effort Drop
implementation for the ProgressBar type that ensures the line is terminated and
flushed: add a finished: bool field to the ProgressBar struct, set finished =
true inside finish(&mut self) after the writeln!/flush succeed, and implement
Drop for ProgressBar to check finished and if false perform a safe writeln! to
stdout_handle and a flush, swallowing/ignoring any errors (don't panic) to avoid
noisy logs on error paths; reference ProgressBar, finish, stdout_handle, and the
new finished flag when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63d61d73-4af8-4506-8131-b0b41e23e157
📒 Files selected for processing (12)
bindings/node/index.d.tsbindings/node/src/lib.rsclang-installer/Cargo.tomlclang-installer/src/downloader/hashing.rsclang-installer/src/downloader/mod.rsclang-installer/src/downloader/native_packages/mod.rsclang-installer/src/downloader/native_packages/unix.rsclang-installer/src/downloader/native_packages/windows.rsclang-installer/src/downloader/pypi.rsclang-installer/src/downloader/static_dist.rsclang-installer/src/progress_bar.rsclang-installer/src/version.rs
✅ Files skipped from review due to trivial changes (1)
- clang-installer/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- clang-installer/src/downloader/mod.rs
- clang-installer/src/downloader/native_packages/mod.rs
Comment on lines
+107
to
+124
| for line in lines { | ||
| // packages are listed as `<name> <id> <version> <source>` | ||
| let mut l = line.split_ascii_whitespace().skip(1); | ||
| if let Some(name) = l.next() | ||
| && name == package_name | ||
| { | ||
| // found the package, check version | ||
| if let Some(ver_cmp) = ver_cmp.clone() { | ||
| // skip id and get version | ||
| l.next(); | ||
| if let Some(ver_str) = l.next() | ||
| && let Ok(ver) = Version::parse(ver_str) | ||
| { | ||
| return ver_cmp.matches(&ver); | ||
| } else { | ||
| // version not found or invalid, treat as not installed | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Winget version parsing skips one token too many.
The comment on line 108 states the format is <name> <id> <version> <source>, but the code at line 109 does split_ascii_whitespace().skip(1) (skipping name), then at line 110 reads id, then at line 116 calls l.next() to skip again before reading version at line 117. This means it's reading: skip name → read id → skip id → read version. But the id was already consumed for the check at line 111, so the l.next() at line 116 is skipping what should be the version.
The logic should be:
- Skip name (done via
.skip(1)) - Read and check id (done at line 110-111)
- Read version directly (line 116's
l.next()is erroneous)
Proposed fix
if let Some(ver_cmp) = ver_cmp.clone() {
- // skip id and get version
- l.next();
if let Some(ver_str) = l.next()
&& let Ok(ver) = Version::parse(ver_str)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for line in lines { | |
| // packages are listed as `<name> <id> <version> <source>` | |
| let mut l = line.split_ascii_whitespace().skip(1); | |
| if let Some(name) = l.next() | |
| && name == package_name | |
| { | |
| // found the package, check version | |
| if let Some(ver_cmp) = ver_cmp.clone() { | |
| // skip id and get version | |
| l.next(); | |
| if let Some(ver_str) = l.next() | |
| && let Ok(ver) = Version::parse(ver_str) | |
| { | |
| return ver_cmp.matches(&ver); | |
| } else { | |
| // version not found or invalid, treat as not installed | |
| return false; | |
| } | |
| for line in lines { | |
| // packages are listed as `<name> <id> <version> <source>` | |
| let mut l = line.split_ascii_whitespace().skip(1); | |
| if let Some(name) = l.next() | |
| && name == package_name | |
| { | |
| // found the package, check version | |
| if let Some(ver_cmp) = ver_cmp.clone() { | |
| if let Some(ver_str) = l.next() | |
| && let Ok(ver) = Version::parse(ver_str) | |
| { | |
| return ver_cmp.matches(&ver); | |
| } else { | |
| // version not found or invalid, treat as not installed | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clang-installer/src/downloader/native_packages/windows.rs` around lines 107 -
124, The winget parsing loop incorrectly skips an extra token before reading the
version: in the for loop where lines are split into l
(split_ascii_whitespace().skip(1)) and you check package_name, remove the extra
l.next() that skips the version; instead after confirming name == package_name
and consuming the id via l.next() for the id check, directly call l.next() once
to get ver_str and parse it with Version::parse (used with
ver_cmp.matches(&ver)); update the logic around ver_cmp.clone() / Version::parse
usage so the version token is read immediately after the id-consumption and
treat missing/invalid version as not installed as before.
Comment on lines
+63
to
+64
| pub fn inc(&mut self, delta: u64) -> Result<()> { | ||
| self.current += delta; |
Contributor
There was a problem hiding this comment.
Clamp and harden step calculation to avoid pathological render loops.
At Line 80 and Line 82, total == 0 and large current/total ratios can yield extreme new_steps, which then drives very large loops at Line 91 and Line 106. Also, Line 64 can overflow current.
Proposed fix
pub fn inc(&mut self, delta: u64) -> Result<()> {
- self.current += delta;
+ self.current = self.current.saturating_add(delta);
self.render()
}
pub fn render(&mut self) -> Result<()> {
let advance_bar = self.total.map(|total| {
- let progress = self.current as f64 / total as f64;
-
- (progress * Self::MAX_BAR_WIDTH as f64).floor() as u32
+ if total == 0 {
+ return Self::MAX_BAR_WIDTH;
+ }
+ let scaled = self
+ .current
+ .saturating_mul(Self::MAX_BAR_WIDTH as u64)
+ / total;
+ scaled.min(Self::MAX_BAR_WIDTH as u64) as u32
});Also applies to: 79-83, 91-96, 106-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clang-installer/src/progress_bar.rs` around lines 63 - 64, The inc method and
the step-calculation code must be hardened: in ProgressBar::inc use checked_add
or saturating_add to update self.current and then clamp self.current to at most
self.total to avoid overflow and runaway values; when computing ratios
(current/total) treat total == 0 specially (e.g., treat total as 1 or mark
complete) and compute a bounded ratio in [0.0,1.0]; compute new_steps = (ratio *
max_steps) but then cap new_steps to a reasonable range (e.g., 0..=max_steps)
before entering the render loops so the loops at the locations that use
new_steps (the bodies referenced around the render loops at Line 91 and Line
106) cannot iterate extremely many times. Apply these same clamping/guard
patterns to all places that use total, current, ratio, or new_steps so the
progress bar cannot drive pathological render loops.
Comment on lines
+117
to
+135
| // check if cache has a suitable version | ||
| let bin_ext = if cfg!(windows) { ".exe" } else { "" }; | ||
| let min_ver = get_min_ver(version_req).ok_or(GetToolError::UnsupportedVersion)?; | ||
| let cached_bin = StaticDistDownloader::get_cache_dir() | ||
| .join("bin") | ||
| .join(format!("{tool}-{min_ver}{bin_ext}")); | ||
| if cached_bin.exists() { | ||
| let version = tool.capture_version(&cached_bin)?; | ||
| if version_req.matches(&version) { | ||
| log::info!( | ||
| "Found {tool} version {version} in cache at path: {:?}", | ||
| cached_bin.to_string_lossy() | ||
| ); | ||
| return Ok(Some(ClangVersion { | ||
| version, | ||
| path: cached_bin, | ||
| })); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Cached binary lookup may miss PyPI-downloaded binaries.
The cached binary lookup at lines 120-122 constructs the path as:
StaticDistDownloader::get_cache_dir()
.join("bin")
.join(format!("{tool}-{min_ver}{bin_ext}"))However, PyPI downloads (pypi.rs line 445) store binaries as {tool}-{ver.major}{ext} in the cache bin/ folder. The version format differs: get_min_ver returns versions like Version::new(12, 0, 0) which renders as "12.0.0", but PyPI uses ver.major (just "12").
This means a cached PyPI binary won't be found by this lookup, causing unnecessary re-downloads.
Consider aligning the cache path format
Either change the lookup to also check for major-only naming, or standardize on full version naming across both downloaders.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clang-installer/src/version.rs` around lines 117 - 135, The cache lookup
builds cached_bin using get_min_ver(version_req) which yields a full semver
(e.g., "12.0.0") but PyPI-stored binaries use a major-only name (see pypi.rs
around the PyPI download naming), so the code in the
StaticDistDownloader::get_cache_dir() -> join("bin") path misses PyPI entries;
update the lookup in the function that builds cached_bin to also try the
major-only variant (e.g., format "{tool}-{min_ver.major}{bin_ext}") or normalize
both producers to the same filename scheme, attempt both candidate paths, and
when a candidate exists call tool.capture_version(&candidate) and validate with
version_req.matches(&version) before returning the ClangVersion with the matched
path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order of precedence, this patch allows installing clang tools from
pipare not required.The executable binary is extracted from the downloaded wheel.
chocolatey,wingetapt(Debian),dnf(Fedora),pacman(arch linux)apt, when first attempt fails, the official LLVM install script is (cached and) used as a fallback approach.MIN_CLANG_TOOLS_VERSIONandMAX_CLANG_TOOLS_VERSIONrespectively.CLANG_TOOLS_TAGat compile time (not runtime).CLANG_TOOLS_REPOat compile time (not runtime).Caching
Caching is enabled per OS standards and specific to cpp-linter use.
Caching does not apply to package managers as they tend to manage caches on their own.
Note, the cached official LLVM install script (for the
aptpackage manager fallback) becomes invalid 24 hours after download.Cache directory can be overridden using the
CPP_LINTER_CACHEenvironment variable.Also upgrade cargo/rust dependencies.
Summary by CodeRabbit
New Features
Chores
Compatibility