From 1e285148e825ef1df9d55c701983a49111ee5aac Mon Sep 17 00:00:00 2001 From: Sheng Yu Date: Fri, 15 Dec 2023 17:30:28 -0500 Subject: [PATCH] fix(remote-build): check for shallowly cloned git repos (#4498) Shallow git repos are not designed to be pushed, so the new remote-builder now raises an error for shallowly cloned repos. core20 and core22 projects in shallow clone repos fall back to the legacy remote-builder to retain compatibility. --- snapcraft/commands/remote.py | 20 +++- snapcraft/remote/__init__.py | 13 ++- snapcraft/remote/errors.py | 13 +++ snapcraft/remote/git.py | 47 +++++++++- snapcraft/remote/remote_builder.py | 3 + tests/unit/commands/test_remote.py | 101 ++++++++++++++++++++ tests/unit/remote/test_git.py | 113 ++++++++++++++++++++++- tests/unit/remote/test_remote_builder.py | 29 +++++- 8 files changed, 332 insertions(+), 7 deletions(-) diff --git a/snapcraft/commands/remote.py b/snapcraft/commands/remote.py index 3028e274b7..8a338ccd4f 100644 --- a/snapcraft/commands/remote.py +++ b/snapcraft/commands/remote.py @@ -30,7 +30,12 @@ from snapcraft.errors import MaintenanceBase, SnapcraftError from snapcraft.legacy_cli import run_legacy from snapcraft.parts import yaml_utils -from snapcraft.remote import AcceptPublicUploadError, RemoteBuilder, is_repo +from snapcraft.remote import ( + AcceptPublicUploadError, + GitType, + RemoteBuilder, + get_git_repo_type, +) from snapcraft.utils import confirm_with_user, get_host_architecture, humanize_list _CONFIRMATION_PROMPT = ( @@ -193,13 +198,24 @@ def _run_new_or_fallback_remote_build(self, base: str) -> None: run_legacy() return - if is_repo(Path().absolute()): + git_type = get_git_repo_type(Path().absolute()) + + if git_type == GitType.NORMAL: emit.debug( "Running new remote-build because project is in a git repository" ) self._run_new_remote_build() return + if git_type == GitType.SHALLOW: + emit.debug("Current git repository is shallow cloned.") + emit.progress( + "Remote build for shallow clones is deprecated " + "and will be removed in core24", + permanent=True, + ) + # fall-through to legacy remote-build + emit.debug("Running fallback remote-build") run_legacy() diff --git a/snapcraft/remote/__init__.py b/snapcraft/remote/__init__.py index c8b74205be..fa97f792ae 100644 --- a/snapcraft/remote/__init__.py +++ b/snapcraft/remote/__init__.py @@ -22,17 +22,26 @@ LaunchpadHttpsError, RemoteBuildError, RemoteBuildFailedError, + RemoteBuildInvalidGitRepoError, RemoteBuildTimeoutError, UnsupportedArchitectureError, ) -from .git import GitRepo, is_repo +from .git import ( + GitRepo, + GitType, + check_git_repo_for_remote_build, + get_git_repo_type, + is_repo, +) from .launchpad import LaunchpadClient from .remote_builder import RemoteBuilder from .utils import get_build_id, humanize_list, rmtree, validate_architectures from .worktree import WorkTree __all__ = [ + "check_git_repo_for_remote_build", "get_build_id", + "get_git_repo_type", "humanize_list", "is_repo", "rmtree", @@ -40,11 +49,13 @@ "AcceptPublicUploadError", "GitError", "GitRepo", + "GitType", "LaunchpadClient", "LaunchpadHttpsError", "RemoteBuilder", "RemoteBuildError", "RemoteBuildFailedError", + "RemoteBuildInvalidGitRepoError", "RemoteBuildTimeoutError", "UnsupportedArchitectureError", "WorkTree", diff --git a/snapcraft/remote/errors.py b/snapcraft/remote/errors.py index 25aaf4d7a9..7d8ca44150 100644 --- a/snapcraft/remote/errors.py +++ b/snapcraft/remote/errors.py @@ -115,3 +115,16 @@ def __init__(self, details: str) -> None: brief = "Remote build failed." super().__init__(brief=brief, details=details) + + +class RemoteBuildInvalidGitRepoError(RemoteBuildError): + """The Git repository is invalid for remote build. + + :param brief: Brief description of error. + :param details: Detailed information. + """ + + def __init__(self, details: str) -> None: + brief = "The Git repository is invalid for remote build." + + super().__init__(brief=brief, details=details) diff --git a/snapcraft/remote/git.py b/snapcraft/remote/git.py index 6bf914cdc5..9b2223a3c7 100644 --- a/snapcraft/remote/git.py +++ b/snapcraft/remote/git.py @@ -20,16 +20,25 @@ import os import subprocess import time +from enum import Enum from pathlib import Path from typing import Optional import pygit2 -from .errors import GitError +from .errors import GitError, RemoteBuildInvalidGitRepoError logger = logging.getLogger(__name__) +class GitType(Enum): + """Type of git repository.""" + + INVALID = 0 + NORMAL = 1 + SHALLOW = 2 + + def is_repo(path: Path) -> bool: """Check if a directory is a git repo. @@ -50,6 +59,42 @@ def is_repo(path: Path) -> bool: ) from error +def get_git_repo_type(path: Path) -> GitType: + """Check if a directory is a git repo and return the type. + + :param path: filepath to check + + :returns: GitType + """ + if is_repo(path): + repo = pygit2.Repository(path) + if repo.is_shallow: + return GitType.SHALLOW + return GitType.NORMAL + + return GitType.INVALID + + +def check_git_repo_for_remote_build(path: Path) -> None: + """Check if a directory meets the requirements of doing remote builds. + + :param path: filepath to check + + :raises RemoteBuildInvalidGitRepoError: if incompatible git repo is found + """ + git_type = get_git_repo_type(path.absolute()) + + if git_type == GitType.INVALID: + raise RemoteBuildInvalidGitRepoError( + f"Could not find a git repository in {str(path)!r}" + ) + + if git_type == GitType.SHALLOW: + raise RemoteBuildInvalidGitRepoError( + "Remote build for shallow cloned git repos are no longer supported" + ) + + class GitRepo: """Git repository class.""" diff --git a/snapcraft/remote/remote_builder.py b/snapcraft/remote/remote_builder.py index d7b393ea9c..889d5deccf 100644 --- a/snapcraft/remote/remote_builder.py +++ b/snapcraft/remote/remote_builder.py @@ -20,6 +20,7 @@ from pathlib import Path from typing import List, Optional +from .git import check_git_repo_for_remote_build from .launchpad import LaunchpadClient from .utils import get_build_id, humanize_list, validate_architectures from .worktree import WorkTree @@ -55,6 +56,8 @@ def __init__( # noqa: PLR0913 pylint: disable=too-many-arguments self._project_name = project_name self._project_dir = project_dir + check_git_repo_for_remote_build(self._project_dir) + if build_id: self._build_id = build_id else: diff --git a/tests/unit/commands/test_remote.py b/tests/unit/commands/test_remote.py index c8c3688f7c..10167887d0 100644 --- a/tests/unit/commands/test_remote.py +++ b/tests/unit/commands/test_remote.py @@ -16,6 +16,9 @@ """Remote-build command tests.""" +import os +import shutil +import subprocess import sys from pathlib import Path from unittest.mock import ANY, call @@ -523,6 +526,104 @@ def test_run_in_repo_newer_than_core22( emitter.assert_debug("Running new remote-build because base is newer than core22") +@pytest.mark.parametrize( + "create_snapcraft_yaml", LEGACY_BASES | {"core22"}, indirect=True +) +@pytest.mark.usefixtures("create_snapcraft_yaml", "mock_confirm", "mock_argv") +def test_run_in_shallow_repo(emitter, mock_run_legacy, new_dir): + """core22 and older bases fall back to legacy remote-build if in a shallow git repo.""" + root_path = Path(new_dir) + git_normal_path = root_path / "normal" + git_normal_path.mkdir() + git_shallow_path = root_path / "shallow" + + shutil.move(root_path / "snap", git_normal_path) + + repo_normal = GitRepo(git_normal_path) + (repo_normal.path / "1").write_text("1") + repo_normal.add_all() + repo_normal.commit("1") + + (repo_normal.path / "2").write_text("2") + repo_normal.add_all() + repo_normal.commit("2") + + (repo_normal.path / "3").write_text("3") + repo_normal.add_all() + repo_normal.commit("3") + + # pygit2 does not support shallow cloning, so we use git directly + subprocess.run( + [ + "git", + "clone", + "--depth", + "1", + git_normal_path.absolute().as_uri(), + git_shallow_path.absolute().as_posix(), + ], + check=True, + ) + + os.chdir(git_shallow_path) + cli.run() + + mock_run_legacy.assert_called_once() + emitter.assert_debug("Current git repository is shallow cloned.") + emitter.assert_debug("Running fallback remote-build") + + +@pytest.mark.parametrize( + "create_snapcraft_yaml", CURRENT_BASES - {"core22"}, indirect=True +) +@pytest.mark.usefixtures( + "create_snapcraft_yaml", "mock_confirm", "mock_argv", "use_new_remote_build" +) +def test_run_in_shallow_repo_unsupported(capsys, new_dir): + """devel / core24 and newer bases run new remote-build in a shallow git repo.""" + root_path = Path(new_dir) + git_normal_path = root_path / "normal" + git_normal_path.mkdir() + git_shallow_path = root_path / "shallow" + + shutil.move(root_path / "snap", git_normal_path) + + repo_normal = GitRepo(git_normal_path) + (repo_normal.path / "1").write_text("1") + repo_normal.add_all() + repo_normal.commit("1") + + (repo_normal.path / "2").write_text("2") + repo_normal.add_all() + repo_normal.commit("2") + + (repo_normal.path / "3").write_text("3") + repo_normal.add_all() + repo_normal.commit("3") + + # pygit2 does not support shallow cloning, so we use git directly + subprocess.run( + [ + "git", + "clone", + "--depth", + "1", + git_normal_path.absolute().as_uri(), + git_shallow_path.absolute().as_posix(), + ], + check=True, + ) + + os.chdir(git_shallow_path) + + # no exception because run() catches it + ret = cli.run() + assert ret != 0 + _, err = capsys.readouterr() + + assert "Remote build for shallow cloned git repos are no longer supported" in err + + ###################### # Architecture tests # ###################### diff --git a/tests/unit/remote/test_git.py b/tests/unit/remote/test_git.py index cb16f28980..9a50a68ae0 100644 --- a/tests/unit/remote/test_git.py +++ b/tests/unit/remote/test_git.py @@ -18,13 +18,22 @@ """Tests for the pygit2 wrapper class.""" import re +import subprocess from pathlib import Path from unittest.mock import ANY import pygit2 import pytest -from snapcraft.remote import GitError, GitRepo, is_repo +from snapcraft.remote import ( + GitError, + GitRepo, + GitType, + RemoteBuildInvalidGitRepoError, + check_git_repo_for_remote_build, + get_git_repo_type, + is_repo, +) def test_is_repo(new_dir): @@ -39,6 +48,54 @@ def test_is_not_repo(new_dir): assert not is_repo(new_dir) +def test_git_repo_type_invalid(new_dir): + """Check if directory is an invalid repo.""" + assert get_git_repo_type(new_dir) == GitType.INVALID + + +def test_git_repo_type_normal(new_dir): + """Check if directory is a repo.""" + GitRepo(new_dir) + + assert get_git_repo_type(new_dir) == GitType.NORMAL + + +def test_git_repo_type_shallow(new_dir): + """Check if directory is a shallow cloned repo.""" + root_path = Path(new_dir) + git_normal_path = root_path / "normal" + git_normal_path.mkdir() + git_shallow_path = root_path / "shallow" + + repo_normal = GitRepo(git_normal_path) + (repo_normal.path / "1").write_text("1") + repo_normal.add_all() + repo_normal.commit("1") + + (repo_normal.path / "2").write_text("2") + repo_normal.add_all() + repo_normal.commit("2") + + (repo_normal.path / "3").write_text("3") + repo_normal.add_all() + repo_normal.commit("3") + + # pygit2 does not support shallow cloning, so we use git directly + subprocess.run( + [ + "git", + "clone", + "--depth", + "1", + git_normal_path.absolute().as_uri(), + git_shallow_path.absolute().as_posix(), + ], + check=True, + ) + + assert get_git_repo_type(git_shallow_path) == GitType.SHALLOW + + def test_is_repo_path_only(new_dir): """Only look at the path for a repo.""" Path("parent-repo/not-a-repo/child-repo").mkdir(parents=True) @@ -476,3 +533,57 @@ def test_push_url_push_error(new_dir): assert raised.value.details is not None assert re.match(expected_error_details, raised.value.details) + + +def test_check_git_repo_for_remote_build_invalid(new_dir): + """Check if directory is an invalid repo.""" + with pytest.raises( + RemoteBuildInvalidGitRepoError, match="Could not find a git repository in" + ): + check_git_repo_for_remote_build(new_dir) + + +def test_check_git_repo_for_remote_build_normal(new_dir): + """Check if directory is a repo.""" + GitRepo(new_dir) + check_git_repo_for_remote_build(new_dir) + + +def test_check_git_repo_for_remote_build_shallow(new_dir): + """Check if directory is a shallow cloned repo.""" + root_path = Path(new_dir) + git_normal_path = root_path / "normal" + git_normal_path.mkdir() + git_shallow_path = root_path / "shallow" + + repo_normal = GitRepo(git_normal_path) + (repo_normal.path / "1").write_text("1") + repo_normal.add_all() + repo_normal.commit("1") + + (repo_normal.path / "2").write_text("2") + repo_normal.add_all() + repo_normal.commit("2") + + (repo_normal.path / "3").write_text("3") + repo_normal.add_all() + repo_normal.commit("3") + + # pygit2 does not support shallow cloning, so we use git directly + subprocess.run( + [ + "git", + "clone", + "--depth", + "1", + git_normal_path.absolute().as_uri(), + git_shallow_path.absolute().as_posix(), + ], + check=True, + ) + + with pytest.raises( + RemoteBuildInvalidGitRepoError, + match="Remote build for shallow cloned git repos are no longer supported", + ): + check_git_repo_for_remote_build(git_shallow_path) diff --git a/tests/unit/remote/test_remote_builder.py b/tests/unit/remote/test_remote_builder.py index 17acf9a206..79835dfb49 100644 --- a/tests/unit/remote/test_remote_builder.py +++ b/tests/unit/remote/test_remote_builder.py @@ -22,7 +22,7 @@ import pytest -from snapcraft.remote import RemoteBuilder, UnsupportedArchitectureError +from snapcraft.remote import GitType, RemoteBuilder, UnsupportedArchitectureError from snapcraft.remote.utils import _SUPPORTED_ARCHS @@ -56,6 +56,24 @@ def fake_remote_builder(new_dir, mock_launchpad_client, mock_worktree): ) +@pytest.fixture() +def mock_git_check(mocker): + """Ignore git check.""" + mock_git_check = mocker.patch( + "snapcraft.remote.git.check_git_repo_for_remote_build" + ) + mock_git_check.return_value = None + return mock_git_check + + +@pytest.fixture() +def mock_git_type_check(mocker): + """Ignore git type check.""" + mock_git_type = mocker.patch("snapcraft.remote.git.get_git_repo_type") + mock_git_type.return_value = GitType.NORMAL + return mock_git_type + + def test_remote_builder_init(mock_launchpad_client, mock_worktree): """Verify remote builder is properly initialized.""" RemoteBuilder( @@ -81,7 +99,8 @@ def test_remote_builder_init(mock_launchpad_client, mock_worktree): ] -@pytest.mark.usefixtures("new_dir") +@pytest.mark.usefixtures("mock_git_check", "mock_git_type_check") +@pytest.mark.usefixtures("new_dir", "mock_git_check") def test_build_id_computed(): """Compute a build id if it is not provided.""" remote_builder = RemoteBuilder( @@ -133,6 +152,7 @@ def test_validate_architectures_unsupported(archs): ) +@pytest.mark.usefixtures("mock_git_check", "mock_git_type_check") @patch("logging.Logger.info") def test_print_status_builds_found( mock_log, mock_launchpad_client, fake_remote_builder @@ -152,6 +172,7 @@ def test_print_status_builds_found( ] +@pytest.mark.usefixtures("mock_git_check", "mock_git_type_check") @patch("logging.Logger.info") def test_print_status_no_build_found(mock_log, fake_remote_builder): """Print the status of a remote build.""" @@ -160,6 +181,7 @@ def test_print_status_no_build_found(mock_log, fake_remote_builder): assert mock_log.mock_calls == [call("No build task(s) found.")] +@pytest.mark.usefixtures("mock_git_check", "mock_git_type_check") @pytest.mark.parametrize("has_builds", (True, False)) def test_has_outstanding_build(has_builds, fake_remote_builder, mock_launchpad_client): """Check for outstanding builds.""" @@ -168,6 +190,7 @@ def test_has_outstanding_build(has_builds, fake_remote_builder, mock_launchpad_c assert fake_remote_builder.has_outstanding_build() == has_builds +@pytest.mark.usefixtures("mock_git_check", "mock_git_type_check") def test_monitor_build(fake_remote_builder, mock_launchpad_client): """Monitor a build.""" fake_remote_builder.monitor_build() @@ -175,6 +198,7 @@ def test_monitor_build(fake_remote_builder, mock_launchpad_client): mock_launchpad_client.return_value.monitor_build.assert_called_once() +@pytest.mark.usefixtures("mock_git_check", "mock_git_type_check") def test_clean_build(fake_remote_builder, mock_launchpad_client, mock_worktree): """Clean a build.""" fake_remote_builder.clean_build() @@ -183,6 +207,7 @@ def test_clean_build(fake_remote_builder, mock_launchpad_client, mock_worktree): mock_worktree.return_value.clean_cache.assert_called_once() +@pytest.mark.usefixtures("mock_git_check", "mock_git_type_check") def test_start_build(fake_remote_builder, mock_launchpad_client, mock_worktree): """Start a build.""" fake_remote_builder.start_build()