From 997752a4395d5c7793efd510c6c61ba82314e432 Mon Sep 17 00:00:00 2001 From: Benjamin Bannier Date: Mon, 20 Jul 2026 09:16:32 +0200 Subject: [PATCH 1/5] Skip source `git_checkout` when HEAD already matches target On every `Manager` construction, `Source.__init__` was unconditionally running `git_checkout` on each source clone, spawning three Git subprocesses (`checkout`, `submodule sync --recursive`, `submodule update --recursive --init`) even when the working tree was already at the target ref. Only call `git_checkout` when the current branch/SHA differs from the target. --- testing/test_source.py | 114 +++++++++++++++++++++++++++++++++++++++++ zeekpkg/source.py | 10 +++- 2 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 testing/test_source.py diff --git a/testing/test_source.py b/testing/test_source.py new file mode 100644 index 0000000..22aa67c --- /dev/null +++ b/testing/test_source.py @@ -0,0 +1,114 @@ +import tempfile +from unittest.mock import MagicMock, patch + +from zeekpkg.source import Source + + +class TestSourceCheckoutSkip: + """Source.__init__ should skip git_checkout when HEAD is already at target.""" + + def _make_mock_repo(self, branch_name: str) -> MagicMock: + branch = MagicMock() + branch.name = branch_name + repo = MagicMock() + repo.active_branch = branch + repo.git.config.return_value = "https://example.com/repo.git" + return repo + + def _make_detached_repo(self, hexsha: str) -> MagicMock: + repo = MagicMock() + type(repo.active_branch).name = property( + lambda self: (_ for _ in ()).throw(TypeError("detached HEAD")), + ) + type(repo).active_branch = property( + lambda self: (_ for _ in ()).throw(TypeError("detached HEAD")), + ) + repo.head.commit.hexsha = hexsha + repo.git.config.return_value = "https://example.com/repo.git" + return repo + + @patch("zeekpkg.source.git_checkout") + @patch("zeekpkg.source.git_default_branch") + @patch("zeekpkg.source.git.Repo") + def test_skips_checkout_when_already_on_target( + self, + mock_repo_cls: MagicMock, + mock_default_branch: MagicMock, + mock_checkout: MagicMock, + ) -> None: + mock_repo_cls.return_value = self._make_mock_repo("main") + mock_default_branch.return_value = "main" + + with tempfile.TemporaryDirectory() as tmp: + Source("test", tmp, "https://example.com/repo.git") + + mock_checkout.assert_not_called() + + @patch("zeekpkg.source.git_checkout") + @patch("zeekpkg.source.git_default_branch") + @patch("zeekpkg.source.git.Repo") + def test_checks_out_when_branch_differs( + self, + mock_repo_cls: MagicMock, + mock_default_branch: MagicMock, + mock_checkout: MagicMock, + ) -> None: + mock_repo_cls.return_value = self._make_mock_repo("old-branch") + mock_default_branch.return_value = "main" + + with tempfile.TemporaryDirectory() as tmp: + Source("test", tmp, "https://example.com/repo.git") + + mock_checkout.assert_called_once() + + @patch("zeekpkg.source.git_checkout") + @patch("zeekpkg.source.git_default_branch") + @patch("zeekpkg.source.git.Repo") + def test_skips_checkout_explicit_version_matches_current( + self, + mock_repo_cls: MagicMock, + mock_default_branch: MagicMock, + mock_checkout: MagicMock, + ) -> None: + mock_repo_cls.return_value = self._make_mock_repo("v1.0") + mock_default_branch.return_value = "main" + + with tempfile.TemporaryDirectory() as tmp: + Source("test", tmp, "https://example.com/repo.git", version="v1.0") + + mock_checkout.assert_not_called() + + @patch("zeekpkg.source.git_checkout") + @patch("zeekpkg.source.git_default_branch") + @patch("zeekpkg.source.git.Repo") + def test_checks_out_when_explicit_version_differs( + self, + mock_repo_cls: MagicMock, + mock_default_branch: MagicMock, + mock_checkout: MagicMock, + ) -> None: + mock_repo_cls.return_value = self._make_mock_repo("main") + mock_default_branch.return_value = "main" + + with tempfile.TemporaryDirectory() as tmp: + Source("test", tmp, "https://example.com/repo.git", version="v2.0") + + mock_checkout.assert_called_once() + + @patch("zeekpkg.source.git_checkout") + @patch("zeekpkg.source.git_default_branch") + @patch("zeekpkg.source.git.Repo") + def test_skips_checkout_detached_head_matches_sha( + self, + mock_repo_cls: MagicMock, + mock_default_branch: MagicMock, + mock_checkout: MagicMock, + ) -> None: + sha = "abc123def456" + mock_repo_cls.return_value = self._make_detached_repo(sha) + mock_default_branch.return_value = "main" + + with tempfile.TemporaryDirectory() as tmp: + Source("test", tmp, "https://example.com/repo.git", version=sha) + + mock_checkout.assert_not_called() diff --git a/zeekpkg/source.py b/zeekpkg/source.py index 7da02a0..fccff00 100644 --- a/zeekpkg/source.py +++ b/zeekpkg/source.py @@ -80,7 +80,15 @@ def __init__( shutil.rmtree(clone_path) self.clone = git_clone(git_url, clone_path, shallow=True) - git_checkout(self.clone, version or git_default_branch(self.clone)) + target = version or git_default_branch(self.clone) + + try: + current = self.clone.active_branch.name + except TypeError: + current = self.clone.head.commit.hexsha + + if current != target: + git_checkout(self.clone, target) def __str__(self) -> str: return self.git_url From 0416daf5fc4f103aed6964d5ea0d25d86011bcbb Mon Sep 17 00:00:00 2001 From: Benjamin Bannier Date: Mon, 20 Jul 2026 09:19:50 +0200 Subject: [PATCH 2/5] Cache `Manager.info()` results within a single invocation `test()` calls `self.info(pkg_path)` and then `validate_dependencies()` calls `self.info(name)` again for the same package with identical arguments. For git-backed packages this means two scratch clones of the same repo. Add `_info_cache` on `Manager` and extract `_info_lookup()` so `info()` returns a cached `PackageInfo` on repeated calls with the same arguments. --- testing/test_manager.py | 63 +++++++++++++++++++++++++++++++++++++++++ zeekpkg/manager.py | 22 ++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 testing/test_manager.py diff --git a/testing/test_manager.py b/testing/test_manager.py new file mode 100644 index 0000000..20f9e79 --- /dev/null +++ b/testing/test_manager.py @@ -0,0 +1,63 @@ +from pathlib import Path +from unittest.mock import patch + +import pytest + +from zeekpkg.manager import Manager +from zeekpkg.package import Package, PackageInfo + + +@pytest.fixture +def manager(tmp_path: Path) -> Manager: + with patch.object(Manager, "discover_builtin_packages", return_value=[]): + return Manager( + state_dir=str(tmp_path / "state"), + script_dir=str(tmp_path / "scripts"), + plugin_dir=str(tmp_path / "plugins"), + ) + + +class TestInfoCache: + """Manager.info() should return cached results on repeated calls.""" + + def test_repeated_call_returns_same_object(self, manager: Manager) -> None: + sentinel = PackageInfo(Package(git_url="https://example.com/pkg.git")) + + with patch.object( + manager, + "_info_lookup", + return_value=sentinel, + ) as mock_lookup: + result1 = manager.info( + "https://example.com/pkg.git", + prefer_installed=False, + ) + result2 = manager.info( + "https://example.com/pkg.git", + prefer_installed=False, + ) + + assert result1 is result2 + mock_lookup.assert_called_once() + + def test_different_args_call_lookup_separately(self, manager: Manager) -> None: + sentinel_a = PackageInfo(Package(git_url="https://example.com/pkg.git")) + sentinel_b = PackageInfo(Package(git_url="https://example.com/pkg.git")) + + with patch.object( + manager, + "_info_lookup", + side_effect=[sentinel_a, sentinel_b], + ) as mock_lookup: + result_installed = manager.info( + "https://example.com/pkg.git", + prefer_installed=True, + ) + result_fresh = manager.info( + "https://example.com/pkg.git", + prefer_installed=False, + ) + + assert result_installed is sentinel_a + assert result_fresh is sentinel_b + assert mock_lookup.call_count == 2 diff --git a/zeekpkg/manager.py b/zeekpkg/manager.py index a11cd8c..d0a2bec 100644 --- a/zeekpkg/manager.py +++ b/zeekpkg/manager.py @@ -273,6 +273,7 @@ def __init__( None # Cached Zeek built-in packages. ) self._builtin_packages_discovered = False # Flag if discovery even worked. + self._info_cache: dict[tuple[str, str | None, bool, bool], PackageInfo] = {} self.zeek_dist = zeek_dist self.state_dir = state_dir self.user_vars = {} if user_vars is None else user_vars @@ -1878,6 +1879,11 @@ def info( A :class:`.package.PackageInfo` object. """ pkg_path = canonical_url(pkg_path) + cache_key = (pkg_path, version, prefer_installed, update_submodules) + + if cache_key in self._info_cache: + return self._info_cache[cache_key] + name = name_from_path(pkg_path) if not is_valid_package_name(name): @@ -1886,6 +1892,22 @@ def info( LOG.debug('getting info on "%s"', pkg_path) + result = self._info_lookup( + pkg_path, + version, + prefer_installed, + update_submodules, + ) + self._info_cache[cache_key] = result + return result + + def _info_lookup( + self, + pkg_path: str, + version: str | None, + prefer_installed: bool, + update_submodules: bool, + ) -> PackageInfo: # Handle built-in packages like installed packages # but avoid looking up the repository information. bpkg_info = self.find_builtin_package(pkg_path) From eff2ea246158942cfbdb607f1760ae732b3b6458 Mon Sep 17 00:00:00 2001 From: Benjamin Bannier Date: Wed, 22 Jul 2026 08:10:00 +0200 Subject: [PATCH 3/5] Disk-persist `zeek --build-info` across invocations `zeek --build-info` ran on every `Manager` construction with no cross-run persistence. Cache the result to `state_dir/zeek_build_info_cache.json`, keyed on the `zeek` binary path, mtime, and size, so subsequent runs with an unchanged binary skip the subprocess entirely. This does not show up in btest benchmarks because each test runs with a fresh `state_dir`, so the cache is always cold. The benefit is real for users: the subprocess is paid on every `zkg` command and is free on all subsequent ones once the binary is unchanged. --- testing/test_manager.py | 99 ++++++++++++++++++++++++++++++++++++++++- zeekpkg/manager.py | 50 +++++++++++++++------ 2 files changed, 135 insertions(+), 14 deletions(-) diff --git a/testing/test_manager.py b/testing/test_manager.py index 20f9e79..33be4cf 100644 --- a/testing/test_manager.py +++ b/testing/test_manager.py @@ -1,5 +1,8 @@ +import json +import os from pathlib import Path -from unittest.mock import patch +from typing import ClassVar +from unittest.mock import MagicMock, patch import pytest @@ -61,3 +64,97 @@ def test_different_args_call_lookup_separately(self, manager: Manager) -> None: assert result_installed is sentinel_a assert result_fresh is sentinel_b assert mock_lookup.call_count == 2 + + +class TestBuildInfoDiskCache: + """`discover_builtin_packages()` uses a disk cache keyed on zeek path, mtime, and size.""" + + _FAKE_BUILD_INFO: ClassVar[dict[str, object]] = { + "zkg": {"provides": [{"name": "spicy", "version": "1.0.0", "commit": "abc"}]}, + } + _ZEEK_PATH: ClassVar[str] = "/usr/bin/zeek" + _CACHE_KEY: ClassVar[dict[str, object]] = { + "zeek_path": "/usr/bin/zeek", + "zeek_mtime": 1234567890.0, + "zeek_size": 4096, + } + + def _mock_stat(self, mock_stat: MagicMock, key: dict[str, object]) -> None: + mock_stat.return_value.st_mtime = key["zeek_mtime"] + mock_stat.return_value.st_size = key["zeek_size"] + + def test_disk_cache_hit_skips_subprocess(self, manager: Manager) -> None: + cache_file = os.path.join(manager.state_dir, "zeek_build_info_cache.json") + + with open(cache_file, "w") as f: + json.dump({"key": self._CACHE_KEY, "build_info": self._FAKE_BUILD_INFO}, f) + + with ( + patch("zeekpkg.manager.get_zeek_info") as mock_zeek_info, + patch("zeekpkg.manager.os.stat") as mock_stat, + patch("zeekpkg.manager.subprocess.check_output") as mock_check_output, + ): + mock_zeek_info.return_value.zeek = self._ZEEK_PATH + self._mock_stat(mock_stat, self._CACHE_KEY) + manager._builtin_packages = None + + manager.discover_builtin_packages() + + mock_check_output.assert_not_called() + + def test_disk_cache_miss_runs_subprocess(self, manager: Manager) -> None: + with ( + patch("zeekpkg.manager.get_zeek_info") as mock_zeek_info, + patch("zeekpkg.manager.os.stat") as mock_stat, + patch("zeekpkg.manager.subprocess.check_output") as mock_check_output, + ): + mock_zeek_info.return_value.zeek = self._ZEEK_PATH + self._mock_stat(mock_stat, self._CACHE_KEY) + mock_check_output.return_value = json.dumps(self._FAKE_BUILD_INFO).encode() + manager._builtin_packages = None + + manager.discover_builtin_packages() + + mock_check_output.assert_called_once() + + def test_stale_mtime_reruns_subprocess(self, manager: Manager) -> None: + cache_file = os.path.join(manager.state_dir, "zeek_build_info_cache.json") + stale_key = {**self._CACHE_KEY, "zeek_mtime": 1000.0} + + with open(cache_file, "w") as f: + json.dump({"key": stale_key, "build_info": self._FAKE_BUILD_INFO}, f) + + with ( + patch("zeekpkg.manager.get_zeek_info") as mock_zeek_info, + patch("zeekpkg.manager.os.stat") as mock_stat, + patch("zeekpkg.manager.subprocess.check_output") as mock_check_output, + ): + mock_zeek_info.return_value.zeek = self._ZEEK_PATH + self._mock_stat(mock_stat, self._CACHE_KEY) + mock_check_output.return_value = json.dumps(self._FAKE_BUILD_INFO).encode() + manager._builtin_packages = None + + manager.discover_builtin_packages() + + mock_check_output.assert_called_once() + + def test_different_path_reruns_subprocess(self, manager: Manager) -> None: + cache_file = os.path.join(manager.state_dir, "zeek_build_info_cache.json") + other_key = {**self._CACHE_KEY, "zeek_path": "/opt/zeek/bin/zeek"} + + with open(cache_file, "w") as f: + json.dump({"key": other_key, "build_info": self._FAKE_BUILD_INFO}, f) + + with ( + patch("zeekpkg.manager.get_zeek_info") as mock_zeek_info, + patch("zeekpkg.manager.os.stat") as mock_stat, + patch("zeekpkg.manager.subprocess.check_output") as mock_check_output, + ): + mock_zeek_info.return_value.zeek = self._ZEEK_PATH + self._mock_stat(mock_stat, self._CACHE_KEY) + mock_check_output.return_value = json.dumps(self._FAKE_BUILD_INFO).encode() + manager._builtin_packages = None + + manager.discover_builtin_packages() + + mock_check_output.assert_called_once() diff --git a/zeekpkg/manager.py b/zeekpkg/manager.py index d0a2bec..0205f9e 100644 --- a/zeekpkg/manager.py +++ b/zeekpkg/manager.py @@ -624,20 +624,44 @@ def discover_builtin_packages(self) -> list[PackageInfo]: LOG.warning("unable to discover builtin-packages: %s", str(e)) return self._builtin_packages + stat = os.stat(zeek_executable) + cache_key = { + "zeek_path": zeek_executable, + "zeek_mtime": stat.st_mtime, + "zeek_size": stat.st_size, + } + cache_file = os.path.join(self.state_dir, "zeek_build_info_cache.json") + build_info = None + try: - build_info_str = subprocess.check_output( - [zeek_executable, "--build-info"], - stderr=subprocess.DEVNULL, - timeout=10, - ) - build_info = json.loads(build_info_str) - except subprocess.CalledProcessError: - # Not a warning() due to being a bit noisy. - LOG.info("unable to discover built-in packages - requires Zeek 6.0") - return self._builtin_packages - except json.JSONDecodeError as e: - LOG.error("unable to parse Zeek's build info output: %s", str(e)) - return self._builtin_packages + with open(cache_file) as f: + cached = json.load(f) + if cached.get("key") == cache_key: + build_info = cached.get("build_info") + except (OSError, json.JSONDecodeError, KeyError): + pass + + if build_info is None: + try: + build_info_str = subprocess.check_output( + [zeek_executable, "--build-info"], + stderr=subprocess.DEVNULL, + timeout=10, + ) + build_info = json.loads(build_info_str) + except subprocess.CalledProcessError: + # Not a warning() due to being a bit noisy. + LOG.info("unable to discover built-in packages - requires Zeek 6.0") + return self._builtin_packages + except json.JSONDecodeError as e: + LOG.error("unable to parse Zeek's build info output: %s", str(e)) + return self._builtin_packages + + try: + with open(cache_file, "w") as f: + json.dump({"key": cache_key, "build_info": build_info}, f) + except OSError as e: + LOG.debug("unable to write zeek build info cache: %s", e) if "zkg" not in build_info or "provides" not in build_info["zkg"]: LOG.warning("missing zkg.provides entry in zeek --build-info output") From 7b33cc87f8febc10b7a18749858a56ad660635df Mon Sep 17 00:00:00 2001 From: Benjamin Bannier Date: Mon, 20 Jul 2026 10:44:19 +0200 Subject: [PATCH 4/5] Skip submodule fetch in `_info()` scratch clone `_info()` clones a package into `scratch_dir` solely to read its metadata (`zkg.meta`), version tags, and default branch. Neither submodule content nor a fully initialised submodule tree is needed for this. The code called `_clone_package()` with `recursive=True` (fetching all submodules at clone time) and `git_checkout()` with `update_submodules=True` (running `git submodule sync --recursive` and `git submodule update --recursive --init`), causing up to four redundant network roundtrips per submodule for packages that would later be cloned again into the test or install staging directory. Hardcode `recursive=False` and `update_submodules=False` in `_info()` since the scratch clone is throwaway and metadata-only. Remove the now-unused `update_submodules` parameter from `_info()`, `_info_lookup()`, and `info()`. --- zeekpkg/manager.py | 25 +++++++------------------ zkg | 2 -- 2 files changed, 7 insertions(+), 20 deletions(-) diff --git a/zeekpkg/manager.py b/zeekpkg/manager.py index 0205f9e..43d0e39 100644 --- a/zeekpkg/manager.py +++ b/zeekpkg/manager.py @@ -273,7 +273,7 @@ def __init__( None # Cached Zeek built-in packages. ) self._builtin_packages_discovered = False # Flag if discovery even worked. - self._info_cache: dict[tuple[str, str | None, bool, bool], PackageInfo] = {} + self._info_cache: dict[tuple[str, str | None, bool], PackageInfo] = {} self.zeek_dist = zeek_dist self.state_dir = state_dir self.user_vars = {} if user_vars is None else user_vars @@ -1874,7 +1874,6 @@ def info( pkg_path: str, version: str | None = "", prefer_installed: bool = True, - update_submodules: bool = True, ) -> PackageInfo: """Retrieves information about a package. @@ -1896,14 +1895,11 @@ def info( The `version` parameter is also ignored when this is set as it uses whatever version of the package is currently installed. - update_submodules (bool): if this is set, git checkout will update - the submodules for the repository. - Returns: A :class:`.package.PackageInfo` object. """ pkg_path = canonical_url(pkg_path) - cache_key = (pkg_path, version, prefer_installed, update_submodules) + cache_key = (pkg_path, version, prefer_installed) if cache_key in self._info_cache: return self._info_cache[cache_key] @@ -1916,12 +1912,7 @@ def info( LOG.debug('getting info on "%s"', pkg_path) - result = self._info_lookup( - pkg_path, - version, - prefer_installed, - update_submodules, - ) + result = self._info_lookup(pkg_path, version, prefer_installed) self._info_cache[cache_key] = result return result @@ -1930,7 +1921,6 @@ def _info_lookup( pkg_path: str, version: str | None, prefer_installed: bool, - update_submodules: bool, ) -> PackageInfo: # Handle built-in packages like installed packages # but avoid looking up the repository information. @@ -1957,7 +1947,7 @@ def _info_lookup( package = Package(git_url=pkg_path) try: - return self._info(package, None, version, update_submodules) + return self._info(package, None, version) except git.GitCommandError as error: LOG.info( 'getting info on "%s": invalid git repo path: %s', @@ -1990,7 +1980,7 @@ def _info_lookup( package = matches[0] try: - return self._info(package, None, version, update_submodules) + return self._info(package, None, version) except git.GitCommandError as error: LOG.info('getting info on "%s": invalid git repo path: %s', pkg_path, error) reason = "git repository is either invalid or unreachable" @@ -2001,7 +1991,6 @@ def _info( package: Package, status: PackageStatus | None, version: str | None, - update_submodules: bool, ) -> PackageInfo: """Retrieves information about a package. @@ -2012,7 +2001,7 @@ def _info( git.GitCommandError: when failing to clone the package repo """ clonepath = os.path.join(self.scratch_dir, package.name) - clone = _clone_package(package, clonepath, version, update_submodules) + clone = _clone_package(package, clonepath, version, recursive=False) versions = git_version_tags(clone) if not version: @@ -2022,7 +2011,7 @@ def _info( version = git_default_branch(clone) try: - git_checkout(clone, version, update_submodules) + git_checkout(clone, version, update_submodules=False) except git.GitCommandError: reason = f'no such commit, branch, or version tag: "{version}"' return PackageInfo(package=package, status=status, invalid_reason=reason) diff --git a/zkg b/zkg index edd2503..f5fa990 100755 --- a/zkg +++ b/zkg @@ -2037,7 +2037,6 @@ def cmd_info( name, version=args.version, prefer_installed=(not args.nolocal), - update_submodules=False, ) if info2.package: @@ -2109,7 +2108,6 @@ def cmd_info( name, vers, prefer_installed=(not args.nolocal), - update_submodules=False, ) pkginfo[name]["metadata"][info3.metadata_version] = {} if info3.metadata_file: From 570bf9b4ddcce339b1481190d02a663bda90e0c4 Mon Sep 17 00:00:00 2001 From: Benjamin Bannier Date: Mon, 20 Jul 2026 10:53:01 +0200 Subject: [PATCH 5/5] Always use a shallow clone for the `_info()` scratch clone The scratch clone exists only to read `zkg.meta`, version tags, and the default branch; none of which require full history. `_clone_package()` used a deep clone whenever a specific version was requested (to safely `git checkout` arbitrary historical commits), but that conservatism is only needed for the operational clone used during install/test staging. Inline `delete_path` + `git_clone(..., shallow=True, recursive=False)` directly in `_info()` so the scratch clone is always a `--depth 1 --no-single-branch` clone regardless of the requested version. This does not show up in btest benchmarks because test packages are local repos created with 1-2 commits -- there is no history to truncate. The benefit is real for users running `zkg info` or `zkg install` against remote packages with non-trivial history, where a shallow clone avoids transferring the full object graph. --- zeekpkg/manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zeekpkg/manager.py b/zeekpkg/manager.py index 43d0e39..5a5a47b 100644 --- a/zeekpkg/manager.py +++ b/zeekpkg/manager.py @@ -2001,7 +2001,8 @@ def _info( git.GitCommandError: when failing to clone the package repo """ clonepath = os.path.join(self.scratch_dir, package.name) - clone = _clone_package(package, clonepath, version, recursive=False) + delete_path(clonepath) + clone = git_clone(package.git_url, clonepath, shallow=True, recursive=False) versions = git_version_tags(clone) if not version: