Skip to content

Commit fabffe3

Browse files
fix: route mcpp add's existence check through the real index resolution
The check probed `read_xpkg_lua(ns, shortName)` directly — the narrowest lookup in the codebase — so it refused two classes of packages that resolve perfectly well at build time: * dotted selectors. `capi.lua` is a namespace path meaning `(mcpplibs.capi, lua)`, then `(capi, lua)`; nothing in any index is NAMED "capi.lua", because `package.name` is a single atomic segment (SPEC-001 §3.2). The literal probe could never match one, and mcpp's own mcpp.toml is written in that form. * anything served by a project `[indices]` entry. The probe only ever read the global registry, while dependency resolution dispatches across local-path, project-clone and global transports. That routing rule now lives in `mcpp.pm.index_route` and both callers go through it, so `mcpp add` and `mcpp build` cannot drift apart again about which packages are real. `[indices]` workspace inheritance likewise moves to `mcpp.project` so the two readers share one copy. The gate also only refuses when absence was PROVEN. A lazily-cloned git index, or a namespace no readable index covers (xim descriptors declare no `namespace` at all, so `(xim, nasm)` can never satisfy the identity gate), now reports "unverified" instead of rejecting — the same fall-through prepare already applies to lazy git indices. Around that: * refresh a stale registry once before refusing, and only when the registry is the index that would have answered, so a package already on disk costs zero network round-trips; * report the identities tried plus a cross-namespace did-you-mean, matching prepare's resolution error; * warn — never fail — when the requested version isn't published for this platform, listing what is. Version tables are per-OS, so "absent here" is not "absent". e2e 12 serves every package from a local `[indices] path`, so it covers the dotted and project-index routes offline instead of depending on the state of the shared registry, and drops its isolated MCPP_HOME, which would otherwise force a full sandbox bootstrap (xlings, index fetch, patchelf, ninja) on a test that is otherwise local file manipulation. Co-authored-by: speak-agent <248744407+speak-agent@users.noreply.github.com>
1 parent 5032fe6 commit fabffe3

6 files changed

Lines changed: 636 additions & 109 deletions

File tree

src/build/prepare.cppm

Lines changed: 23 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import mcpp.fetcher;
3939
import mcpp.fetcher.progress;
4040
import mcpp.pm.resolver;
4141
import mcpp.pm.index_spec;
42+
import mcpp.pm.index_route;
4243
import mcpp.pm.mangle;
4344
import mcpp.pm.compat;
4445
import mcpp.pm.dep_spec;
@@ -694,20 +695,10 @@ prepare_build(bool print_fingerprint,
694695
m->targetOverrides[triple] = entry;
695696
}
696697
}
697-
// Inherit workspace indices if member doesn't define any. A
698-
// relative `[indices].path` was declared at the WORKSPACE root,
699-
// so it must resolve against `*root` (still the workspace root
700-
// here), not the member directory — otherwise every member
701-
// needs its own `../`-prefixed copy of the same declaration
702-
// (#224).
703-
if (m->indices.empty() && !wsManifest->indices.empty()) {
704-
m->indices = wsManifest->indices;
705-
for (auto& [_, idx] : m->indices) {
706-
if (idx.is_local() && idx.path.is_relative()) {
707-
idx.path = std::filesystem::weakly_canonical(*root / idx.path);
708-
}
709-
}
710-
}
698+
// Inherit workspace indices if member doesn't define any. `*root`
699+
// is still the workspace root here, which is what a relative
700+
// `[indices].path` was written against (#224).
701+
mcpp::project::inherit_workspace_indices(*m, *wsManifest, *root);
711702

712703
mcpp::ui::status("Workspace", std::format("building member '{}'", targetMember));
713704
root = memberDir;
@@ -730,14 +721,7 @@ prepare_build(bool print_fingerprint,
730721
}
731722
}
732723
// Inherit workspace indices if member doesn't define any
733-
if (m->indices.empty() && !wsm->indices.empty()) {
734-
m->indices = wsm->indices;
735-
for (auto& [_, idx] : m->indices) {
736-
if (idx.is_local() && idx.path.is_relative()) {
737-
idx.path = std::filesystem::weakly_canonical(wsRoot / idx.path);
738-
}
739-
}
740-
}
724+
mcpp::project::inherit_workspace_indices(*m, *wsm, wsRoot);
741725
}
742726
}
743727
}
@@ -1455,28 +1439,19 @@ prepare_build(bool print_fingerprint,
14551439
// different version is needed. Returns the dep's effective root (where
14561440
// mcpp.toml lives) and a fully loaded manifest.
14571441
using LoadedDep = std::pair<std::filesystem::path, mcpp::manifest::Manifest>;
1458-
// Helper: find the IndexSpec for a namespace from the manifest's [indices].
1459-
// Returns nullptr if the namespace maps to the default/builtin index.
1442+
// Index routing — WHICH index answers for a namespace and how its
1443+
// descriptors are read — lives in mcpp.pm.index_route, shared with the
1444+
// `mcpp add` existence gate so the two cannot disagree about which
1445+
// packages are real (#305/#307). `cfg` is filled in per call: the route is
1446+
// rebuilt on demand because `root` moves when a workspace member is
1447+
// selected above.
1448+
auto index_route = [&](mcpp::config::GlobalConfig* cfg = nullptr) {
1449+
return mcpp::pm::IndexRoute{ &m->indices, *root, cfg };
1450+
};
14601451
auto findIndexForNs = [&](const std::string& ns)
14611452
-> const mcpp::pm::IndexSpec*
14621453
{
1463-
if (ns.empty() || ns == std::string(mcpp::pm::kDefaultNamespace)) {
1464-
// R6: `[indices] default = {...}` (normalized to
1465-
// kDefaultNamespace by toml.cppm) redirects the default
1466-
// namespace — return it when present instead of unconditionally
1467-
// falling back to the builtin index.
1468-
auto it = m->indices.find(std::string(mcpp::pm::kDefaultNamespace));
1469-
return it == m->indices.end() ? nullptr : &it->second;
1470-
}
1471-
if (auto it = m->indices.find(ns); it != m->indices.end()) {
1472-
return &it->second;
1473-
}
1474-
auto root = ns.substr(0, ns.find('.'));
1475-
for (auto& [idxName, spec] : m->indices) {
1476-
if (idxName == ns) return &spec;
1477-
if (idxName == root) return &spec;
1478-
}
1479-
return nullptr;
1454+
return index_route().find_for_ns(ns);
14801455
};
14811456

14821457
// Identity-first candidate probe. A candidate is DISAMBIGUATED by the
@@ -1510,19 +1485,7 @@ prepare_build(bool print_fingerprint,
15101485
{
15111486
auto cfg = get_cfg();
15121487
if (!cfg) return std::nullopt;
1513-
1514-
auto* idxSpec = findIndexForNs(coord.namespace_);
1515-
if (idxSpec && idxSpec->is_local()) {
1516-
auto indexPath = mcpp::config::resolve_project_index_path(*root, *idxSpec);
1517-
return mcpp::fetcher::Fetcher::read_xpkg_lua_from_path(
1518-
indexPath, coord.namespace_, coord.shortName);
1519-
}
1520-
if (idxSpec && !idxSpec->is_builtin()) {
1521-
return mcpp::fetcher::Fetcher::read_xpkg_lua_from_project_data(
1522-
*root, coord.namespace_, coord.shortName);
1523-
}
1524-
mcpp::fetcher::Fetcher fetcher(**cfg);
1525-
return fetcher.read_xpkg_lua(coord.namespace_, coord.shortName);
1488+
return index_route(*cfg).read(coord);
15261489
};
15271490

15281491
auto xpkgLuaMatchesCandidate =
@@ -1613,14 +1576,10 @@ prepare_build(bool print_fingerprint,
16131576
// package that would have materialized a moment later. Local path
16141577
// indices and the builtin index are both readable here, so they stay
16151578
// under the strict rule below.
1616-
bool anyLazyGitIndex = false;
1617-
for (auto& c : candidates) {
1618-
auto* idx = findIndexForNs(c.namespace_);
1619-
if (idx && !idx->is_builtin() && !idx->is_local()) {
1620-
anyLazyGitIndex = true;
1621-
break;
1622-
}
1623-
}
1579+
bool anyLazyGitIndex = std::ranges::any_of(candidates,
1580+
[&](const mcpp::pm::DependencyCoordinate& c) {
1581+
return index_route().lazy_git(c.namespace_);
1582+
});
16241583

16251584
// T9 (#278) — no candidate resolved. This used to fall through to
16261585
// `candidates.front()` SILENTLY, so mcpp carried on with a namespace
@@ -1641,17 +1600,8 @@ prepare_build(bool print_fingerprint,
16411600
// error string (see Fetcher::scan_fqns_with_short_name).
16421601
std::string hint;
16431602
if (auto cfg = get_cfg()) {
1644-
mcpp::fetcher::Fetcher fetcher(**cfg);
1645-
auto roots = fetcher.builtin_index_roots();
1646-
for (auto& [idxName, idxSpec] : m->indices) {
1647-
if (idxSpec.is_local()) {
1648-
roots.push_back(
1649-
mcpp::config::resolve_project_index_path(
1650-
*root, idxSpec));
1651-
}
1652-
}
1653-
auto fqns = mcpp::fetcher::Fetcher::scan_fqns_with_short_name(
1654-
roots, candidates.front().shortName);
1603+
auto fqns = mcpp::pm::cross_namespace_matches(
1604+
index_route(*cfg), candidates.front().shortName);
16551605
if (!fqns.empty()) {
16561606
hint += "\n a package with this name exists under "
16571607
"another namespace:";

src/pm/commands.cppm

Lines changed: 127 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,22 +7,68 @@
77
// into the pm subsystem so `cli.cppm` is responsible only for the
88
// global CLI framework + non-pm commands.
99
//
10-
// Strict zero-behavior-change move: every line below is identical to
11-
// what previously lived in `cli.cppm`, only the surrounding namespace
12-
// has changed.
10+
// The bodies started life as a strict zero-behavior-change move out of
11+
// `cli.cppm`; `cmd_add` has since grown the index existence gate (#305).
1312

1413
export module mcpp.pm.commands;
1514

1615
import std;
1716
import mcpp.config;
18-
import mcpp.fetcher;
19-
import mcpp.fetcher.progress;
20-
import mcpp.manifest; // kDefaultNamespace alias
17+
import mcpp.fetcher.progress; // bootstrap progress for load_or_init
18+
import mcpp.manifest; // kDefaultNamespace alias
2119
import mcpp.lockfile; // load / write (still via shim)
20+
import mcpp.platform.axis; // HostPlatform for the published-version check
21+
import mcpp.pm.dep_spec; // DependencyCoordinate
22+
import mcpp.pm.dependency_selector; // same candidates the manifest parser derives
23+
import mcpp.pm.index_route; // shared index routing (with mcpp.build.prepare)
24+
import mcpp.pm.resolver; // is_version_constraint
2225
import mcpp.project; // shared find_manifest_root
2326
import mcpp.ui;
27+
import mcpp.xlings; // index freshness
2428
import mcpplibs.cmdline;
2529

30+
namespace mcpp::pm::commands::detail {
31+
32+
// Render candidate coordinates the way prepare's resolution error does, so a
33+
// rejected `mcpp add` and a failed `mcpp build` name the same identities.
34+
inline std::string format_tried(
35+
const std::vector<mcpp::pm::DependencyCoordinate>& candidates) {
36+
std::string tried;
37+
for (auto& c : candidates) {
38+
if (!tried.empty()) tried += ", ";
39+
tried += c.namespace_.empty()
40+
? std::format("(no namespace, {})", c.shortName)
41+
: std::format("({}, {})", c.namespace_, c.shortName);
42+
}
43+
return tried;
44+
}
45+
46+
// A version the descriptor does not publish is worth flagging but not worth
47+
// refusing over: version tables are per-OS, so "absent for this host" is not
48+
// "absent". Say what IS published and let the user decide — the alternative is
49+
// a hard failure on a dependency that resolves fine on the platform it was
50+
// added for.
51+
inline void warn_unpublished_version(std::string_view lua,
52+
std::string_view display,
53+
const std::string& version) {
54+
if (mcpp::pm::is_version_constraint(version)) return;
55+
auto versions = mcpp::manifest::list_xpkg_versions(
56+
lua, mcpp::platform::HostPlatform::current());
57+
if (versions.empty()) return;
58+
if (std::ranges::find(versions, version) != versions.end()) return;
59+
60+
std::string avail;
61+
for (auto& v : versions) {
62+
if (!avail.empty()) avail += ", ";
63+
avail += v;
64+
}
65+
mcpp::ui::warning(std::format(
66+
"'{}' has no version {} published for this platform — available: {}",
67+
display, version, avail));
68+
}
69+
70+
} // namespace mcpp::pm::commands::detail
71+
2672
export namespace mcpp::pm::commands {
2773

2874
inline int cmd_add(const mcpplibs::cmdline::ParsedArgs& parsed) {
@@ -71,22 +117,91 @@ inline int cmd_add(const mcpplibs::cmdline::ParsedArgs& parsed) {
71117
return 2;
72118
}
73119

74-
// Validate package existence against the configured index before mutating
75-
// mcpp.toml. A missing package is a hard error: we don't want to write an
76-
// invalid dependency that only fails later during build.
120+
// ── Existence gate (#305) ──────────────────────────────────────────
121+
// Refuse to write a dependency no index can serve, so a typo fails here
122+
// instead of halfway into the next `mcpp build`. Two rules keep the gate
123+
// from refusing packages that are perfectly real:
124+
//
125+
// • It probes the SAME candidates the manifest parser will derive from
126+
// the key about to be written. A dotted selector is a namespace path,
127+
// not a name: `capi.lua` means `(mcpplibs.capi, lua)` then
128+
// `(capi, lua)`, and a literal `(mcpplibs, "capi.lua")` probe can
129+
// never match one — `package.name` is a single atomic segment
130+
// (SPEC-001 §3.2), so nothing in any index is named "capi.lua".
131+
// • It reads through mcpp.pm.index_route, the same routing
132+
// `mcpp.build.prepare` resolves dependencies with. A package served by
133+
// a project `[indices]` entry therefore counts as present, and a
134+
// namespace no readable index can speak for is reported as unverified
135+
// rather than rejected: refusing an add is only correct when absence
136+
// was actually proven.
77137
{
138+
auto selector = explicitNamespace
139+
? mcpp::pm::make_direct_dependency_selector(ns, shortName, nameSpec)
140+
: mcpp::pm::resolve_dependency_selector(
141+
nameSpec,
142+
mcpp::pm::DependencySelectorMode::OmittedMcpplibsPriority);
143+
78144
auto cfg = mcpp::config::load_or_init(
79145
/*quiet=*/false, mcpp::fetcher::make_bootstrap_progress_callback());
80146
if (!cfg) {
81147
mcpp::ui::error(cfg.error().message);
82148
return 4;
83149
}
84-
mcpp::fetcher::Fetcher f(*cfg);
85-
if (!f.read_xpkg_lua(ns, shortName)) {
150+
151+
auto indices = mcpp::pm::effective_indices(*root);
152+
mcpp::pm::IndexRoute route{ &indices, *root, &*cfg };
153+
auto found = mcpp::pm::lookup_descriptor(route, selector.candidates);
154+
155+
// Does the shared registry answer for any identity we tried? It is the
156+
// only index a refresh can do anything about — a project
157+
// `[indices] path = …` is whatever the user has on disk.
158+
const bool registryInvolved = std::ranges::any_of(selector.candidates,
159+
[&](const mcpp::pm::DependencyCoordinate& c) {
160+
auto* idx = route.find_for_ns(c.namespace_);
161+
return !idx || idx->is_builtin();
162+
});
163+
164+
// Only pay for a refresh when the answer was "no" and the registry
165+
// that would have answered is stale — a package that is already on
166+
// disk costs zero network round-trips.
167+
if (!found.hit && found.conclusive && registryInvolved) {
168+
auto xlEnv = mcpp::config::make_xlings_env(*cfg);
169+
if (!mcpp::xlings::is_index_fresh(xlEnv, cfg->searchTtlSeconds)) {
170+
mcpp::ui::status("Updating", "package index (auto-refresh)");
171+
mcpp::xlings::ensure_index_fresh(
172+
xlEnv, cfg->searchTtlSeconds, /*quiet=*/true);
173+
found = mcpp::pm::lookup_descriptor(route, selector.candidates);
174+
}
175+
}
176+
177+
if (!found.hit && found.conclusive) {
178+
std::string hint;
179+
if (!selector.candidates.empty()) {
180+
for (auto& fqn : mcpp::pm::cross_namespace_matches(
181+
route, selector.candidates.front().shortName)) {
182+
hint += "\n " + fqn;
183+
}
184+
}
185+
if (!hint.empty()) {
186+
hint = "\n a package with this name exists under another "
187+
"namespace:" + hint;
188+
}
189+
if (registryInvolved) {
190+
hint += "\n hint: `mcpp index update` if it was published "
191+
"recently";
192+
}
86193
mcpp::ui::error(std::format(
87-
"package '{}' not found in any configured index", shortName));
194+
"package '{}' not found in any configured index\n tried: {}{}",
195+
nameSpec, detail::format_tried(selector.candidates), hint));
88196
return 2;
89197
}
198+
if (!found.hit) {
199+
mcpp::ui::warning(std::format(
200+
"'{}' could not be verified — no readable index covers that "
201+
"namespace yet; adding it unchecked", nameSpec));
202+
} else {
203+
detail::warn_unpublished_version(found.hit->lua, nameSpec, version);
204+
}
90205
}
91206

92207
std::ifstream in(manifestPath);

0 commit comments

Comments
 (0)