diff --git a/charmcraft/services/provider.py b/charmcraft/services/provider.py index 951cca117..a2de13b67 100644 --- a/charmcraft/services/provider.py +++ b/charmcraft/services/provider.py @@ -17,7 +17,12 @@ """Service class for creating providers.""" from __future__ import annotations -import atexit +import contextlib +import io +from collections.abc import Generator + +from craft_application.models import BuildInfo +from overrides import override try: import fcntl @@ -27,17 +32,41 @@ import pathlib from typing import cast +import craft_application import craft_providers from craft_application import services from craft_cli import emit from craft_providers import bases -from charmcraft import env +from charmcraft import env, models class ProviderService(services.ProviderService): """Business logic for getting providers.""" + def __init__( + self, + app: craft_application.AppMetadata, + services: craft_application.ServiceFactory, + *, + project: models.CharmcraftProject, + work_dir: pathlib.Path, + build_plan: list[BuildInfo], + provider_name: str | None = None, + install_snap: bool = True, + ) -> None: + super().__init__( + app, + services, + project=project, + work_dir=work_dir, + build_plan=build_plan, + provider_name=provider_name, + install_snap=install_snap, + ) + self._cache_path = None + self._lock = None + def setup(self) -> None: """Set up the provider service for Charmcraft.""" super().setup() @@ -64,11 +93,13 @@ def get_base( If no cache_path is included, adds one. """ - cache_path = cast(pathlib.Path, kwargs.get("cache_path", env.get_host_shared_cache_path())) - our_lock = _maybe_lock_cache(cache_path) + self._cache_path = cast( + pathlib.Path, kwargs.get("cache_path", env.get_host_shared_cache_path()) + ) + self._lock = _maybe_lock_cache(self._cache_path) # Forward the shared cache path. - kwargs["cache_path"] = cache_path if our_lock else None + kwargs["cache_path"] = self._cache_path if self._lock else None return super().get_base( base_name, instance_name=instance_name, @@ -76,11 +107,31 @@ def get_base( **kwargs, # type: ignore[arg-type] ) - -def _maybe_lock_cache(path: pathlib.Path) -> bool: + @override + @contextlib.contextmanager + def instance( + self, + build_info: BuildInfo, + *, + work_dir: pathlib.Path, + allow_unstable: bool = True, + **kwargs: bool | str | None, + ) -> Generator[craft_providers.Executor, None, None]: + with super().instance( + build_info, work_dir=work_dir, allow_unstable=allow_unstable, **kwargs + ) as instance: + try: + yield instance + finally: + if fcntl is not None and self._lock: + fcntl.flock(self._lock, fcntl.LOCK_UN) + self._lock.close() + + +def _maybe_lock_cache(path: pathlib.Path) -> io.TextIOBase | None: """Lock the cache so we only have one copy of Charmcraft using it at a time.""" if fcntl is None: # Don't lock on Windows - just don't cache. - return False + return None cache_lock_path = path / "charmcraft.lock" emit.trace("Attempting to lock the cache path") @@ -92,14 +143,11 @@ def _maybe_lock_cache(path: pathlib.Path) -> bool: emit.progress( "Shared cache locked by another process; running without cache.", permanent=True ) - return False + return None else: pid = str(os.getpid()) lock_file.write(pid) lock_file.flush() os.fsync(lock_file.fileno()) emit.trace(f"Cache path locked by this process ({pid})") - atexit.register(fcntl.flock, lock_file, fcntl.LOCK_UN) - atexit.register(cache_lock_path.unlink, missing_ok=True) - - return True + return lock_file diff --git a/pyproject.toml b/pyproject.toml index 128bfd863..f967a85a0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -330,6 +330,9 @@ lint.ignore = [ # Allow Pydantic's `@validator` decorator to trigger class method treatment. classmethod-decorators = ["pydantic.validator"] +[tool.ruff.lint.pydocstyle] +ignore-decorators = ["overrides.overrides", "overrides.override", "typing.overload", "typing.override"] + [tool.ruff.lint.per-file-ignores] "tests/**.py" = [ # Some things we want for the moin project are unnecessary in tests. "D", # Ignore docstring rules in tests diff --git a/tests/integration/services/test_provider.py b/tests/integration/services/test_provider.py index ba93b5bdd..6294e29bc 100644 --- a/tests/integration/services/test_provider.py +++ b/tests/integration/services/test_provider.py @@ -16,6 +16,7 @@ """Integration tests for the provider service.""" import pathlib +import subprocess import sys import pytest @@ -35,19 +36,25 @@ def test_lock_cache( ): cache_path = tmp_path / "cache" cache_path.mkdir() + lock_file = cache_path / "charmcraft.lock" + bash_lock_cmd = ["bash", "-c", f"flock -n {lock_file} true"] provider = service_factory.provider provider_kwargs = { "build_info": default_build_info, "work_dir": pathlib.Path(__file__).parent, "cache_path": cache_path, } - assert not (cache_path / "charmcraft.lock").exists() + assert not lock_file.exists() with provider.instance(**provider_kwargs): # Test that the cache lock gets created - assert (cache_path / "charmcraft.lock").is_file() + assert lock_file.is_file() + with pytest.raises(subprocess.CalledProcessError): + # Another process should not be able to lock the file. + subprocess.run(bash_lock_cmd, check=True) - (cache_path / "charmcraft.lock").write_text("Test that file isn't locked.") + # After exiting we should be able to lock the file. + subprocess.run(bash_lock_cmd, check=True) @pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows") @@ -59,8 +66,16 @@ def test_locked_cache_no_cache( ): cache_path = tmp_path / "cache" cache_path.mkdir() - _maybe_lock_cache(cache_path) - assert (cache_path / "charmcraft.lock").exists() + lock_file = cache_path / "charmcraft.lock" + + bash_lock_cmd = ["bash", "-c", f"flock -n {lock_file} true"] + # Check that we can lock the file from another process. + subprocess.run(bash_lock_cmd, check=True) + _ = _maybe_lock_cache(cache_path) + # And now we can't. + with pytest.raises(subprocess.CalledProcessError): + subprocess.run(bash_lock_cmd, check=True) + provider = service_factory.provider provider_kwargs = { "build_info": default_build_info, @@ -69,9 +84,15 @@ def test_locked_cache_no_cache( } with provider.instance(**provider_kwargs) as instance: - # Because we've already locked the cache, we don't get a subdirectory in the cache. - assert list(cache_path.iterdir()) == [cache_path / "charmcraft.lock"] - # Create a file in the cache and ensure it's not visible in the outer fs instance.execute_run(["touch", "/root/.cache/cache_cached"]) + + # Because we've already locked the cache, we don't get a subdirectory in + # the cache, and thus the touch command inside there only affected the + # instance cache and not the shared cache. + assert list(cache_path.iterdir()) == [cache_path / "charmcraft.lock"] + emitter.assert_progress( + "Shared cache locked by another process; running without cache.", permanent=True + ) + assert not (tmp_path / "cache_cached").exists()