diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..907684b --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,129 @@ +# Mama — Claude Notes + +Hand-written notes for Claude. Capture style rules and codebase invariants that +keep biting future-Claude. Update as the codebase teaches new lessons. + +## Code style + +- **Line length: up to 130 columns.** Don't wrap a single expression unless it + actually exceeds 130 cols. +- **Never split a single expression over 3+ lines.** Two lines max, joined with + `+ \` for string concatenation. +- **When wrapping at a `(`, continue on the same line, then align the + continuation under the character just inside the opening parenthesis.** Do NOT + break right after `(`. +- **One-liner `if` for a single short statement.** Use `if cond: do_thing()` on + one line when the body is a single short call. + +### Examples + +```python +# GOOD - single short statement, one-liner +if dep.config.verbose: error(f' {dep.name: <16} {msg}') + +# BAD - 2 lines for a single short statement +if dep.config.verbose: + error(f' {dep.name: <16} {msg}') + +# GOOD - fits in 130 cols, one line +raise RuntimeError(f'papa_deploy refused: {package_full_path} contains a mama_shim marker.') + +# BAD - 3 lines for an expression that fits +raise RuntimeError( + f'papa_deploy refused: {package_full_path} contains a mama_shim marker.' +) + +# GOOD - doesn't fit 130 cols: continue on first line, align under `(` +raise RuntimeError(f'Target {dep.name} requires network to clone but network is unavailable.' + \ + ' Check your connection or use a cached artifactory package.') + +# GOOD - same pattern with implicit string concat +console(f'{indent}Artifactory CACHE (size-match) ' + f'{os.path.basename(local_file)} ({get_file_size_str(size)})') + +# BAD - break after opening paren +raise RuntimeError( + f'Target {dep.name} requires network to clone but network is unavailable.' + f' Check your connection or use a cached artifactory package.') +``` + +## Path handling — forward slashes everywhere + +The project standardises on forward slashes on every platform, including +Windows. The utility is `mama.util.normalized_path()` (which calls +`os.path.abspath` then `.replace('\\', '/')`). + +- After any function that may return a backslash path (notably + `tempfile.TemporaryDirectory()` on Windows), pass the result through + `normalized_path()` BEFORE interpolating into a shell command string. +- `shlex.split()` (which `SubProcess` uses) eats backslashes as escapes — a raw + Windows path embedded in a command string silently corrupts. +- For directory cleanup on Windows: `tempfile.TemporaryDirectory(prefix='...', + ignore_cleanup_errors=True)` — git leaves read-only files in `.git/objects/` + that trip `shutil.rmtree`. + +## Subprocess: the two-tool rule + +There are two primitives. They are NOT interchangeable. + +- **`SubProcess.run(cmd, cwd=, io_func=, timeout=)`** — the project's standard + wrapper. Uses `subprocess.Popen` + `pty.openpty()` on UNIX (child sees a real + TTY for git's progress output) and plain `Popen` with pipes on Windows. + Multi-thread safe. Has timeout. **Use this for everything by default.** +- **`subprocess.run(...)` directly** — only for the rare case where you need to + suppress stderr entirely (`stderr=subprocess.DEVNULL`) and a timeout but don't + want the live progress UI. The current example is the post-blob:none `git + show HEAD:` in `Git.fetch_self_version_from_remote` — its lazy fetch + spews `remote: ...` chatter we don't want surfaced. + +When deviating from `SubProcess.run`, document why in the function docstring. + +**Never** use `os.system("cd && cmd")` — `SubProcess.run(cmd, +cwd=)` is the correct idiom. SubProcess uses `execve`, not a shell, so +`cd` and `&&` aren't valid. + +**Never** use `os.forkpty()` directly anywhere in this codebase. Python 3.12 +flags it as unsafe in multi-threaded programs, and mama runs heavy parallel +loads. + +## Git commit style + +- Single line, `: ` prefix. Examples: + `feat:`, `fix:`, `refactor:`, `release:`, `cleanup:`. +- No `Co-Authored-By` trailer in this repo (different from many others). +- Atomic commits: one logical change per commit. Bug fix + refactor → two + commits, even when in one session. + +## Artifactory + git status invariants + +- **A 404 from artifactory for a git dep is NORMAL** (no prebuilt for current + commit). It must NOT wipe the `git_status` file. Wiping the status causes the + next `mama update` to read empty status → `check_status` → "SCM change + detected" → spurious full rebuild. `check_status` already detects real + url/tag/branch/commit changes via direct comparison. +- A 404 IS fatal for `is_pkg` deps (those URLs are mandatory). +- Shim probe (`try_load_artifactory_shim`) only runs when there's NO existing + working tree (`not self.is_real_clone()`). For an already-cloned dep, the + regular `fetch + reset` path is correct; running the probe in addition just + re-clones into a tempdir and does nothing useful. + +## SSH multiplex / parallel loading + +- `mama update` auto-enables `parallel_load`. The `fetch_slot` semaphore caps + concurrent git fetches at `parallel_max` (default 20). Independent of the + worker thread count. +- The shim probe's `SubProcess.run` calls go through `fetch_slot` too — count + the slot acquisitions per probe (one for the clone, possibly one for `git + show`). +- `ensure_master_for_url` is idempotent and serialised per-host. + +## Tests + +- Test directories under `tests/test_/`. Each is a pytest package. +- Mock external IO (subprocess, urlopen, ftplib) heavily. Tests must not hit + the network unless integration-flavored (`test_git_pin_change/`, + `test_papa_deploy/`). +- When patching: `patch('mama..')` — patch where it's looked up, + not where it's defined. +- Always run the **full** suite (`python -m pytest tests/`) before committing. + Total runtime ≈ 35 seconds. diff --git a/mama/_version.py b/mama/_version.py index 937f0b1..17a0cb8 100644 --- a/mama/_version.py +++ b/mama/_version.py @@ -1,2 +1,2 @@ # this is parsed by pyproject.toml and defines current mamabuild version -__version__ = "0.11.33" +__version__ = "0.12.01" diff --git a/mama/artifactory.py b/mama/artifactory.py index f121f43..a8d8ca7 100644 --- a/mama/artifactory.py +++ b/mama/artifactory.py @@ -1,7 +1,6 @@ from __future__ import annotations import os, sys, ftplib, traceback, getpass from typing import List, Tuple, TYPE_CHECKING -from urllib.error import HTTPError from .types.git import Git from .types.local_source import LocalSource @@ -10,7 +9,7 @@ from .types.asset import Asset from .utils.system import Color, System, console, error import mama.package as package -from .util import download_file, normalized_join, try_unzip +from .util import download_file, normalized_join, try_unzip, is_network_error from .papa_deploy import PapaFileInfo @@ -170,6 +169,7 @@ def artifactory_upload(ftp:ftplib.FTP_TLS, target_name:str, file_path:str): size = os.path.getsize(file_path) transferred = 0 lastpercent = 0 + indent = f' - {target_name: <16} ' with open(file_path, 'rb') as f: def print_progress(bytes): nonlocal transferred, lastpercent, size @@ -180,8 +180,8 @@ def print_progress(bytes): n = int(percent / 2) left = '=' * n right = ' ' * int(50 - n) - print(f'\r |{left}>{right}| {percent:>3} %', end='') - print(f' |>{" ":50}| {0:>3} %', end='') + console(f'\r{indent}|{left}>{right}| {percent:>3} %', end='') + console(f'{indent}|>{" ":50}| {0:>3} %', end='') # chdir into FTP_ROOT/target_name/ try: ftp.cwd(target_name) @@ -189,7 +189,7 @@ def print_progress(bytes): ftp.mkd(target_name) # create subdirectory if needed ftp.cwd(target_name) ftp.storbinary(f'STOR {os.path.basename(file_path)}', f, callback=print_progress) - print(f'\r |{"="*50}>| 100 %') + console(f'\r{indent}|{"="*50}>| 100 %') def artifact_already_exists(ftp:ftplib.FTP_TLS, target:BuildTarget, file_path:str): @@ -270,11 +270,16 @@ def artifactory_load_target(target:BuildTarget, deploy_path, num_files_copied) - def _fetch_package(target:BuildTarget, url, archive, cache_dir): + if not target.config.is_network_available(): + return None remote_file = f'http://{url}/{target.name}/{archive}.zip' try: - return download_file(remote_file, cache_dir, force=True, - message=f' Artifactory fetch {url}/{archive} ') + return download_file(remote_file, cache_dir, force=True, + message=f' - {target.name: <16} Artifactory fetch {url}/{archive} ', + name=target.name) except Exception as e: + if is_network_error(e): + target.config.mark_network_unavailable() if target.config.verbose or target.config.force_artifactory: error(f' Artifactory fetch failed with {e} {url}/{archive}.zip') @@ -283,14 +288,11 @@ def _fetch_package(target:BuildTarget, url, archive, cache_dir): if d.is_pkg: raise RuntimeError(f'Artifactory package {d} did not exist at {url}') - # if server gives us 404, then we need to wipe the git_status and re-initialize - # the dependency source from scratch - if d.is_git: - d: Git = d - if isinstance(e, HTTPError) and e.code == 404: - if target.config.verbose: - error(f' Resetting Git status file: {target.name}') - d.reset_status(target.dep) + # NB: a 404 here for a git dep is normal (no prebuilt archive uploaded + # for the current commit). DO NOT wipe git_status - check_status already + # detects url/tag/branch/commit changes from the mamafile; wiping the + # status causes the *next* `mama update` to falsely report 'SCM change + # detected' and trigger a full rebuild of an already-up-to-date dep. return None @@ -317,7 +319,7 @@ def artifactory_fetch_and_reconfigure(target:BuildTarget) -> Tuple[bool, list]: archive = artifactory_archive_name(target) if not archive: return (False, None) - + cache_dir = target.dep.dep_dir #target.dep.workspace local_file = normalized_join(cache_dir, f'{archive}.zip') @@ -334,5 +336,67 @@ def artifactory_fetch_and_reconfigure(target:BuildTarget) -> Tuple[bool, list]: local_file = _fetch_package(target, url, archive, cache_dir) if not local_file: return (False, None) - console(f' Artifactory unzip {archive}') + console(f' - {target.name: <16} Artifactory unzip {archive}') return unzip_and_load_target(target, local_file) + + +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 + + # First probe: commit-hash-based archive name. Works for the common case. + probe_target = BuildTarget(name=dep.name, config=config, dep=dep, args=dep.target_args) + fetched, dependencies = artifactory_fetch_and_reconfigure(probe_target) + + # Fallback: dep may pin target.version (e.g. boost 1.60), so its archive + # name doesn't include the commit hash. Sparse-fetch only the mamafile, + # grep self.version, and re-probe with that version. + if not fetched: + version = git.fetch_self_version_from_remote(dep) + if version: + if config.verbose: + console(f' {dep.name} shim probe: retrying with self.version={version}', color=Color.YELLOW) + probe_target = BuildTarget(name=dep.name, config=config, dep=dep, args=dep.target_args) + probe_target.version = version + fetched, dependencies = artifactory_fetch_and_reconfigure(probe_target) + + if not fetched: + # Reset any side effect on the dep so the clone path can run cleanly. + dep.from_artifactory = False + return (None, None) + + # Hit: persist marker and return the configured target. + archive = artifactory_archive_name(probe_target) + dep.write_shim_marker(archive_name=archive or '', commit_hash=commit_hash) + config.update_stats.record_shim() + if config.print: + console(f' - Target {dep.name: <16} SHIM FETCHED {archive}', color=Color.GREEN) + + return (probe_target, dependencies) diff --git a/mama/build_config.py b/mama/build_config.py index da65dbf..ff1cf63 100644 --- a/mama/build_config.py +++ b/mama/build_config.py @@ -1,5 +1,5 @@ from __future__ import annotations -import os, sys, tempfile, platform, psutil, shutil +import os, sys, tempfile, platform, psutil, shutil, threading, time from typing import List, TYPE_CHECKING from mama.platforms.oclea import Oclea from mama.platforms.xilinx import Xilinx @@ -17,6 +17,54 @@ if TYPE_CHECKING: from .build_dependency import BuildDependency +class UpdateStats: + """Counts and times clone/pull/shim-fetch activity during the load phase.""" + def __init__(self): + self._lock = threading.Lock() + self.cloned = 0 + self.pulled = 0 + self.shim_fetched = 0 + self._start = None + self._duration = 0.0 + + def start(self): + self._start = time.monotonic() + + def stop(self): + if self._start is not None: + self._duration = time.monotonic() - self._start + self._start = None + + def record_clone(self): + with self._lock: self.cloned += 1 + + def record_pull(self): + with self._lock: self.pulled += 1 + + def record_shim(self): + with self._lock: self.shim_fetched += 1 + + @property + def total(self) -> int: + return self.cloned + self.pulled + self.shim_fetched + + @property + def duration(self) -> float: + return self._duration + + def summary_line(self) -> str: + """One-line summary, or '' if nothing happened.""" + if self.total == 0: + return '' + parts = [] + if self.shim_fetched: parts.append(f'{self.shim_fetched} shim-fetched') + if self.pulled: parts.append(f'{self.pulled} pulled') + if self.cloned: parts.append(f'{self.cloned} cloned') + # Local import to avoid circular dependency with util + from .util import get_time_str + return f'Updated {self.total} target(s): {", ".join(parts)} in {get_time_str(self._duration)}' + + ### # Mama Build Configuration is created only once in the root project working directory # This configuration is then passed down to dependencies @@ -54,6 +102,7 @@ def __init__(self, args): self.sanitize = None # gcc/clang: -fsanitize=[thread|leak|address|undefined] self.coverage = None # gcc/clang: gcov | msvc: /fsanitize-coverage=edge self.coverage_report = None # runs gcovr to generate coverage report + self.update_stats = UpdateStats() # clone/pull/shim counters for the load phase summary self.enable_clang_tidy = False # enables clang-tidy static analysis during build self.clang_tidy_path = None # resolved path to clang-tidy executable # supported platforms @@ -124,6 +173,7 @@ def __init__(self, args): self.workspaces_root = util.normalized_path(os.getenv('HOMEPATH')) else: self.workspaces_root = os.getenv('HOME') + self._network_available = None # None=untested, True/False=result self.unused_args = [] self.loaded_dependencies : dict[str, BuildDependency] = {} self.parse_args(args) @@ -1184,3 +1234,14 @@ def no_specific_target(self) -> bool: """ True if no target or 'all' was specified from cmdline """ return self.no_target() or self.targets_all() + def is_network_available(self) -> bool: + """Lazily cached: True until a clearly network-related failure marks it False.""" + return self._network_available is not False + + def mark_network_unavailable(self): + if self._network_available is not False: + if self.print: + from .utils.system import console, Color + console(' Network unavailable — using cached packages where possible', color=Color.YELLOW) + self._network_available = False + diff --git a/mama/build_dependency.py b/mama/build_dependency.py index f530c1a..7b96d46 100644 --- a/mama/build_dependency.py +++ b/mama/build_dependency.py @@ -6,12 +6,15 @@ from .types.git import Git from .types.local_source import LocalSource from .utils.system import Color, console, error -from .artifactory import artifactory_fetch_and_reconfigure +from .artifactory import artifactory_fetch_and_reconfigure, try_load_artifactory_shim from .util import normalized_join, normalized_path, read_text_from, write_text_to, read_lines_from from .parse_mamafile import parse_mamafile, update_mamafile_tag, update_cmakelists_tag import mama.package as package +MAMA_SHIM_FILENAME = 'mama_shim' + + if TYPE_CHECKING: from .build_config import BuildConfig from .build_target import BuildTarget @@ -36,6 +39,7 @@ def __init__(self, parent:BuildDependency, config:BuildConfig, self.currently_loading = False self.from_artifactory = False # if true, this Dependency was loaded from Artifactory self.did_check_artifactory = False # if true, artifactory was already checked and can be skipped + self._is_shim_cache = None # tri-state cache for is_artifactory_shim() self.is_root = parent is None # Root deps are always built self.children: List[BuildDependency] = [] self.product_sources = [] @@ -198,6 +202,71 @@ def build_dir_exists(self): return os.path.exists(self.build_dir) + def mama_shim_file(self) -> str: + """ Marker file path identifying this dep as an artifactory shim. """ + return normalized_join(self.build_dir, MAMA_SHIM_FILENAME) + + + def is_artifactory_shim(self) -> bool: + """True if this dep was loaded from artifactory without a git clone. + Cached: state only changes via write/remove_shim_marker and dirty().""" + if self._is_shim_cache is None: + self._is_shim_cache = self.dep_source.is_git \ + and os.path.exists(self.mama_shim_file()) \ + and not self.is_real_clone() + return self._is_shim_cache + + + def is_real_clone(self) -> bool: + """ True if this dep has an actual git working tree on disk. """ + return self.src_dir is not None and os.path.exists(f'{self.src_dir}/.git') + + + def write_shim_marker(self, archive_name: str, commit_hash: str): + """ + Persist shim metadata so subsequent runs (and Phase 7 transitions) + can identify the shim and know which archive backed it. + """ + git: Git = self.dep_source + lines = [ + 'shim 1', + f'name {self.name}', + f'url {git.url}', + f'branch {git.branch or ""}', + f'tag {git.tag or ""}', + f'hash {commit_hash}', + f'archive {archive_name}', + ] + write_text_to(self.mama_shim_file(), '\n'.join(lines) + '\n') + # Invalidate (not set True): a real .git may also be present. + self._is_shim_cache = None + + + def read_shim_marker(self) -> dict: + """ + Returns a dict of shim metadata, or empty dict if no marker. + Keys: name, url, branch, tag, hash, archive. + """ + result = {} + path = self.mama_shim_file() + if not os.path.exists(path): + return result + for line in read_lines_from(path): + line = line.rstrip() + if not line or line == 'shim 1': + continue + key, _, value = line.partition(' ') + result[key] = value + return result + + + def remove_shim_marker(self): + path = self.mama_shim_file() + if os.path.exists(path): + os.remove(path) + self._is_shim_cache = False + + def create_build_dir_if_needed(self): if not os.path.exists(self.build_dir): # check to avoid Access Denied errors os.makedirs(self.build_dir, exist_ok=True) @@ -227,6 +296,9 @@ def _load_target(self) -> BuildTarget: return self.target def _git_checkout_if_needed(self) -> bool: + # Shims have no working tree; upstream check happens via ls-remote in try_load_artifactory_shim. + if self.is_artifactory_shim(): + return False if not self.is_root and self.dep_source.is_git: git:Git = self.dep_source return git.dependency_checkout(self) @@ -248,7 +320,29 @@ def _load(self): self._update_dep_name_and_dirs(self.name) self.create_build_dir_if_needed() - git_changed = self._git_checkout_if_needed() ## pull Git before loading target Mamafile + loaded_from_pkg = False + git_changed = False + + # Try artifactory shim BEFORE the expensive git clone. + # For non-root git deps with no existing working tree, probe artifactory + # using the commit hash from `git ls-remote` (no clone). On hit, load + # papa.txt exports/deps and skip clone. If a real clone already exists, + # we skip the shim probe - the user already paid the clone cost and + # the regular update path (fetch+reset) is the right call. + if not self.is_root and self.dep_source.is_git \ + and not self.is_real_clone() \ + and self.can_fetch_artifactory(print=False, which='SHIM'): + shim_target, shim_deps = try_load_artifactory_shim(self) + if shim_target is not None: + self.target = shim_target + self.did_check_artifactory = True + if shim_deps: + for dep_source in shim_deps: + self.add_child(dep_source) + loaded_from_pkg = True + + if not loaded_from_pkg: + git_changed = self._git_checkout_if_needed() ## pull Git before loading target Mamafile target = self._load_target() ## load target for Git and Src @@ -257,9 +351,8 @@ def _load(self): # if artifactory_fetch_and_reconfigure succeeds, it will overwrite products and libs # and sets self.from_artifactory - loaded_from_pkg = False should_load_art = self.should_load_artifactory() - if should_load_art and self.can_fetch_artifactory(print=True, which='LOAD'): + if not loaded_from_pkg and should_load_art and self.can_fetch_artifactory(print=True, which='LOAD'): self.did_check_artifactory = True fetched, dependencies = artifactory_fetch_and_reconfigure(target) if fetched: @@ -268,7 +361,7 @@ def _load(self): loaded_from_pkg = True elif self.dep_source.is_pkg: raise RuntimeError(f' - Target {self.name} failed to load artifactory pkg {self.dep_source}') - elif should_load_art and self.is_force_art_target(): + elif not loaded_from_pkg and should_load_art and self.is_force_art_target(): raise RuntimeError(f' - Target {self.name} failed to find artifactory pkg {self.dep_source} but `art` was specified') # load any build products from previous builds @@ -353,6 +446,12 @@ def build(r): console(f' - Target {target.name: <16} BUILD [{r}] {args}', color=Color.YELLOW) return True + # Artifactory shim: no source on disk, nothing to build from. The shim was + # already (re-)loaded during _load(); a rebuild requires `mama unshallow` + # to convert it to a real clone first. + if self.is_artifactory_shim(): + return False + if conf.target and not is_target: # if we called: "target=SpecificProject" return False # skip build if target doesn't match @@ -498,10 +597,17 @@ def mamafile_exists(self): def update_mamafile_tag(self): + # Shims have no source; the mamafile we'd be tagging doesn't exist on disk. + # Explicit short-circuit so a future parent-mamafile fetch (Phase 2 target.version + # probe) doesn't accidentally flag the shim as "modified" every run. + if self.is_artifactory_shim(): + return False return self.src_dir and update_mamafile_tag(self.config, self.mamafile_path(), self.build_dir) def update_cmakelists_tag(self): + if self.is_artifactory_shim(): + return False return self.src_dir and update_cmakelists_tag(self.config, self.cmakelists_path(), self.build_dir) @@ -635,3 +741,6 @@ def dirty(self): if os.path.exists(papafile): os.remove(papafile) if self.config.verbose: console(' dirty: removed papa.txt') + + # remove shim marker so next build re-evaluates artifactory freshness + self.remove_shim_marker() diff --git a/mama/build_target.py b/mama/build_target.py index 2515ec3..bf0d398 100644 --- a/mama/build_target.py +++ b/mama/build_target.py @@ -1504,14 +1504,39 @@ def _execute_deploy_tasks(self): if not (for_all or no_targets or one_target): return # not going to deploy + # Shim is read-only: its papa.txt and unzipped tree must not be overwritten + # by a re-deploy or re-upload. The artifactory already has the package. + if self.dep.is_artifactory_shim(): + if self.config.print: + console(f' - Target {self.name: <16} DEPLOY skipped (artifactory shim)', color=Color.YELLOW) + console(f' To repackage from source, run: mama unshallow {self.name}') + return + self.deploy() # user customization if self.config.upload: papa_upload_to(self, self.papa_path) + def _require_source(self, action: str) -> bool: + """ + For commands that need source on disk (test, start, open), + refuse on shims with a clear message pointing at `mama unshallow`. + Returns True if the action may proceed, False if it was refused. + """ + if not self.dep.is_artifactory_shim(): + return True + if self.config.print: + console(f' - Target {self.name: <16} {action.upper()} skipped: artifactory shim has no source on disk', + color=Color.YELLOW) + console(f' To fetch source, run: mama unshallow {self.name}') + return False + + def _execute_run_tasks(self): if self.is_test_target(): + if not self._require_source('test'): + return test_args = self.config.test.lstrip() if self.config.test_until_failure > 0: start = time.time() @@ -1532,6 +1557,8 @@ def _execute_run_tasks(self): if self.config.start: # start only if it's the current target or root target if self.is_current_target() or (self.dep.is_root and self.config.no_specific_target()): + if not self._require_source('start'): + return start_args = self.config.start.lstrip() if self.config.print: console(f' - Starting {self.name} {start_args}') self.start(start_args) diff --git a/mama/dependency_chain.py b/mama/dependency_chain.py index 4ad289e..9ee9ef6 100644 --- a/mama/dependency_chain.py +++ b/mama/dependency_chain.py @@ -435,6 +435,7 @@ def load_dependency_chain(root: BuildDependency): ssh_multiplex.init_fetch_semaphore(root.config.parallel_max) + root.config.update_stats.start() with concurrent.futures.ThreadPoolExecutor(max_workers=256) as e: def load_dependency(dep: BuildDependency): if dep.already_loaded: @@ -454,6 +455,10 @@ def load_dependency(dep: BuildDependency): dep.after_load() return changed load_dependency(root) + root.config.update_stats.stop() + summary = root.config.update_stats.summary_line() + if summary and root.config.print: + console(f' {summary}', color=Color.BLUE) def print_dependencies(root: BuildDependency): diff --git a/mama/main.py b/mama/main.py index afbbe93..4dc4faf 100644 --- a/mama/main.py +++ b/mama/main.py @@ -121,7 +121,13 @@ def open_project(config: BuildConfig, root_dependency: BuildDependency): found = root_dependency if name == 'root' else find_dependency(root_dependency, name) if not found: raise KeyError(f'No project named {name}') - + + # `mama open ` has no source dir to open; tell the user how to materialize one. + if found.is_artifactory_shim(): + console(f'Target {found.name} is an artifactory shim — no source files available locally.', color=Color.YELLOW) + console(f'To fetch source, run: mama unshallow {found.name}') + return + if config.msvc: solutions = glob_with_extensions(found.build_dir, ['.sln']) if solutions: diff --git a/mama/papa_deploy.py b/mama/papa_deploy.py index c8324ed..e7721bd 100644 --- a/mama/papa_deploy.py +++ b/mama/papa_deploy.py @@ -124,12 +124,19 @@ def append(relpath, abs_include): def papa_deploy_to(target:BuildTarget, package_full_path:str, - r_includes:bool, r_dylibs:bool, + r_includes:bool, r_dylibs:bool, r_syslibs:bool, r_assets:bool): config = target.config detail_echo = config.print and target.is_current_target() and (not config.test) if detail_echo: console(f' - PAPA Deploy {package_full_path}') + # Defense-in-depth: never write into a directory holding a shim marker. A + # misconfigured caller could pass the shim's build_dir directly, which would + # corrupt the artifactory snapshot (papa.txt + unzipped tree) the next mama + # run depends on. The proper deploy-skip lives in _execute_deploy_tasks. + if os.path.exists(os.path.join(package_full_path, 'mama_shim')): + raise RuntimeError(f'papa_deploy refused: {package_full_path} contains a mama_shim marker.') + dependencies = _gather_dependencies(target) if not os.path.exists(package_full_path): # check to avoid Access Denied errors diff --git a/mama/types/git.py b/mama/types/git.py index 078bb8b..abd0b47 100644 --- a/mama/types/git.py +++ b/mama/types/git.py @@ -1,12 +1,12 @@ from __future__ import annotations from typing import TYPE_CHECKING -import os, shutil, stat, string +import os, shutil, stat, string, time, re, tempfile, subprocess from .dep_source import DepSource from ..utils.system import Color, System, console, error -from ..utils.sub_process import SubProcess, execute, execute_piped, execute_piped_echo +from ..utils.sub_process import SubProcess, execute_piped, execute_piped_echo from ..utils import ssh_multiplex -from ..util import is_dir_empty, save_file_if_contents_changed, read_lines_from, path_join +from ..util import is_dir_empty, save_file_if_contents_changed, read_lines_from, path_join, is_network_error, get_time_str, normalized_path if TYPE_CHECKING: @@ -63,12 +63,27 @@ def get_papa_string(self): def run_git(self, dep: BuildDependency, git_command, throw=True): - cmd = f"cd {dep.src_dir} && git {git_command}" + # Shim has no .git; `cd src_dir && git ...` would walk up and hit the wrong repo. + if dep.is_artifactory_shim(): + msg = f'Target {dep.name} is an artifactory shim; cannot run `git {git_command}`' + if dep.config.verbose: error(f' {dep.name: <16} {msg}') + if throw: raise RuntimeError(msg) + return 1 + cmd = f"git {git_command}" if dep.config.verbose: - console(f' {dep.name: <16} git {git_command}', color=Color.YELLOW) + console(f' {dep.name: <16} {cmd}', color=Color.YELLOW) ssh_multiplex.ensure_master_for_url(self.url) + # Capture and prefix each line so parallel updates don't tear and the + # user can see which target said what (e.g. 'remote: Enumerating ...'). + def prefixed(p:SubProcess, line:str): + line = line.rstrip() + if line: console(f' {dep.name: <16} {line}') with ssh_multiplex.fetch_slot(): - return execute(cmd, throw=throw) + # cwd= instead of `cd && cmd` because SubProcess uses execve, not a shell. + result = SubProcess.run(cmd, cwd=dep.src_dir, io_func=prefixed) + if result != 0 and throw: + raise RuntimeError(f'{cmd} (in {dep.src_dir}) failed with return code {result}') + return result def _has_local_modifications(self, dep: BuildDependency) -> bool: @@ -93,6 +108,73 @@ def get_current_repository_commit(dep: BuildDependency): def is_hex_string(s: str) -> bool: return len(s) > 0 and all(c in string.hexdigits for c in s) + + # Captures only quoted literals; f-strings / computed values miss on purpose. + _SELF_VERSION_RE = re.compile(r"""^\s*self\.version\s*=\s*['"]([^'"]+)['"]""", re.MULTILINE) + + @staticmethod + def extract_self_version(mamafile_text: str): + """Find `self.version = ''` in a mamafile. Returns the version + string or None. Computed values (f-strings, function calls) are + intentionally not handled - those callers must full-clone.""" + m = Git._SELF_VERSION_RE.search(mamafile_text) + return m.group(1) if m else None + + + def fetch_self_version_from_remote(self, dep: BuildDependency): + """Fetches just the dep's mamafile to read `self.version` without + pulling the full repo. Used by the shim probe for version-pinned deps + (e.g. boost 1.60) where the archive name doesn't track the commit hash. + + Two-tool design: the clone goes through SubProcess.run (for the live + progress UI), but the one-shot `git show` uses subprocess.run + timeout + because SubProcess.run uses os.forkpty() which is unsafe in heavy + parallel mode and has no timeout to abort a stuck lazy fetch. + + Returns the version string or None on any failure.""" + if not dep.config.is_network_available(): + return None + mamafile_name = self.mamafile or 'mamafile.py' + branch = self.branch or self.tag or '' + branch_arg = f' --branch {branch}' if branch and not Git.is_hex_string(branch) else '' + try: + # ignore_cleanup_errors: on Windows git sets read-only on .git/objects/*, + # which trips shutil.rmtree. normalized_path: project convention is + # forward slashes; raw tempdir paths on Windows use backslashes that + # would be eaten by shlex.split inside SubProcess. + with tempfile.TemporaryDirectory(prefix='mama_probe_', ignore_cleanup_errors=True) as tmp: + tmp = normalized_path(tmp) + clone_cmd = f'git clone --depth=1 --filter=blob:none --no-checkout{branch_arg} {self.url} {tmp}' + result, _, elapsed = self._run_git_with_filtered_progress(dep, clone_cmd, label='PROBE') + if result != 0: + if dep.config.print: + console(f'\r - Target {dep.name: <16} PROBE FAILED ({result}) after {elapsed} ', color=Color.RED) + return None + # subprocess.run, not SubProcess.run: see docstring above. + # stderr=DEVNULL drops the lazy-fetch's `remote: ...` noise. + try: + cp = subprocess.run(['git', '-C', tmp, 'show', f'HEAD:{mamafile_name}'], + stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, + timeout=30) + except subprocess.TimeoutExpired: + if dep.config.verbose: error(f' {dep.name: <16} PROBE timed out fetching mamafile') + return None + if cp.returncode != 0: + return None + content = cp.stdout.decode('utf-8', errors='replace') + if not content: + return None + version = Git.extract_self_version(content) + if dep.config.print and version: + console(f'\r - Target {dep.name: <16} PROBE FOUND self.version={version} in {elapsed}', color=Color.BLUE) + return version + except Exception as e: + if is_network_error(e): + dep.config.mark_network_unavailable() + if dep.config.verbose: + error(f' {self.name} sparse-probe failed: {e}') + return None + def init_commit_hash(self, dep: BuildDependency, use_cache: bool, fetch_remote: bool): """ Gets the latest commit hash, based on git source tag and branch options. @@ -123,6 +205,8 @@ def init_commit_hash(self, dep: BuildDependency, use_cache: bool, fetch_remote: # can we fetch the latest commit from remote instead? if fetch_remote: + if not dep.config.is_network_available(): + return None arguments = 'HEAD' try: if self.branch: arguments = self.branch @@ -135,6 +219,8 @@ def init_commit_hash(self, dep: BuildDependency, use_cache: bool, fetch_remote: console(f' {self.name} git ls-remote {self.url} {arguments}: {result}', color=Color.YELLOW) return result except Exception as e: + if is_network_error(e): + dep.config.mark_network_unavailable() if dep.config.verbose: error(f' {self.name} git ls-remote {self.url} {arguments} failed: {e}') return None @@ -156,6 +242,8 @@ def fetch_origin(self, dep: BuildDependency): branch = self.branch_or_tag() if Git.is_hex_string(branch): return # no need to fetch if we're pinned to a specific commit hash + if not dep.config.is_network_available(): + return if self.tag: self.run_git(dep, f"fetch origin tag {branch} -q") else: @@ -261,9 +349,14 @@ def reclone_wipe(self, dep: BuildDependency): shutil.rmtree(dep.dep_dir) - def clone_with_filtered_progress(self, dep: BuildDependency, clone_args: str, clone_to_dir: str): + def _run_git_with_filtered_progress(self, dep: BuildDependency, cmd: str, label: str): + """Run a git command with progress filtered into one redrawn status line. + Returns (exit_code, captured_output, elapsed_str). Does not raise. + Used by full clone and by the sparse mamafile probe so both share the + same nice UI instead of spewing git's raw remote: output.""" output = '' current_percent = -1 + start = time.monotonic() def print_output(p:SubProcess, line:str): nonlocal output, current_percent if 'remote: Counting objects:' in line or \ @@ -286,7 +379,8 @@ def print_output(p:SubProcess, line:str): elif 'Receiving objects:' in line: status = 'receiving objects ' elif 'Resolving deltas:' in line: status = 'resolving deltas ' elif 'Updating files:' in line: status = 'updating files ' - console(f'\r - Target {dep.name: <16} CLONE {status} {current_percent:3}%', end='') + elapsed = get_time_str(time.monotonic() - start) + console(f'\r - Target {dep.name: <16} {label} {status} {current_percent:3}% ({elapsed})', end='') elif 'Cloning into ' in line: return elif 'Are you sure you want to continue connecting' in line: @@ -300,31 +394,38 @@ def print_output(p:SubProcess, line:str): if dep.config.verbose: console(line) - # run the command, working dir not needed since it should be a full path in the clone_args - cmd = f'git clone {clone_args} {clone_to_dir}' if dep.config.verbose: console(f' {dep.name: <16} {cmd}') ssh_multiplex.ensure_master_for_url(self.url) with ssh_multiplex.fetch_slot(): result = SubProcess.run(cmd, io_func=print_output) + return result, output, get_time_str(time.monotonic() - start) + - # handle the result: + def clone_with_filtered_progress(self, dep: BuildDependency, clone_args: str, clone_to_dir: str): + cmd = f'git clone {clone_args} {clone_to_dir}' + result, output, elapsed = self._run_git_with_filtered_progress(dep, cmd, label='CLONE') + if result == 0: + dep.config.update_stats.record_clone() if dep.config.print: if result == 0: - console(f'\r - Target {dep.name: <16} CLONE SUCCESS ', color=Color.BLUE) + console(f'\r - Target {dep.name: <16} CLONE SUCCESS {elapsed} ', color=Color.BLUE) if dep.config.verbose and output: console(output, end='') else: - console(f'\r - Target {dep.name: <16} CLONE FAILED ({result}) ', color=Color.RED) + console(f'\r - Target {dep.name: <16} CLONE FAILED ({result}) after {elapsed} ', color=Color.RED) if output: console(output, end='') - raise RuntimeError(f'Target {self.name} clone failed: {cmd}') + raise RuntimeError(f'Target {self.name} clone failed after {elapsed}: {cmd}') def clone_or_pull(self, dep: BuildDependency, wiped=False): # by default we create a shallow clone, unless unshallow is specified in config or this dep unshallow = dep.config.unshallow or (not self.shallow) if is_dir_empty(dep.src_dir): + if not dep.config.is_network_available(): + raise RuntimeError(f'Target {dep.name} requires network to clone but network is unavailable.' + \ + ' Check your connection or use a cached artifactory package.') if not wiped and dep.config.print: console(f" - Target {dep.name: <16} CLONE because src is missing", color=Color.BLUE) br_or_tag = self.branch_or_tag() @@ -335,6 +436,10 @@ def clone_or_pull(self, dep: BuildDependency, wiped=False): self.clone_with_filtered_progress(dep, clone_args, dep.src_dir) self.checkout_current_branch_or_tag(dep, is_commit_pin=is_commit_pin) else: + if not dep.config.is_network_available(): + if dep.config.print: + console(f" - Target {dep.name: <16} SKIP PULL (network unavailable, using cached source)", color=Color.YELLOW) + return if dep.config.print: console(f" - Pulling {dep.name: <16} SCM change detected", color=Color.BLUE) # check for local modifications before potentially destructive operations @@ -356,6 +461,7 @@ def clone_or_pull(self, dep: BuildDependency, wiped=False): else: self.run_git(dep, "fetch -q", throw=False) self.run_git(dep, "reset --hard @{upstream} -q") # https://git-scm.com/docs/gitrevisions#Documentation/gitrevisions.txt-branchnameupstreamegmasterupstreamu + dep.config.update_stats.record_pull() def unshallow(self, dep: BuildDependency): diff --git a/mama/util.py b/mama/util.py index 89376aa..b3d1aa9 100644 --- a/mama/util.py +++ b/mama/util.py @@ -199,10 +199,17 @@ def get_time_str(seconds: float): return f'{int(seconds/(24*60*60))}d {int((seconds%(24*60*60))/(60*60))}h {int(seconds/60)%60}m {int(seconds)%60}s' -def download_file(remote_url:str, local_dir:str, force=False, message=None): +def download_file(remote_url:str, local_dir:str, force=False, message=None, name:str=None): + """Downloads remote_url into local_dir. + - force=False: use any existing local file without contacting the server. + - force=True: open the connection and compare Content-Length; skip the + body transfer when sizes match (used for artifactory fetches so we don't + re-download archives already on disk). + - name: optional target name for prefixing log lines under parallel updates.""" local_file = os.path.join(local_dir, os.path.basename(remote_url)) - if not force and os.path.exists(local_file): # download file? - console(f" Using locally cached {local_file}") + indent = f' - {name: <16} ' if name else ' ' + if not force and os.path.exists(local_file): + console(f'{indent}Using locally cached {local_file}') return local_file start = time.time() if not os.path.exists(local_dir): @@ -219,8 +226,18 @@ def download_file(remote_url:str, local_dir:str, force=False, message=None): with request.urlopen(remote_url, context=ctx, timeout=5) as urlfile: size = urlfile.info()['Content-Length'] size = int(size.strip()) if size else None + + # Size-match cache: skip the body transfer entirely when the local + # file matches the remote Content-Length. Costs one HTTP round-trip + # (already paid by opening the connection); saves the whole body. + if size is not None and os.path.exists(local_file) \ + and os.path.getsize(local_file) == size: + console(f'{indent}Artifactory CACHE (size-match) ' + f'{os.path.basename(local_file)} ({get_file_size_str(size)})') + return local_file + if not message: message = f'Downloading {remote_url}' - print(f'{message} {get_file_size_str(size) if size else "unknown size"}') + console(f'{message} {get_file_size_str(size) if size else "unknown size"}') if not size: return None @@ -230,7 +247,7 @@ def download_file(remote_url:str, local_dir:str, force=False, message=None): report_interval = max(1, int((100*1024*1024) / size)) transferred = 0 lastpercent = 0 - print(f' |{" ":50}<| {0:>3}%', end='') + console(f'{indent}|{" ":50}<| {0:>3}%', end='') with open(local_file, 'wb') as output: while transferred < size: data = urlfile.read(32*1024) # large chunks plz @@ -245,12 +262,12 @@ def download_file(remote_url:str, local_dir:str, force=False, message=None): right = '=' * n left = ' ' * int(50 - n) elapsed = time.time() - start - print(f'\r |{left}<{right}| {percent:>3}% ({get_time_str(elapsed)})', end='') + console(f'\r{indent}|{left}<{right}| {percent:>3}% ({get_time_str(elapsed)})', end='') # report actual percent here, just incase something goes wrong elapsed = time.time() - start percent = int((transferred / size) * 100.0) - print(f'\r |<{"="*50}| {percent:>3}% ({get_time_str(elapsed)})') + console(f'\r{indent}|<{"="*50}| {percent:>3}% ({get_time_str(elapsed)})') return local_file @@ -471,3 +488,54 @@ def copy_if_needed(src: str, dst: str, filter: list = None) -> bool: else: return copy_file(src, dst, filter) + +def is_network_error(e: Exception) -> bool: + """ + Returns True only if the exception clearly indicates network unavailability + (DNS failure, connection refused/reset, timeout). Returns False for auth + errors (SSH key rejected, HTTP 401/403), HTTP 404, and anything ambiguous. + """ + import subprocess, socket + from urllib.error import HTTPError, URLError + + if isinstance(e, subprocess.TimeoutExpired): + return True + if isinstance(e, HTTPError): + return False + if isinstance(e, URLError): + reason = getattr(e, 'reason', None) + if isinstance(reason, (socket.timeout, socket.gaierror, + ConnectionRefusedError, ConnectionResetError, + TimeoutError, OSError)): + return True + return not isinstance(reason, str) + if isinstance(e, (ConnectionRefusedError, ConnectionResetError, + TimeoutError, socket.timeout, socket.gaierror)): + return True + if isinstance(e, OSError): + import errno + if e.errno in (errno.ENETUNREACH, errno.EHOSTUNREACH, + errno.ECONNREFUSED, errno.ETIMEDOUT, errno.ECONNRESET): + return True + + msg = str(e).lower() + auth_patterns = [ + 'permission denied', 'authentication failed', + 'host key verification failed', + 'returned error: 401', 'returned error: 403', + 'invalid credentials', + ] + for p in auth_patterns: + if p in msg: + return False + network_patterns = [ + 'could not resolve host', 'connection refused', + 'connection timed out', 'network is unreachable', + 'no route to host', 'name or service not known', + 'temporary failure in name resolution', 'connection reset', + ] + for p in network_patterns: + if p in msg: + return True + return False + diff --git a/mama/utils/nonblocking_io.py b/mama/utils/nonblocking_io.py deleted file mode 100644 index 40f8600..0000000 --- a/mama/utils/nonblocking_io.py +++ /dev/null @@ -1,26 +0,0 @@ -import os, sys - -IS_POSIX = 'linux' in sys.platform.lower() or 'darwin' in sys.platform.lower() - -if IS_POSIX: - def set_nonblocking(fd): - import fcntl - flag = fcntl.fcntl(fd, fcntl.F_GETFL) - fcntl.fcntl(fd, fcntl.F_SETFL, flag | os.O_NONBLOCK) -else: - def set_nonblocking(fd): - import msvcrt - from ctypes import windll, byref, wintypes, WinError, POINTER - from ctypes.wintypes import HANDLE, DWORD, BOOL - PIPE_NOWAIT = DWORD(0x00000001) - def pipe_no_wait(pipefd): - SetNamedPipeHandleState = windll.kernel32.SetNamedPipeHandleState - SetNamedPipeHandleState.argtypes = [HANDLE, POINTER(DWORD), POINTER(DWORD), POINTER(DWORD)] - SetNamedPipeHandleState.restype = BOOL - h = msvcrt.get_osfhandle(pipefd) - res = windll.kernel32.SetNamedPipeHandleState(h, byref(PIPE_NOWAIT), None, None) - if res == 0: - print(WinError()) - return False - return True - return pipe_no_wait(fd) diff --git a/mama/utils/sub_process.py b/mama/utils/sub_process.py index 05ad69d..44221fc 100644 --- a/mama/utils/sub_process.py +++ b/mama/utils/sub_process.py @@ -1,215 +1,220 @@ -import os, shlex, shutil -from signal import SIGTERM -from errno import ECHILD +import os, shlex, shutil, threading import subprocess -from time import sleep -from .nonblocking_io import set_nonblocking from .system import System, console, error +# Linux/macOS: we allocate a PTY for the child so git etc. still see a TTY +# (preserves progress output and isatty checks). The pty.openpty() syscall +# does NOT fork - it just creates a master/slave fd pair - so it's safe to +# call from a worker thread. subprocess.Popen does the actual fork via +# posix_spawn/vfork which is multi-thread-safe, unlike the older os.forkpty +# which Python 3.12 flags with a DeprecationWarning specifically because of +# the threaded-deadlock risk. +if not System.windows: + import pty + + class SubProcess: """ - An alternative to subprocess.Popen with redirectable IO - using fork and forktty on UNIX. + Subprocess wrapper with optional line-by-line output capture. + + With ``io_func`` set, child's combined stdout+stderr is fed to ``io_func`` + one line at a time by a background reader thread. On UNIX a PTY is + allocated so the child sees a TTY (gets coloured/progress output). - Windows version uses standard subprocess.Popen with pipes + Without ``io_func``, the child inherits the parent's stdout/stderr - + used for things like `mama test` where output should flow directly. - Any redirected stdout/stderr which needs to retain its - terminal colors etc, should use this SubProcess + Replaces the previous os.fork/os.forkpty-based implementation which + was unsafe in multi-threaded programs (Python 3.12 deprecation warning, + real deadlocks under heavy parallel load). """ - def __init__(self, cmd, cwd, env=None, io_func=None): + def __init__(self, cmd, cwd=None, env=None, io_func=None): self.io_func = io_func self.status = None + self.process = None + self._reader_thread = None + self._reader_exc = None # exception raised inside io_func (re-raised in run()) + self._master_fd = None # UNIX PTY master fd; None on Windows or no-io_func paths env = env if env else os.environ.copy() - args = shlex.split(cmd) + args = shlex.split(cmd) if isinstance(cmd, str) else list(cmd) + # Resolve the executable ourselves so we don't have to ask the shell + # (avoids shell quoting/escaping pitfalls; same logic as before). executable = args[0] - if os.path.isfile(executable): # it's something like `./run_tests` or `/usr/bin/gcc` + if os.path.isfile(executable): executable = os.path.abspath(executable) elif System.windows and os.path.isfile(executable + '.exe'): executable = os.path.abspath(executable + '.exe') - else: # lookup from PATH - executable = shutil.which(args[0]) - if not executable: - raise OSError(f"SubProcess failed to start: {args[0]} not found in PATH") + else: + resolved = shutil.which(executable) + if not resolved: + raise OSError(f"SubProcess failed to start: {executable} not found in PATH") + executable = resolved args[0] = executable + if io_func is None: + # No capture: child inherits parent's stdio (terminal direct). + self.process = subprocess.Popen(args, cwd=cwd, env=env) + return + if System.windows: - self.process = None + # No PTY on Windows; merge stderr into stdout pipe, read line by line. + # text + bufsize=1 gives line-buffered Unicode lines on the parent side. + self.process = subprocess.Popen( + args, cwd=cwd, env=env, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, bufsize=1, universal_newlines=True, + ) + else: + # Allocate a PTY pair; child gets the slave end as its stdin/stdout/stderr. + self._master_fd, slave = pty.openpty() try: - stdout = subprocess.PIPE if io_func else None - stderr = subprocess.STDOUT if io_func else None - self.process = subprocess.Popen(args, cwd=cwd, env=env, shell=True, - universal_newlines=True, - stdout=stdout, - stderr=stderr) - except Exception as e: - raise RuntimeError(f"Popen failed {args}: {e}") - else: # all UNIX based systems support fork or forkpty - # FD visible only for the parent process, - # and can be used to read the child PTY output - self.parent_fd = 0 - if io_func: - self.pid, self.parent_fd = os.forkpty() + self.process = subprocess.Popen( + args, cwd=cwd, env=env, + stdin=slave, stdout=slave, stderr=slave, + close_fds=True, + ) + finally: + # Parent doesn't need the slave once Popen has it. + os.close(slave) + + self._reader_thread = threading.Thread(target=self._read_loop, daemon=True) + self._reader_thread.start() + + + def _read_loop(self): + try: + if self._master_fd is not None: + self._read_loop_pty() else: - self.pid = os.fork() + self._read_loop_pipe() + except Exception as e: + # Capture so run() can surface it. Don't crash the reader thread. + self._reader_exc = e + - # 0: inside the child process, PID inside the parent process - if self.pid == -1: - raise OSError(f"SubProcess failed to start: {cmd}") - elif self.pid == 0: # child process: - if cwd: os.chdir(cwd) - # execve: universal, but requires full path to program - os.execve(executable, args, env) - else: # parent process: - # set the parent FD as non-blocking, otherwise the async tasks will never finish - set_nonblocking(self.parent_fd) + def _read_loop_pty(self): + """Drain the PTY master until the child closes the slave end.""" + buf = bytearray() + try: + while True: + try: + chunk = os.read(self._master_fd, 8192) + except OSError: + # On Linux a closed slave produces EIO on the master. Treat as EOF. + break + if not chunk: + break + buf.extend(chunk) + while True: + nl = buf.find(b'\n') + if nl < 0: + break + line = bytes(buf[:nl]).decode('utf-8', errors='replace').rstrip('\r') + self.io_func(self, line) + del buf[:nl + 1] + finally: + if buf: + line = bytes(buf).decode('utf-8', errors='replace').rstrip('\r') + self.io_func(self, line) - def close(self): - self.kill() - if System.windows: - self.process.wait(1.0) - self.process = None - else: - if self.parent_fd: - os.close(self.parent_fd) - self.parent_fd = 0 + def _read_loop_pipe(self): + """Drain the stdout pipe (Windows path).""" + stdout = self.process.stdout + if not stdout: + return + for line in stdout: + self.io_func(self, line.rstrip('\r\n')) + + + def write(self, text: str): + """Send `text` to the child's stdin (used for interactive prompts + like SSH host-key acceptance).""" + if self._master_fd is not None: + os.write(self._master_fd, text.encode('utf-8')) + elif self.process and self.process.stdin and not self.process.stdin.closed: + try: + self.process.stdin.write(text) + self.process.stdin.flush() + except (BrokenPipeError, OSError): + pass def kill(self): - if System.windows: - self.process.kill() - else: - pid, self.pid = (self.pid, 0) - if pid > 0: + if self.process and self.process.poll() is None: + try: + self.process.terminate() + self.process.wait(timeout=1.0) + except subprocess.TimeoutExpired: try: - os.kill(pid, SIGTERM) - except: + self.process.kill() + self.process.wait(timeout=1.0) + except Exception: pass + except Exception: + pass - def try_wait(self): - """ Returns EXIT_STATUS int if process has finished, otherwise None """ - if System.windows: - self.status = self.process.poll() - return self.status - else: + def close(self): + self.kill() + # Reader thread exits when the PTY master sees EOF (slave closed by + # the child or by Popen's __exit__). Join briefly to drain any + # trailing buffered output the io_func hasn't seen yet. + if self._reader_thread: + self._reader_thread.join(timeout=2.0) + self._reader_thread = None + if self._master_fd is not None: try: - r, status = os.waitpid(self.pid, os.WNOHANG) - if r == self.pid: # r == pid: process finished - self.status = self._handle_exitstatus(status) - except OSError as e: - if e.errno == ECHILD: - self.status = -1 # ECHILD: no such child - return self.status + os.close(self._master_fd) + except OSError: + pass + self._master_fd = None - def _handle_exitstatus(self, status): - if os.WIFSIGNALED(status): - return -os.WTERMSIG(status) - elif os.WIFEXITED(status): - return os.WEXITSTATUS(status) - return -1 - - - def _parse_lines(self, text: str): - end = len(text) - start = 0 - line = '' - while start < end: - current = text.find('\n', start) - if current != -1: - eol = current - if (eol-start) > 0 and text[eol-1] == '\r': - eol -= 1 # drop the '\r' - line = text[start:eol] - start = current + 1 - else: # last token: - line = text[start:] - start = end - self.io_func(self, line) - - - def read_output(self) -> bool: - """ - Returns TRUE if output was read. - Calls self.io_func(line) for every line that was read. - Newlines are INCLUDED. - """ - try: - if System.windows: - if not self.process.stdout or self.process.stdout.closed: - return False - - text = self.process.stdout.readline() - # console(f'line: {text} status={self.process.poll()}', end='') - got_bytes = len(text) > 0 - if self.io_func and got_bytes: - self._parse_lines(text) - return got_bytes - else: - if not self.parent_fd: - return False - - data: bytes = os.read(self.parent_fd, 8192) - got_bytes = len(data) > 0 - if self.io_func and got_bytes: - text = data.decode() - self._parse_lines(text) - return got_bytes - except OSError as _: - # when in non-blocking IO, EAGAIN will be thrown if there's no data - # and when the other process closes the pipes - return False - - - def read_outputs(self, max_blocks=-1) -> bool: - """ Reads output multiple times until max_blocks calls of read_output() are done """ - num_reads = 0 - while self.read_output(): - num_reads += 1 - if max_blocks != -1 and num_reads >= max_blocks: - break # we've read enough - return num_reads > 0 + def try_wait(self): + """Returns the exit status if the child has finished, else None. + Kept for backwards-compat with callers that used the old polling API.""" + if self.process is None: + return self.status + rc = self.process.poll() + if rc is not None: + self.status = rc + return self.status - def write(self, text: str): - """ Writes the text to the process stdin """ - if System.windows: - if self.process.stdin and not self.process.stdin.closed: - self.process.stdin.write(text) - elif self.parent_fd: - os.write(self.parent_fd, text.encode()) @staticmethod - def run(cmd, cwd=None, env=None, io_func=None): + def run(cmd, cwd=None, env=None, io_func=None, timeout=None): """ - Runs the titled sub-process with `cmd` using fork or forktty if io_func is set - - cmd: full command string - - cwd: working dir for the subprocess - - env: execution environment, or None for default env - - io_func: if set, this callback will receive SubProcess p reference and each line from output - if None, then output is echoed as normal to stdout/stderr - - ``` - SubProcess.run('tool', 'cmake xyz', env) - SubProcess.run('tool', 'cmake xyz', io_func=lambda p, line: print(line)) - ``` + Runs `cmd` and returns its exit status. + - cmd: command string (shlex.split) or list of args. + - cwd: working directory for the child. + - env: environment dict, defaults to os.environ. + - io_func: callback `(SubProcess, line:str)` for each output line; + if None, child inherits parent's std streams. + - timeout: kill the child after this many seconds (raises + subprocess.TimeoutExpired). Default: no timeout. """ - p = SubProcess(cmd, cwd, env=env, io_func=io_func) + p = SubProcess(cmd, cwd=cwd, env=env, io_func=io_func) try: - while p.try_wait() is None: - p.read_outputs(max_blocks=1) - sleep(0.01) - p.read_outputs() # read any trailing output + try: + p.status = p.process.wait(timeout=timeout) + except subprocess.TimeoutExpired: + p.kill() + raise finally: p.close() + if p._reader_exc is not None: + raise p._reader_exc return p.status def execute(command, echo=False, throw=True): - """ + """ Executes a command and returns the status code. - command: command string - echo: if True, prints the command to console @@ -223,7 +228,6 @@ def execute(command, echo=False, throw=True): return retcode -# TODO: use new SubProcess.run instead def execute_piped(command, cwd=None, timeout=None, throw=True): """ Executes a command and returns the piped outout string diff --git a/mama/utils/system.py b/mama/utils/system.py index 839c7c4..464c459 100644 --- a/mama/utils/system.py +++ b/mama/utils/system.py @@ -1,4 +1,4 @@ -import sys, subprocess, platform +import sys, subprocess, platform, threading from termcolor import colored is_windows = sys.platform == 'win32' @@ -48,9 +48,23 @@ def get_colored_text(text:str, color): return colored(text, color=color) if color else text +# Serialize writes and finalize any pending progress line before a normal +# status print, so parallel redraws don't get glued to status lines. +_console_lock = threading.Lock() +_progress_active = False # last write left cursor mid-row + + def console(text:str, color=None, end="\n"): """ Always flush to support most build environments """ - print(get_colored_text(text, color), end=end, flush=True) + global _progress_active + # Cheap O(1) check: redraws start with \r to reset the cursor; only those + # may overwrite an in-flight progress line. Anything else gets a leading \n. + is_redraw = text.startswith('\r') + with _console_lock: + if _progress_active and not is_redraw: + print() + print(get_colored_text(text, color), end=end, flush=True) + _progress_active = (end != '\n') def error(text:str): diff --git a/tests/test_artifactory_404_status/test_artifactory_404_status.py b/tests/test_artifactory_404_status/test_artifactory_404_status.py new file mode 100644 index 0000000..1caa8ce --- /dev/null +++ b/tests/test_artifactory_404_status/test_artifactory_404_status.py @@ -0,0 +1,131 @@ +"""Regression test for the 'SCM change detected on second mama update' bug. + +Background: when artifactory returned 404 for a git dep (normal — there's just +no prebuilt archive for the current commit), the previous _fetch_package code +deleted the git_status file via Git.reset_status(). The next ``mama update`` +then read an empty status, treated the dep as first-time, and printed +``Pulling X SCM change detected`` followed by a full rebuild — even though +nothing in the source had changed. + +This test pins the corrected behaviour: a 404 on a git dep MUST NOT touch +the git_status file. The mamafile-level url/tag/branch/commit comparison in +check_status already handles legitimate source changes; 404 only means +"no archive for this commit on the server", which is normal and benign. +""" +from __future__ import annotations + +import os +import sys +import tempfile +import shutil +from unittest.mock import Mock, patch +from urllib.error import HTTPError + +import pytest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..'))) +from mama import artifactory as art # noqa: E402 +from mama.types.git import Git # noqa: E402 + + +def _make_target_with_status(tmpdir): + """Build a BuildTarget-shaped stub whose git_status file already exists.""" + git = Git(name='libfoo', url='https://example.com/libfoo.git', + branch='main', tag='', mamafile=None, shallow=True, args=[]) + + config = Mock() + config.is_network_available.return_value = True + config.verbose = False + config.force_artifactory = False + + dep = Mock() + dep.name = 'libfoo' + dep.build_dir = tmpdir + dep.dep_source = git + dep.config = config + + target = Mock() + target.name = 'libfoo' + target.config = config + target.dep = dep + + # Pre-populate the git_status file as a successful prior run would have. + status_path = git.git_status_file(dep) + os.makedirs(os.path.dirname(status_path), exist_ok=True) + with open(status_path, 'w') as f: + f.write(git.format_git_status(git.url, git.tag, git.branch, 'abc1234')) + return target, status_path + + +def _http_404(): + """A 404 HTTPError instance matching what urllib.request.urlopen raises.""" + return HTTPError(url='http://example.com/x.zip', code=404, + msg='Not Found', hdrs=None, fp=None) + + +def test_404_does_not_wipe_git_status(): + """The bug: a 404 fetch was deleting git_status, causing the next + `mama update` to report 'SCM change detected' on an unchanged dep.""" + tmpdir = tempfile.mkdtemp(prefix='mama_404_test_') + try: + target, status_path = _make_target_with_status(tmpdir) + assert os.path.exists(status_path), 'precondition: status file exists' + + with patch('mama.artifactory.download_file', side_effect=_http_404()): + result = art._fetch_package(target, 'example.com', 'libfoo-abc1234', tmpdir) + + assert result is None, 'fetch must report miss' + assert os.path.exists(status_path), ( + 'git_status was deleted on 404 — this is the regression bug. ' + 'A 404 means "no archive for this commit", not "git source is stale".' + ) + finally: + shutil.rmtree(tmpdir, ignore_errors=True) + + +def test_404_on_is_pkg_still_raises(): + """For an artifactory-only pkg dep (not git), a 404 IS fatal — + those URLs must exist.""" + from mama.types.artifactory_pkg import ArtifactoryPkg + tmpdir = tempfile.mkdtemp(prefix='mama_404_test_') + try: + pkg = ArtifactoryPkg(name='libfoo', version='1.0', fullname='libfoo-1.0') + + config = Mock() + config.is_network_available.return_value = True + config.verbose = False + config.force_artifactory = False + + dep = Mock() + dep.name = 'libfoo' + dep.build_dir = tmpdir + dep.dep_source = pkg + dep.config = config + + target = Mock() + target.name = 'libfoo' + target.config = config + target.dep = dep + + with patch('mama.artifactory.download_file', side_effect=_http_404()): + with pytest.raises(RuntimeError, match='did not exist'): + art._fetch_package(target, 'example.com', 'libfoo-1.0', tmpdir) + finally: + shutil.rmtree(tmpdir, ignore_errors=True) + + +def test_non_404_network_error_does_not_wipe_git_status_either(): + """Connection refused / timeout should also leave status untouched — + these are transient and shouldn't trigger a spurious rebuild later.""" + tmpdir = tempfile.mkdtemp(prefix='mama_404_test_') + try: + target, status_path = _make_target_with_status(tmpdir) + + with patch('mama.artifactory.is_network_error', return_value=True), \ + patch('mama.artifactory.download_file', side_effect=ConnectionRefusedError()): + result = art._fetch_package(target, 'example.com', 'libfoo-abc1234', tmpdir) + + assert result is None + assert os.path.exists(status_path) + finally: + shutil.rmtree(tmpdir, ignore_errors=True) diff --git a/tests/test_artifactory_shim/__init__.py b/tests/test_artifactory_shim/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/test_artifactory_shim/test_network_flag.py b/tests/test_artifactory_shim/test_network_flag.py new file mode 100644 index 0000000..c6b91ea --- /dev/null +++ b/tests/test_artifactory_shim/test_network_flag.py @@ -0,0 +1,103 @@ +""" +Tests for the reactive network availability flag. + +The flag is set once on first clear network failure and cached globally +so subsequent targets skip network operations instantly. +""" +import socket +import subprocess +from unittest.mock import Mock +from urllib.error import URLError, HTTPError + +from mama.util import is_network_error +from mama.build_config import BuildConfig + + +# --------------------------------------------------------------------------- +# is_network_error classification +# --------------------------------------------------------------------------- + +def test_timeout_is_network_error(): + e = subprocess.TimeoutExpired(cmd='git ls-remote', timeout=5) + assert is_network_error(e) is True + + +def test_connection_refused_is_network_error(): + assert is_network_error(ConnectionRefusedError()) is True + + +def test_socket_timeout_is_network_error(): + assert is_network_error(socket.timeout('timed out')) is True + + +def test_dns_failure_is_network_error(): + assert is_network_error(socket.gaierror('Name or service not known')) is True + + +def test_urlerror_with_socket_reason_is_network_error(): + e = URLError(reason=socket.timeout('timed out')) + assert is_network_error(e) is True + + +def test_http_401_is_not_network_error(): + e = HTTPError(url='http://x', code=401, msg='Unauthorized', hdrs=None, fp=None) + assert is_network_error(e) is False + + +def test_http_403_is_not_network_error(): + e = HTTPError(url='http://x', code=403, msg='Forbidden', hdrs=None, fp=None) + assert is_network_error(e) is False + + +def test_http_404_is_not_network_error(): + e = HTTPError(url='http://x', code=404, msg='Not Found', hdrs=None, fp=None) + assert is_network_error(e) is False + + +def test_permission_denied_in_message_is_not_network_error(): + e = RuntimeError('fatal: Permission denied (publickey)') + assert is_network_error(e) is False + + +def test_host_key_verification_failed_is_not_network_error(): + e = RuntimeError('Host key verification failed.') + assert is_network_error(e) is False + + +def test_connection_timed_out_in_message_is_network_error(): + e = RuntimeError('ssh: connect to host github.com: Connection timed out') + assert is_network_error(e) is True + + +def test_could_not_resolve_host_is_network_error(): + e = RuntimeError("fatal: unable to access: Could not resolve host: github.com") + assert is_network_error(e) is True + + +def test_ambiguous_error_is_not_network_error(): + e = RuntimeError('something unexpected happened') + assert is_network_error(e) is False + + +# --------------------------------------------------------------------------- +# BuildConfig flag behavior +# --------------------------------------------------------------------------- + +def test_config_network_available_by_default(): + config = BuildConfig(['build']) + assert config.is_network_available() is True + + +def test_config_mark_network_unavailable_sticks(): + config = BuildConfig(['build']) + config.print = False + config.mark_network_unavailable() + assert config.is_network_available() is False + + +def test_config_mark_network_unavailable_is_idempotent(): + config = BuildConfig(['build']) + config.print = False + config.mark_network_unavailable() + config.mark_network_unavailable() # no crash, no duplicate messages + assert config.is_network_available() is False diff --git a/tests/test_artifactory_shim/test_shim_buildtarget.py b/tests/test_artifactory_shim/test_shim_buildtarget.py new file mode 100644 index 0000000..76e1f8a --- /dev/null +++ b/tests/test_artifactory_shim/test_shim_buildtarget.py @@ -0,0 +1,115 @@ +""" +Tests for BuildTarget-level shim behavior: +- _require_source() refuses on a shim, allows on a clone +- _execute_deploy_tasks short-circuits on a shim without calling deploy() +""" +import os +import tempfile +import shutil +from unittest.mock import Mock, patch + +from mama.build_dependency import BuildDependency +from mama.build_target import BuildTarget +from mama.types.git import Git + + +def _make_dep_and_target(tmpdir, as_shim: bool): + config = Mock() + config.artifactory_ftp = 'ftp.example.com' + config.workspaces_root = tmpdir + config.global_workspace = False + config.platform_build_dir_name.return_value = 'linux' + config.verbose = False + config.print = False + config.loaded_dependencies = {} + # platform aliases for BuildTarget.__init__ + config.msvc = False + config.linux = True + config.macos = False + config.ios = False + config.android = None + config.raspi = False + config.oclea = None + config.xilinx = None + config.mips = None + config.imx8mp = None + config.yocto_linux = None + config.debug = False + config.prefer_ninja = False + config.ninja_path = '' + config.cmake_command = 'cmake' + config.deploy = True + config.upload = False + config.no_target.return_value = False + config.targets_all.return_value = False + config.target_matches.return_value = True # treat as current target + + git = Git(name='libfoo', url='https://example.com/libfoo.git', + branch='main', tag='', mamafile=None, shallow=True, args=[]) + dep = BuildDependency(parent=None, config=config, workspace='packages', dep_source=git) + dep.is_root = False + dep.create_build_dir_if_needed() + + if as_shim: + dep.write_shim_marker(archive_name='libfoo-linux-22-gcc11.3-x64-release-abc1234', + commit_hash='abc1234') + + target = BuildTarget(name='libfoo', config=config, dep=dep, args=[]) + return dep, target + + +def test_require_source_refuses_on_shim(): + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, target = _make_dep_and_target(tmpdir, as_shim=True) + assert target._require_source('test') is False + finally: + shutil.rmtree(tmpdir) + + +def test_require_source_allows_on_non_shim(): + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, target = _make_dep_and_target(tmpdir, as_shim=False) + assert target._require_source('test') is True + finally: + shutil.rmtree(tmpdir) + + +def test_execute_deploy_tasks_skips_deploy_for_shim(): + """A shim must not call user-defined deploy() or papa_upload_to().""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, target = _make_dep_and_target(tmpdir, as_shim=True) + # Replace deploy() with a sentinel that would fail the test if called. + called = {'deploy': False, 'upload': False} + + def fake_deploy(): + called['deploy'] = True + + target.deploy = fake_deploy + + with patch('mama.build_target.papa_upload_to') as upload_mock: + target._execute_deploy_tasks() + upload_mock.assert_not_called() + + assert not called['deploy'], 'deploy() must not be invoked on a shim' + finally: + shutil.rmtree(tmpdir) + + +def test_execute_deploy_tasks_runs_deploy_for_non_shim(): + """Sanity check: non-shim still calls deploy().""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, target = _make_dep_and_target(tmpdir, as_shim=False) + called = {'deploy': False} + + def fake_deploy(): + called['deploy'] = True + + target.deploy = fake_deploy + target._execute_deploy_tasks() + assert called['deploy'] + finally: + shutil.rmtree(tmpdir) diff --git a/tests/test_artifactory_shim/test_shim_guards.py b/tests/test_artifactory_shim/test_shim_guards.py new file mode 100644 index 0000000..e1cd832 --- /dev/null +++ b/tests/test_artifactory_shim/test_shim_guards.py @@ -0,0 +1,289 @@ +""" +Tests for shim-aware guards: +- _should_build refuses to rebuild a shim +- update_mamafile_tag / update_cmakelists_tag short-circuit for shims +- _execute_deploy_tasks skips deploy for shims +- BuildTarget._require_source returns False for shims +- papa_deploy_to refuses on a shim destination +- dirty() removes the shim marker +""" +import os +import tempfile +import shutil +from unittest.mock import Mock, patch + +import pytest + +from mama.build_dependency import BuildDependency +from mama.types.git import Git +from mama.papa_deploy import papa_deploy_to + + +def _make_dep(tmpdir): + config = Mock() + config.artifactory_ftp = 'ftp.example.com' + config.workspaces_root = tmpdir + config.global_workspace = False + config.platform_build_dir_name.return_value = 'linux' + config.verbose = False + config.print = False + config.loaded_dependencies = {} + config.target_matches.return_value = False + # for _execute_deploy_tasks + config.deploy = True + config.upload = False + config.no_target.return_value = False + config.targets_all.return_value = False + # for _should_build + config.build = True + config.update = False + config.clean = False + config.rebuild = False + config.run_cmake_configure = False + config.target = None + + git = Git(name='libfoo', url='https://example.com/libfoo.git', + branch='main', tag='', mamafile=None, shallow=True, args=[]) + dep = BuildDependency(parent=None, config=config, workspace='packages', dep_source=git) + dep.is_root = False + dep.create_build_dir_if_needed() + return dep + + +def _make_shim(tmpdir): + dep = _make_dep(tmpdir) + dep.write_shim_marker(archive_name='libfoo-linux-22-gcc11.3-x64-release-abc1234', + commit_hash='abc1234') + return dep + + +# --------------------------------------------------------------------------- +# update_mamafile_tag / update_cmakelists_tag short-circuit +# --------------------------------------------------------------------------- + +def test_update_mamafile_tag_returns_false_for_shim(): + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + assert dep.is_artifactory_shim() + # Even though src_dir is None-ish and would normally short-circuit to False, + # we want this to be defensively False regardless of mamafile presence. + assert dep.update_mamafile_tag() is False + assert dep.update_cmakelists_tag() is False + finally: + shutil.rmtree(tmpdir) + + +# --------------------------------------------------------------------------- +# _should_build refuses to rebuild a shim +# --------------------------------------------------------------------------- + +def test_should_build_returns_false_for_shim_even_with_update_target(): + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + # simulate `mama update libfoo`: would normally trigger build('update target=libfoo') + dep.config.update = True + dep.config.target = 'libfoo' + + target_mock = Mock() + target_mock.name = 'libfoo' + target_mock.args = [] + target_mock.build_products = [] + + result = dep._should_build(dep.config, target_mock, + is_target=True, git_changed=False, loaded_from_pkg=True) + assert result is False + finally: + shutil.rmtree(tmpdir) + + +def test_should_build_returns_false_for_shim_with_clean_target(): + """`mama clean libfoo` would normally short-circuit to build('cleaned target').""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + dep.config.clean = True + dep.config.target = 'libfoo' + + target_mock = Mock() + target_mock.name = 'libfoo' + target_mock.args = [] + + result = dep._should_build(dep.config, target_mock, + is_target=True, git_changed=False, loaded_from_pkg=True) + assert result is False + finally: + shutil.rmtree(tmpdir) + + +# --------------------------------------------------------------------------- +# dirty() removes the shim marker +# --------------------------------------------------------------------------- + +def test_dirty_removes_shim_marker(): + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + # `dirty` reads dep.target.build_products; supply a Mock that returns []. + target_mock = Mock() + target_mock.build_products = [] + dep.target = target_mock + + assert os.path.exists(dep.mama_shim_file()) + dep.dirty() + assert not os.path.exists(dep.mama_shim_file()) + assert not dep.is_artifactory_shim() + finally: + shutil.rmtree(tmpdir) + + +# --------------------------------------------------------------------------- +# papa_deploy_to refuses on shim destination +# --------------------------------------------------------------------------- + +def test_papa_deploy_to_refuses_with_shim_marker_in_destination(): + """If a caller passes the shim's build_dir as the deploy destination, + papa_deploy_to must raise rather than overwrite the artifactory snapshot.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + # destination has a mama_shim marker → must refuse + target = Mock() + target.config.print = False + target.config.verbose = False + target.config.test = False + target.is_current_target.return_value = False + target.name = 'libfoo' + + with pytest.raises(RuntimeError, match='mama_shim marker'): + papa_deploy_to(target, dep.build_dir, + r_includes=False, r_dylibs=False, + r_syslibs=False, r_assets=False) + finally: + shutil.rmtree(tmpdir) + + +# --------------------------------------------------------------------------- +# _git_checkout_if_needed / Git.run_git refuse to touch a shim's "working tree" +# --------------------------------------------------------------------------- + +def test_git_checkout_if_needed_short_circuits_for_shim(): + """Without this guard, a shim that misses on re-probe falls through to + dependency_checkout → check_status → `git fetch origin ` which + walks up the parent dir and queries the wrong repo's remote.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + # Even though dep.dep_source.is_git is True and is_root is False, + # the shim guard must short-circuit before dependency_checkout runs. + called = [] + with patch.object(Git, 'dependency_checkout', + side_effect=lambda d: called.append(d) or True): + result = dep._git_checkout_if_needed() + assert result is False + assert called == [], 'dependency_checkout must not run on a shim' + finally: + shutil.rmtree(tmpdir) + + +def test_run_git_raises_on_shim(): + """Defense-in-depth: any caller that still reaches run_git on a shim + must hit a clear RuntimeError instead of silently corrupting state.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + git: Git = dep.dep_source + with pytest.raises(RuntimeError, match='artifactory shim'): + git.run_git(dep, 'fetch origin main -q') + finally: + shutil.rmtree(tmpdir) + + +def test_run_git_returns_nonzero_when_not_throwing_on_shim(): + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + git: Git = dep.dep_source + # throw=False path: callers like _has_local_modifications must + # still see a non-zero status, not silently succeed. + result = git.run_git(dep, 'diff --quiet HEAD', throw=False) + assert result != 0 + finally: + shutil.rmtree(tmpdir) + + +# --------------------------------------------------------------------------- +# is_artifactory_shim caches the filesystem check +# --------------------------------------------------------------------------- + +def test_is_artifactory_shim_caches_filesystem_stat(): + """Called per-progress-tick and per-git-op, so it must not stat on every call.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + # First call populates the cache. + assert dep.is_artifactory_shim() is True + # Subsequent calls must not stat anything. + with patch('os.path.exists', side_effect=AssertionError('stat called')): + for _ in range(10): + assert dep.is_artifactory_shim() is True + finally: + shutil.rmtree(tmpdir) + + +def test_is_artifactory_shim_cache_updates_on_remove(): + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_shim(tmpdir) + assert dep.is_artifactory_shim() is True + dep.remove_shim_marker() + # Cache must reflect the removal without restating. + with patch('os.path.exists', side_effect=AssertionError('stat called')): + assert dep.is_artifactory_shim() is False + finally: + shutil.rmtree(tmpdir) + + +def test_is_artifactory_shim_cache_invalidated_on_write(): + """Write must invalidate (not preset True) so a coexisting .git wins.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_dep(tmpdir) + assert dep.is_artifactory_shim() is False + dep.write_shim_marker(archive_name='archive', commit_hash='abc1234') + # Recomputes; no .git so it is in fact a shim now. + assert dep.is_artifactory_shim() is True + finally: + shutil.rmtree(tmpdir) + + +def test_papa_deploy_to_succeeds_for_normal_destination(): + """Sanity: a non-shim destination still works.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + deploy_dir = os.path.join(tmpdir, 'deploy', 'libfoo') + try: + os.makedirs(deploy_dir, exist_ok=True) + + target = Mock() + target.config.print = False + target.config.verbose = False + target.config.test = False + target.is_current_target.return_value = False + target.name = 'libfoo' + target.exported_includes = [] + target.exported_libs = [] + target.exported_syslibs = [] + target.exported_assets = [] + target.includes_root = ('', '', '') + target.children.return_value = [] + target.build_dir.return_value = deploy_dir + target.source_dir.return_value = deploy_dir + + # no mama_shim in deploy_dir → must not raise + papa_deploy_to(target, deploy_dir, + r_includes=False, r_dylibs=False, + r_syslibs=False, r_assets=False) + assert os.path.exists(os.path.join(deploy_dir, 'papa.txt')) + finally: + shutil.rmtree(tmpdir) diff --git a/tests/test_artifactory_shim/test_shim_load_integration.py b/tests/test_artifactory_shim/test_shim_load_integration.py new file mode 100644 index 0000000..5f15584 --- /dev/null +++ b/tests/test_artifactory_shim/test_shim_load_integration.py @@ -0,0 +1,180 @@ +""" +End-to-end test of the lazy-clone path through BuildDependency._load(). + +The critical regression we guard against here: any change that re-orders the +shim probe vs. the git clone, or that fails to gate the clone on shim success, +would re-introduce the original slowness this feature was designed to remove. +""" +import os +import tempfile +import shutil +from unittest.mock import Mock, patch + +import mama.artifactory as artifactory_mod +from mama.build_dependency import BuildDependency +from mama.build_target import BuildTarget +from mama.types.git import Git + + +def _make_dep(tmpdir): + config = Mock() + config.artifactory_ftp = 'ftp.example.com' + config.workspaces_root = tmpdir + config.global_workspace = False + config.platform_build_dir_name.return_value = 'linux' + config.verbose = False + config.print = False + config.loaded_dependencies = {} + config.target_matches.return_value = False + config.force_artifactory = False + config.disable_artifactory = False + # commands off — pure load-only run + config.build = False + config.update = False + config.clean = False + config.rebuild = False + config.run_cmake_configure = False + config.target = None + config.list = False + # platform aliases + config.msvc = False + config.linux = True + config.macos = False + config.ios = False + config.android = None + config.raspi = False + config.oclea = None + config.xilinx = None + config.mips = None + config.imx8mp = None + config.yocto_linux = None + config.debug = False + config.prefer_ninja = False + config.ninja_path = '' + config.cmake_command = 'cmake' + # needed by artifactory_archive_name + config.get_distro_info.return_value = ('ubuntu', 22, 4) + config.compiler_version.return_value = 'gcc11.3' + config.arch = 'x64' + config.release = True + config.sanitize = None + config.sanitizer_suffix.return_value = '' + + git = Git(name='libfoo', url='https://example.com/libfoo.git', + branch='main', tag='', mamafile=None, shallow=True, args=[]) + dep = BuildDependency(parent=None, config=config, workspace='packages', dep_source=git) + dep.is_root = False # override: tests don't have a real parent chain + return dep + + +def _fake_successful_fetch(probe_target): + """Stand-in for artifactory_fetch_and_reconfigure on success.""" + probe_target.dep.from_artifactory = True + probe_target.exported_includes = ['/fake/include'] + return (True, []) # no child deps + + +def _fake_failed_fetch(probe_target): + """Stand-in for artifactory_fetch_and_reconfigure on miss.""" + return (False, None) + + +def test_load_uses_shim_and_skips_clone(): + """Shim probe success ⇒ Git.dependency_checkout (the clone path) is never called.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_dep(tmpdir) + + with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ + patch.object(artifactory_mod, 'artifactory_fetch_and_reconfigure', + side_effect=_fake_successful_fetch), \ + patch.object(Git, 'dependency_checkout') as clone_mock: + dep._load() + + clone_mock.assert_not_called() + assert dep.from_artifactory is True + # marker persisted so subsequent runs detect it + assert os.path.exists(dep.mama_shim_file()) + assert dep.is_artifactory_shim() + finally: + shutil.rmtree(tmpdir) + + +def test_load_falls_back_to_clone_on_shim_miss(): + """Shim probe miss ⇒ Git.dependency_checkout MUST run.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_dep(tmpdir) + + 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) as clone_mock: + dep._load() + + clone_mock.assert_called_once() + assert not dep.from_artifactory + assert not os.path.exists(dep.mama_shim_file()) + finally: + shutil.rmtree(tmpdir) + + +def test_load_skips_shim_when_noart_flag_set(): + """noart ⇒ no probe, no shim marker, clone runs.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_dep(tmpdir) + dep.config.disable_artifactory = True + + with patch.object(Git, 'init_commit_hash') as hash_mock, \ + patch.object(artifactory_mod, 'artifactory_fetch_and_reconfigure') as fetch_mock, \ + patch.object(Git, 'dependency_checkout', return_value=False) as clone_mock: + dep._load() + + # shim probe must not have run + hash_mock.assert_not_called() + fetch_mock.assert_not_called() + # but clone must have + clone_mock.assert_called_once() + assert not os.path.exists(dep.mama_shim_file()) + finally: + shutil.rmtree(tmpdir) + + +def test_load_does_not_set_did_check_artifactory_on_shim_miss(): + """A shim miss must leave the post-clone probe path eligible (target.version case).""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_dep(tmpdir) + + 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.) + finally: + shutil.rmtree(tmpdir) + + +def test_load_sets_did_check_artifactory_on_shim_hit(): + """A shim hit must mark did_check_artifactory True so the post-clone probe is skipped.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep = _make_dep(tmpdir) + + with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ + patch.object(artifactory_mod, 'artifactory_fetch_and_reconfigure', + side_effect=_fake_successful_fetch), \ + patch.object(Git, 'dependency_checkout') as clone_mock: + dep._load() + + assert dep.did_check_artifactory is True + clone_mock.assert_not_called() + finally: + shutil.rmtree(tmpdir) diff --git a/tests/test_artifactory_shim/test_shim_marker.py b/tests/test_artifactory_shim/test_shim_marker.py new file mode 100644 index 0000000..e5f32bf --- /dev/null +++ b/tests/test_artifactory_shim/test_shim_marker.py @@ -0,0 +1,104 @@ +""" +Unit tests for the artifactory shim marker file. + +The marker (`{build_dir}/mama_shim`) is the persistent source of truth for +'this dep was loaded from artifactory without a clone'. These tests verify +the roundtrip and detection helpers without spinning up any real BuildTarget. +""" +import os +import tempfile +import shutil +from unittest.mock import Mock + +from mama.build_dependency import BuildDependency, MAMA_SHIM_FILENAME +from mama.types.git import Git + + +def _make_dep_in_tempdir(): + """Construct a real BuildDependency wired to a temp workspace, no clone.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + config = Mock() + config.artifactory_ftp = None + config.workspaces_root = tmpdir + config.global_workspace = False + config.platform_build_dir_name.return_value = 'linux' + config.verbose = False + config.print = False + config.loaded_dependencies = {} + + git = Git(name='libfoo', url='https://example.com/libfoo.git', + branch='main', tag='', mamafile=None, shallow=True, args=[]) + dep = BuildDependency(parent=None, config=config, workspace='packages', dep_source=git) + dep.is_root = False # the constructor sets is_root from parent=None; override for tests + dep.create_build_dir_if_needed() + return dep, tmpdir + + +def test_no_marker_means_not_shim(): + dep, tmpdir = _make_dep_in_tempdir() + try: + assert not dep.is_artifactory_shim() + assert not dep.is_real_clone() + finally: + shutil.rmtree(tmpdir) + + +def test_write_then_detect_shim(): + dep, tmpdir = _make_dep_in_tempdir() + try: + dep.write_shim_marker(archive_name='libfoo-linux-22-gcc11.3-x64-release-abc1234', + commit_hash='abc1234') + assert os.path.exists(dep.mama_shim_file()) + assert dep.is_artifactory_shim() + assert not dep.is_real_clone() + finally: + shutil.rmtree(tmpdir) + + +def test_shim_marker_roundtrip(): + dep, tmpdir = _make_dep_in_tempdir() + try: + dep.write_shim_marker(archive_name='libfoo-linux-22-gcc11.3-x64-release-abc1234', + commit_hash='abc1234') + data = dep.read_shim_marker() + assert data['name'] == 'libfoo' + assert data['url'] == 'https://example.com/libfoo.git' + assert data['branch'] == 'main' + assert data['tag'] == '' + assert data['hash'] == 'abc1234' + assert data['archive'] == 'libfoo-linux-22-gcc11.3-x64-release-abc1234' + finally: + shutil.rmtree(tmpdir) + + +def test_remove_shim_marker_is_idempotent(): + dep, tmpdir = _make_dep_in_tempdir() + try: + dep.write_shim_marker(archive_name='x', commit_hash='y') + assert os.path.exists(dep.mama_shim_file()) + dep.remove_shim_marker() + assert not os.path.exists(dep.mama_shim_file()) + # second remove should not raise + dep.remove_shim_marker() + finally: + shutil.rmtree(tmpdir) + + +def test_real_clone_takes_precedence_over_shim(): + """If both .git and mama_shim are present, is_artifactory_shim is False.""" + dep, tmpdir = _make_dep_in_tempdir() + try: + dep.write_shim_marker(archive_name='x', commit_hash='y') + # fake a .git directory in src_dir to simulate a real clone + os.makedirs(dep.src_dir, exist_ok=True) + os.makedirs(os.path.join(dep.src_dir, '.git'), exist_ok=True) + assert dep.is_real_clone() + assert not dep.is_artifactory_shim() + finally: + shutil.rmtree(tmpdir) + + +def test_shim_filename_constant(): + """Hardcode-check the marker filename. The defense-in-depth check in + papa_deploy_to relies on the literal 'mama_shim'.""" + assert MAMA_SHIM_FILENAME == 'mama_shim' diff --git a/tests/test_artifactory_shim/test_shim_probe.py b/tests/test_artifactory_shim/test_shim_probe.py new file mode 100644 index 0000000..d535b2f --- /dev/null +++ b/tests/test_artifactory_shim/test_shim_probe.py @@ -0,0 +1,187 @@ +""" +Unit tests for the artifactory shim probe path. + +These tests exercise `try_load_artifactory_shim` and the surrounding gating +without contacting any real server or git remote. The fetch+unzip+load chain +is stubbed at `artifactory_fetch_and_reconfigure`. +""" +import os +import tempfile +import shutil +from unittest.mock import patch, Mock + +import mama.artifactory as artifactory_mod +from mama.artifactory import try_load_artifactory_shim +from mama.build_dependency import BuildDependency +from mama.types.git import Git + + +def _make_dep(tmpdir, artifactory_ftp='ftp.example.com'): + config = Mock() + config.artifactory_ftp = artifactory_ftp + config.workspaces_root = tmpdir + config.global_workspace = False + config.platform_build_dir_name.return_value = 'linux' + config.verbose = False + config.print = False + config.loaded_dependencies = {} + config.target_matches.return_value = False + # used inside BuildTarget.__init__ via _update_platform_aliases + config.msvc = False + config.linux = True + config.macos = False + config.ios = False + config.android = None + config.raspi = False + config.oclea = None + config.xilinx = None + config.mips = None + config.imx8mp = None + config.yocto_linux = None + config.debug = False + config.prefer_ninja = False + config.ninja_path = '' + config.cmake_command = 'cmake' + # needed by artifactory_archive_name + config.get_distro_info.return_value = ('ubuntu', 22, 4) + config.compiler_version.return_value = 'gcc11.3' + config.arch = 'x64' + config.release = True + config.sanitize = None + config.sanitizer_suffix.return_value = '' + config.update = False + + git = Git(name='libfoo', url='https://example.com/libfoo.git', + branch='main', tag='', mamafile=None, shallow=True, args=[]) + dep = BuildDependency(parent=None, config=config, workspace='packages', dep_source=git) + dep.is_root = False + dep.create_build_dir_if_needed() + return dep, config, git + + +def test_shim_probe_no_artifactory_returns_none(): + """When artifactory_ftp is unset, the shim probe must be a no-op.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, _, _ = _make_dep(tmpdir, artifactory_ftp=None) + target, deps = try_load_artifactory_shim(dep) + assert target is None + assert deps is None + # marker never written + assert not os.path.exists(dep.mama_shim_file()) + finally: + shutil.rmtree(tmpdir) + + +def test_shim_probe_unresolvable_hash_returns_none(): + """If ls-remote / cache / .git all fail, init_commit_hash returns None and + the probe must bail without touching state.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, _, git = _make_dep(tmpdir) + with patch.object(Git, 'init_commit_hash', return_value=None): + target, deps = try_load_artifactory_shim(dep) + assert target is None + assert deps is None + assert not os.path.exists(dep.mama_shim_file()) + assert not dep.from_artifactory + finally: + shutil.rmtree(tmpdir) + + +def test_shim_probe_fetch_fails_returns_none_and_clears_state(): + """When artifactory_fetch_and_reconfigure returns (False, None), the probe + must reset from_artifactory so the clone path can run cleanly.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, _, _ = _make_dep(tmpdir) + with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ + patch.object(artifactory_mod, 'artifactory_fetch_and_reconfigure', + return_value=(False, None)) as fetch_mock: + target, deps = try_load_artifactory_shim(dep) + assert target is None + assert deps is None + assert not os.path.exists(dep.mama_shim_file()) + assert not dep.from_artifactory + fetch_mock.assert_called_once() + finally: + shutil.rmtree(tmpdir) + + +def test_shim_probe_fetch_succeeds_writes_marker(): + """On fetch success, the probe must return a target + deps and persist a marker.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, _, _ = _make_dep(tmpdir) + fake_deps = ['some_dep_source_placeholder'] + + def fake_fetch(probe_target): + # mimic artifactory_load_target's side effect on the dep: + probe_target.dep.from_artifactory = True + probe_target.exported_includes = ['/fake/include'] + return (True, fake_deps) + + with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ + patch.object(artifactory_mod, 'artifactory_fetch_and_reconfigure', + side_effect=fake_fetch): + target, deps = try_load_artifactory_shim(dep) + + assert target is not None + assert deps is fake_deps + assert target.exported_includes == ['/fake/include'] + # marker persisted + marker = dep.read_shim_marker() + assert marker['hash'] == 'abc1234' + assert marker['url'] == 'https://example.com/libfoo.git' + assert dep.is_artifactory_shim() + finally: + shutil.rmtree(tmpdir) + + +def test_shim_probe_uses_resolved_hash_not_tag(): + """The probe must call init_commit_hash; for a non-hex tag this triggers + ls-remote internally. We assert the hash threaded through to the marker.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, _, _ = _make_dep(tmpdir) + + def fake_fetch(probe_target): + probe_target.dep.from_artifactory = True + return (True, []) + + with patch.object(Git, 'init_commit_hash', return_value='def5678') as hash_mock, \ + patch.object(artifactory_mod, 'artifactory_fetch_and_reconfigure', + side_effect=fake_fetch): + target, _ = try_load_artifactory_shim(dep) + + assert target is not None + hash_mock.assert_called_once() + # use_cache=True, fetch_remote=True per Phase 1 contract + args, kwargs = hash_mock.call_args + assert kwargs.get('use_cache') is True + assert kwargs.get('fetch_remote') is True + + marker = dep.read_shim_marker() + assert marker['hash'] == 'def5678' + finally: + shutil.rmtree(tmpdir) + + +def test_shim_probe_skipped_for_non_git_dep(): + """Local / pkg deps must never enter the shim probe path.""" + tmpdir = tempfile.mkdtemp(prefix='mama_shim_test_') + try: + dep, _, _ = _make_dep(tmpdir) + # mutate dep_source to look non-git + dep.dep_source.is_git = False + + with patch.object(Git, 'init_commit_hash') as hash_mock, \ + patch.object(artifactory_mod, 'artifactory_fetch_and_reconfigure') as fetch_mock: + target, deps = try_load_artifactory_shim(dep) + + assert target is None + assert deps is None + hash_mock.assert_not_called() + fetch_mock.assert_not_called() + finally: + shutil.rmtree(tmpdir) diff --git a/tests/test_clone_timing/test_clone_timing.py b/tests/test_clone_timing/test_clone_timing.py new file mode 100644 index 0000000..2f711b6 --- /dev/null +++ b/tests/test_clone_timing/test_clone_timing.py @@ -0,0 +1,48 @@ +"""Unit tests for ``mama.util.get_time_str``. + +This formatter is used in several places — build timings, download progress, +and (newly) clone progress — so its boundary behaviour matters. None of the +existing test suites covered it, so these pin down the format at each scale +boundary (ms / s / m / h / d) and at the transitions between them. +""" +from __future__ import annotations + +import os +import sys + +import pytest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..'))) +from mama.util import get_time_str # noqa: E402 + + +@pytest.mark.parametrize('seconds,expected', [ + # sub-second: milliseconds, integer-truncated + (0, '0ms'), + (0.001, '1ms'), + (0.5, '500ms'), + (0.999, '999ms'), + + # under a minute: one decimal place + (1, '1.0s'), + (1.5, '1.5s'), + (42, '42.0s'), + (59.9, '59.9s'), + + # 1m–59m: 'Xm Ys' (note the space — already established project style) + (60, '1m 0s'), + (67, '1m 7s'), # the example from the user request + (125, '2m 5s'), + (3599, '59m 59s'), + + # 1h–23h: 'Xh Ym Zs' + (3600, '1h 0m 0s'), + (3661, '1h 1m 1s'), + (86399, '23h 59m 59s'), + + # 1d+: 'Xd Yh Zm Ws' + (86400, '1d 0h 0m 0s'), + (90061, '1d 1h 1m 1s'), +]) +def test_get_time_str(seconds, expected): + assert get_time_str(seconds) == expected diff --git a/tests/test_console_progress/test_console_progress.py b/tests/test_console_progress/test_console_progress.py new file mode 100644 index 0000000..8dc40a3 --- /dev/null +++ b/tests/test_console_progress/test_console_progress.py @@ -0,0 +1,87 @@ +"""Unit tests for the parallel-aware ``console()`` finalizer. + +The bug being prevented: during parallel updates, one thread's ``\\r``-redrawn +progress bar (``console('\\r... 47% ...', end='')``) and another thread's +status line (``console(' - Target X SHIM FETCHED')``) used to get glued +together as ``...47% - Target X SHIM FETCHED``. Now ``console()`` tracks +whether the cursor is mid-progress and emits a leading newline before any +status print so the progress bar ends cleanly on its own row. +""" +from __future__ import annotations + +import os +import sys +import threading + +import pytest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..'))) +from mama.utils import system # noqa: E402 + + +@pytest.fixture +def reset_progress_state(): + system._progress_active = False + yield + system._progress_active = False + + +class TestProgressFinalization: + def test_status_line_after_progress_gets_leading_newline( + self, capsys, reset_progress_state): + system.console('\r |==== | 40% (1s)', end='') + system.console(' - Target foo SHIM FETCHED') + out = capsys.readouterr().out + # The status line must start on its own row, not be glued to the bar. + assert '40% (1s)\n - Target foo SHIM FETCHED\n' in out + + def test_progress_redraw_does_not_get_extra_newline( + self, capsys, reset_progress_state): + # Repeated \r-redraws of the same progress bar must overwrite each + # other on the same row; we must NOT inject a newline between them. + system.console('\r | 20% |', end='') + system.console('\r | 40% |', end='') + system.console('\r | 60% |', end='') + assert capsys.readouterr().out == '\r | 20% |\r | 40% |\r | 60% |' + + def test_progress_final_newline_clears_state( + self, capsys, reset_progress_state): + system.console('\r | 50% |', end='') + # 100% line ends with default '\n' - it commits the progress. + system.console('\r |100% |') + # Subsequent normal status must NOT get a spurious extra newline. + system.console(' - Target done') + assert capsys.readouterr().out == '\r | 50% |\r |100% |\n - Target done\n' + + def test_status_print_without_progress_active_is_unaffected( + self, capsys, reset_progress_state): + system.console('hello') + system.console('world') + # No leading newline injected when no progress was active. + assert capsys.readouterr().out == 'hello\nworld\n' + + def test_initial_progress_bar_without_carriage_return_still_tracked( + self, capsys, reset_progress_state): + # The first frame of an upload progress bar is printed without \r + # (e.g. artifactory upload prints ' |> ...| 0 %' with end=''). + # Subsequent status writes must still know to finalize it. + system.console(' |> | 0 %', end='') + system.console(' - Target X') + assert capsys.readouterr().out == ' |> | 0 %\n - Target X\n' + + +class TestThreadSafety: + def test_parallel_writers_never_tear_within_a_single_call( + self, capsys, reset_progress_state): + """Each console() call is atomic. Concurrent writes must produce + only complete strings, never partial interleaving inside a string.""" + msgs = [f'msg-{i:04d}' for i in range(200)] + def worker(text): + system.console(text) + threads = [threading.Thread(target=worker, args=(m,)) for m in msgs] + for t in threads: t.start() + for t in threads: t.join() + out = capsys.readouterr().out + # Every message must appear intact exactly once on its own line. + lines = [l for l in out.split('\n') if l] + assert sorted(lines) == sorted(msgs) diff --git a/tests/test_download_cache/test_download_cache.py b/tests/test_download_cache/test_download_cache.py new file mode 100644 index 0000000..22d08cf --- /dev/null +++ b/tests/test_download_cache/test_download_cache.py @@ -0,0 +1,140 @@ +"""Tests for the size-match cache and target-prefix in download_file. + +Background: ``mama update`` re-fetches every artifactory archive on each run +because ``_fetch_package`` passes ``force=True`` to ``download_file``. The +size-match cache lets us still skip the body transfer when the local file's +size matches the remote's Content-Length, costing only the HTTP round-trip +we'd open anyway. The ``name`` parameter prefixes every log line with the +target name so parallel updates produce readable output instead of progress +bars from one target glued to status lines from another. +""" +from __future__ import annotations + +import io +import os +import sys +from unittest.mock import patch, MagicMock + +import pytest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..'))) +from mama.util import download_file # noqa: E402 + + +def _mock_urlopen(content: bytes, content_length=None): + """Build a context-manager-returning mock that mimics urllib.request.urlopen.""" + body = io.BytesIO(content) + cm = MagicMock() + cm.info.return_value = {'Content-Length': str(content_length if content_length is not None else len(content))} + cm.read = body.read + cm.__enter__ = lambda self: cm + cm.__exit__ = lambda self, *a: None + return cm + + +class TestSizeMatchCache: + def test_skips_body_when_local_size_matches_remote(self, tmp_path, capsys): + """Saves the body transfer for an already-downloaded artifactory archive.""" + local_dir = str(tmp_path) + # Pre-populate a file at the URL's basename with known size. + cached_path = tmp_path / 'archive.zip' + cached_path.write_bytes(b'x' * 1024) + + # Server says 1024 bytes — same as local. download_file should not + # read any bytes from the body. + opened = _mock_urlopen(b'NEW' * 100, content_length=1024) + opened.read = MagicMock(side_effect=AssertionError('body should not be read')) + + with patch('mama.util.request.urlopen', return_value=opened): + result = download_file('http://x.example/archive.zip', local_dir, force=True) + assert result == str(cached_path) + # Cached file still has the original contents - body was not touched. + assert cached_path.read_bytes() == b'x' * 1024 + + def test_downloads_when_local_size_differs_from_remote(self, tmp_path): + local_dir = str(tmp_path) + cached_path = tmp_path / 'archive.zip' + cached_path.write_bytes(b'old' * 100) # 300 bytes locally + new_body = b'NEW' * 200 # 600 bytes from server + opened = _mock_urlopen(new_body, content_length=600) + + with patch('mama.util.request.urlopen', return_value=opened): + result = download_file('http://x.example/archive.zip', local_dir, force=True) + assert result == str(cached_path) + # File was actually re-downloaded with new contents. + assert cached_path.read_bytes() == new_body + + def test_downloads_when_no_local_file(self, tmp_path): + local_dir = str(tmp_path) + new_body = b'BODY' * 50 + opened = _mock_urlopen(new_body, content_length=200) + + with patch('mama.util.request.urlopen', return_value=opened): + result = download_file('http://x.example/new.zip', local_dir, force=True) + assert os.path.exists(result) + assert open(result, 'rb').read() == new_body + + def test_force_false_uses_cache_without_contacting_server(self, tmp_path): + """With force=False the function must not touch the network at all.""" + local_dir = str(tmp_path) + cached_path = tmp_path / 'a.zip' + cached_path.write_bytes(b'hello') + + with patch('mama.util.request.urlopen', side_effect=AssertionError('must not open URL')): + result = download_file('http://x.example/a.zip', local_dir, force=False) + assert result == str(cached_path) + + def test_size_match_reported_to_user(self, tmp_path, capsys): + local_dir = str(tmp_path) + cached_path = tmp_path / 'archive.zip' + cached_path.write_bytes(b'x' * 1024) + opened = _mock_urlopen(b'unused', content_length=1024) + + with patch('mama.util.request.urlopen', return_value=opened): + download_file('http://x.example/archive.zip', local_dir, force=True) + out = capsys.readouterr().out + assert 'Artifactory CACHE (size-match)' in out + + +class TestTargetPrefix: + def test_name_prefixes_cached_message(self, tmp_path, capsys): + local_dir = str(tmp_path) + (tmp_path / 'a.zip').write_bytes(b'x') + download_file('http://x/a.zip', local_dir, force=False, name='libfoo') + out = capsys.readouterr().out + assert 'libfoo' in out + assert 'Using locally cached' in out + + def test_name_prefixes_size_match_message(self, tmp_path, capsys): + local_dir = str(tmp_path) + (tmp_path / 'a.zip').write_bytes(b'x' * 8) + opened = _mock_urlopen(b'unused', content_length=8) + with patch('mama.util.request.urlopen', return_value=opened): + download_file('http://x/a.zip', local_dir, force=True, name='libfoo') + out = capsys.readouterr().out + assert 'libfoo' in out + + def test_name_prefixes_progress_bar(self, tmp_path, capsys): + local_dir = str(tmp_path) + # 200 KB body so report_interval is small enough that progress bars actually print. + body = b'A' * (200 * 1024) + opened = _mock_urlopen(body, content_length=len(body)) + with patch('mama.util.request.urlopen', return_value=opened): + download_file('http://x/a.zip', local_dir, force=True, name='libfoo') + out = capsys.readouterr().out + # The redrawn progress line must carry the target name, otherwise a + # parallel run's status lines have no way to indicate which target + # they belong to. + assert 'libfoo' in out + # And the existing bar format is preserved. + assert '|' in out and '%' in out + + def test_no_name_keeps_unprefixed_format(self, tmp_path, capsys): + local_dir = str(tmp_path) + (tmp_path / 'a.zip').write_bytes(b'x') + download_file('http://x/a.zip', local_dir, force=False) + out = capsys.readouterr().out + # When no name is given, the line uses plain 4-space indent (no '- '). + assert 'Using locally cached' in out + # Must not produce a "- " bullet prefix when there's no target context. + assert ' - ' not in out diff --git a/tests/test_self_version_probe/test_self_version_probe.py b/tests/test_self_version_probe/test_self_version_probe.py new file mode 100644 index 0000000..a493858 --- /dev/null +++ b/tests/test_self_version_probe/test_self_version_probe.py @@ -0,0 +1,240 @@ +"""Tests for the sparse-mamafile shim fallback. + +When a dep pins ``self.version`` (e.g. ``boost 1.60``), the archive name in +artifactory doesn't track the commit hash, so the commit-hash-based shim +probe always misses. To avoid full-cloning every fresh checkout, the shim +sparse-clones just the dep's mamafile, greps ``self.version``, and re-probes +artifactory with that explicit version. + +These tests cover: +* the regex extraction (literal quotes only — f-strings/computed values miss) +* the shim probe falling through to the version-based probe on hash miss +* the shim probe NOT calling the sparse-fetch when the hash probe hits +* full miss still falling through cleanly to the clone path +""" +from __future__ import annotations + +import os +import subprocess +import sys +import tempfile +from unittest.mock import Mock, patch + +import pytest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..'))) +from mama.types.git import Git # noqa: E402 +from mama import artifactory as art # noqa: E402 + + +class TestExtractSelfVersion: + @pytest.mark.parametrize('text,expected', [ + ("self.version = '1.0'", '1.0'), + ('self.version = "1.60"', '1.60'), + ("self.version='2.3.4'", '2.3.4'), + (" self.version = '0.9.1-beta'", '0.9.1-beta'), + ("self.version = '1.0' # the version", '1.0'), + # multi-line mamafile: the assignment lives inside init() + ("class P:\n def init(self):\n self.version = '7.7'\n", '7.7'), + ]) + def test_matches_literal_assignment(self, text, expected): + assert Git.extract_self_version(text) == expected + + @pytest.mark.parametrize('text', [ + # f-string: don't try to evaluate + "self.version = f'{major}.{minor}'", + # function call: don't try to evaluate + "self.version = compute_version()", + # bare variable + "self.version = MY_VERSION", + # never assigned + "class P:\n def init(self):\n self.name = 'libfoo'\n", + # commented out + "# self.version = '1.0'", + # comparison, not assignment (no '=') + "if self.version == '1.0': pass", + # empty + "", + ]) + def test_returns_none_for_non_literal(self, text): + assert Git.extract_self_version(text) is None + + def test_first_assignment_wins(self): + # Defensive: a mamafile that conditionally re-assigns. We don't try + # to handle this perfectly; we just grab the first literal match. + text = ( + "self.version = '1.0'\n" + "if something: self.version = '2.0'\n" + ) + assert Git.extract_self_version(text) == '1.0' + + +def _make_dep(branch='main', mamafile_field=''): + config = Mock() + config.artifactory_ftp = 'ftp.example.com' + config.verbose = False + config.print = False + config.is_network_available.return_value = True + config.update_stats = Mock() + config.target_matches.return_value = False + + git = Git(name='libfoo', url='https://example.com/libfoo.git', + branch=branch, tag='', mamafile=mamafile_field, + shallow=True, args=[]) + dep = Mock() + dep.name = 'libfoo' + dep.config = config + dep.dep_source = git + dep.target_args = [] + dep.from_artifactory = False + dep.write_shim_marker = Mock() + return dep, git + + +class TestFetchSelfVersionFromRemote: + """Clone goes through _run_git_with_filtered_progress (for the live UI), + but the one-shot git-show uses subprocess.run with stderr=DEVNULL + + timeout so a stuck lazy fetch can't deadlock the whole executor.""" + + def _patch_clone(self, return_code=0): + def fake(self_, dep_, cmd, label): + return return_code, '', '100ms' + return patch.object(Git, '_run_git_with_filtered_progress', new=fake) + + def _patch_show(self, stdout=b'', returncode=0): + return patch('mama.types.git.subprocess.run', + return_value=Mock(returncode=returncode, stdout=stdout)) + + def test_returns_version_when_mamafile_has_literal(self): + dep, git = _make_dep() + body = b"class P:\n def init(self):\n self.version = '1.60'\n" + with self._patch_clone(), self._patch_show(stdout=body): + assert git.fetch_self_version_from_remote(dep) == '1.60' + + def test_returns_none_when_clone_fails(self): + dep, git = _make_dep() + with self._patch_clone(return_code=128), \ + patch('mama.types.git.subprocess.run') as mock_show: + assert git.fetch_self_version_from_remote(dep) is None + mock_show.assert_not_called() + + def test_returns_none_when_git_show_fails(self): + """git show returns non-zero (e.g. file not in repo).""" + dep, git = _make_dep() + with self._patch_clone(), self._patch_show(returncode=128): + assert git.fetch_self_version_from_remote(dep) is None + + def test_returns_none_on_show_timeout(self): + """A stuck lazy fetch must not hang the executor forever.""" + dep, git = _make_dep() + with self._patch_clone(), \ + patch('mama.types.git.subprocess.run', + side_effect=subprocess.TimeoutExpired(cmd='git', timeout=30)): + assert git.fetch_self_version_from_remote(dep) is None + + def test_returns_none_when_network_unavailable(self): + dep, git = _make_dep() + dep.config.is_network_available.return_value = False + with patch.object(Git, '_run_git_with_filtered_progress') as mock_clone, \ + patch('mama.types.git.subprocess.run') as mock_show: + assert git.fetch_self_version_from_remote(dep) is None + mock_clone.assert_not_called() + mock_show.assert_not_called() + + def test_uses_custom_mamafile_path_when_dep_specifies_one(self): + dep, git = _make_dep(mamafile_field='subdir/mama_alt.py') + captured = {} + def fake_show(cmd, **kw): + captured['cmd'] = cmd + return Mock(returncode=0, stdout=b"self.version = '3.1'") + with self._patch_clone(), \ + patch('mama.types.git.subprocess.run', side_effect=fake_show): + assert git.fetch_self_version_from_remote(dep) == '3.1' + # argv: ['git', '-C', tmp, 'show', 'HEAD:subdir/mama_alt.py'] + assert 'HEAD:subdir/mama_alt.py' in captured['cmd'] + + def test_uses_blobless_no_checkout_clone_and_probe_label(self): + """Regression guard: probe clone must stay blob-less + no-checkout, + and the clone must be labelled PROBE so update_stats doesn't + mis-record it as a full clone.""" + dep, git = _make_dep() + captured = {} + def fake_clone(self_, dep_, cmd, label): + captured['cmd'] = cmd + captured['label'] = label + return 0, '', '100ms' + with patch.object(Git, '_run_git_with_filtered_progress', new=fake_clone), \ + self._patch_show(stdout=b"self.version = '1.0'"): + git.fetch_self_version_from_remote(dep) + assert '--filter=blob:none' in captured['cmd'] + assert '--no-checkout' in captured['cmd'] + assert '--depth=1' in captured['cmd'] + # PROBE label keeps it from being mis-recorded as a full clone in update_stats. + assert captured['label'] == 'PROBE' + + +class TestShimProbeFallback: + def test_hash_hit_skips_version_probe(self): + """If the hash-based probe hits, we must NOT do a sparse-clone.""" + dep, git = _make_dep() + dep.dep_source = git + + with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ + patch.object(Git, 'fetch_self_version_from_remote') as mock_version, \ + patch('mama.artifactory.artifactory_fetch_and_reconfigure', + return_value=(True, [])), \ + patch('mama.artifactory.artifactory_archive_name', return_value='libfoo-x-abc1234'), \ + patch('mama.artifactory.BuildTarget', return_value=Mock(name='probe')) \ + if False else patch('mama.build_target.BuildTarget', side_effect=lambda **kw: Mock(name='probe', version=None)): + target, deps = art.try_load_artifactory_shim(dep) + assert target is not None + mock_version.assert_not_called() + + def test_hash_miss_falls_through_to_version_probe(self): + """If the hash-based probe misses, fetch self.version and retry.""" + dep, git = _make_dep() + + # First fetch returns (False, None) [hash probe miss], + # second returns (True, []) [version probe hit]. + fetch_calls = [] + def fake_fetch(target): + fetch_calls.append(getattr(target, 'version', None)) + return (True, []) if getattr(target, 'version', None) == '1.0' else (False, None) + + with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ + patch.object(Git, 'fetch_self_version_from_remote', return_value='1.0') as mock_version, \ + patch('mama.artifactory.artifactory_fetch_and_reconfigure', side_effect=fake_fetch), \ + patch('mama.artifactory.artifactory_archive_name', return_value='libfoo-x-1.0'), \ + patch('mama.build_target.BuildTarget', side_effect=lambda **kw: Mock(name='probe', version=None)): + target, deps = art.try_load_artifactory_shim(dep) + assert target is not None + mock_version.assert_called_once_with(dep) + # Two probes happened: first without version, then with version='1.0'. + assert fetch_calls == [None, '1.0'] + + def test_hash_miss_and_no_self_version_returns_none(self): + """Genuine miss: hash didn't hit, no self.version found. Caller will + full-clone.""" + dep, git = _make_dep() + + with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ + patch.object(Git, 'fetch_self_version_from_remote', return_value=None), \ + patch('mama.artifactory.artifactory_fetch_and_reconfigure', + return_value=(False, None)), \ + patch('mama.build_target.BuildTarget', side_effect=lambda **kw: Mock(name='probe', version=None)): + target, deps = art.try_load_artifactory_shim(dep) + assert target is None + # from_artifactory must be reset so the clone path runs cleanly. + assert dep.from_artifactory is False + + def test_hash_miss_with_self_version_but_still_no_archive_returns_none(self): + """Even with self.version, artifactory may genuinely not have it.""" + dep, git = _make_dep() + + with patch.object(Git, 'init_commit_hash', return_value='abc1234'), \ + patch.object(Git, 'fetch_self_version_from_remote', return_value='9.9'), \ + patch('mama.artifactory.artifactory_fetch_and_reconfigure', + return_value=(False, None)), \ + patch('mama.build_target.BuildTarget', side_effect=lambda **kw: Mock(name='probe', version=None)): + target, deps = art.try_load_artifactory_shim(dep) + assert target is None diff --git a/tests/test_sub_process/test_sub_process.py b/tests/test_sub_process/test_sub_process.py new file mode 100644 index 0000000..80d556b --- /dev/null +++ b/tests/test_sub_process/test_sub_process.py @@ -0,0 +1,214 @@ +"""Direct unit tests for the SubProcess class. + +Background: SubProcess used to wrap os.fork / os.forkpty, which Python 3.12 +flags as unsafe in multi-threaded programs (DeprecationWarning at startup, +real deadlock potential under heavy parallel mama load). The rewrite uses +subprocess.Popen with an optional pty.openpty() pair on UNIX so the child +still sees a TTY (preserving git's progress output and isatty checks). + +These tests pin the behavioural contract: +* run() returns the child's exit status +* io_func is called once per line of combined stdout+stderr +* cwd / env / timeout parameters are honoured +* write() can deliver stdin to the child (used for SSH host-key prompts) +* The child sees a TTY on UNIX when io_func is set +* Reader-thread exceptions resurface in run() rather than being silently lost +* Missing executables raise OSError early, not deadlock the worker + +Commands are issued via `sys.executable -c '...'` to stay portable across +Linux / macOS / Windows. +""" +from __future__ import annotations + +import os +import sys +import subprocess +import tempfile + +import pytest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..'))) +from mama.utils.sub_process import SubProcess # noqa: E402 + + +PY = sys.executable + + +def _py_run(code: str, io_func=None, cwd=None, env=None, timeout=None): + """Run `python -c ""` via SubProcess. Returns (status, lines_seen).""" + lines = [] + if io_func is None: + def collect(p, line): lines.append(line) + io_func = collect + status = SubProcess.run([PY, '-c', code], cwd=cwd, env=env, + io_func=io_func, timeout=timeout) + return status, lines + + +class TestExitStatus: + def test_run_returns_zero_on_success(self): + status, _ = _py_run('import sys; sys.exit(0)') + assert status == 0 + + def test_run_returns_nonzero_exit_code(self): + status, _ = _py_run('import sys; sys.exit(7)') + assert status == 7 + + def test_run_without_io_func_inherits_stdio(self, capfd): + """No io_func: child writes flow straight through the parent's + stdout/stderr. capfd captures what would normally hit the terminal.""" + status = SubProcess.run([PY, '-c', 'print("hello-no-iofunc")']) + assert status == 0 + # Captured at the OS level (capfd) - not via pytest's capsys. + assert 'hello-no-iofunc' in capfd.readouterr().out + + +class TestIoFunc: + def test_each_line_delivered_once(self): + _, lines = _py_run('print("alpha"); print("beta"); print("gamma")') + assert lines == ['alpha', 'beta', 'gamma'] + + def test_stderr_is_merged_into_io_func(self): + """SubProcess merges stderr into the same stream so the io_func can + see both. This is essential for git: progress goes to stderr.""" + _, lines = _py_run('import sys; print("out"); print("err", file=sys.stderr)') + assert 'out' in lines + assert 'err' in lines + + def test_no_trailing_carriage_return_on_lines(self): + """Either via PTY or pipe, the io_func must receive bare lines - + no stray '\\r' from text-mode normalisation, no '\\n'.""" + _, lines = _py_run('print("plain")') + assert 'plain' in lines + assert not any(line.endswith('\r') or line.endswith('\n') for line in lines) + + def test_io_func_exception_is_re_raised_by_run(self): + """A bug inside io_func must surface, not silently kill the reader + thread. Otherwise debugging is impossible.""" + def broken(p, line): + raise RuntimeError(f'callback boom on line={line!r}') + with pytest.raises(RuntimeError, match='callback boom'): + SubProcess.run([PY, '-c', 'print("hi")'], io_func=broken) + + +class TestCwd: + def test_cwd_is_honored(self, tmp_path): + marker = tmp_path / 'sentinel.txt' + marker.write_text('found-it') + # Use relative open inside the child to prove cwd was set. + _, lines = _py_run('print(open("sentinel.txt").read())', cwd=str(tmp_path)) + assert lines == ['found-it'] + + +class TestEnv: + def test_env_var_passes_through(self): + env = os.environ.copy() + env['MAMA_TEST_X'] = 'hello-env' + _, lines = _py_run('import os; print(os.environ["MAMA_TEST_X"])', env=env) + assert lines == ['hello-env'] + + def test_default_env_inherits_parent(self): + os.environ['MAMA_TEST_Y'] = 'inherited' + try: + _, lines = _py_run('import os; print(os.environ["MAMA_TEST_Y"])') + assert lines == ['inherited'] + finally: + del os.environ['MAMA_TEST_Y'] + + +class TestTimeout: + def test_long_running_command_times_out(self): + with pytest.raises(subprocess.TimeoutExpired): + SubProcess.run( + [PY, '-c', 'import time; time.sleep(30)'], + io_func=lambda p, line: None, + timeout=0.3, + ) + + def test_fast_command_does_not_time_out(self): + status = SubProcess.run( + [PY, '-c', 'print("done")'], + io_func=lambda p, line: None, + timeout=10.0, + ) + assert status == 0 + + +class TestStdinWrite: + def test_write_delivers_to_child(self): + """The interactive prompt case: SubProcess.write() must reach the + child's stdin. Used by clone_with_filtered_progress to auto-accept + SSH host key prompts.""" + lines = [] + def echoer(p, line): + lines.append(line) + # As soon as the child prints "READY", send something back. + if line == 'READY': + p.write('the-secret\n') + # Child prints READY, reads one line of stdin, prints it back. + status = SubProcess.run( + [PY, '-c', 'import sys; print("READY"); sys.stdout.flush(); print("got:" + input())'], + io_func=echoer, + ) + assert status == 0 + assert 'READY' in lines + assert 'got:the-secret' in lines + + +@pytest.mark.skipif(sys.platform == 'win32', reason='PTY behaviour is UNIX-only') +class TestPtyOnUnix: + def test_child_sees_a_tty_when_io_func_is_set(self): + """The whole reason we use pty.openpty(): git inspects isatty(stderr) + to decide whether to emit progress lines like 'Receiving objects: ...'. + Without a PTY, that progress output disappears.""" + _, lines = _py_run('import sys; print(sys.stdout.isatty())') + assert lines == ['True'] + + def test_child_does_not_see_a_tty_without_io_func(self, capfd): + """Symmetry: without io_func, no PTY is allocated - the child gets + the parent's actual stdout. That stdout MAY or MAY NOT be a TTY + depending on the test runner; what we lock down here is just that + there's no spurious PTY-faking when io_func is omitted.""" + # We can't easily assert isatty() result here because pytest's capfd + # may or may not give the child a TTY. What we CAN assert is that + # the child runs and exits cleanly with no io_func. + status = SubProcess.run([PY, '-c', 'import sys; sys.exit(0)']) + assert status == 0 + + +class TestErrorPaths: + def test_missing_executable_raises_oserror(self): + with pytest.raises(OSError, match='not found in PATH'): + SubProcess.run('this-binary-does-not-exist-mama-42', io_func=lambda p, l: None) + + def test_string_cmd_is_shlex_split(self): + """Backwards-compat: cmd as a single string is shlex.split into args.""" + _, lines = _py_run('print("from-string-cmd")') + # If shlex.split is broken we'd never reach here cleanly. + assert lines == ['from-string-cmd'] + + def test_list_cmd_is_passed_through(self): + lines = [] + SubProcess.run([PY, '-c', 'print("list-cmd")'], + io_func=lambda p, l: lines.append(l)) + assert 'list-cmd' in lines + + +class TestNoForkptyDeprecationWarning: + """Regression guard for the original motivation: the old implementation + triggered a Python 3.12 DeprecationWarning on every forkpty() call from + a multi-threaded program (a real deadlock risk). The rewrite must not.""" + + def test_run_does_not_emit_forkpty_warning(self): + import warnings + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + SubProcess.run([PY, '-c', 'print("x")'], + io_func=lambda p, l: None) + forkpty_warnings = [ + w for w in caught + if 'forkpty' in str(w.message).lower() + ] + assert forkpty_warnings == [], ( + f'forkpty deprecation warning came back: {forkpty_warnings}' + ) diff --git a/tests/test_update_stats/test_update_stats.py b/tests/test_update_stats/test_update_stats.py new file mode 100644 index 0000000..c7349bf --- /dev/null +++ b/tests/test_update_stats/test_update_stats.py @@ -0,0 +1,139 @@ +"""Unit tests for the load-phase clone/pull/shim summary. + +After `mama update`, the dependency-load phase prints a one-line summary like +``Updated 12 target(s): 9 shim-fetched, 2 pulled, 1 cloned in 6.3s`` so a user +can spot which packages are slow to update. These tests cover the counter +class itself and its summary formatting at the empty / single-kind / mixed / +ordering edges. +""" +from __future__ import annotations + +import os +import sys +import threading +import time + +import pytest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..'))) +from mama.build_config import UpdateStats # noqa: E402 + + +class TestCounters: + def test_initial_state(self): + s = UpdateStats() + assert s.cloned == 0 + assert s.pulled == 0 + assert s.shim_fetched == 0 + assert s.total == 0 + assert s.summary_line() == '' + + def test_increments(self): + s = UpdateStats() + s.record_clone() + s.record_pull(); s.record_pull() + s.record_shim(); s.record_shim(); s.record_shim() + assert s.cloned == 1 + assert s.pulled == 2 + assert s.shim_fetched == 3 + assert s.total == 6 + + def test_thread_safety(self): + """100 threads each incrementing all three counters must produce exact totals.""" + s = UpdateStats() + def worker(): + for _ in range(100): + s.record_clone() + s.record_pull() + s.record_shim() + threads = [threading.Thread(target=worker) for _ in range(20)] + for t in threads: t.start() + for t in threads: t.join() + assert s.cloned == 2000 + assert s.pulled == 2000 + assert s.shim_fetched == 2000 + assert s.total == 6000 + + +class TestTiming: + def test_duration_zero_until_started(self): + s = UpdateStats() + assert s.duration == 0.0 + + def test_duration_captured_between_start_and_stop(self): + s = UpdateStats() + s.start() + time.sleep(0.02) + s.stop() + assert s.duration >= 0.02 + assert s.duration < 0.5 # sanity ceiling + + def test_stop_without_start_is_noop(self): + s = UpdateStats() + s.stop() # must not crash + assert s.duration == 0.0 + + +class TestSummaryLine: + def test_empty_when_nothing_happened(self): + s = UpdateStats() + s.start(); s.stop() + assert s.summary_line() == '' + + def test_single_kind_clone(self): + s = UpdateStats() + s.record_clone() + s.start(); s.stop() + line = s.summary_line() + assert 'Updated 1 target(s)' in line + assert '1 cloned' in line + assert 'shim-fetched' not in line + assert 'pulled' not in line + + def test_single_kind_shim(self): + s = UpdateStats() + s.record_shim() + assert '1 shim-fetched' in s.summary_line() + + def test_mixed_kinds_show_all_present(self): + s = UpdateStats() + s.record_clone() + s.record_pull(); s.record_pull() + s.record_shim(); s.record_shim(); s.record_shim() + line = s.summary_line() + assert 'Updated 6 target(s)' in line + assert '3 shim-fetched' in line + assert '2 pulled' in line + assert '1 cloned' in line + + def test_summary_includes_duration(self): + s = UpdateStats() + s.record_shim() + s.start() + time.sleep(0.01) + s.stop() + # get_time_str renders sub-second as Nms + line = s.summary_line() + assert 'in ' in line + assert 'ms' in line or 's' in line # either is valid depending on timing + + def test_kinds_order_is_shim_pull_clone(self): + """Stable ordering keeps the summary readable; shim is cheapest, clone is slowest.""" + s = UpdateStats() + s.record_clone() + s.record_pull() + s.record_shim() + line = s.summary_line() + # shim-fetched should appear before pulled, which appears before cloned + i_shim = line.index('shim-fetched') + i_pull = line.index('pulled') + i_clone = line.index('cloned') + assert i_shim < i_pull < i_clone + + def test_kinds_with_zero_count_omitted(self): + s = UpdateStats() + s.record_pull() + line = s.summary_line() + assert '1 pulled' in line + # zero counts should not appear + assert '0 ' not in line