-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add cross-channel install guard for cargo, winget, and Homebrew #93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4e0aa50
e9cc9c6
a620eaa
9113a7c
5c8d2db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| mod install_channel { | ||
| #![allow(dead_code)] | ||
| include!("src/util/install_channel.rs"); | ||
| } | ||
|
|
||
| fn is_cargo_install_build() -> bool { | ||
| if std::env::var("CARGO_INSTALL_ROOT").is_ok() { | ||
| return true; | ||
| } | ||
| if let Ok(out_dir) = std::env::var("OUT_DIR") { | ||
| let normalized = out_dir.replace('\\', "/").to_ascii_lowercase(); | ||
| if normalized.contains("cargo-install") { | ||
| return true; | ||
| } | ||
| } | ||
| false | ||
| } | ||
|
|
||
| fn main() { | ||
| if is_cargo_install_build() | ||
| && install_channel::run_cargo_install_guard() != std::process::ExitCode::SUCCESS | ||
| { | ||
| std::process::exit(1); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| #Requires -Version 5.1 | ||
| <# | ||
| .SYNOPSIS | ||
| Install numan via cargo after checking for conflicting installs from other channels. | ||
| #> | ||
| [CmdletBinding()] | ||
| param( | ||
| [Parameter(ValueFromRemainingArguments = $true)] | ||
| [string[]]$CargoArgs = @("install", "--path", ".") | ||
| ) | ||
|
|
||
| $ErrorActionPreference = "Stop" | ||
|
|
||
| $repoRoot = Split-Path -Parent $PSScriptRoot | ||
| $manifest = Join-Path $repoRoot "Cargo.toml" | ||
|
|
||
| Write-Host "Checking for conflicting numan installs..." | ||
| & cargo run --quiet --bin numan-install-guard --manifest-path $manifest -- cargo | ||
| if ($LASTEXITCODE -ne 0) { | ||
| exit $LASTEXITCODE | ||
| } | ||
|
|
||
| Write-Host "Running: cargo $($CargoArgs -join ' ')" | ||
| & cargo @CargoArgs --manifest-path $manifest | ||
| exit $LASTEXITCODE | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
| # Install numan via Homebrew after checking for conflicting installs from other channels. | ||
| set -euo pipefail | ||
|
|
||
| repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
|
|
||
| echo "Checking for conflicting numan installs..." | ||
| cargo run --quiet --bin numan-install-guard --manifest-path "${repo_root}/Cargo.toml" -- brew | ||
| echo "Running: brew install tonythethompson/numan/numan ${*}" | ||
| brew install tonythethompson/numan/numan "$@" | ||
|
Comment on lines
+7
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Homebrew install ignores numan_root The install-time scripts scripts/install-homebrew.sh, scripts/install-winget.ps1, and scripts/install-cargo.ps1 invoke brew install, winget install, and cargo install, respectively, which perform installations into tool/system-managed locations not constrained under $NUMAN_ROOT. Compliance requires install-time code to keep all filesystem writes confined to paths rooted under $NUMAN_ROOT. Agent Prompt
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| #Requires -Version 5.1 | ||
| <# | ||
| .SYNOPSIS | ||
| Install numan via winget after checking for conflicting installs from other channels. | ||
| #> | ||
| [CmdletBinding()] | ||
| param( | ||
| [string[]]$WingetArgs = @("install", "tonythethompson.numan") | ||
| ) | ||
|
|
||
| $ErrorActionPreference = "Stop" | ||
|
|
||
| $repoRoot = Split-Path -Parent $PSScriptRoot | ||
| $manifest = Join-Path $repoRoot "Cargo.toml" | ||
|
|
||
| Write-Host "Checking for conflicting numan installs..." | ||
| & cargo run --quiet --bin numan-install-guard --manifest-path $manifest -- winget | ||
| if ($LASTEXITCODE -ne 0) { | ||
| exit $LASTEXITCODE | ||
| } | ||
|
|
||
| Write-Host "Running: winget $($WingetArgs -join ' ')" | ||
| & winget @WingetArgs | ||
| exit $LASTEXITCODE |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| use std::env; | ||
| use std::process::ExitCode; | ||
|
|
||
| use numan_cli::util::install_channel::{ | ||
| run_cargo_install_guard, run_homebrew_install_guard, run_winget_install_guard, | ||
| }; | ||
|
|
||
| fn main() -> ExitCode { | ||
| match env::args().nth(1).map(|s| s.to_ascii_lowercase()) { | ||
| Some(arg) if arg == "cargo" => run_cargo_install_guard(), | ||
| Some(arg) if arg == "winget" => run_winget_install_guard(), | ||
| Some(arg) if arg == "brew" || arg == "homebrew" => run_homebrew_install_guard(), | ||
| _ => { | ||
| eprintln!("usage: numan-install-guard <cargo|winget|brew>"); | ||
| ExitCode::from(2) | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+8
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'Cargo.toml|main.rs' . | sed 's#^\./##'
echo
echo "== install-guard Cargo.toml =="
cat -n install-guard/Cargo.toml
echo
echo "== install-guard main.rs =="
cat -n install-guard/src/main.rs
echo
echo "== clap usages in repo =="
rg -n "use clap|clap::|Command|Subcommand" --glob '*.rs' .
echo
echo "== cargo dependencies containing clap =="
rg -n "clap\s*=|dependencies" Cargo.toml install-guard/Cargo.tomlRepository: tonythethompson/numan Length of output: 15778 🏁 Script executed (no clone): Length of output: 178 🌐 Web query:
💡 Result: To use the derive API in clap version 4, you must enable the derive feature flag [1][2]. You can enable this feature by adding clap to your Cargo.toml with the derive feature enabled [2]: cargo add clap --features derive Once enabled, the derive API allows you to define your command-line interface using structs, enums, and derive macros such as #[derive(Parser)], #[derive(Args)], #[derive(Subcommand)], and #[derive(ValueEnum)] [1][3]. The derive feature works by depending on the clap_derive crate [4]. You can refer to the official clap documentation, specifically the Derive Reference and the Derive Tutorial, for detailed usage examples and configuration options [1][2][5]. Citations:
Use
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,9 @@ use crate::util::hints::{ | |
| ACTIVE_PLUGIN_MUTATION_GATED_FIX, CMD_ACTIVATE, CMD_DEACTIVATE, CMD_DOCTOR_FIX, CMD_INIT, | ||
| CMD_INIT_REFRESH, CMD_REGISTRY_SYNC, CMD_SETUP_NU, CMD_USE, | ||
| }; | ||
| use crate::util::install_channel::{ | ||
| classify_binary_path, discover_path_installations, InstallChannel, | ||
| }; | ||
| use crate::util::stdio_redirect::StdoutToStderr; | ||
|
|
||
| const SCHEMA_VERSION: u32 = 1; | ||
|
|
@@ -195,6 +198,7 @@ pub fn run_checks_with_options( | |
| let mut findings = Vec::new(); | ||
|
|
||
| check_root_layout(root, &mut findings); | ||
| check_install_channels(&mut findings); | ||
|
qodo-code-review[bot] marked this conversation as resolved.
|
||
| check_active_version_marker(root, &mut findings); | ||
| let nu_paths = check_nu_paths(root, options, &mut findings); | ||
| check_nu_environments(root, options, &mut findings); | ||
|
|
@@ -322,6 +326,65 @@ fn nu_is_available(root: &Path) -> bool { | |
| false | ||
| } | ||
|
|
||
| fn check_install_channels(findings: &mut Vec<Finding>) { | ||
| // Doctor uses the cheap PATH-only scan; package-manager trees are checked by install guards. | ||
| let installs = discover_path_installations(); | ||
| if installs.len() <= 1 { | ||
| return; | ||
| } | ||
|
|
||
| let channels: std::collections::HashSet<InstallChannel> = | ||
| installs.iter().map(|install| install.channel).collect(); | ||
| if channels.len() <= 1 { | ||
| return; | ||
|
Comment on lines
+329
to
+339
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Avoid duplicate uninstall hints when multiple installs share the same channel.
Suggested implementation: fn check_install_channels(findings: &mut Vec<Finding>) {
let installs = discover_installations();
if installs.len() <= 1 {
return;
}
// Collect all distinct install channels
let channels: std::collections::HashSet<InstallChannel> =
installs.iter().map(|install| install.channel).collect();
if channels.len() <= 1 {
return;
}
// Deduplicate uninstall hints per InstallChannel by:
// 1. Iterating over each unique channel
// 2. Picking one representative install for that channel
// 3. Collecting its uninstall_hint() into a HashSet<String>
//
// The resulting `_uninstall_hints` set can be used when constructing
// the Finding's fix to avoid duplicated uninstall commands.
let _uninstall_hints: std::collections::HashSet<String> = channels
.into_iter()
.filter_map(|channel| {
installs
.iter()
.find(|install| install.channel == channel)
.map(|install| install.uninstall_hint())
})
.collect();To fully apply the deduplicated uninstall hints in the
|
||
| } | ||
|
|
||
| let current = std::env::current_exe().ok(); | ||
| let current_channel = current | ||
| .as_ref() | ||
| .map(|path| classify_binary_path(path)) | ||
| .unwrap_or(InstallChannel::Unknown); | ||
|
|
||
| let mut lines = | ||
| vec!["Multiple numan installs from different package managers were detected:".to_string()]; | ||
| for install in &installs { | ||
| lines.push(format!( | ||
| " {} ({})", | ||
| install.path.display(), | ||
| install.channel.label() | ||
| )); | ||
| } | ||
| lines.push(format!( | ||
| "This session is running the {} build.", | ||
| current_channel.label() | ||
| )); | ||
| lines.push( | ||
| "Keep one install channel to avoid PATH ambiguity. Uninstall the others before reinstalling." | ||
| .to_string(), | ||
| ); | ||
|
|
||
| let fix = installs | ||
| .iter() | ||
| .filter(|install| install.channel != current_channel) | ||
| .filter_map(|install| install.channel.uninstall_hint()) | ||
| .collect::<std::collections::HashSet<_>>() | ||
| .into_iter() | ||
| .collect::<Vec<_>>() | ||
| .join("; "); | ||
|
|
||
| findings.push(finding( | ||
| "install.multiple_channels", | ||
| Severity::Warn, | ||
| lines.join("\n"), | ||
| if fix.is_empty() { | ||
| None | ||
| } else { | ||
| Some(fix.as_str()) | ||
| }, | ||
| RepairTier::Manual, | ||
| )); | ||
| } | ||
|
|
||
| /// Detect a present-but-unreadable `nu_state/active-version.json`. | ||
| /// | ||
| /// `find_nu_executable_with_root` treats marker read errors as soft misses and | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Resolve the default Cargo path from the script directory.
$repoRootuses the script location, but the default--path .uses the caller’s current directory. If a user invokes this wrapper outside the checkout, Cargo installs the wrong directory or fails to find a manifest. Set the default path after$repoRootis known.Proposed fix
Also applies to: 18-19, 31-32
🤖 Prompt for AI Agents