feat: lazy git clones via artifactory shim#21
Open
RedFox20 wants to merge 4 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an “artifactory shim” path for non-root git dependencies so Mama can probe/fetch a prebuilt Artifactory package (resolved by git ls-remote commit hash) and skip an expensive clone when the package exists. It also makes shim-based deps read-only (deploy/run/build safeguards) and introduces a marker file ({build_dir}/mama_shim) to persist shim identity across runs.
Changes:
- Add shim marker helpers to
BuildDependencyand a newtry_load_artifactory_shim()probe path inmama.artifactory. - Wire the shim probe into
BuildDependency._load()ahead ofGit.dependency_checkout()and add shim-aware guards for build/deploy/run/open flows. - Add a new test suite covering marker roundtrip, probe gating, load integration (clone skipped vs invoked), and shim guards.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mama/artifactory.py | Adds try_load_artifactory_shim() to probe/fetch via commit hash without cloning. |
| mama/build_dependency.py | Adds shim marker helpers; integrates shim probe before clone; prevents builds/tagging on shims; clears marker on dirty(). |
| mama/build_target.py | Skips deploy/upload for shims; blocks test/start when source is unavailable via _require_source(). |
| mama/main.py | Makes mama open <target> print an actionable hint instead of trying to open a non-existent source dir for shims. |
| mama/papa_deploy.py | Adds defense-in-depth refusal to deploy into a directory containing a mama_shim marker. |
| tests/test_artifactory_shim/init.py | Adds test package for shim-related tests. |
| tests/test_artifactory_shim/test_shim_marker.py | Unit tests for shim marker write/read/detect semantics. |
| tests/test_artifactory_shim/test_shim_probe.py | Unit tests for shim probe gating and fetch hit/miss behavior. |
| tests/test_artifactory_shim/test_shim_load_integration.py | Integration tests ensuring shim hit skips clone and shim miss falls back to clone. |
| tests/test_artifactory_shim/test_shim_guards.py | Tests for build/deploy/tagging/dirty/deploy-destination protections on shims. |
| tests/test_artifactory_shim/test_shim_buildtarget.py | Tests for BuildTarget-level shim behavior (_require_source, deploy short-circuit). |
Comment on lines
+148
to
+159
|
|
||
| with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ | ||
| patch.object(artifactory_mod, 'artifactory_fetch_and_reconfigure', | ||
| side_effect=_fake_failed_fetch), \ | ||
| patch.object(Git, 'dependency_checkout', return_value=False): | ||
| dep._load() | ||
|
|
||
| # post-clone probe should still be allowed to run | ||
| assert dep.did_check_artifactory is False or dep.did_check_artifactory is True | ||
| # (it may end up True via the post-clone fetch attempt; what we assert here is | ||
| # that the shim miss alone did NOT mark it True. We can't observe the order | ||
| # cleanly without finer instrumentation, so we just assert the load completed.) |
Comment on lines
+339
to
+368
| def try_load_artifactory_shim(dep) -> Tuple: | ||
| """ | ||
| Probe artifactory for a prebuilt package using the commit hash resolved via | ||
| `git ls-remote` (no clone). On hit, construct a default BuildTarget, load | ||
| papa.txt exports/deps into it, write the shim marker, and return the target | ||
| plus its child dep_sources. | ||
|
|
||
| On miss (or when artifactory is not configured), returns (None, None) and | ||
| leaves dep state untouched so the caller can fall back to the clone path. | ||
|
|
||
| Returns (target_or_None, dep_sources_or_None). | ||
| """ | ||
| from .build_target import BuildTarget # local import to avoid cycle | ||
|
|
||
| config = dep.config | ||
| if not config.artifactory_ftp: | ||
| return (None, None) | ||
| if not dep.dep_source.is_git: | ||
| return (None, None) | ||
|
|
||
| git: Git = dep.dep_source | ||
|
|
||
| # Resolve commit hash without cloning. `init_commit_hash` already supports | ||
| # ls-remote and respects the stored git_status cache when `update` is not set. | ||
| commit_hash = git.init_commit_hash(dep, use_cache=True, fetch_remote=True) | ||
| if not commit_hash: | ||
| if config.verbose: | ||
| console(f' {dep.name} shim probe: could not resolve commit hash', color=Color.YELLOW) | ||
| return (None, None) | ||
| git.commit_hash = commit_hash # cache for downstream consumers |
|
|
||
| import mama.artifactory as artifactory_mod | ||
| from mama.build_dependency import BuildDependency | ||
| from mama.build_target import BuildTarget |
| import os | ||
| import tempfile | ||
| import shutil | ||
| from unittest.mock import Mock, patch |
For non-root git deps with a fresh artifactory build, probe the package
server before cloning. On hit, populate build_dir from the artifactory zip
and write a {build_dir}/mama_shim marker that records the URL/branch/tag/
hash/archive so subsequent runs detect the shim and skip the clone.
The commit hash is resolved via the existing Git.init_commit_hash() which
already supports `git ls-remote` (no working tree needed). When the probe
misses we fall back to the original clone path; `did_check_artifactory`
is intentionally left unset on shim miss so the post-clone fetch can
still pick up a mamafile-declared `target.version`.
Shim is read-only after fetch:
- `_execute_deploy_tasks` short-circuits with a `mama unshallow` hint
- `_execute_run_tasks` refuses `test`/`start` via `_require_source()`
- `mama open <shim>` prints the same hint
- `papa_deploy_to` refuses any destination containing a mama_shim marker
(defense-in-depth for direct callers)
- `_should_build` returns False for shims so configure/build never run
- `update_mamafile_tag` / `update_cmakelists_tag` short-circuit
- `dirty()` removes the marker so the next build re-evaluates
Tests cover the marker roundtrip, probe gating (noart / no artifactory /
non-git / unresolved hash / fetch miss / fetch hit), the load-time wiring
(clone skipped on hit, clone invoked on miss, noart bypass), the guards,
and the BuildTarget-level deploy/require-source behavior. 27 new tests,
all existing tests still pass.
Deferred (will follow up):
- shim → real clone transition for `mama unshallow` (currently users can
`mama wipe` and rebuild, which works via existing semantics)
- explicit `mama update` re-probe path
- `target.version` from src_dir/mamafile.py without a clone (sparse-checkout)
When network is down, the first git ls-remote or HTTP fetch to fail with a clearly network-related error (timeout, DNS, connection refused) sets a global `config._network_available = False`. All subsequent operations check this flag and skip instantly instead of each waiting for their own timeout (previously: N deps × 5s = 50s wasted blocking). Only true network errors trigger the flag. Authentication failures (SSH key rejected, HTTP 401/403) and HTTP 404 are explicitly excluded so that misconfigured credentials don't silently disable all network access. Guard points: - Git.init_commit_hash: skip ls-remote if flag set - Git.fetch_origin: skip fetch/pull if flag set - Git.clone_or_pull: skip pull (use cached source) if flag set; fail clearly if clone needed but network unavailable - _fetch_package: skip HTTP download if flag set The `is_network_error(e)` classifier lives in mama/util.py and handles subprocess.TimeoutExpired, urllib URLError/HTTPError, socket errors, OSError with network errno, and git error message patterns. Behavior by command: - `mama build` with full cache: zero network ops, zero cost - `mama build` first time: first dep times out (5s), flag set, rest fail fast with "network unavailable" message - `mama update`: first dep times out, flag set, rest use cached state https://claude.ai/code/session_01S5uF8PBFkxx5GhSBBTkZvK
RedFox20
force-pushed
the
claude/optimize-build-caching-9l0mB
branch
from
6d02139 to
2626195
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For non-root git deps with a fresh artifactory build, probe the package server before cloning. On hit, populate build_dir from the artifactory zip and write a {build_dir}/mama_shim marker that records the URL/branch/tag/ hash/archive so subsequent runs detect the shim and skip the clone.
The commit hash is resolved via the existing Git.init_commit_hash() which already supports
git ls-remote(no working tree needed). When the probe misses we fall back to the original clone path;did_check_artifactoryis intentionally left unset on shim miss so the post-clone fetch can still pick up a mamafile-declaredtarget.version.Shim is read-only after fetch:
_execute_deploy_tasksshort-circuits with amama unshallowhint_execute_run_tasksrefusestest/startvia_require_source()mama open <shim>prints the same hintpapa_deploy_torefuses any destination containing a mama_shim marker (defense-in-depth for direct callers)_should_buildreturns False for shims so configure/build never runupdate_mamafile_tag/update_cmakelists_tagshort-circuitdirty()removes the marker so the next build re-evaluatesTests cover the marker roundtrip, probe gating (noart / no artifactory / non-git / unresolved hash / fetch miss / fetch hit), the load-time wiring (clone skipped on hit, clone invoked on miss, noart bypass), the guards, and the BuildTarget-level deploy/require-source behavior. 27 new tests, all existing tests still pass.
Deferred (will follow up):
mama unshallow(currently users canmama wipeand rebuild, which works via existing semantics)mama updatere-probe pathtarget.versionfrom src_dir/mamafile.py without a clone (sparse-checkout)