Skip to content

Commit 01346cf

Browse files
committed
Fix independent root dependency selection
1 parent b581c2b commit 01346cf

7 files changed

Lines changed: 268 additions & 35 deletions

File tree

.agents/docs/2026-06-02-imgui-mcpp-dependency-fixes.md

Lines changed: 62 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,18 @@ The imgui validation exposed two separate mcpp-side problems.
5959
`imgui.backend.*` modules. The lock file also obscured the real dependency
6060
source.
6161

62+
4. Bare dependency selectors could not fall back to independent root packages.
63+
64+
`resolve_dependency_selector("imgui")` produced only `mcpplibs/imgui`.
65+
That missed the intended rule that bare names first try the omitted
66+
`mcpplibs` root and then fall back to an independent root package.
67+
68+
The failure had a second layer: the default-namespace strict lookup uses
69+
`imgui.lua` as its canonical filename, which is the same descriptor filename
70+
used by an independent root `imgui` package. Without checking the xpkg
71+
descriptor's declared `package.name` / `package.namespace`, the first
72+
candidate could consume the independent package as `mcpplibs.imgui`.
73+
6274
## Changes In This Branch
6375

6476
- `src/build/flags.cppm`
@@ -94,6 +106,28 @@ The imgui validation exposed two separate mcpp-side problems.
94106
- Added a branch dependency regression that verifies `mcpp update <dep>`
95107
refreshes a moved branch and that lock source metadata is marked as git.
96108

109+
- `src/pm/dependency_selector.cppm`
110+
- Bare selectors now produce ordered candidates:
111+
`mcpplibs/<name>`, then independent root `<name>`.
112+
113+
- `src/manifest.cppm`
114+
- Added `extract_xpkg_name()` alongside `extract_xpkg_namespace()` so resolver
115+
code can validate package identity from the xpkg descriptor.
116+
117+
- `src/cli.cppm`
118+
- Candidate selection now validates that a matched xpkg descriptor belongs to
119+
the candidate coordinate before selecting it.
120+
- Independent root packages keep an empty package namespace in `mcpp.lock`
121+
instead of being rewritten as `mcpplibs`.
122+
123+
- `tests/unit/test_pm_compat.cpp`
124+
- Added a bare selector regression for `imgui -> mcpplibs/imgui, then imgui`.
125+
126+
- `tests/e2e/63_bare_dependency_peer_root_priority.sh`
127+
- Added a no-network regression that provides only an independent root
128+
`imgui` package in a temporary default index and verifies `imgui = "1.0.0"`
129+
builds/runs without locking the package as `mcpplibs`.
130+
97131
## Evidence So Far
98132

99133
- Red test for the include bug:
@@ -116,7 +150,7 @@ The imgui validation exposed two separate mcpp-side problems.
116150

117151
- Green e2e after rebuilding the mcpp CLI with the resolver fix:
118152
- Command:
119-
`MCPP=target/x86_64-linux-gnu/85da010ca4e7d6e2/bin/mcpp bash tests/e2e/60_stale_xpkg_cache_reinstall.sh`
153+
`MCPP=<mcpp-bin> bash tests/e2e/60_stale_xpkg_cache_reinstall.sh`
120154
- Result: `OK`
121155

122156
- Boundary regression caught and fixed:
@@ -143,7 +177,7 @@ The imgui validation exposed two separate mcpp-side problems.
143177

144178
- Green e2e after rebuilding the mcpp CLI with the git dependency fix:
145179
- Command:
146-
`MCPP=target/x86_64-linux-gnu/ea28c45f9dcd4fed/bin/mcpp bash tests/e2e/24_git_dependency.sh`
180+
`MCPP=<mcpp-bin> bash tests/e2e/24_git_dependency.sh`
147181
- Result: `OK`
148182

149183
- Focused regression after the final git dependency change:
@@ -152,9 +186,31 @@ The imgui validation exposed two separate mcpp-side problems.
152186
- `tests/e2e/24_git_dependency.sh`: `OK`
153187
- `tests/e2e/62_dotted_dependency_selector_priority.sh`: `OK`
154188

189+
- Red unit test for bare selector fallback before the fix:
190+
- `DependencySelector.BareSelectorBuildsOmittedMcpplibsThenPeerRootCandidates`
191+
failed because `selector.candidates.size()` was `1`, expected `2`.
192+
193+
- Green focused verification after the bare selector and candidate identity fix:
194+
- `mcpp test -- --gtest_filter=DependencySelector.BareSelectorBuildsOmittedMcpplibsThenPeerRootCandidates`:
195+
`18 passed; 0 failed`
196+
- `tests/e2e/62_dotted_dependency_selector_priority.sh`: `OK`
197+
- `tests/e2e/63_bare_dependency_peer_root_priority.sh`: `OK`
198+
199+
- Full local verification after the final edits:
200+
- `mcpp test`: `18 passed; 0 failed`
201+
- `tests/e2e/run_all.sh`: `67 passed, 0 failed, 0 skipped`
202+
- `tests/e2e/62_dotted_dependency_selector_priority.sh`: `OK`
203+
- `tests/e2e/63_bare_dependency_peer_root_priority.sh`: `OK`
204+
205+
- External mcpp-index imgui smoke with the rebuilt mcpp CLI:
206+
- Command shape: `MCPP=<mcpp-bin> bash tests/smoke_imgui_module.sh`
207+
- Result: `Dear ImGui 1.92.8 module package ok`
208+
- Observed install target: `mcpplibs:imgui@0.0.1`, meaning default index
209+
package `imgui`; not `mcpplibs.imgui`.
210+
155211
- Fresh external `imgui-private` git consumer with rebuilt mcpp CLI:
156212
- Command:
157-
`MCPP_HOME=/tmp/imgui-private-fixed-mcpp-home target/x86_64-linux-gnu/ea28c45f9dcd4fed/bin/mcpp run`
213+
`MCPP_HOME=<tmp-home> <mcpp-bin> run`
158214
- Result:
159215
`external git consumer imported Dear ImGui 1.92.8`
160216
- Lock source:
@@ -181,3 +237,6 @@ The imgui validation exposed two separate mcpp-side problems.
181237
corrected.
182238
- The mcpp fixes should be submitted as a separate PR from imgui package/index
183239
updates.
240+
- Public `imgui` package identity is independent root `imgui`, not
241+
`mcpplibs.imgui`; mcpp's omitted-`mcpplibs` priority must not rewrite that
242+
identity after the fallback candidate is selected.

src/cli.cppm

Lines changed: 47 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1714,6 +1714,42 @@ prepare_build(bool print_fingerprint,
17141714
return std::nullopt;
17151715
};
17161716

1717+
auto candidateQualifiedName =
1718+
[](std::string_view ns, std::string_view shortName) {
1719+
if (ns.empty()) return std::string(shortName);
1720+
return std::format("{}.{}", ns, shortName);
1721+
};
1722+
1723+
auto xpkgLuaMatchesCandidate =
1724+
[&](const mcpp::pm::DependencyCoordinate& coord,
1725+
std::string_view luaContent,
1726+
bool allowLegacyBareDefault) {
1727+
auto luaName = mcpp::manifest::extract_xpkg_name(luaContent);
1728+
if (luaName.empty()) return true;
1729+
1730+
auto luaNs = mcpp::manifest::extract_xpkg_namespace(luaContent);
1731+
auto qname = candidateQualifiedName(coord.namespace_, coord.shortName);
1732+
1733+
if (coord.namespace_.empty()) {
1734+
return luaNs.empty() && luaName == coord.shortName;
1735+
}
1736+
1737+
if (coord.namespace_ == mcpp::pm::kDefaultNamespace) {
1738+
if (luaNs == coord.namespace_) {
1739+
return luaName == coord.shortName || luaName == qname;
1740+
}
1741+
if (luaNs.empty() && luaName == qname) return true;
1742+
return allowLegacyBareDefault
1743+
&& luaNs.empty()
1744+
&& luaName == coord.shortName;
1745+
}
1746+
1747+
if (luaNs == coord.namespace_) {
1748+
return luaName == coord.shortName || luaName == qname;
1749+
}
1750+
return luaNs.empty() && luaName == qname;
1751+
};
1752+
17171753
auto dependencyCoordinates =
17181754
[](const mcpp::manifest::DependencySpec& spec,
17191755
const std::string& depName) {
@@ -1741,7 +1777,9 @@ prepare_build(bool print_fingerprint,
17411777
auto selected = candidates.front();
17421778
if (spec.isVersion() && candidates.size() > 1) {
17431779
for (auto& candidate : candidates) {
1744-
if (readStrictLuaForCandidate(candidate)) {
1780+
auto lua = readStrictLuaForCandidate(candidate);
1781+
if (lua && xpkgLuaMatchesCandidate(
1782+
candidate, *lua, /*allowLegacyBareDefault=*/false)) {
17451783
selected = candidate;
17461784
break;
17471785
}
@@ -1904,9 +1942,7 @@ prepare_build(bool print_fingerprint,
19041942
if (!childSpec.isVersion()) continue;
19051943

19061944
ResolvedKey childKey{
1907-
childSpec.namespace_.empty()
1908-
? std::string{mcpp::manifest::kDefaultNamespace}
1909-
: childSpec.namespace_,
1945+
childSpec.namespace_,
19101946
childSpec.shortName.empty() ? childName : childSpec.shortName,
19111947
};
19121948
if (auto child = loadVersionDep(
@@ -2344,9 +2380,7 @@ prepare_build(bool print_fingerprint,
23442380
}
23452381

23462382
ResolvedKey key{
2347-
spec.namespace_.empty()
2348-
? std::string{mcpp::manifest::kDefaultNamespace}
2349-
: spec.namespace_,
2383+
spec.namespace_,
23502384
spec.shortName.empty() ? name : spec.shortName,
23512385
};
23522386
const std::string sourceKind =
@@ -3041,17 +3075,20 @@ prepare_build(bool print_fingerprint,
30413075
}
30423076
} else {
30433077
lp.namespace_ = spec.namespace_.empty()
3044-
? std::string(mcpp::pm::kDefaultNamespace)
3078+
? std::string{}
30453079
: spec.namespace_;
30463080
lp.version = spec.version;
30473081
// Use the namespace and resolved version as the source identifier.
30483082
// For custom indices, include the index name for traceability.
3049-
lp.source = std::format("index+{}@{}", lp.namespace_, lp.version);
3083+
auto sourceIndex = lp.namespace_.empty()
3084+
? std::string(mcpp::pm::kDefaultNamespace)
3085+
: lp.namespace_;
3086+
lp.source = std::format("index+{}@{}", sourceIndex, lp.version);
30503087
// Use a deterministic hash based on namespace + name + version.
30513088
// A future PR can replace this with a real content hash from the
30523089
// xpkg.lua's declared sha256 or from the install plan.
30533090
std::hash<std::string> hasher;
3054-
auto hashInput = std::format("{}:{}@{}", lp.namespace_, name, lp.version);
3091+
auto hashInput = std::format("{}:{}@{}", sourceIndex, name, lp.version);
30553092
lp.hash = std::format("fnv1a:{:016x}", hasher(hashInput));
30563093
}
30573094
lock.packages.push_back(std::move(lp));

src/manifest.cppm

Lines changed: 42 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,10 @@ list_xpkg_versions(std::string_view luaContent, std::string_view platform);
246246
// Returns empty string if the field is absent (legacy descriptors).
247247
std::string extract_xpkg_namespace(std::string_view luaContent);
248248

249+
// Extract the `name` field from an xpkg .lua's `package = { ... }` block.
250+
// Returns empty string if the field is absent.
251+
std::string extract_xpkg_name(std::string_view luaContent);
252+
249253
// Resolve the lib-root path for a manifest:
250254
// 1. `[lib].path` if explicitly set (cargo-style override),
251255
// 2. otherwise the convention `src/<package-tail>.cppm`, where
@@ -885,7 +889,7 @@ std::expected<Manifest, ManifestError> parse_string(std::string_view content,
885889
// Accepted forms:
886890
// acme = "git@gitlab.example.com:platform/mcpp-index.git" # short: value = url
887891
// acme-stable = { url = "git@...", tag = "v2.0" } # long: inline table
888-
// local-dev = { path = "/home/user/my-packages" } # local path
892+
// local-dev = { path = "<path>/my-packages" } # local path
889893
// mcpplibs = { url = "https://...", rev = "abc123" } # pin built-in
890894
if (auto* indices_t = doc->get_table("indices")) {
891895
for (auto& [k, v] : *indices_t) {
@@ -1180,6 +1184,34 @@ std::string top_level_table_body_for_key(std::string_view body, std::string_view
11801184
return {};
11811185
}
11821186

1187+
std::string top_level_string_value_for_key(std::string_view body, std::string_view wantedKey) {
1188+
LuaCursor cur { body };
1189+
cur.skip_ws_and_comments();
1190+
while (!cur.eof()) {
1191+
auto key = cur.read_key();
1192+
if (key.empty()) {
1193+
cur.skip_ws_and_comments();
1194+
if (cur.eof()) break;
1195+
++cur.pos;
1196+
continue;
1197+
}
1198+
cur.skip_ws_and_comments();
1199+
if (!cur.consume('=')) {
1200+
cur.skip_ws_and_comments();
1201+
continue;
1202+
}
1203+
cur.skip_ws_and_comments();
1204+
if (key == wantedKey && (cur.peek() == '"' || cur.peek() == '\'')) {
1205+
return cur.read_string();
1206+
}
1207+
if (cur.peek() == '{') cur.skip_table();
1208+
else if (cur.peek() == '"' || cur.peek() == '\'') (void)cur.read_string();
1209+
else (void)cur.read_bareword();
1210+
cur.skip_ws_and_comments();
1211+
}
1212+
return {};
1213+
}
1214+
11831215
// Strip Lua line comments (`-- ...\n`) and string contents from text,
11841216
// replacing them with spaces of the same length so positions are
11851217
// preserved. This is a simple-but-correct way to make the scanner
@@ -1315,26 +1347,15 @@ McppField extract_mcpp_field(std::string_view luaContent) {
13151347
}
13161348

13171349
std::string extract_xpkg_namespace(std::string_view luaContent) {
1318-
// Look for `namespace = "..."` inside the `package = { ... }` block.
1319-
// Use sanitized text (comments/strings stripped) for key search,
1320-
// then read the quoted value from the original text.
1321-
auto sanitized = strip_lua_comments_and_strings(luaContent);
1322-
auto pos = sanitized.find("namespace");
1323-
if (pos == std::string::npos) return {};
1324-
// Walk past "namespace" + optional whitespace + "="
1325-
auto p = pos + 9; // strlen("namespace")
1326-
while (p < sanitized.size() && (sanitized[p] == ' ' || sanitized[p] == '\t')) ++p;
1327-
if (p >= sanitized.size() || sanitized[p] != '=') return {};
1328-
++p;
1329-
while (p < sanitized.size() && (sanitized[p] == ' ' || sanitized[p] == '\t')) ++p;
1330-
// Read the quoted string from ORIGINAL text at the same offset.
1331-
if (p >= luaContent.size() || luaContent[p] != '"') return {};
1332-
++p;
1333-
std::string result;
1334-
while (p < luaContent.size() && luaContent[p] != '"') {
1335-
result.push_back(luaContent[p++]);
1336-
}
1337-
return result;
1350+
auto packageBody = top_level_table_body_for_key(luaContent, "package");
1351+
if (packageBody.empty()) return {};
1352+
return top_level_string_value_for_key(packageBody, "namespace");
1353+
}
1354+
1355+
std::string extract_xpkg_name(std::string_view luaContent) {
1356+
auto packageBody = top_level_table_body_for_key(luaContent, "package");
1357+
if (packageBody.empty()) return {};
1358+
return top_level_string_value_for_key(packageBody, "name");
13381359
}
13391360

13401361
std::vector<std::string>

src/pm/dependency_selector.cppm

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ inline DependencySelector resolve_dependency_selector(
7171
.namespace_ = std::string(kDefaultNamespace),
7272
.shortName = segments.front(),
7373
});
74+
out.candidates.push_back(DependencyCoordinate{
75+
.namespace_ = {},
76+
.shortName = segments.front(),
77+
});
7478
return out;
7579
}
7680

src/pm/lock_io.cppm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ struct LockedIndex {
2727

2828
struct LockedPackage {
2929
std::string name;
30-
std::string namespace_; // index namespace (v2+); empty = "mcpplibs"
30+
std::string namespace_; // package namespace (v2+); empty = independent root
3131
std::string version;
3232
std::string source; // e.g. "index+mcpplibs@abc123def..."
3333
std::string hash; // "sha256:..." or "fnv1a:..."

0 commit comments

Comments
 (0)