feat: do not use system package managers by default#372
Conversation
📝 WalkthroughWalkthroughAdds an ChangesSystem Package Manager Opt-In Flag
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
resolves #364 Actually, the default depends on the state of a CI env var. - if CI env var is present and its value is one of "true", "on", or "1", then system package managers are used by default. - if CI is not present (or its value is not "true", "on", or "1"), then the new `--mod-sys` switch can allow explicit opt-in to use system package managers.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
+ Coverage 92.35% 92.38% +0.03%
==========================================
Files 23 23
Lines 3597 3612 +15
==========================================
+ Hits 3322 3337 +15
Misses 275 275 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clang-tools-manager/src/main.rs`:
- Around line 152-153: The SetTrue action with env binding on the mod_sys field
incorrectly treats any presence of the CI environment variable as true, ignoring
its actual value. Remove the env = "CI" attribute from the #[arg(...)] macro on
the mod_sys field definition. In the main function, after parsing the Args
struct, manually check and parse the CI environment variable using proper
truthy/falsey validation (checking for values like "true", "1", "on" as
explicitly true and anything else as false). Pass the manually validated boolean
value to the eval_tool function call instead of using the mod_sys field
directly.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 618be6ae-dc71-4174-9cd7-a18ca2b0e057
📒 Files selected for processing (4)
clang-tools-manager/Cargo.tomlclang-tools-manager/src/main.rsclang-tools-manager/src/version.rscpp-linter/src/clang_tools/mod.rs
| #[arg(long, action = clap::ArgAction::SetTrue, env = "CI")] | ||
| pub mod_sys: bool, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In clap v4 derive, for #[arg(action = clap::ArgAction::SetTrue, env = "CI")]on abool, does any present CIvalue (includingfalse) set the flag to true by default?
💡 Result:
In clap v4, when using #[arg(action = clap::ArgAction::SetTrue, env = "CI")] on a bool, the presence of the CI environment variable does not automatically set the flag to true regardless of its content [1]. When an environment variable is provided for an argument, clap processes the variable's value to determine the argument's state [1]. For flags using ArgAction::SetTrue, clap interprets the environment variable based on standard "truthy" or "falsey" rules [1]. If the environment variable is set to a "falsey" value (such as "false", "no", "off", "0", or "n"), the argument will be parsed as false [1]. The flag is only set to true if the environment variable contains a "truthy" value [1]. If the environment variable is absent, the flag defaults to false [2][1]. Therefore, any present CI value does not force the flag to true; rather, clap evaluates the content of the variable to determine whether to enable the flag [1].
Citations:
- 1: https://docs.rs/clap/4.1.8/clap/struct.Arg.html
- 2: https://docs.rs/clap/4.3/clap/enum.ArgAction.html
🏁 Script executed:
# Check the clap version in Cargo.toml
find . -name "Cargo.toml" -type f | head -5 | xargs grep -l "clap" 2>/dev/null | head -1 | xargs cat | grep -A 5 "clap"Repository: cpp-linter/cpp-linter-rs
Length of output: 284
🏁 Script executed:
# View the actual code around the flagged lines
cat -n clang-tools-manager/src/main.rs | sed -n '145,160p'Repository: cpp-linter/cpp-linter-rs
Length of output: 742
🏁 Script executed:
# View the second location mentioned (177-182)
cat -n clang-tools-manager/src/main.rs | sed -n '170,185p'Repository: cpp-linter/cpp-linter-rs
Length of output: 710
🌐 Web query:
clap 4.6.1 ArgAction::SetTrue environment variable parsing behavior falsey values
💡 Result:
In clap v4.6.1, ArgAction::SetTrue does not inherently interpret "falsey" values when reading from an environment variable [1][2]. By design, ArgAction::SetTrue does not accept values on the command line [1][2]. When an argument with this action is configured to read from an environment variable via.env, the presence of the environment variable is generally treated as "true" (i.e., the flag is raised), regardless of the specific content of that variable [3][4]. If you require specific environment variable values (such as "false", "0", or "off") to be parsed as "false" for an argument that would otherwise be enabled by the mere presence of an environment variable, you must use a value_parser that explicitly handles those cases [1][3][4]. For example, using a parser like FalseyValueParser allows you to define custom boolean logic for environment variables: - The parser can be configured to recognize specific "false" literals like "n", "no", "f", "false", "off", or "0" [3][4]. - When such a parser is attached to an ArgAction::SetTrue argument, the environment variable's content is evaluated against the parser's logic rather than simply triggering the flag based on existence [3][4]. - Without such a parser, the existence of the environment variable typically results in the flag being set to true [3][4]. Note that ArgAction::SetTrue requires ArgMatches::get_flag to check the result, rather than legacy methods like ArgMatches::is_present [5]. [5][1][3]
Citations:
- 1: https://docs.rs/clap/latest/i686-pc-windows-msvc/clap/enum.ArgAction.html
- 2: https://docs.diesel.rs/master/src/clap_builder/builder/action.rs.html
- 3: https://docs.rs/clap/4.4.6/clap/struct.Arg.html
- 4: https://docs.rs/clap/4.1.8/clap/struct.Arg.html
- 5: https://github.com/clap-rs/clap/blob/v4.6.1/CHANGELOG.md
🏁 Script executed:
# Check if there's another similar env binding around lines 177-182
cat -n clang-tools-manager/src/main.rs | sed -n '140,200p' | grep -E "(#\[arg|env =)"Repository: cpp-linter/cpp-linter-rs
Length of output: 139
SetTrue action with env = "CI" treats presence as true regardless of value
On Line 152, #[arg(long, action = clap::ArgAction::SetTrue, env = "CI")] sets the flag to true whenever the CI environment variable is present, regardless of its value. This means CI=false, CI=0, or CI=no will all enable the flag, breaking the intended "true"|"on"|"1" gate and potentially reintroducing unexpected system package-manager installs.
Remove the env = "CI" binding and manually parse the environment variable with proper truthy/falsey validation:
🔧 Suggested fix
- #[arg(long, action = clap::ArgAction::SetTrue, env = "CI")]
+ #[arg(long, action = clap::ArgAction::SetTrue)]
pub mod_sys: bool,Then in the main function:
let options = CliOptions::parse();
+ let in_ci = std::env::var("CI")
+ .is_ok_and(|v| ["true", "on", "1"].contains(&v.to_ascii_lowercase().as_str()));
+ let allow_system_package_manager = options.mod_sys || in_ci;Update the call to eval_tool (around line 177-182):
- options.mod_sys,
+ allow_system_package_manager,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clang-tools-manager/src/main.rs` around lines 152 - 153, The SetTrue action
with env binding on the mod_sys field incorrectly treats any presence of the CI
environment variable as true, ignoring its actual value. Remove the env = "CI"
attribute from the #[arg(...)] macro on the mod_sys field definition. In the
main function, after parsing the Args struct, manually check and parse the CI
environment variable using proper truthy/falsey validation (checking for values
like "true", "1", "on" as explicitly true and anything else as false). Pass the
manually validated boolean value to the eval_tool function call instead of using
the mod_sys field directly.
1 participant
resolves #364
Actually, the default depends on the state of a CI env var.
--mod-sysswitch can allow explicit opt-in to use system package managers.Summary by CodeRabbit
--mod-sysflag to control system package manager usage during clang tool installation, with automatic detection of CI environments for optimized behavior.