Skip to content

feat: reject mcpp add for packages not present in the index - #307

Merged
Sunrisepeak merged 2 commits into
mcpp-community:mainfrom
ZheFeng7110:feat/mcpp-add-check
Jul 29, 2026
Merged

feat: reject mcpp add for packages not present in the index#307
Sunrisepeak merged 2 commits into
mcpp-community:mainfrom
ZheFeng7110:feat/mcpp-add-check

Conversation

@ZheFeng7110

Copy link
Copy Markdown
Member

Summary

  • Validate that the requested package exists in a configured index before mutating mcpp.toml.
  • If the package is not found, print an error and exit without writing the dependency.
  • Propagate config::load_or_init failures instead of silently skipping the check.
  • Update the tests/e2e/12_add_command.sh E2E test to use real packages and add a regression case for the missing-package scenario.

Closes #305

Test plan

  • mcpp build passes
  • tests/e2e/12_add_command.sh passes

Validate that the requested package exists in a configured index before
mutating mcpp.toml. If the package is not found, print an error and exit
without writing the dependency. Also propagate config::load_or_init failures.

Update tests/e2e/12_add_command.sh to use real packages and add a
regression case for the missing-package scenario. Update
23_remove_update.sh to seed fake dependencies directly in mcpp.toml so
remove/update remain tested independently of the add index check.

Closes mcpp-community#305
@ZheFeng7110
ZheFeng7110 force-pushed the feat/mcpp-add-check branch from ed248a3 to 5032fe6 Compare July 29, 2026 07:25

@speak-agent speak-agent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Right problem (#305), but the gate uses the narrowest lookup path in the codebase, so two classes of valid packages are now rejected.

1. Dotted selectors are rejected. cmd_add passes the whole selector as shortName, but read_xpkg_lua's identity gate requires id.name == shortName and SPEC-001 makes package.name a single atomic segment. capi.lua is a real index entry (namespace = "mcpplibs.capi", name = "lua") and a documented form; verified through the same lookup: mcpp new probe --template capi.luatemplate package 'capi.lua' not found in the index. The PR also deletes the e2e case that covered this (added by 37cbc83) and contradicts the comment directly above it.

}
mcpp::fetcher::Fetcher f(*cfg);
if (!f.read_xpkg_lua(ns, shortName)) {
mcpp::ui::error(std::format(
"package '{}' not found in any configured index", shortName));
return 2;

mcpp/docs/05-mcpp-toml.md

Lines 305 to 311 in fde3b70

# Dotted selector: try mcpplibs.<path> first, then fall back to the sibling peer root.
# For example, imgui.core is tried in order as mcpplibs.imgui/core, then imgui/core.
[dependencies]
capi.lua = "0.0.3"
compat.gtest = "1.15.2"
imgui.core = "0.0.1"
imgui.backend.glfw_opengl3 = "0.0.1"

2. [indices] custom/local indices are ignored. Build-time lookup is a three-way dispatch (local path index / project data / global); the gate only uses the global one and never reads [indices] from mcpp.toml. In the 120_ws_root_indices.sh layout, mcpp add x:widget2@1.0.0 fails while mcpp build resolves it.

mcpp/src/build/prepare.cppm

Lines 1513 to 1526 in fde3b70

auto* idxSpec = findIndexForNs(coord.namespace_);
if (idxSpec && idxSpec->is_local()) {
auto indexPath = mcpp::config::resolve_project_index_path(*root, *idxSpec);
return mcpp::fetcher::Fetcher::read_xpkg_lua_from_path(
indexPath, coord.namespace_, coord.shortName);
}
if (idxSpec && !idxSpec->is_builtin()) {
return mcpp::fetcher::Fetcher::read_xpkg_lua_from_project_data(
*root, coord.namespace_, coord.shortName);
}
mcpp::fetcher::Fetcher fetcher(**cfg);
return fetcher.read_xpkg_lua(coord.namespace_, coord.shortName);
};

3. No index refresh before the lookup. mcpp build and mcpp index search call ensure_index_fresh first; the gate only calls load_or_init. A stale or newly published entry yields a false "not found", and the message gives no way out — compare create.cppm, which says "check the name, or run mcpp index update". The message also prints shortName only, dropping the namespace (compat:nopepackage 'nope' not found).

mcpp/src/build/prepare.cppm

Lines 1320 to 1324 in fde3b70

auto xlEnv = mcpp::config::make_xlings_env(**cfg2);
if (!mcpp::xlings::is_index_fresh(xlEnv, (*cfg2)->searchTtlSeconds)) {
mcpp::ui::status("Updating", "package index (auto-refresh)");
mcpp::xlings::ensure_index_fresh(
xlEnv, (*cfg2)->searchTtlSeconds, /*quiet=*/true);

}
}
if (!lua) {
return std::unexpected(std::format(
"template package '{}' not found in the index "
"(check the name, or run `mcpp index update`)", spec.pkg));
}

4. e2e 12 now requires a full sandbox bootstrap but declares no # requires:. It exports an isolated MCPP_HOME, so every run downloads xlings, fetches the package index, patchelf and ninja. fresh-sandbox is deliberately not enabled on Windows, and without a requires line this test is not skipped there.

export MCPP_HOME="$TMP/mcpp-home"
cd "$TMP"
"$MCPP" new myapp > /dev/null
cd myapp
# (1) Default-namespace dep via `<ns>:<name>@<ver>` where ns is the default
# (mcpplibs). Since 0.0.10+ default namespace is "mcpplibs", this lands as a
# bare key under [dependencies], NOT in [dependencies.mcpplibs].
"$MCPP" add mcpplibs:cmdline@0.0.1 > /dev/null
grep -qE '^\[dependencies\]' mcpp.toml || { cat mcpp.toml; echo "no [dependencies] section"; exit 1; }

# but it's not a proper mcpp-compatible GCC. Don't add gcc capability.
# fresh-sandbox: not yet reliable on Windows — xlings LLVM auto-install
# into temp MCPP_HOME dirs has path/copy issues. Enable once resolved.
;;

Suggested shape: resolve the input with resolve_dependency_selector, then check the candidates through the same three-way dispatch prepare.cppm uses (readStrictLuaForCandidate), accepting on any candidate hit; call ensure_index_fresh first; put the full spec plus an mcpp index update hint in the error; restore the dotted-selector e2e case and add # requires: fresh-sandbox to test 12.

…tion

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>
@speak-agent

Copy link
Copy Markdown
Member

Pushed a follow-up commit reworking the gate. The direction (#305) was right; the lookup it was built on was the narrowest one in the tree, so it refused packages that resolve fine at build time.

Routing is now shared. mcpp.pm.index_route owns "which index answers for a namespace, and how its descriptors are read", and both mcpp add and mcpp.build.prepare go through it. That closes the two false rejections:

  • dotted selectors — capi.lua is a namespace path meaning (mcpplibs.capi, lua), then (capi, lua). Nothing in any index is named capi.lua (package.name is a single atomic segment, SPEC-001 §3.2), so a literal probe could never match. mcpp's own mcpp.toml is written in that form.
  • anything served by a project [indices] entry — the old probe read only the global registry, while dependency resolution dispatches across local-path, project-clone and global transports.

[indices] workspace inheritance moved into mcpp.project for the same reason: two copies of that rule is how the two readers ended up disagreeing.

Refuse only what is provably absent. 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 failing — the same fall-through prepare already applies to lazy git indices.

Cost and diagnostics. A stale registry is refreshed 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. Errors list the identities tried plus a cross-namespace did-you-mean, matching prepare's resolution error. A version the descriptor doesn't publish for this platform is a warning with the available list, not a failure — version tables are per-OS, so "absent here" is not "absent".

Tests. tests/e2e/12_add_command.sh serves every package from a local [indices] path, so it covers the dotted, project-index and workspace-inheritance routes offline instead of depending on the shared registry's state; it also 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. New tests/unit/test_pm_index_route.cpp pins the routing rules, including "a literal dotted short name never matches".

Out of scope here, worth a separate issue: mcpp::pm::resolve_semver also reads the global registry directly, so a SemVer constraint on a package served by a project [indices] entry cannot resolve. Same family of defect, different code path.

@Sunrisepeak
Sunrisepeak merged commit 9959be5 into mcpp-community:main Jul 29, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

希望mcpp add 包指令增加检查

3 participants