Skip to content

Commit 99250a5

Browse files
authored
fix(build): cache custom-index dependency objects (#86)
* fix(build): cache custom-index dependency objects * fix(build): quiet custom index bootstrap * fix(build): keep custom index status visible
1 parent e29373e commit 99250a5

3 files changed

Lines changed: 255 additions & 11 deletions

File tree

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
# BMI Cache And Custom Index Build Fix
2+
3+
## Context
4+
5+
`xlings` now builds through `mcpp` and uses a local project index:
6+
7+
```toml
8+
[indices]
9+
xlings = { path = "mcpp" }
10+
```
11+
12+
During a first build, `mcpp build` currently prints `Fetching custom index repos (first use)` followed by `xlings update` output, then emits warnings such as:
13+
14+
```text
15+
warning: bmi cache populate failed for tinyhttps@0.2.3: expected build output missing: .../obj/tinyhttps.m.o
16+
warning: bmi cache populate failed for xlings.libarchive@3.8.7: expected build output missing: .../obj/xxhash.o
17+
```
18+
19+
The build itself can still succeed, but the dependency BMI cache is not populated for affected packages.
20+
21+
## Root Cause
22+
23+
There are two related build-tool issues.
24+
25+
1. Project custom-index bootstrap is too broad.
26+
27+
`mcpp` seeds `.mcpp/.xlings.json` for non-builtin `[indices]` and treats the project index data as uninitialized when no `xim-indexrepos.json` or cloned `pkgs/` directory exists. It then runs `xlings update`, which prints all configured xlings index update output. For a local path index, the local `pkgs/` tree already exists and can be read directly, so the update path is mostly noise and can create extra first-build work.
28+
29+
2. BMI cache records object basenames instead of object paths relative to `obj/`.
30+
31+
The build plan correctly writes objects under collision-avoiding subdirectories, for example:
32+
33+
```text
34+
obj/mcpplibs_tinyhttps_src/tinyhttps.m.o
35+
obj/xlings_libarchive_libarchive/xxhash.o
36+
obj/xlings_zlib_zlib-1.3.2/compress.o
37+
```
38+
39+
The cache populate list currently stores only `cu.object.filename()`, so it later searches for:
40+
41+
```text
42+
obj/tinyhttps.m.o
43+
obj/xxhash.o
44+
obj/compress.o
45+
```
46+
47+
Those files do not exist, so `populate_from()` returns `expected build output missing`.
48+
49+
There is also a cache-key correctness gap: all dependency cache entries currently use the default index name. Custom-index dependencies should carry their real namespace/index identity so cache entries from different indices do not collide.
50+
51+
## Fix Plan
52+
53+
1. Add regression coverage first.
54+
- Add an e2e regression that builds a local-path custom-index dependency with duplicate object basenames.
55+
- Assert that local-path indices do not trigger first-use `xlings update` noise.
56+
- Assert that the cache manifest is written under the custom index name and preserves nested object paths.
57+
- Assert that a second cold build reuses the dependency BMI cache.
58+
59+
2. Fix artifact path recording.
60+
- When collecting dependency artifacts in `src/cli.cppm`, store object paths relative to `ctx.outputDir / "obj"` instead of only the object filename.
61+
- Keep BMI filenames as basenames because compiler BMI output is intentionally flat in the active BMI directory.
62+
63+
3. Fix dependency cache identity.
64+
- Use the dependency namespace or custom index name in `CacheKey::indexName` instead of always using the global default index.
65+
- Preserve builtin/default behavior for existing `mcpplibs` packages.
66+
67+
4. Reduce local custom-index update noise.
68+
- Treat local `{ path = ... }` indices as initialized when the source path has a `pkgs/` directory.
69+
- Keep remote custom indices on the existing project `xlings update` path.
70+
71+
5. Verify.
72+
- Run targeted BMI cache unit tests.
73+
- Run the broader unit suite or project test command available in the repository.
74+
- Run a local `mcpp build` scenario when feasible to ensure the original warning no longer appears.
75+
76+
## Dynamic Notes
77+
78+
- If tests reveal that `CacheKey::indexName` is used as a namespace rather than a repository identity, prefer the package namespace as the cache partition and document the remaining repository-revision hardening as follow-up.
79+
- If local path indices still need xlings project metadata for install operations, skip only the update call, not `.mcpp/.xlings.json` seeding.
80+
- Implemented regression as `tests/e2e/49_bmi_cache_nested_custom_index.sh` because the defect is in CLI dependency resolution plus build-plan artifact collection, not only the lower-level cache copy functions.
81+
- Local verification: `mcpp build` succeeded, then `MCPP=target/x86_64-linux-gnu/4d24c8b57fdbbbb4/bin/mcpp bash tests/e2e/49_bmi_cache_nested_custom_index.sh` returned `OK`.
82+
- `mcpp test -- --gtest_filter=BmiCache.*` initially exposed an unrelated local environment issue: `~/.mcpp/registry/data/xpkgs/xim-x-binutils/2.42` only had `.xpkg.lua` and no `bin/as`. Re-running with a temporary `MCPP_HOME` pointed at the complete installed mcpp registry passed: 15 test binaries ok, 0 failed.
83+
- Remote custom-index first-use sync keeps the mcpp-level `Fetching custom index repos (first use)` status, but calls `update_index(..., quiet=true)` so `mcpp build` does not expose xlings update output. A non-zero update exits with a concise mcpp-level error.

src/cli.cppm

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1375,11 +1375,10 @@ prepare_build(bool print_fingerprint,
13751375
if (cfg2) {
13761376
mcpp::config::ensure_project_index_dir(**cfg2, *root, m->indices);
13771377

1378-
// On first build, the project xlings data root may be empty because
1379-
// ensure_project_index_dir only writes .xlings.json but doesn't
1380-
// trigger clone/link creation. Check whether there are any custom
1381-
// non-builtin indices and whether the project data roots have index content.
1382-
// If not, run xlings update before dependency resolution.
1378+
// On first build, the project index data root may be empty because
1379+
// ensure_project_index_dir only writes .xlings.json but does not
1380+
// trigger clone/link creation. Local path indices are read directly;
1381+
// remote custom indices are synced quietly before dependency resolution.
13831382
bool hasCustomIndices = false;
13841383
for (auto& [idxName, spec] : m->indices) {
13851384
if (!spec.is_builtin()) {
@@ -1390,9 +1389,21 @@ prepare_build(bool print_fingerprint,
13901389
if (hasCustomIndices) {
13911390
bool needsClone = !mcpp::config::project_index_data_initialized(*root);
13921391
if (needsClone) {
1393-
mcpp::ui::status("Fetching", "custom index repos (first use)");
1394-
auto projEnv = mcpp::config::make_project_xlings_env(**cfg2, *root);
1395-
mcpp::xlings::update_index(projEnv);
1392+
bool needsRemoteUpdate = false;
1393+
for (auto& [idxName, spec] : m->indices) {
1394+
if (spec.is_builtin() || spec.is_local()) continue;
1395+
needsRemoteUpdate = true;
1396+
break;
1397+
}
1398+
if (needsRemoteUpdate) {
1399+
mcpp::ui::status("Fetching", "custom index repos (first use)");
1400+
auto projEnv = mcpp::config::make_project_xlings_env(**cfg2, *root);
1401+
int rc = mcpp::xlings::update_index(projEnv, /*quiet=*/true);
1402+
if (rc != 0) {
1403+
return std::unexpected(
1404+
"project custom index update failed; run `mcpp index update` for details");
1405+
}
1406+
}
13961407
}
13971408
}
13981409
}
@@ -1406,6 +1417,11 @@ prepare_build(bool print_fingerprint,
14061417
// unique_ptr is not load-bearing for liveness — it's a leftover from
14071418
// an earlier design and harmless).
14081419
std::vector<std::unique_ptr<mcpp::manifest::Manifest>> dep_manifests;
1420+
std::vector<std::string> dep_cache_indices;
1421+
auto cache_index_name = [](std::string_view ns) {
1422+
if (ns.empty()) return std::string(mcpp::pm::kDefaultNamespace);
1423+
return std::string(ns);
1424+
};
14091425

14101426
struct ResolvedKey {
14111427
std::string ns;
@@ -1581,8 +1597,9 @@ prepare_build(bool print_fingerprint,
15811597
// Route xpkg.lua reading through the appropriate index.
15821598
std::optional<std::string> luaContent;
15831599
if (idxSpec && idxSpec->is_local()) {
1600+
auto indexPath = mcpp::config::resolve_project_index_path(*root, *idxSpec);
15841601
luaContent = mcpp::fetcher::Fetcher::read_xpkg_lua_from_path(
1585-
idxSpec->path, shortName);
1602+
indexPath, shortName);
15861603
} else if (idxSpec && !idxSpec->is_builtin()) {
15871604
luaContent = mcpp::fetcher::Fetcher::read_xpkg_lua_from_project_data(
15881605
*root, ns, shortName);
@@ -1930,6 +1947,7 @@ prepare_build(bool print_fingerprint,
19301947

19311948
dep_manifests.push_back(
19321949
std::make_unique<mcpp::manifest::Manifest>(std::move(stagedManifest)));
1950+
dep_cache_indices.push_back(cache_index_name(key.ns));
19331951
packages.push_back({secStage, *dep_manifests.back()});
19341952
auto added = propagateIncludeDirs(secStage, *dep_manifests.back());
19351953

@@ -2154,6 +2172,7 @@ prepare_build(bool print_fingerprint,
21542172
// by depIndex (the SemVer merger needs to overwrite the slot).
21552173
dep_manifests.push_back(
21562174
std::make_unique<mcpp::manifest::Manifest>(std::move(*dep_manifest)));
2175+
dep_cache_indices.push_back(cache_index_name(key.ns));
21572176
packages.push_back({dep_root, *dep_manifests.back()});
21582177

21592178
// Record this dep as resolved so future encounters of the same
@@ -2288,6 +2307,27 @@ prepare_build(bool print_fingerprint,
22882307
if (cfg2) {
22892308
std::error_code mkEc;
22902309
std::filesystem::create_directories(ctx.outputDir, mkEc);
2310+
auto usable_object_rel = [](const std::filesystem::path& rel)
2311+
-> std::optional<std::string>
2312+
{
2313+
auto s = rel.generic_string();
2314+
if (s.empty() || s == "." || s == ".." || s.starts_with("../")) {
2315+
return std::nullopt;
2316+
}
2317+
return s;
2318+
};
2319+
auto object_cache_path = [&](const std::filesystem::path& objectPath) {
2320+
if (objectPath.is_absolute()) {
2321+
if (auto s = usable_object_rel(
2322+
objectPath.lexically_relative(ctx.outputDir / "obj"))) {
2323+
return *s;
2324+
}
2325+
}
2326+
if (auto s = usable_object_rel(objectPath.lexically_relative("obj"))) {
2327+
return *s;
2328+
}
2329+
return objectPath.filename().generic_string();
2330+
};
22912331
for (std::size_t i = 1; i < packages.size(); ++i) { // skip [0] = main
22922332
const auto& pkgRoot = packages[i];
22932333
const auto& depName = pkgRoot.manifest.package.name;
@@ -2312,7 +2352,9 @@ prepare_build(bool print_fingerprint,
23122352
mcpp::bmi_cache::CacheKey key {
23132353
.mcppHome = (*cfg2)->mcppHome,
23142354
.fingerprint = fp.hex,
2315-
.indexName = (*cfg2)->defaultIndex,
2355+
.indexName = i - 1 < dep_cache_indices.size()
2356+
? dep_cache_indices[i - 1]
2357+
: (*cfg2)->defaultIndex,
23162358
.packageName = depName,
23172359
.version = depVer,
23182360
.bmiDirName = std::string(bmiT.bmiDir),
@@ -2336,7 +2378,7 @@ prepare_build(bool print_fingerprint,
23362378
bmi += std::string(bmiT.bmiExt);
23372379
arts.bmiFiles.push_back(std::move(bmi));
23382380
}
2339-
arts.objFiles.push_back(cu.object.filename().string());
2381+
arts.objFiles.push_back(object_cache_path(cu.object));
23402382
}
23412383

23422384
if (mcpp::bmi_cache::is_cached(key)) {
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
#!/usr/bin/env bash
2+
# requires: gcc fresh-sandbox
3+
# Regression test for dependency BMI cache population when a custom-index
4+
# package produces object files under collision-avoidance subdirectories.
5+
set -e
6+
7+
TMP=$(mktemp -d)
8+
trap "rm -rf $TMP" EXIT
9+
10+
export MCPP_HOME="$TMP/mcpp-home"
11+
source "$(dirname "$0")/_inherit_toolchain.sh"
12+
13+
INDEX_DIR="$TMP/local-index"
14+
mkdir -p "$INDEX_DIR/pkgs/c"
15+
cat > "$INDEX_DIR/pkgs/c/collision-lib.lua" <<'EOF'
16+
package = {
17+
spec = "1",
18+
name = "collision-lib",
19+
description = "Collision object path package",
20+
licenses = {"MIT"},
21+
type = "package",
22+
xpm = {
23+
linux = {
24+
["1.0.0"] = {
25+
url = "https://example.invalid/collision-lib-1.0.0.tar.gz",
26+
sha256 = "0000000000000000000000000000000000000000000000000000000000000000",
27+
},
28+
},
29+
},
30+
mcpp = {
31+
language = "c++23",
32+
import_std = true,
33+
sources = { "src/**/*.cppm" },
34+
targets = { ["collision-lib"] = { kind = "lib" } },
35+
deps = {},
36+
},
37+
}
38+
EOF
39+
40+
mkdir -p "$TMP/project/app"
41+
cd "$TMP/project/app"
42+
43+
mkdir -p src .mcpp/.xlings/data/xpkgs/local-dev.collision-lib/1.0.0/src/a \
44+
.mcpp/.xlings/data/xpkgs/local-dev.collision-lib/1.0.0/src/b
45+
46+
cat > src/main.cpp <<'EOF'
47+
import std;
48+
import collision.a;
49+
import collision.b;
50+
int main() {
51+
std::println("{} {}", collision_a(), collision_b());
52+
return 0;
53+
}
54+
EOF
55+
56+
cat > .mcpp/.xlings/data/xpkgs/local-dev.collision-lib/1.0.0/src/a/foo.cppm <<'EOF'
57+
export module collision.a;
58+
export int collision_a() { return 1; }
59+
EOF
60+
61+
cat > .mcpp/.xlings/data/xpkgs/local-dev.collision-lib/1.0.0/src/b/foo.cppm <<'EOF'
62+
export module collision.b;
63+
export int collision_b() { return 2; }
64+
EOF
65+
66+
cat > mcpp.toml <<EOF
67+
[package]
68+
name = "app"
69+
version = "0.1.0"
70+
71+
[indices]
72+
local-dev = { path = "$INDEX_DIR" }
73+
74+
[dependencies]
75+
"local-dev.collision-lib" = "1.0.0"
76+
77+
[targets.app]
78+
kind = "bin"
79+
main = "src/main.cpp"
80+
EOF
81+
82+
"$MCPP" build > build.log 2>&1 || { cat build.log; exit 1; }
83+
84+
if grep -q "Fetching custom index repos" build.log; then
85+
echo "FAIL: local path index should not trigger xlings update noise"
86+
cat build.log
87+
exit 1
88+
fi
89+
90+
if grep -q "bmi cache populate failed" build.log; then
91+
echo "FAIL: BMI cache populate warning should not appear"
92+
cat build.log
93+
exit 1
94+
fi
95+
96+
manifest="$(find "$MCPP_HOME/bmi" -path "*/deps/local-dev/*collision-lib@1.0.0/manifest.txt" | head -1)"
97+
[[ -n "$manifest" ]] || {
98+
echo "FAIL: missing custom-index BMI manifest"
99+
find "$MCPP_HOME/bmi" -maxdepth 6 -type f | sort
100+
cat build.log
101+
exit 1
102+
}
103+
104+
grep -Eq '^obj: .+/foo\.m\.o$' "$manifest" || {
105+
echo "FAIL: manifest did not preserve nested object path"
106+
cat "$manifest"
107+
exit 1
108+
}
109+
110+
rm -rf target
111+
"$MCPP" build > build2.log 2>&1 || { cat build2.log; exit 1; }
112+
113+
grep -q "Cached local-dev.collision-lib v1.0.0" build2.log || {
114+
echo "FAIL: second cold build did not reuse BMI cache"
115+
cat build2.log
116+
exit 1
117+
}
118+
119+
echo "OK"

0 commit comments

Comments
 (0)