diff --git a/.agents/docs/2026-06-02-imgui-mcpp-dependency-fixes.md b/.agents/docs/2026-06-02-imgui-mcpp-dependency-fixes.md index 16794c6..8192bf1 100644 --- a/.agents/docs/2026-06-02-imgui-mcpp-dependency-fixes.md +++ b/.agents/docs/2026-06-02-imgui-mcpp-dependency-fixes.md @@ -2,7 +2,9 @@ ## Scope -Branch: `codex/imgui-mcpp-dependency-fixes` +Initial branch: `codex/imgui-mcpp-dependency-fixes` + +Follow-up branch: `codex/fix-git-dependency-cache-lock` This document tracks the mcpp-side changes discovered while validating the `imgui-private` package and its `compat.imgui` / `compat.glfw` / @@ -41,6 +43,22 @@ The imgui validation exposed two separate mcpp-side problems. imgui and GLFW consumers, and failed with undefined `glfwInit`, `glfwCreateWindow`, etc. +3. Git branch dependencies reused stale source clones and wrote misleading + lock entries. + + `prepare_build()` cached git dependencies by `url + ref-kind + ref`. For + `branch = "main"`, that cache key stays stable even after the remote branch + advances, so `mcpp update ` removed the lock entry but the next build + still reused the old clone. The lock writer also treated every non-path + dependency as a registry dependency and emitted `source = "index+mcpplibs@"` + for git dependencies. + + Impact: a consumer using `imgui = { git = ".../imgui-private.git", branch = + "main" }` could keep compiling an old cached checkout that only provided the + obsolete `imgui` module instead of the current `imgui.core` and + `imgui.backend.*` modules. The lock file also obscured the real dependency + source. + ## Changes In This Branch - `src/build/flags.cppm` @@ -64,6 +82,18 @@ The imgui validation exposed two separate mcpp-side problems. - Added a stale-cache regression that creates an old unmarked xpkg layout and verifies mcpp reinstalls it instead of linking against the stale contents. +- `src/cli.cppm` + - For `branch` git dependencies, resolve the branch to a concrete commit with + `git ls-remote` before creating the cache key. + - Include the resolved commit in the git cache key so an advanced branch maps + to a fresh cache directory while unchanged branches still reuse the cache. + - Record git dependencies in `mcpp.lock` as `git+#branch=@` + instead of `index+...`. + +- `tests/e2e/24_git_dependency.sh` + - Added a branch dependency regression that verifies `mcpp update ` + refreshes a moved branch and that lock source metadata is marked as git. + ## Evidence So Far - Red test for the include bug: @@ -105,10 +135,42 @@ The imgui validation exposed two separate mcpp-side problems. - `tests/e2e/57_static_dep_shared_artifact.sh`: `OK` - `tests/e2e/60_stale_xpkg_cache_reinstall.sh`: `OK` +- Red e2e for git dependency lock/cache behavior before the fix: + - Command: + `MCPP=$(command -v mcpp) bash tests/e2e/24_git_dependency.sh` + - Result: failed with `FAIL: git dep lock source is not marked as git` and a + lock entry containing `source = "index+mcpplibs@"`. + +- Green e2e after rebuilding the mcpp CLI with the git dependency fix: + - Command: + `MCPP=target/x86_64-linux-gnu/ea28c45f9dcd4fed/bin/mcpp bash tests/e2e/24_git_dependency.sh` + - Result: `OK` + +- Focused regression after the final git dependency change: + - `mcpp test`: `18 passed; 0 failed` + - `tests/e2e/23_remove_update.sh`: `OK` + - `tests/e2e/24_git_dependency.sh`: `OK` + - `tests/e2e/62_dotted_dependency_selector_priority.sh`: `OK` + +- Fresh external `imgui-private` git consumer with rebuilt mcpp CLI: + - Command: + `MCPP_HOME=/tmp/imgui-private-fixed-mcpp-home target/x86_64-linux-gnu/ea28c45f9dcd4fed/bin/mcpp run` + - Result: + `external git consumer imported Dear ImGui 1.92.8` + - Lock source: + `git+git@github.com:mcpplibs/imgui-private.git#branch=main@07ea0c3c088331517e1eda898c267d966173206d` + +- `imgui-private` package validation with rebuilt mcpp CLI and existing + dependency caches: + - `mcpp clean && mcpp build && mcpp test`: `backend_test ... ok`, + `imgui_test ... ok` + - `examples/basic`: `Dear ImGui 1.92.8 module frame ok` + - `examples/glfw_opengl3`: build completed successfully + ## Pending Verification -- Re-run imgui-private with the rebuilt mcpp binary without manually deleting - package directories. +- Before opening a PR, decide whether to run the full e2e suite or rely on the + focused unit/e2e/imgui-private validation plus CI. - Before opening a PR, decide whether to also run the full e2e suite or leave that to CI after the focused dependency set above. diff --git a/src/cli.cppm b/src/cli.cppm index 254842f..d32b66b 100644 --- a/src/cli.cppm +++ b/src/cli.cppm @@ -1565,6 +1565,11 @@ prepare_build(bool print_fingerprint, std::string version; }; std::vector dep_cache_identities; + struct GitLockIdentity { + std::string source; + std::string hash; + }; + std::map root_git_lock_identities; struct ResolvedKey { std::string ns; @@ -2615,8 +2620,11 @@ prepare_build(bool print_fingerprint, if (dep_root.is_relative()) dep_root = base / dep_root; dep_root = std::filesystem::weakly_canonical(dep_root); } else if (spec.isGit()) { - // Git-based (M4 #5): clone into ~/.mcpp/git/// - // and treat as a path dep from there. + // Git-based (M4 #5): clone into ~/.mcpp/git// and treat + // as a path dep from there. Branch refs are floating, so resolve + // them to a commit before forming the cache key; this lets + // `mcpp update ` pick up a moved branch without deleting + // unrelated git caches. auto mcppHome = [] { if (auto* e = std::getenv("MCPP_HOME"); e && *e) return std::filesystem::path(e); @@ -2624,11 +2632,32 @@ prepare_build(bool print_fingerprint, return std::filesystem::path(e) / ".mcpp"; return std::filesystem::current_path() / ".mcpp"; }(); - // Cache key: hash(url + refkind + ref). Avoids collisions across - // different revs of the same repo. + std::string resolvedGitRev = spec.gitRev; + if (spec.gitRefKind == "branch") { + auto ref = std::format("refs/heads/{}", spec.gitRev); + auto cmd = std::format( + "git ls-remote {} {} 2>&1", + mcpp::platform::shell::quote(spec.git), + mcpp::platform::shell::quote(ref)); + auto r = mcpp::platform::process::capture(cmd); + if (r.exit_code != 0) { + return std::unexpected(std::format( + "git ls-remote of '{}' failed:\n{}", spec.git, r.output)); + } + std::istringstream is(r.output); + is >> resolvedGitRev; + if (resolvedGitRev.empty()) { + return std::unexpected(std::format( + "git branch '{}' not found in '{}'", spec.gitRev, spec.git)); + } + } + + // Cache key: hash(url + refkind + declared ref + resolved commit). + // For fixed rev/tag deps the declared ref is also the resolved ref. std::hash H; auto urlHash = std::format("{:016x}", - H(spec.git + "|" + spec.gitRefKind + "|" + spec.gitRev)); + H(spec.git + "|" + spec.gitRefKind + "|" + spec.gitRev + + "|" + resolvedGitRev)); auto gitRoot = mcppHome / "git" / urlHash; std::error_code ec; std::filesystem::create_directories(gitRoot.parent_path(), ec); @@ -2638,10 +2667,12 @@ prepare_build(bool print_fingerprint, std::string cloneCmd; if (spec.gitRefKind == "branch") { cloneCmd = std::format( - "git clone --depth 1 --branch {} {} {} 2>&1", + "git clone --depth 1 --branch {} {} {} && cd {} && git checkout --quiet {} 2>&1", mcpp::platform::shell::quote(spec.gitRev), mcpp::platform::shell::quote(spec.git), - mcpp::platform::shell::quote(gitRoot.string())); + mcpp::platform::shell::quote(gitRoot.string()), + mcpp::platform::shell::quote(gitRoot.string()), + mcpp::platform::shell::quote(resolvedGitRev)); } else { // For tag/rev: full clone, then checkout (depth-1 may miss the rev). cloneCmd = std::format( @@ -2663,6 +2694,17 @@ prepare_build(bool print_fingerprint, } } } + if (item.consumerDepIndex == kMainConsumer) { + auto source = std::format("git+{}#{}={}", + spec.git, spec.gitRefKind, spec.gitRev); + if (spec.gitRefKind == "branch") source += "@" + resolvedGitRev; + root_git_lock_identities[name] = GitLockIdentity{ + .source = std::move(source), + .hash = std::format("fnv1a:{:016x}", H(spec.git + "|" + + spec.gitRefKind + "|" + spec.gitRev + "|" + + resolvedGitRev)), + }; + } dep_root = gitRoot; } // (version-source: dep_root + manifest are loaded together via @@ -2985,19 +3027,33 @@ prepare_build(bool print_fingerprint, if (spec.isPath()) continue; mcpp::lockfile::LockedPackage lp; lp.name = name; - lp.namespace_ = spec.namespace_.empty() - ? std::string(mcpp::pm::kDefaultNamespace) - : spec.namespace_; - lp.version = spec.version; - // Use the namespace and resolved version as the source identifier. - // For custom indices, include the index name for traceability. - lp.source = std::format("index+{}@{}", lp.namespace_, lp.version); - // Use a deterministic hash based on namespace + name + version. - // A future PR can replace this with a real content hash from the - // xpkg.lua's declared sha256 or from the install plan. - std::hash hasher; - auto hashInput = std::format("{}:{}@{}", lp.namespace_, name, lp.version); - lp.hash = std::format("fnv1a:{:016x}", hasher(hashInput)); + if (spec.isGit()) { + auto gitIt = root_git_lock_identities.find(name); + lp.version = spec.gitRev; + if (gitIt == root_git_lock_identities.end()) { + lp.source = std::format("git+{}#{}={}", + spec.git, spec.gitRefKind, spec.gitRev); + std::hash hasher; + lp.hash = std::format("fnv1a:{:016x}", hasher(lp.source)); + } else { + lp.source = gitIt->second.source; + lp.hash = gitIt->second.hash; + } + } else { + lp.namespace_ = spec.namespace_.empty() + ? std::string(mcpp::pm::kDefaultNamespace) + : spec.namespace_; + lp.version = spec.version; + // Use the namespace and resolved version as the source identifier. + // For custom indices, include the index name for traceability. + lp.source = std::format("index+{}@{}", lp.namespace_, lp.version); + // Use a deterministic hash based on namespace + name + version. + // A future PR can replace this with a real content hash from the + // xpkg.lua's declared sha256 or from the install plan. + std::hash hasher; + auto hashInput = std::format("{}:{}@{}", lp.namespace_, name, lp.version); + lp.hash = std::format("fnv1a:{:016x}", hasher(hashInput)); + } lock.packages.push_back(std::move(lp)); } if (!lock.packages.empty() || !lock.indices.empty()) { diff --git a/tests/e2e/24_git_dependency.sh b/tests/e2e/24_git_dependency.sh index 93e623f..be24ca2 100755 --- a/tests/e2e/24_git_dependency.sh +++ b/tests/e2e/24_git_dependency.sh @@ -88,5 +88,105 @@ test -n "$(ls -A "$MCPP_HOME/git" 2>/dev/null)" || { echo "FAIL: git cache empty build2=$("$MCPP" build 2>&1) echo "$build2" | grep -q 'Cloning' && { echo "FAIL: re-cloned on rebuild"; exit 1; } || true +# 3. Branch refs should update when the user asks for mcpp update. +mkdir "$TMP/branch-origin" && cd "$TMP/branch-origin" +git init --quiet +git checkout -B main --quiet +git config user.email "test@local" +git config user.name "test" +"$MCPP" new branchlib >/dev/null +mv branchlib/* branchlib/.??* . 2>/dev/null || true +rmdir branchlib 2>/dev/null || true +cat > src/greet.cppm <<'EOF' +export module branchlib.greet; +import std; +export auto greet() -> void { std::println("branch dep v1"); } +EOF +rm src/main.cpp +cat > mcpp.toml <<'EOF' +[package] +name = "branchlib" +version = "0.1.0" +[language] +standard = "c++23" +modules = true +import_std = true +[modules] +sources = ["src/**/*.cppm"] +[targets.branchlib] +kind = "lib" +EOF +git add -A >/dev/null +git commit --quiet -m "v1" + +cd "$TMP" +"$MCPP" new branchapp >/dev/null +cd branchapp +cat > src/main.cpp <<'EOF' +import std; +import branchlib.greet; +int main() { greet(); return 0; } +EOF +cat > mcpp.toml < branch-v1.log 2>&1 +triple=$(ls -d target/*/ | head -1) +fp_dir=$(ls "$triple") +out=$(${triple}${fp_dir}/bin/branchapp) +[[ "$out" == *"branch dep v1"* ]] || { + echo "FAIL: branch dep v1 not invoked: $out" + cat branch-v1.log; exit 1; } + +grep -q 'source = "git+' mcpp.lock || { + echo "FAIL: git dep lock source is not marked as git" + cat mcpp.lock; exit 1; } +grep -q 'branch=main' mcpp.lock || { + echo "FAIL: git dep lock source does not record branch ref" + cat mcpp.lock; exit 1; } +! grep -q 'source = "index+' mcpp.lock || { + echo "FAIL: git dep lock source incorrectly uses index source" + cat mcpp.lock; exit 1; } + +cd "$TMP/branch-origin" +cat > src/greet.cppm <<'EOF' +export module branchlib.greet; +import std; +export auto greet() -> void { std::println("branch dep v2"); } +EOF +git add -A >/dev/null +git commit --quiet -m "v2" + +cd "$TMP/branchapp" +"$MCPP" update branchlib >/dev/null +"$MCPP" clean >/dev/null +"$MCPP" build > branch-v2.log 2>&1 +triple=$(ls -d target/*/ | head -1) +fp_dir=$(ls "$triple") +out=$(${triple}${fp_dir}/bin/branchapp) +[[ "$out" == *"branch dep v2"* ]] || { + echo "FAIL: branch dep did not refresh after mcpp update: $out" + echo "--- branch-v2.log ---" + cat branch-v2.log + echo "--- mcpp.lock ---" + cat mcpp.lock + exit 1 +} + # Cleanup so trap doesn't blow up if any subprocess holds files. echo "OK"