From 49475053043c8079726901d742fb9ba95ecdf6b1 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 13 Nov 2026 06:10:08 +0000 Subject: [PATCH 1/5] fix: make clang tool install logs say what actually happened - Log the version that was found or installed, not the requested minimum ("v17.0.0" for "17", "v18.1.8" for "18.1.8"). - Drop " does not have a version ... installed", which was printed for three different reasons and read like "not available". - Log a found binary that does not match the requirement. - Use INFO/WARN for steps that fall back to another source; ERROR is left for when every source fails. apt's stderr before trying LLVM's apt repository is now a debug log. - Check installed apt packages with dpkg-query: `apt list --installed` exits with 0 even when the package is not installed. - Explain in `--version` docs that `18.1.8` means 18.1.8 or a newer 18.x release, and `=18.1.8` means exactly that version. --- .../src/downloader/native_packages/mod.rs | 70 ++++++++----------- .../src/downloader/native_packages/unix.rs | 43 +++++++++--- clang-tools-manager/src/version.rs | 8 ++- cpp-linter/src/cli/mod.rs | 3 + 4 files changed, 72 insertions(+), 52 deletions(-) diff --git a/clang-tools-manager/src/downloader/native_packages/mod.rs b/clang-tools-manager/src/downloader/native_packages/mod.rs index 0016fbb5..0fc7380d 100644 --- a/clang-tools-manager/src/downloader/native_packages/mod.rs +++ b/clang-tools-manager/src/downloader/native_packages/mod.rs @@ -77,51 +77,37 @@ pub async fn try_install_package( ) -> Result, GetToolError> { let os_pkg_managers = get_available_package_managers(); if os_pkg_managers.is_empty() { - log::error!("No supported package managers found on the system."); + log::warn!("No supported package managers found on the system."); return Ok(None); - } else { - for mgr in os_pkg_managers { - log::info!("Trying to install {tool} v{min_version} using {mgr} package manager."); - let pkg_name = mgr.get_package_name(tool); - if mgr.is_installed_package(&pkg_name, Some(min_version)) - && let Ok(path) = - tool.get_exe_path(&RequestedVersion::Requirement(version_req.clone())) - && let Ok(version) = tool.capture_version(&path) - && version_req.matches(&version) - { - log::info!( - "Found {tool} version matching {version_req} installed via {mgr} package manager." + } + for mgr in os_pkg_managers { + let pkg_name = mgr.get_package_name(tool); + if mgr.is_installed_package(&pkg_name, Some(min_version)) + && let Ok(path) = tool.get_exe_path(&RequestedVersion::Requirement(version_req.clone())) + && let Ok(version) = tool.capture_version(&path) + && version_req.matches(&version) + { + log::info!("Found {tool} version {version} installed via {mgr} package manager."); + return Ok(Some(ClangVersion { version, path })); + } + log::info!("Installing {tool} {version_req} using {mgr} package manager."); + match mgr.install_package(&pkg_name, Some(min_version)).await { + Ok(()) => { + let path = + tool.get_exe_path(&RequestedVersion::Requirement(version_req.clone()))?; + let version = tool.capture_version(&path)?; + if version_req.matches(&version) { + log::info!("Installed {tool} version {version} using {mgr} package manager."); + return Ok(Some(ClangVersion { version, path })); + } + log::warn!( + "{mgr} package manager provides {tool} version {version}, which does not match the requirement {version_req}." ); - return Ok(Some(ClangVersion { version, path })); - } else { - log::info!( - "{mgr} package manager does not have a version of {tool} matching {version_req} installed." + } + Err(e) => { + log::warn!( + "Failed to install {tool} {version_req} using {mgr} package manager: {e}" ); - match mgr.install_package(&pkg_name, Some(min_version)).await { - Ok(()) => { - log::info!( - "Successfully installed {tool} v{min_version} using {mgr} package manager." - ); - let path = - tool.get_exe_path(&RequestedVersion::Requirement(version_req.clone()))?; - let version = tool.capture_version(&path)?; - if version_req.matches(&version) { - log::info!( - "Installed {tool} version {version} matches the requirement {version_req}." - ); - return Ok(Some(ClangVersion { version, path })); - } else { - log::error!( - "Installed {tool} version {version} does not match the requirement {version_req}." - ); - } - } - Err(e) => { - log::error!( - "Failed to install {tool} v{min_version} using {mgr} package manager: {e}" - ); - } - } } } } diff --git a/clang-tools-manager/src/downloader/native_packages/unix.rs b/clang-tools-manager/src/downloader/native_packages/unix.rs index 90545228..22a20999 100644 --- a/clang-tools-manager/src/downloader/native_packages/unix.rs +++ b/clang-tools-manager/src/downloader/native_packages/unix.rs @@ -155,12 +155,12 @@ impl PackageManager for UnixPackageManager { if matches!(self, UnixPackageManager::Apt) && let Some(version) = version { - log::error!( - "Failed to install {package_id} via apt: {}", - String::from_utf8_lossy(&output.stderr) - ); log::info!( - "trying to install from official LLVM PPA repository (for Debian-based `apt` package manager)" + "{package_id} is not available from the configured apt sources; trying the official LLVM apt repository." + ); + log::debug!( + "`apt install {package_id}` failed: {}", + String::from_utf8_lossy(&output.stderr) ); return llvm_apt_install::install_llvm_via_apt( version.major.to_string(), @@ -182,10 +182,13 @@ impl PackageManager for UnixPackageManager { match self { #[cfg(target_os = "linux")] UnixPackageManager::Apt => { - let output = cmd - .args(["list", "--installed", package_id.as_str()]) + // `apt list --installed ` exits with 0 even if the package is not installed. + let output = Command::new("dpkg-query") + .args(["--show", "--showformat=${db:Status-Status}"]) + .arg(package_id.as_str()) .output(); - output.is_ok_and(|out| out.status.success()) + output + .is_ok_and(|out| out.status.success() && out.stdout.as_slice() == b"installed") } #[cfg(target_os = "linux")] UnixPackageManager::Dnf => { @@ -387,6 +390,11 @@ mod tests { let tmp = setup(); let ver = Version::new(17, 0, 0); for mgr in all_managers() { + #[cfg(target_os = "linux")] + if matches!(mgr, UnixPackageManager::Apt) { + // apt is checked with `dpkg-query`, which is tested separately. + continue; + } // package manager not found assert!(!mgr.is_installed_package("clang", Some(&ver))); fake_exe(tmp.path(), mgr.as_str(), SUCCESS); @@ -396,6 +404,25 @@ mod tests { } } + #[cfg(target_os = "linux")] + #[test] + fn is_installed_apt_package() { + let tmp = setup(); + let mgr = UnixPackageManager::Apt; + let ver = Version::new(17, 0, 0); + // `dpkg-query` not found + assert!(!mgr.is_installed_package("clang-format", Some(&ver))); + // package is installed + fake_exe(tmp.path(), "dpkg-query", "printf installed"); + assert!(mgr.is_installed_package("clang-format", Some(&ver))); + // package is known but not installed (`dpkg-query` still exits with 0) + fake_exe(tmp.path(), "dpkg-query", "printf not-installed"); + assert!(!mgr.is_installed_package("clang-format", Some(&ver))); + // package is unknown + fake_exe(tmp.path(), "dpkg-query", FAILURE); + assert!(!mgr.is_installed_package("clang-format", None)); + } + #[tokio::test] async fn install_with_sudo() { let tmp = setup(); diff --git a/clang-tools-manager/src/version.rs b/clang-tools-manager/src/version.rs index 919d0a61..7ea7f588 100644 --- a/clang-tools-manager/src/version.rs +++ b/clang-tools-manager/src/version.rs @@ -115,6 +115,10 @@ impl RequestedVersion { ); return Ok(Some(ClangVersion { version, path })); } + log::info!( + "Found {tool} version {version} at path: {:?}, which does not match the requirement {version_req}.", + path.to_string_lossy() + ); } // check if cache has a suitable version @@ -155,7 +159,7 @@ impl RequestedVersion { match try_install_package(tool, version_req, &min_ver).await { Ok(Some(result)) => return Ok(Some(result)), Ok(None) => {} - Err(e) => log::error!( + Err(e) => log::warn!( "Failed to install {tool} {version_req} using a system package manager: {e}" ), } @@ -165,7 +169,7 @@ impl RequestedVersion { let bin = match PyPiDownloader::download_tool(tool, version_req, directory).await { Ok(bin) => bin, Err(e) => { - log::error!("Failed to download {tool} {version_req} from PyPi: {e}"); + log::warn!("Failed to download {tool} {version_req} from PyPi: {e}"); log::info!("Falling back to downloading {tool} static binaries."); match StaticDistDownloader::download_tool(tool, version_req, directory) .await diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index 4e8c04e3..6770f72c 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -96,6 +96,9 @@ pub struct GeneralOptions { /// Accepted options are: /// /// - A semantic version specifier, eg. `>=10, <13`, `=12.0.1`, or simply `16`. + /// A version without an operator is a minimum within that major version: + /// `18.1.8` accepts 18.1.8 or a newer 18.x release, and `16` accepts any 16.x release. + /// Use `=18.1.8` to require exactly that version. /// - A blank string (`''`) to use the platform's default /// installed version. /// - A path to where the clang tools are From 43386b7e79efb8488d113d03c70caec9f7cd2be5 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 13 Nov 2026 06:10:49 +0000 Subject: [PATCH 2/5] chore: add dpkg words to cspell --- cspell.config.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cspell.config.yml b/cspell.config.yml index f9431dcb..dcc7da85 100644 --- a/cspell.config.yml +++ b/cspell.config.yml @@ -23,6 +23,7 @@ words: - declval - devel - Doherty + - dpkg - dtolnay - eabi - ecure @@ -118,6 +119,7 @@ words: - serde - Shen - shenxianpeng + - showformat - splitn - SRCROOT - startswith From 6f06e031303c88f5b879736c55fda5bafb165227 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 13 Nov 2026 14:27:49 +0000 Subject: [PATCH 3/5] fix: don't guess why apt install failed --- clang-tools-manager/src/downloader/native_packages/unix.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-manager/src/downloader/native_packages/unix.rs b/clang-tools-manager/src/downloader/native_packages/unix.rs index 22a20999..cff9f049 100644 --- a/clang-tools-manager/src/downloader/native_packages/unix.rs +++ b/clang-tools-manager/src/downloader/native_packages/unix.rs @@ -156,7 +156,7 @@ impl PackageManager for UnixPackageManager { && let Some(version) = version { log::info!( - "{package_id} is not available from the configured apt sources; trying the official LLVM apt repository." + "`apt install {package_id}` failed; trying the official LLVM apt repository." ); log::debug!( "`apt install {package_id}` failed: {}", From 789260b39883b1201ca0afe625cb868090ac6de9 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 13 Nov 2026 19:02:26 +0000 Subject: [PATCH 4/5] fix: skip system package managers for an exact version They install a tool by major version only, so for `=18.1.8` they could install (and leave behind) another 18.x release before cpp-linter moves on to PyPI or the static binaries. --- clang-tools-manager/src/version.rs | 70 +++++++++++++++++++++++++++--- cpp-linter/src/cli/mod.rs | 3 +- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/clang-tools-manager/src/version.rs b/clang-tools-manager/src/version.rs index 7ea7f588..10e3710a 100644 --- a/clang-tools-manager/src/version.rs +++ b/clang-tools-manager/src/version.rs @@ -156,12 +156,18 @@ impl RequestedVersion { // try a system package manager first (if allowed) if allow_system_package_manager { - match try_install_package(tool, version_req, &min_ver).await { - Ok(Some(result)) => return Ok(Some(result)), - Ok(None) => {} - Err(e) => log::warn!( - "Failed to install {tool} {version_req} using a system package manager: {e}" - ), + if requires_exact_version(version_req) { + log::info!( + "Skipping system package managers: they only install {tool} by major version, but {version_req} requires an exact version." + ); + } else { + match try_install_package(tool, version_req, &min_ver).await { + Ok(Some(result)) => return Ok(Some(result)), + Ok(None) => {} + Err(e) => log::warn!( + "Failed to install {tool} {version_req} using a system package manager: {e}" + ), + } } } @@ -205,6 +211,18 @@ impl RequestedVersion { } } +/// Returns true if `version_req` only allows specific minor or patch releases (eg. `=18.1.8` or `=18.1`). +/// +/// System package managers can only install a tool by its major version, +/// so they could install (and leave behind) a version that does not satisfy such a requirement. +fn requires_exact_version(version_req: &VersionReq) -> bool { + !version_req.comparators.is_empty() + && version_req + .comparators + .iter() + .all(|c| c.op == semver::Op::Exact && c.minor.is_some()) +} + /// Returns the lowest supported version that satisfies `version_req`. pub fn get_min_ver(version_req: &VersionReq) -> Option { StaticDistDownloader::get_major_version_range() @@ -293,11 +311,29 @@ mod tests { use semver::VersionReq; use tempfile::TempDir; - use super::{RequestedVersion, RequestedVersionParsingError}; + use super::{RequestedVersion, RequestedVersionParsingError, requires_exact_version}; use crate::{ClangTool, utils::normalize_path}; // See also crate::tool::tests module for other `RequestedVersion::from_str()` tests. + #[test] + fn exact_version_requirements() { + for (req, exact) in [ + ("=18.1.8", true), + ("=18.1", true), + ("=18", false), + ("18.1.8", false), + (">=18.1, <18.2", false), + ("*", false), + ] { + assert_eq!( + requires_exact_version(&VersionReq::parse(req).unwrap()), + exact, + "{req}" + ); + } + } + #[test] fn lowest_match_in_major() { for (req, major, expected) in [ @@ -711,6 +747,26 @@ mod tests { assert!(matches!(err, GetToolError::NotFound)); } + #[tokio::test] + async fn eval_exact_requirement_skips_package_manager() { + let env = TestEnv::new(); + env.seed_empty_pypi(); + env.seed_static_dist(true); + let marker = env.bin.path().join("installed"); + // A fake `brew` that would "install" clang-format (by creating a marker file). + fake_exe( + env.bin.path(), + "brew", + &format!( + "if [ \"$1\" = \"list\" ]; then exit 1; fi\n: > \"{}\"", + marker.to_string_lossy() + ), + ); + let result = env.eval(req("=17.0.6"), true).await.unwrap().unwrap(); + assert_version(&env.out.path().join("clang-format-17"), result, "17.0.6"); + assert!(!marker.exists()); + } + #[tokio::test] async fn eval_package_manager_error_falls_back() { let env = TestEnv::new(); diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index 6770f72c..773f9beb 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -98,7 +98,8 @@ pub struct GeneralOptions { /// - A semantic version specifier, eg. `>=10, <13`, `=12.0.1`, or simply `16`. /// A version without an operator is a minimum within that major version: /// `18.1.8` accepts 18.1.8 or a newer 18.x release, and `16` accepts any 16.x release. - /// Use `=18.1.8` to require exactly that version. + /// Use `=18.1.8` to require exactly that version. System package managers + /// are not used for such a version because they only install by major version. /// - A blank string (`''`) to use the platform's default /// installed version. /// - A path to where the clang tools are From 34120732d6bd245044010587fa180556b261680c Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 13 Nov 2026 19:05:49 +0000 Subject: [PATCH 5/5] fix: one exact comparator is enough to skip package managers --- clang-tools-manager/src/version.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/clang-tools-manager/src/version.rs b/clang-tools-manager/src/version.rs index 10e3710a..652e769e 100644 --- a/clang-tools-manager/src/version.rs +++ b/clang-tools-manager/src/version.rs @@ -213,14 +213,15 @@ impl RequestedVersion { /// Returns true if `version_req` only allows specific minor or patch releases (eg. `=18.1.8` or `=18.1`). /// +/// All comparators must match, so one exact comparator (with a minor version) is enough. +/// /// System package managers can only install a tool by its major version, /// so they could install (and leave behind) a version that does not satisfy such a requirement. fn requires_exact_version(version_req: &VersionReq) -> bool { - !version_req.comparators.is_empty() - && version_req - .comparators - .iter() - .all(|c| c.op == semver::Op::Exact && c.minor.is_some()) + version_req + .comparators + .iter() + .any(|c| c.op == semver::Op::Exact && c.minor.is_some()) } /// Returns the lowest supported version that satisfies `version_req`. @@ -322,6 +323,7 @@ mod tests { ("=18.1.8", true), ("=18.1", true), ("=18", false), + ("=17.0.6, >=17", true), ("18.1.8", false), (">=18.1, <18.2", false), ("*", false),