From c047dcd82222208b78e47360a08a975266130ed9 Mon Sep 17 00:00:00 2001 From: Danny Ward Date: Wed, 14 Oct 2026 03:40:00 -0400 Subject: [PATCH 1/2] fix: accept split toolchain remove specs --- src/cli.cppm | 7 ++++++- src/cli/cmd_toolchain.cppm | 3 ++- src/toolchain/lifecycle.cppm | 6 ++++-- tests/e2e/26_toolchain_management.sh | 25 +++++++++++++++++++++++-- 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/cli.cppm b/src/cli.cppm index 2d766eef2..8b090fa81 100644 --- a/src/cli.cppm +++ b/src/cli.cppm @@ -795,7 +795,12 @@ int run(int argc, char** argv) { "Default build target (omit = host)"))) .subcommand(cl::App("remove") .description("Uninstall a toolchain") - .arg(cl::Arg("spec").help("@").required()) + // Match `default`: accept either `gcc@16.1.0` or `gcc 16.1.0`. + // Partial-version spelling follows `default` too. + .arg(cl::Arg("spec") + .help("[@] (version may be partial)") + .required()) + .arg(cl::Arg("version").help("(optional, alternative to @-form)")) .option(cl::Option("target").takes_value().help( "Remove the payload for instead of the host one"))) .action(wrap_rc(cmd_toolchain))) diff --git a/src/cli/cmd_toolchain.cppm b/src/cli/cmd_toolchain.cppm index 4cd57023a..ec2539989 100644 --- a/src/cli/cmd_toolchain.cppm +++ b/src/cli/cmd_toolchain.cppm @@ -56,7 +56,8 @@ export int cmd_toolchain(const mcpplibs::cmdline::ParsedArgs& parsed) { return mcpp::toolchain::toolchain_set_default( *cfg, sub_parsed.positional(0), sub_parsed.positional(1), targetArg); if (subname == "remove") - return mcpp::toolchain::toolchain_remove(*cfg, sub_parsed.positional(0), targetArg); + return mcpp::toolchain::toolchain_remove( + *cfg, sub_parsed.positional(0), sub_parsed.positional(1), targetArg); mcpp::ui::error(std::format("unknown toolchain subcommand '{}'", subname)); return 2; diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index f3d8e58e3..def7418a2 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -1269,9 +1269,10 @@ export int toolchain_set_default(const mcpp::config::GlobalConfig& cfg, // `mcpp toolchain remove [--target ]` — uninstall a payload. export int toolchain_remove(const mcpp::config::GlobalConfig& cfg, const std::string& pos0, + const std::string& pos1, const std::string& targetArg = {}) { auto xlEnv = mcpp::config::make_xlings_env(cfg); - auto parsedSpec = mcpp::toolchain::parse_toolchain_spec(pos0); + auto parsedSpec = mcpp::toolchain::parse_toolchain_spec(pos0, pos1); if (parsedSpec && mcpp::toolchain::is_system_toolchain(*parsedSpec)) { mcpp::ui::error( "msvc@system is the machine's own Visual Studio, managed by the " @@ -1281,7 +1282,8 @@ export int toolchain_remove(const mcpp::config::GlobalConfig& cfg, return 1; } if (!parsedSpec || parsedSpec->version.empty()) { - mcpp::ui::error(std::format("invalid spec '{}'", pos0)); + const auto input = pos1.empty() ? pos0 : std::format("{} {}", pos0, pos1); + mcpp::ui::error(std::format("invalid spec '{}'", input)); return 2; } mcpp::toolchain::print_compat_hint(*parsedSpec); diff --git a/tests/e2e/26_toolchain_management.sh b/tests/e2e/26_toolchain_management.sh index c6468b858..fdd90a4e2 100755 --- a/tests/e2e/26_toolchain_management.sh +++ b/tests/e2e/26_toolchain_management.sh @@ -60,6 +60,9 @@ if [[ $rc -ne 0 ]]; then exit 1 fi grep -q 'Installed' /tmp/_inst.log || { cat /tmp/_inst.log; echo "FAIL: no Installed line"; exit 1; } +# Both accepted spellings should work when the payload is already installed. +"$MCPP" toolchain install gcc@16.1.0 > /tmp/_inst_at.log 2>&1 || { + cat /tmp/_inst_at.log; echo "FAIL: @-form install failed"; exit 1; } # 4) toolchain list now non-empty out=$("$MCPP" toolchain list 2>&1) @@ -71,6 +74,10 @@ echo "$out" | grep -q '16.1.0' || { echo "FAIL: list missing version: $out"; exi grep -q 'Default' /tmp/_def.log || { cat /tmp/_def.log; echo "FAIL: no Default msg"; exit 1; } out=$("$MCPP" toolchain list 2>&1) echo "$out" | grep -q '\*' || { echo "FAIL: default mark missing: $out"; exit 1; } +"$MCPP" toolchain default gcc 16.1.0 > /tmp/_def_split.log 2>&1 || { + cat /tmp/_def_split.log; echo "FAIL: space-separated default failed"; exit 1; } +grep -q 'Default' /tmp/_def_split.log || { + cat /tmp/_def_split.log; echo "FAIL: no Default msg for space-separated form"; exit 1; } # 6) Build now succeeds (project has no [toolchain]; uses global default) "$MCPP" build > /tmp/_build.log 2>&1 || { cat /tmp/_build.log; echo "FAIL: build failed"; exit 1; } @@ -98,8 +105,22 @@ echo "$out" | grep -q 'default toolchain = gcc@16.1.0' || { # 9) Remove rm -rf target -"$MCPP" toolchain remove gcc@16.1.0 > /tmp/_rm.log 2>&1 -grep -q 'Removed' /tmp/_rm.log || { cat /tmp/_rm.log; echo "FAIL: no Removed msg"; exit 1; } +"$MCPP" toolchain remove gcc 16.1.0 > /tmp/_rm.log 2>&1 +grep -q 'Removed' /tmp/_rm.log || { cat /tmp/_rm.log; echo "FAIL: no Removed msg for space-separated form"; exit 1; } +# The issue's cross-target spelling should parse too and reach the payload +# lookup, even when that target is not installed in this test workspace. +rc=0 +"$MCPP" toolchain remove gcc 16.1.0 --target x86_64-linux-musl > /tmp/_rm_target.log 2>&1 || rc=$? +[[ "$rc" -ne 0 ]] || { echo "FAIL: removing an absent target toolchain should fail"; exit 1; } +grep -q 'is not installed' /tmp/_rm_target.log || { + cat /tmp/_rm_target.log; echo "FAIL: split form with --target did not reach installed-payload check"; exit 1; } +# The compact spelling remains valid too; after removal it should reach the +# installed-payload check rather than fail spec parsing. +rc=0 +"$MCPP" toolchain remove gcc@16.1.0 > /tmp/_rm_at.log 2>&1 || rc=$? +[[ "$rc" -ne 0 ]] || { echo "FAIL: removing an absent toolchain should fail"; exit 1; } +grep -q 'is not installed' /tmp/_rm_at.log || { + cat /tmp/_rm_at.log; echo "FAIL: @-form remove did not reach installed-payload check"; exit 1; } # 10) After removing, the toolchain dir is gone, but config.toml still # points to it as default. Verify list shows nothing for that compiler now. From bb40f8a9405308caf07333f8556dafe676be696e Mon Sep 17 00:00:00 2001 From: Sunrisepeak Date: Thu, 15 Oct 2026 13:12:04 +0900 Subject: [PATCH 2/2] fix: toolchain remove takes an exact version, and says so when it is missing `remove` does not resolve a partial version the way `default` does (no resolve_version_match): `mcpp toolchain remove gcc 15` with 15.1.0 installed reports "gcc@15 is not installed". The help no longer claims a partial version works. A removal deletes one payload, so it names that payload. A family with no version (`mcpp toolchain remove gcc`) still printed "invalid spec 'gcc'", the message #284 reports as reading like a wrong family name. A spec the parser rejects now carries the parser's own reason, as `default` does, and a family alone says the exact version is missing. e2e 26 asserts the missing-version message and drops the repeated `install gcc@16.1.0`, which only re-ran an install that was already done. --- src/cli.cppm | 8 +++++--- src/toolchain/lifecycle.cppm | 18 ++++++++++++++---- tests/e2e/26_toolchain_management.sh | 10 +++++++--- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/cli.cppm b/src/cli.cppm index 8b090fa81..d076e30d8 100644 --- a/src/cli.cppm +++ b/src/cli.cppm @@ -795,10 +795,12 @@ int run(int argc, char** argv) { "Default build target (omit = host)"))) .subcommand(cl::App("remove") .description("Uninstall a toolchain") - // Match `default`: accept either `gcc@16.1.0` or `gcc 16.1.0`. - // Partial-version spelling follows `default` too. + // Same dual-form as `default`: `gcc@16.1.0` or `gcc 16.1.0`. + // NOT partial versions: `default` resolves `gcc 15` to the + // highest installed 15.x.y, but a removal must name the one + // payload it deletes, so the version here is exact. .arg(cl::Arg("spec") - .help("[@] (version may be partial)") + .help("[@] (exact version)") .required()) .arg(cl::Arg("version").help("(optional, alternative to @-form)")) .option(cl::Option("target").takes_value().help( diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index def7418a2..b0f2a95d5 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -1273,7 +1273,12 @@ export int toolchain_remove(const mcpp::config::GlobalConfig& cfg, const std::string& targetArg = {}) { auto xlEnv = mcpp::config::make_xlings_env(cfg); auto parsedSpec = mcpp::toolchain::parse_toolchain_spec(pos0, pos1); - if (parsedSpec && mcpp::toolchain::is_system_toolchain(*parsedSpec)) { + if (!parsedSpec) { + mcpp::ui::error(std::format( + "{}; e.g. `mcpp toolchain remove gcc 16.1.0`", parsedSpec.error())); + return 2; + } + if (mcpp::toolchain::is_system_toolchain(*parsedSpec)) { mcpp::ui::error( "msvc@system is the machine's own Visual Studio, managed by the " "Visual Studio Installer — mcpp cannot remove it.\n" @@ -1281,9 +1286,14 @@ export int toolchain_remove(const mcpp::config::GlobalConfig& cfg, " mcpp toolchain remove msvc@"); return 1; } - if (!parsedSpec || parsedSpec->version.empty()) { - const auto input = pos1.empty() ? pos0 : std::format("{} {}", pos0, pos1); - mcpp::ui::error(std::format("invalid spec '{}'", input)); + // A family alone is a well-formed spec that names no payload. Say + // that, rather than "invalid spec 'gcc'", which reads as if the family + // itself were wrong (#284). + if (parsedSpec->version.empty()) { + mcpp::ui::error(std::format( + "`toolchain remove` needs the exact version to uninstall; " + "e.g. `mcpp toolchain remove {0} ` or `{0}@`", + mcpp::toolchain::family_name(parsedSpec->family))); return 2; } mcpp::toolchain::print_compat_hint(*parsedSpec); diff --git a/tests/e2e/26_toolchain_management.sh b/tests/e2e/26_toolchain_management.sh index fdd90a4e2..932428742 100755 --- a/tests/e2e/26_toolchain_management.sh +++ b/tests/e2e/26_toolchain_management.sh @@ -60,9 +60,6 @@ if [[ $rc -ne 0 ]]; then exit 1 fi grep -q 'Installed' /tmp/_inst.log || { cat /tmp/_inst.log; echo "FAIL: no Installed line"; exit 1; } -# Both accepted spellings should work when the payload is already installed. -"$MCPP" toolchain install gcc@16.1.0 > /tmp/_inst_at.log 2>&1 || { - cat /tmp/_inst_at.log; echo "FAIL: @-form install failed"; exit 1; } # 4) toolchain list now non-empty out=$("$MCPP" toolchain list 2>&1) @@ -105,6 +102,13 @@ echo "$out" | grep -q 'default toolchain = gcc@16.1.0' || { # 9) Remove rm -rf target +# A family alone names no payload: the error says the version is missing, +# not that the spec is invalid (which reads as a wrong family name, #284). +rc=0 +"$MCPP" toolchain remove gcc > /tmp/_rm_nover.log 2>&1 || rc=$? +[[ "$rc" -eq 2 ]] || { cat /tmp/_rm_nover.log; echo "FAIL: remove without a version should exit 2 (got $rc)"; exit 1; } +grep -q 'needs the exact version' /tmp/_rm_nover.log || { + cat /tmp/_rm_nover.log; echo "FAIL: remove without a version did not name the missing version"; exit 1; } "$MCPP" toolchain remove gcc 16.1.0 > /tmp/_rm.log 2>&1 grep -q 'Removed' /tmp/_rm.log || { cat /tmp/_rm.log; echo "FAIL: no Removed msg for space-separated form"; exit 1; } # The issue's cross-target spelling should parse too and reach the payload