From 6a0f23af15d8f4d46346e22abb36d3160789fcaa Mon Sep 17 00:00:00 2001 From: sunrisepeak Date: Sun, 24 May 2026 10:48:42 +0800 Subject: [PATCH] fix: `mcpp self config --mirror X` hangs on fresh MCPP_HOME MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: on a fresh MCPP_HOME, write_default_xlings_json seeds .xlings.json with the hardcoded `"CN"` default, regardless of --mirror. Then load_or_init's xlings sandbox bootstrap (ensure_init / patchelf / ninja install) reads that mirror and downloads via CN. Only after the bootstrap finishes does cmd_self_config invoke xlings::config_set_mirror to update the file — too late: the user's --mirror choice never reached the very network roundtrip they were trying to redirect. For an overseas user the CN mirror is slow or unreachable, so the "download" phase hangs. Fix (~15 lines): plumb the user's mirror choice through to the seed. - `config::write_default_xlings_json` grows `mirror_override` param. - `config::load_or_init` grows `initial_mirror` param (default empty; all existing callers unchanged). Passed straight through to write_default_xlings_json, so when set it overrides the "CN" default for the FRESH seed. - `cmd_self_config` parses --mirror first, validates, then forwards it as load_or_init's initial_mirror. The subsequent xlings::config_set_mirror still runs (idempotent on fresh, required for the already-initialized case where seed was skipped). - TODO comment at seed_xlings_json's `"CN"` default about the long-term direction (flip default to GLOBAL, or auto-detect). For an already-initialized MCPP_HOME (`.xlings.json` already exists), the seed is skipped — no behavior change there. config_set_mirror handles that case as before. Test: tests/e2e/46_self_config_mirror_no_bootstrap.sh asserts that after a fresh `mcpp self config --mirror GLOBAL`, the seeded .xlings.json has `mirror: GLOBAL` (not the default CN). Then round-trips back to CN to exercise the post-init path. Existing 38_self_config_mirror.sh still passes: it always calls `mcpp self env` first, which doesn't pass an initial_mirror, so the "default seed is CN" invariant is preserved. --- src/cli.cppm | 32 ++++++++---- src/config.cppm | 23 ++++++-- src/xlings.cppm | 14 +++++ .../e2e/46_self_config_mirror_no_bootstrap.sh | 52 +++++++++++++++++++ 4 files changed, 107 insertions(+), 14 deletions(-) create mode 100755 tests/e2e/46_self_config_mirror_no_bootstrap.sh diff --git a/src/cli.cppm b/src/cli.cppm index 08ff3ae..dbce2fb 100644 --- a/src/cli.cppm +++ b/src/cli.cppm @@ -4355,26 +4355,40 @@ std::string upper_ascii(std::string s) { } int cmd_self_config(const mcpplibs::cmdline::ParsedArgs& parsed) { - auto cfg = mcpp::config::load_or_init(/*quiet=*/false, make_bootstrap_progress_callback()); + auto mirror = parsed.option_or_empty("mirror").value(); + if (!mirror.empty()) { + mirror = upper_ascii(std::move(mirror)); + if (mirror != "CN" && mirror != "GLOBAL") { + mcpp::ui::error(std::format( + "invalid mirror '{}'; expected CN or GLOBAL", mirror)); + return 2; + } + } + + // When --mirror is given AND this is a fresh MCPP_HOME, seed .xlings.json + // with the user's choice on the very first write so the immediately- + // following xlings sandbox bootstrap (patchelf / ninja download) uses + // their mirror — not the historical CN default that an overseas user + // is trying to redirect away from. For an already-initialized MCPP_HOME + // the seed is skipped and config_set_mirror below updates the existing + // file via xlings. + // + // TODO(mirror-default): the default "CN" lives in + // mcpp::xlings::seed_xlings_json — see the matching note there for the + // long-term plan (flip default to GLOBAL, or auto-detect on first init). + auto cfg = mcpp::config::load_or_init( + /*quiet=*/false, make_bootstrap_progress_callback(), mirror); if (!cfg) { mcpp::ui::error(cfg.error().message); return 4; } auto env = mcpp::config::make_xlings_env(*cfg); - auto mirror = parsed.option_or_empty("mirror").value(); if (mirror.empty()) { auto rc = mcpp::xlings::config_show(env); return rc == 0 ? 0 : 1; } - mirror = upper_ascii(std::move(mirror)); - if (mirror != "CN" && mirror != "GLOBAL") { - mcpp::ui::error(std::format( - "invalid mirror '{}'; expected CN or GLOBAL", mirror)); - return 2; - } - auto rc = mcpp::xlings::config_set_mirror(env, mirror, /*quiet=*/true); if (rc != 0) { mcpp::ui::error(std::format("failed to set xlings mirror to {}", mirror)); diff --git a/src/config.cppm b/src/config.cppm index 799df32..834395e 100644 --- a/src/config.cppm +++ b/src/config.cppm @@ -186,9 +186,17 @@ using BootstrapFile = mcpp::xlings::BootstrapFile; using BootstrapProgress = mcpp::xlings::BootstrapProgress; using BootstrapProgressCallback = mcpp::xlings::BootstrapProgressCallback; +// `initial_mirror`: when seeding a *fresh* .xlings.json (file did not +// already exist), use this as the mirror value instead of the default. +// Empty means "use the default". Used by `mcpp self config --mirror X` +// so that the very first network roundtrip (during the immediately- +// following xlings sandbox bootstrap) goes through the user's chosen +// mirror, not the historical CN default. No effect when .xlings.json +// already exists — that case is handled by xlings::config_set_mirror. std::expected load_or_init( bool quiet = false, - BootstrapProgressCallback onBootstrapProgress = {}); + BootstrapProgressCallback onBootstrapProgress = {}, + std::string_view initial_mirror = {}); // Pretty-print resolved config for `mcpp env` command. void print_env(const GlobalConfig& cfg); @@ -310,7 +318,8 @@ default_backend = "ninja" } bool write_default_xlings_json(const std::filesystem::path& path, - const std::vector& repos) + const std::vector& repos, + std::string_view mirror_override = {}) { // Delegate to xlings module. Convert IndexRepo vec to pair span. std::vector> pairs; @@ -320,7 +329,10 @@ bool write_default_xlings_json(const std::filesystem::path& path, // construct a temporary Env with home = path.parent_path(). mcpp::xlings::Env env; env.home = path.parent_path(); - mcpp::xlings::seed_xlings_json(env, pairs); + if (mirror_override.empty()) + mcpp::xlings::seed_xlings_json(env, pairs); + else + mcpp::xlings::seed_xlings_json(env, pairs, mirror_override); return std::filesystem::exists(path); } @@ -385,7 +397,8 @@ void ensure_sandbox_patchelf(const GlobalConfig& cfg, bool quiet, std::expected load_or_init( bool quiet, - BootstrapProgressCallback onBootstrapProgress) + BootstrapProgressCallback onBootstrapProgress, + std::string_view initial_mirror) { GlobalConfig cfg; @@ -507,7 +520,7 @@ std::expected load_or_init( // without losing xlings' active subos or version bindings. auto xjson = cfg.xlingsHome() / ".xlings.json"; if (!std::filesystem::exists(xjson)) { - write_default_xlings_json(xjson, cfg.indexRepos); + write_default_xlings_json(xjson, cfg.indexRepos, initial_mirror); } else { mcpp::fallback::migrate_xlings_json_index_names(xjson); } diff --git a/src/xlings.cppm b/src/xlings.cppm index 1696e85..dc66032 100644 --- a/src/xlings.cppm +++ b/src/xlings.cppm @@ -197,6 +197,20 @@ int install_with_progress(const Env& env, std::string_view target, // ─── Sandbox lifecycle ────────────────────────────────────────────── // Write .xlings.json seed file. +// +// TODO(mirror-default): the default `"CN"` is the historical setting for +// the project's initial Chinese-mainland user base, but it bites overseas +// users (and CI on GitHub-hosted runners) — the first network roundtrip +// goes through a CN mirror that is slow/unreachable for them. The +// `mcpp self config --mirror X` flow now passes the user's choice as an +// override through to here, so they can pick the right mirror BEFORE the +// first download. Longer term, consider: +// (a) flip the default to "GLOBAL" and have CN users opt in via +// `mcpp self config --mirror CN` (smaller blast radius once docs +// cover the switch); or +// (b) auto-detect on first init (env hint like LANG, a quick HEAD probe +// to github.com vs. ghproxy with a tight timeout, and pin the +// winning value into .xlings.json). void seed_xlings_json(const Env& env, std::span> repos, std::string_view mirror = "CN"); diff --git a/tests/e2e/46_self_config_mirror_no_bootstrap.sh b/tests/e2e/46_self_config_mirror_no_bootstrap.sh new file mode 100755 index 0000000..4ab2556 --- /dev/null +++ b/tests/e2e/46_self_config_mirror_no_bootstrap.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# requires: +# 46_self_config_mirror_fresh_seed.sh — `mcpp self config --mirror X` on a +# fresh MCPP_HOME must seed .xlings.json with X from the very first write, +# so the immediately-following sandbox bootstrap (patchelf / ninja +# download) uses the user's chosen mirror instead of the historical CN +# default. +# +# This is the user-visible fix for the "fresh-install mirror config hangs +# on overseas networks" report: previously the seed always wrote +# `"mirror": "CN"` regardless of --mirror, and bootstrap downloads went +# through an unreachable CN proxy before xlings::config_set_mirror had a +# chance to change the value. +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT + +export MCPP_HOME="$TMP/fresh" + +# Sanity: file doesn't exist yet — we're truly fresh. +[[ ! -f "$MCPP_HOME/registry/.xlings.json" ]] || { + echo "FAIL: precondition — registry/.xlings.json already exists"; exit 1; } + +"$MCPP" self config --mirror GLOBAL > "$TMP/out.log" 2>&1 || { + cat "$TMP/out.log" + echo "FAIL: 'self config --mirror GLOBAL' exited non-zero on fresh MCPP_HOME" + exit 1 +} + +# After the command the seeded .xlings.json must reflect GLOBAL, NOT the +# default CN. This is the property the fix guarantees on the seed path — +# the OLD code seeded "CN" first and only switched to GLOBAL via a +# post-bootstrap xlings call, leaving the patchelf/ninja download going +# through CN. +grep -q '"mirror": "GLOBAL"' "$MCPP_HOME/registry/.xlings.json" || { + cat "$MCPP_HOME/registry/.xlings.json" + echo "FAIL: .xlings.json mirror should be GLOBAL after fresh-init --mirror" + exit 1 +} + +# Round-trip back to CN — also via --mirror — exercises the post-init +# path (xlings config --mirror) since .xlings.json now exists. +"$MCPP" self config --mirror CN > "$TMP/cn.log" 2>&1 || { + cat "$TMP/cn.log"; echo "FAIL: switching back to CN failed"; exit 1; } +grep -q '"mirror": "CN"' "$MCPP_HOME/registry/.xlings.json" || { + cat "$MCPP_HOME/registry/.xlings.json" + echo "FAIL: round-trip to CN did not stick" + exit 1 +} + +echo "OK"