Skip to content

Commit 0d33212

Browse files
committed
fix: mcpp self config --mirror X hangs on fresh MCPP_HOME
Root cause: `cmd_self_config` called the full `load_or_init`, whose step 7 (xlings sandbox bootstrap — `xlings self init`, ensure_patchelf, ensure_ninja) hits the network using the *current* `.xlings.json` mirror. On a fresh MCPP_HOME that's the seeded default `"CN"`, so the act of running `mcpp self config --mirror GLOBAL` would download from CN before having a chance to switch — the very chicken-and-egg the command is supposed to break. Fix: - `config::load_or_init` grows a `skip_xlings_bootstrap` parameter (default false, all existing callers unchanged). When true, steps 1-6 still run (dirs / config.toml / .xlings.json seed / xlings binary acquisition — all local) but step 7 (the network roundtrip) is skipped. - New `xlings::set_mirror_in_xlings_json(path, mirror)`: in-place text edit of just the `"mirror"` field, preserving every other key (no subprocess, no network, safe pre-bootstrap). - `cmd_self_config` rewritten: * `--mirror X` path: `load_or_init(skip_xlings_bootstrap=true)` then `set_mirror_in_xlings_json`. Pure local file ops, completes in ms. * no-arg show path: same `skip_xlings_bootstrap=true`, since `xlings config` only reads `.xlings.json` (doesn't need patchelf or ninja). The next mcpp invocation that genuinely needs bootstrap (build / run / toolchain install) runs the full bootstrap with the just-saved mirror in effect. TODO comments added at both `seed_xlings_json` and `cmd_self_config` about the long-term direction of the default mirror — either flip to `GLOBAL` (with docs covering the CN opt-in switch) or auto-detect (env hint + a quick HEAD probe with a tight timeout, pinning the winning value). Test: tests/e2e/46_self_config_mirror_no_bootstrap.sh — fresh MCPP_HOME, runs `mcpp self config --mirror GLOBAL` under a 10s timeout, asserts .xlings.json picks up GLOBAL and that xim-x-patchelf / xim-x-ninja were NOT installed by this command (proving no network bootstrap fired). Round-trips back to CN as a second pure-local edit. Existing test 38_self_config_mirror.sh still passes: it always called `mcpp self env` first (which uses the original full-bootstrap path), so the "default mirror is CN" invariant is preserved.
1 parent 73ffe30 commit 0d33212

4 files changed

Lines changed: 199 additions & 24 deletions

File tree

src/cli.cppm

Lines changed: 47 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4355,33 +4355,58 @@ std::string upper_ascii(std::string s) {
43554355
}
43564356

43574357
int cmd_self_config(const mcpplibs::cmdline::ParsedArgs& parsed) {
4358-
auto cfg = mcpp::config::load_or_init(/*quiet=*/false, make_bootstrap_progress_callback());
4359-
if (!cfg) {
4360-
mcpp::ui::error(cfg.error().message);
4361-
return 4;
4362-
}
4363-
4364-
auto env = mcpp::config::make_xlings_env(*cfg);
43654358
auto mirror = parsed.option_or_empty("mirror").value();
4366-
if (mirror.empty()) {
4367-
auto rc = mcpp::xlings::config_show(env);
4368-
return rc == 0 ? 0 : 1;
4369-
}
43704359

4371-
mirror = upper_ascii(std::move(mirror));
4372-
if (mirror != "CN" && mirror != "GLOBAL") {
4373-
mcpp::ui::error(std::format(
4374-
"invalid mirror '{}'; expected CN or GLOBAL", mirror));
4375-
return 2;
4360+
// `--mirror` write path: deliberately skip the xlings sandbox bootstrap
4361+
// (patchelf + ninja install over the network) so that switching the
4362+
// mirror does NOT itself trigger the very network roundtrip the user
4363+
// is trying to redirect. Mirror is persisted directly into .xlings.json
4364+
// — no xlings subprocess needed for that, and the next mcpp command
4365+
// (build / run / toolchain install) will run the bootstrap with the
4366+
// chosen mirror in effect.
4367+
//
4368+
// TODO(mirror-default): see the matching comment on seed_xlings_json
4369+
// for the long-term plan (flip default to GLOBAL or auto-detect).
4370+
if (!mirror.empty()) {
4371+
mirror = upper_ascii(std::move(mirror));
4372+
if (mirror != "CN" && mirror != "GLOBAL") {
4373+
mcpp::ui::error(std::format(
4374+
"invalid mirror '{}'; expected CN or GLOBAL", mirror));
4375+
return 2;
4376+
}
4377+
4378+
auto cfg = mcpp::config::load_or_init(
4379+
/*quiet=*/true,
4380+
make_bootstrap_progress_callback(),
4381+
/*skip_xlings_bootstrap=*/true);
4382+
if (!cfg) {
4383+
mcpp::ui::error(cfg.error().message);
4384+
return 4;
4385+
}
4386+
4387+
auto xlingsJson = cfg->xlingsHome() / ".xlings.json";
4388+
if (!mcpp::xlings::set_mirror_in_xlings_json(xlingsJson, mirror)) {
4389+
mcpp::ui::error(std::format(
4390+
"failed to update mirror in '{}'", xlingsJson.string()));
4391+
return 1;
4392+
}
4393+
mcpp::ui::status("Configured", std::format("xlings mirror = {}", mirror));
4394+
return 0;
43764395
}
43774396

4378-
auto rc = mcpp::xlings::config_set_mirror(env, mirror, /*quiet=*/true);
4379-
if (rc != 0) {
4380-
mcpp::ui::error(std::format("failed to set xlings mirror to {}", mirror));
4381-
return 1;
4397+
// Show path: also takes the lite init — `xlings config` only reads
4398+
// .xlings.json, so the sandbox bootstrap isn't required for it either.
4399+
auto cfg = mcpp::config::load_or_init(
4400+
/*quiet=*/false,
4401+
make_bootstrap_progress_callback(),
4402+
/*skip_xlings_bootstrap=*/true);
4403+
if (!cfg) {
4404+
mcpp::ui::error(cfg.error().message);
4405+
return 4;
43824406
}
4383-
mcpp::ui::status("Configured", std::format("xlings mirror = {}", mirror));
4384-
return 0;
4407+
auto env = mcpp::config::make_xlings_env(*cfg);
4408+
auto rc = mcpp::xlings::config_show(env);
4409+
return rc == 0 ? 0 : 1;
43854410
}
43864411

43874412
// Used both by `mcpp explain <CODE>` (top-level) and `mcpp self explain

src/config.cppm

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,17 @@ using BootstrapFile = mcpp::xlings::BootstrapFile;
186186
using BootstrapProgress = mcpp::xlings::BootstrapProgress;
187187
using BootstrapProgressCallback = mcpp::xlings::BootstrapProgressCallback;
188188

189+
// `skip_xlings_bootstrap`: when true, steps 1-6 still run (dirs, config.toml,
190+
// .xlings.json seed, xlings binary placement — all local, no network), but
191+
// step 7 (xlings self init + ensure_patchelf + ensure_ninja, which all hit
192+
// the network) is skipped. Used by `mcpp self config --mirror` so the user
193+
// can change the mirror BEFORE the very first network roundtrip — otherwise
194+
// the act of running `self config` would itself trigger a CN-mirror
195+
// bootstrap that they were trying to avoid (chicken-and-egg).
189196
std::expected<GlobalConfig, ConfigError> load_or_init(
190197
bool quiet = false,
191-
BootstrapProgressCallback onBootstrapProgress = {});
198+
BootstrapProgressCallback onBootstrapProgress = {},
199+
bool skip_xlings_bootstrap = false);
192200

193201
// Pretty-print resolved config for `mcpp env` command.
194202
void print_env(const GlobalConfig& cfg);
@@ -385,7 +393,8 @@ void ensure_sandbox_patchelf(const GlobalConfig& cfg, bool quiet,
385393

386394
std::expected<GlobalConfig, ConfigError> load_or_init(
387395
bool quiet,
388-
BootstrapProgressCallback onBootstrapProgress)
396+
BootstrapProgressCallback onBootstrapProgress,
397+
bool skip_xlings_bootstrap)
389398
{
390399
GlobalConfig cfg;
391400

@@ -538,6 +547,13 @@ std::expected<GlobalConfig, ConfigError> load_or_init(
538547
// TODO(xlings-upstream): collapse into a single
539548
// `xlings sandbox bootstrap --home <X>` once that command exists
540549
// upstream (see docs/short-term-vs-long-track plan).
550+
//
551+
// Skipped by `mcpp self config --mirror` so that command never hits
552+
// the network. The next mcpp invocation that needs bootstrap (build,
553+
// run, toolchain install) will run this block with the just-saved
554+
// mirror in effect.
555+
if (skip_xlings_bootstrap) return cfg;
556+
541557
ensure_sandbox_xlings_binary(cfg, quiet);
542558
ensure_sandbox_init(cfg, quiet);
543559
{

src/xlings.cppm

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,10 +197,34 @@ int install_with_progress(const Env& env, std::string_view target,
197197
// ─── Sandbox lifecycle ──────────────────────────────────────────────
198198

199199
// Write .xlings.json seed file.
200+
//
201+
// TODO(mirror-default): the default `"CN"` here is the historical setting
202+
// for the project's initial Chinese-mainland user base, but it bites
203+
// overseas users (and CI on GitHub-hosted runners) — the first network
204+
// roundtrip goes through a CN mirror that is slow/unreachable for them.
205+
// Two reasonable future directions:
206+
// (a) flip the default to `"GLOBAL"` and have CN users opt in via
207+
// `mcpp self config --mirror CN` (smaller blast radius once docs
208+
// cover the switch);
209+
// (b) auto-detect on first init (env hint like `LANG`, a quick HEAD
210+
// to github.com vs. ghproxy with a tight timeout, and pin the
211+
// winning value into .xlings.json).
212+
// Either way, this default is a deliberate placeholder, not a final
213+
// decision. Discussion: ../docs/ (none yet — open an issue when ready).
200214
void seed_xlings_json(const Env& env,
201215
std::span<const std::pair<std::string,std::string>> repos,
202216
std::string_view mirror = "CN");
203217

218+
// Update only the `"mirror"` field of an existing .xlings.json in place,
219+
// preserving every other key (including any user-added index_repos that
220+
// were not seeded by mcpp). If the file is absent or unparseable, the
221+
// caller is responsible for having seeded it first; this function returns
222+
// false in that case so the caller can fall back to seed_xlings_json.
223+
//
224+
// No subprocess. No network. Safe to call before xlings sandbox bootstrap.
225+
bool set_mirror_in_xlings_json(const std::filesystem::path& xlingsJson,
226+
std::string_view mirror);
227+
204228
// Persist the xlings mirror selection in .xlings.json via xlings itself.
205229
int config_show(const Env& env);
206230
int config_set_mirror(const Env& env, std::string_view mirror, bool quiet = false);
@@ -790,6 +814,43 @@ void seed_xlings_json(const Env& env,
790814
write_file(path, json);
791815
}
792816

817+
bool set_mirror_in_xlings_json(const std::filesystem::path& xlingsJson,
818+
std::string_view mirror)
819+
{
820+
std::error_code ec;
821+
if (!std::filesystem::exists(xlingsJson, ec)) return false;
822+
823+
std::ifstream is(xlingsJson);
824+
if (!is) return false;
825+
std::stringstream ss; ss << is.rdbuf();
826+
std::string text = ss.str();
827+
828+
// Find the existing `"mirror"` key and replace just its string value,
829+
// preserving every other character (whitespace, key order, comments
830+
// would survive — though .xlings.json is strict JSON). The format we
831+
// emit in seed_xlings_json always contains a mirror key, so this path
832+
// is the expected one. Callers that see false should re-seed the file.
833+
static constexpr std::string_view kKey = "\"mirror\"";
834+
auto keyPos = text.find(kKey);
835+
if (keyPos == std::string::npos) return false;
836+
837+
auto p = keyPos + kKey.size();
838+
while (p < text.size() && (text[p] == ' ' || text[p] == '\t' || text[p] == ':'))
839+
++p;
840+
if (p >= text.size() || text[p] != '"') return false;
841+
842+
auto valStart = p + 1;
843+
auto valEnd = text.find('"', valStart);
844+
if (valEnd == std::string::npos) return false;
845+
846+
text.replace(valStart, valEnd - valStart, mirror);
847+
848+
std::ofstream os(xlingsJson, std::ios::trunc);
849+
if (!os) return false;
850+
os << text;
851+
return static_cast<bool>(os);
852+
}
853+
793854
int config_show(const Env& env) {
794855
auto cmd = std::format("{} config", build_command_prefix(env));
795856
return mcpp::platform::process::run_silent(cmd);
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
#!/usr/bin/env bash
2+
# requires:
3+
# 46_self_config_mirror_no_bootstrap.sh — `mcpp self config --mirror X` must
4+
# NOT trigger the xlings sandbox bootstrap (patchelf + ninja download). The
5+
# whole point of this command is to let the user redirect the mirror BEFORE
6+
# the first network roundtrip; the previous implementation made it
7+
# impossible to do so because the command itself would download from CN.
8+
set -e
9+
10+
TMP=$(mktemp -d)
11+
trap "rm -rf $TMP" EXIT
12+
13+
# Truly fresh MCPP_HOME — no inherited toolchain, no inherited bootstrap.
14+
export MCPP_HOME="$TMP/fresh-mcpp-home"
15+
unset MCPP_VENDORED_XLINGS # the test must work without a pre-staged xlings
16+
17+
# 10s budget. Real local file writes take milliseconds; if the command
18+
# tries to install patchelf / ninja it will blow this budget. timeout(124)
19+
# is distinguished from a normal error so the failure message is clear.
20+
TIMEOUT_CMD=""
21+
if command -v timeout &>/dev/null; then TIMEOUT_CMD=timeout
22+
elif command -v gtimeout &>/dev/null; then TIMEOUT_CMD=gtimeout
23+
fi
24+
if [[ -z "$TIMEOUT_CMD" ]]; then
25+
# No timeout available — proceed without it; the e2e runner caps the test.
26+
"$MCPP" self config --mirror GLOBAL > "$TMP/out.log" 2>&1
27+
rc=$?
28+
else
29+
"$TIMEOUT_CMD" 10 "$MCPP" self config --mirror GLOBAL > "$TMP/out.log" 2>&1
30+
rc=$?
31+
fi
32+
if [[ $rc -ne 0 ]]; then
33+
cat "$TMP/out.log"
34+
if [[ $rc -eq 124 ]]; then
35+
echo "FAIL: 'self config --mirror' hung (>10s) — it should be a pure local write, not bootstrap"
36+
else
37+
echo "FAIL: 'self config --mirror' exited $rc on fresh MCPP_HOME"
38+
fi
39+
exit 1
40+
fi
41+
42+
# .xlings.json must exist with the requested mirror.
43+
grep -q '"mirror": "GLOBAL"' "$MCPP_HOME/registry/.xlings.json" || {
44+
cat "$MCPP_HOME/registry/.xlings.json" 2>/dev/null || true
45+
echo "FAIL: .xlings.json did not pick up GLOBAL"
46+
exit 1
47+
}
48+
49+
# Crucial: bootstrap tools must NOT have been installed by this command.
50+
# If they were, the user's mirror choice came too late to matter.
51+
for tool in xim-x-patchelf xim-x-ninja; do
52+
if [[ -d "$MCPP_HOME/registry/data/xpkgs/$tool" ]]; then
53+
ls "$MCPP_HOME/registry/data/xpkgs/$tool"
54+
echo "FAIL: '$tool' was installed by self config --mirror — that path"
55+
echo " defeats the whole purpose of letting the user redirect"
56+
echo " the mirror before the first network roundtrip."
57+
exit 1
58+
fi
59+
done
60+
61+
# Round-trip: switching back to CN should also be a pure local edit.
62+
"$MCPP" self config --mirror CN > "$TMP/cn.log" 2>&1 || {
63+
cat "$TMP/cn.log"
64+
echo "FAIL: switching mirror back to CN failed"
65+
exit 1
66+
}
67+
grep -q '"mirror": "CN"' "$MCPP_HOME/registry/.xlings.json" || {
68+
cat "$MCPP_HOME/registry/.xlings.json"
69+
echo "FAIL: .xlings.json did not pick up CN on switch back"
70+
exit 1
71+
}
72+
73+
echo "OK"

0 commit comments

Comments
 (0)