From 57433493095cf624b2156d7d03b8164437e40014 Mon Sep 17 00:00:00 2001 From: Callahan Kovacs Date: Thu, 23 Jan 2025 15:19:41 -0600 Subject: [PATCH] feat(remotebuild): filter build-plan with --platform or --build-for Allows using '--platform' or '--build-for' to filter entries in the 'architectures' or 'platforms' keys in a project file. If the project file doesn't contain an 'architectures' or 'platforms' entry, then '--platform' and '--build-for' define the architectures to build for. Signed-off-by: Callahan Kovacs --- docs/explanation/remote-build.rst | 19 +- snapcraft/application.py | 10 +- snapcraft/commands/remote.py | 180 +++++++++++------- .../architectures-filtered/arguments.txt | 1 + .../architectures-filtered/expected-snaps.txt | 1 + .../architectures-filtered/snapcraft.yaml | 18 ++ tests/spread/core22/remote-build/task.yaml | 1 + .../snaps/platforms-filtered/arguments.txt | 1 + .../platforms-filtered/expected-snaps.txt | 1 + .../snaps/platforms-filtered/snapcraft.yaml | 20 ++ tests/spread/core24/remote-build/task.yaml | 1 + tests/unit/commands/test_remote.py | 114 +++++++++-- tests/unit/test_application.py | 12 +- 13 files changed, 285 insertions(+), 94 deletions(-) create mode 100644 tests/spread/core22/remote-build/snaps/architectures-filtered/arguments.txt create mode 100644 tests/spread/core22/remote-build/snaps/architectures-filtered/expected-snaps.txt create mode 100644 tests/spread/core22/remote-build/snaps/architectures-filtered/snapcraft.yaml create mode 100644 tests/spread/core24/remote-build/snaps/platforms-filtered/arguments.txt create mode 100644 tests/spread/core24/remote-build/snaps/platforms-filtered/expected-snaps.txt create mode 100644 tests/spread/core24/remote-build/snaps/platforms-filtered/snapcraft.yaml diff --git a/docs/explanation/remote-build.rst b/docs/explanation/remote-build.rst index 9e95faad52..c41031cd9b 100644 --- a/docs/explanation/remote-build.rst +++ b/docs/explanation/remote-build.rst @@ -89,12 +89,20 @@ Current ``--platform`` and ``--build-for`` behave differently than they do for :ref:`lifecycle commands`. -``--platform`` or ``--build-for`` can only be provided when the ``platforms`` -or ``architectures`` keywords are not defined in the project metadata -(`[12]`_). +If the project file contains a ``platforms`` or ``architectures`` key, then +``--platform`` or ``--build-for`` filter the build plan defined by that key. +For more information about build plans and filtering, see +:ref:`Build plans `. -These keywords are mutually exclusive and must be a comma-separated list of -debian architectures. +If the project file doesn't contain a ``platforms`` or ``architectures`` entry, +then ``--platform`` or ``--build-for`` define the architectures to build for. + +It the project file doesn't contain a ``platforms`` or ``architectures`` entry +and ``--platform`` or ``--build-for`` are not provided, Snapcraft will build +for the host system's architecture. + +These ``--platform`` and ``--build-for`` arguments are mutually exclusive and +must be a comma-separated list of debian architectures. ``core22`` snaps can only use ``--build-for``. ``core24`` and newer snaps can use ``--platform`` or ``--build-for``. @@ -180,6 +188,5 @@ Launchpad is not able to parse this notation (`[9]`_). .. _`[8]`: https://bugs.launchpad.net/snapcraft/+bug/2007789 .. _`[9]`: https://bugs.launchpad.net/snapcraft/+bug/2042167 .. _`[10]`: https://github.com/canonical/snapcraft/issues/4885 -.. _`[12]`: https://github.com/canonical/snapcraft/issues/4992 .. _`[13]`: https://github.com/canonical/snapcraft/issues/4996 .. _`[14]`: https://github.com/canonical/snapcraft/issues/4995 diff --git a/snapcraft/application.py b/snapcraft/application.py index 836e2e4801..1e747689df 100644 --- a/snapcraft/application.py +++ b/snapcraft/application.py @@ -27,7 +27,7 @@ import craft_cli import craft_parts import craft_store -from craft_application import Application, AppMetadata, remote, util +from craft_application import Application, AppMetadata, launchpad, remote, util from craft_application.commands import get_other_command_group from craft_cli import emit from craft_parts.plugins.plugins import PluginType @@ -224,6 +224,14 @@ def _run_inner(self) -> int: err.doc_slug = "/explanation/remote-build" self._emit_error(err) return_code = err.retcode + except launchpad.LaunchpadError as err: + self._emit_error( + craft_cli.errors.CraftError( + f"{err}", doc_slug="/explanation/remote-build" + ), + cause=err, + ) + return_code = 1 return return_code diff --git a/snapcraft/commands/remote.py b/snapcraft/commands/remote.py index a38ea34a1f..3e96610ee3 100644 --- a/snapcraft/commands/remote.py +++ b/snapcraft/commands/remote.py @@ -24,14 +24,17 @@ from pathlib import Path from typing import Any, cast +import craft_application.errors import lazr.restfulclient.errors -from craft_application import errors +from craft_application import errors, launchpad +from craft_application.application import filter_plan from craft_application.commands import ExtensibleCommand from craft_application.errors import RemoteBuildError from craft_application.launchpad.models import Build, BuildState from craft_application.remote.utils import get_build_id from craft_application.util import humanize_list from craft_cli import emit +from craft_platforms import DebianArchitecture from overrides import overrides from snapcraft import models @@ -57,9 +60,16 @@ class RemoteBuildCommand(ExtensibleCommand): architecture are retrieved and will be available in the local filesystem. - If not specified in the snapcraft.yaml file, the list of - architectures to build can be set using the --platforms option. - If both are specified, an error will occur. + If the project contains a ``platforms`` or ``architectures`` key, + then project's build plan is used. The build plan can be filtered + using the ``--platform`` or ``--build-for`` arguments. + + If the project doesn't contain a ``platforms`` or ``architectures`` key, + then the architectures to build for are defined by the ``--platform`` or + ``--build-for`` arguments. + + If there are no architectures defined in the project or as arguments, + then the default behavior is to build for the host architecture. Interrupted remote builds can be resumed using the --recover option, followed by the build number informed when the remote @@ -154,56 +164,17 @@ def _validate(self, parsed_args: argparse.Namespace) -> None: retcode=os.EX_NOPERM, ) - build_fors = parsed_args.remote_build_build_fors or None - platforms = parsed_args.remote_build_platforms or None - parameter = "--build-for" if build_fors else "--platform" if platforms else None - keyword = ( - "architectures" - if self.project._architectures_in_yaml - else "platforms" if self.project.platforms else None - ) - - if keyword and parameter: + if ( + parsed_args.remote_build_platforms + and self.project.get_effective_base() == "core22" + ): raise errors.RemoteBuildError( - f"{parameter!r} cannot be used when {keyword!r} is in the snapcraft.yaml.", - resolution=f"Remove {parameter!r} from the command line or remove {keyword!r} in the snapcraft.yaml.", + "'--platform' cannot be used for core22 snaps.", + resolution="Use '--build-for' instead.", doc_slug="/explanation/remote-build.html", retcode=os.EX_CONFIG, ) - if platforms: - if self.project.get_effective_base() == "core22": - raise errors.RemoteBuildError( - "'--platform' cannot be used for core22 snaps.", - resolution="Use '--build-for' instead.", - doc_slug="/explanation/remote-build.html", - retcode=os.EX_CONFIG, - ) - for platform in platforms: - if platform not in SUPPORTED_ARCHS: - raise errors.RemoteBuildError( - f"Unsupported platform {platform!r}.", - resolution=( - "Use a supported debian architecture. Supported " - f"architectures are: {humanize_list(SUPPORTED_ARCHS, 'and')}" - ), - doc_slug="/explanation/remote-build.html", - retcode=os.EX_CONFIG, - ) - - if build_fors: - for build_for in build_fors: - if build_for not in SUPPORTED_ARCHS: - raise errors.RemoteBuildError( - f"Unsupported build-for architecture {build_for!r}.", - resolution=( - "Use a supported debian architecture. Supported " - f"architectures are: {humanize_list(SUPPORTED_ARCHS, 'and')}" - ), - doc_slug="/explanation/remote-build.html", - retcode=os.EX_CONFIG, - ) - self._validate_single_artifact_per_build_on() def _validate_single_artifact_per_build_on(self) -> None: @@ -274,14 +245,10 @@ def _run( # noqa: PLR0915 [too-many-statements] emit.trace(f"Project directory: {project_dir}") self._validate(parsed_args) - if parsed_args.remote_build_build_fors: - architectures = parsed_args.remote_build_build_fors - elif parsed_args.remote_build_platforms: - architectures = parsed_args.remote_build_platforms - else: - architectures = self._get_project_build_fors() - - emit.debug(f"Architectures to build for: {architectures}") + archs = self._get_archs( + build_fors=parsed_args.remote_build_build_fors, + platforms=parsed_args.remote_build_platforms, + ) if parsed_args.launchpad_timeout: emit.debug(f"Setting timeout to {parsed_args.launchpad_timeout} seconds") @@ -296,10 +263,8 @@ def _run( # noqa: PLR0915 [too-many-statements] "Starting new build. It may take a while to upload large projects." ) try: - builds = builder.start_builds( - project_dir, architectures=architectures or None - ) - except RemoteBuildError: + builds = builder.start_builds(project_dir, architectures=archs) + except (RemoteBuildError, launchpad.LaunchpadError): emit.progress("Starting build failed.", permanent=True) emit.progress("Cleaning up") builder.cleanup() @@ -407,12 +372,95 @@ def _monitor_and_complete( # noqa: PLR0912, PLR0915 ) return return_code - def _get_project_build_fors(self) -> list[str]: - """Get a unique list of build-for architectures from the project. + def _get_archs(self, build_fors: list[str], platforms: list[str]) -> list[str]: + """Get the architectures to build for. + + If the project contains a ``platforms`` or ``architectures`` key, then project's + build plan is used to determine the architectures to build for. The build plan + can be filtered using the ``--platform`` or ``--build-for`` arguments. + + If the project doesn't contain a ``platforms`` or ``architectures`` key, then + the architectures to build for are defined by the ``--platform`` or + ``--build-for`` arguments. + + If there are no architectures defined in the project or as arguments, then the + default behavior is to build for the host architecture. + + :param build_fors: A list of build-for entries. + :param platforms: A list of platforms. + + :raises EmptyBuildPlanError: If the build plan is filtered to an empty list. + :raises RemoteBuildError: If an unsupported architecture is provided. :returns: A list of architectures. """ - build_fors = set({build_info.build_for for build_info in self.build_plan}) - emit.debug(f"Parsed build-for architectures from build plan: {build_fors}") + archs: list[str] = [] + # core22 projects with an `architectures` key will have a corresponding `platforms` + # key when the project is unmarshalled + if self.project.platforms or self.project._architectures_in_yaml: + # if the project has platforms, then the `--platforms` and `--build-for` arguments act as filters + if build_fors: + emit.debug("Filtering the build plan using the '--build-for' argument.") + for build_for in build_fors: + filtered_build_plan = filter_plan( + self.build_plan, + platform=None, + build_for=build_for, + host_arch=None, + ) + archs.extend([info.build_for for info in filtered_build_plan]) + if not archs: + raise craft_application.errors.EmptyBuildPlanError() + elif platforms: + emit.debug("Filtering the build plan using the '--platforms' argument.") + for platform in platforms: + filtered_build_plan = filter_plan( + self.build_plan, + platform=platform, + build_for=None, + host_arch=None, + ) + archs.extend([info.build_for for info in filtered_build_plan]) + if not archs: + raise craft_application.errors.EmptyBuildPlanError() + else: + emit.debug("Using the project's build plan") + archs = [build_info.build_for for build_info in self.build_plan] + # No platforms in the project means '--build-for' and '--platforms' no longer act as filters. + # Instead, they define the architectures to build for. + elif build_fors: + emit.debug("Using '--build-for' as the list of architectures to build for") + _validate_archs(build_fors, "build-for architecture") + archs = build_fors + elif platforms: + emit.debug("Using '--platforms' as the list of architectures to build for") + _validate_archs(platforms, "platform") + archs = platforms + # default is to build for the host architecture + else: + archs = [str(DebianArchitecture.from_host())] + emit.debug( + f"Using host architecture {archs[0]} because no architectures were " + "defined in the project or as a command-line argument." + ) + + emit.debug(f"Architectures to build for: {humanize_list(archs, 'and')}") + return archs - return list(build_fors) + +def _validate_archs(archs: list[str], key_name: str) -> None: + """Validate that a list of architectures are valid. + + :raises RemoteBuildError: If an unsupported architecture is provided. + """ + for arch in archs: + if arch not in SUPPORTED_ARCHS: + raise errors.RemoteBuildError( + f"Unsupported {key_name} {arch!r}.", + resolution=( + "Use a supported debian architecture. Supported " + f"architectures are: {humanize_list(SUPPORTED_ARCHS, 'and')}" + ), + doc_slug="/explanation/remote-build.html", + retcode=os.EX_CONFIG, + ) diff --git a/tests/spread/core22/remote-build/snaps/architectures-filtered/arguments.txt b/tests/spread/core22/remote-build/snaps/architectures-filtered/arguments.txt new file mode 100644 index 0000000000..2bbc7b956e --- /dev/null +++ b/tests/spread/core22/remote-build/snaps/architectures-filtered/arguments.txt @@ -0,0 +1 @@ +--build-for amd64 diff --git a/tests/spread/core22/remote-build/snaps/architectures-filtered/expected-snaps.txt b/tests/spread/core22/remote-build/snaps/architectures-filtered/expected-snaps.txt new file mode 100644 index 0000000000..24a021e27a --- /dev/null +++ b/tests/spread/core22/remote-build/snaps/architectures-filtered/expected-snaps.txt @@ -0,0 +1 @@ +test-snap-architectures-filtered-core22_1.0_amd64.snap diff --git a/tests/spread/core22/remote-build/snaps/architectures-filtered/snapcraft.yaml b/tests/spread/core22/remote-build/snaps/architectures-filtered/snapcraft.yaml new file mode 100644 index 0000000000..d7b5d5a761 --- /dev/null +++ b/tests/spread/core22/remote-build/snaps/architectures-filtered/snapcraft.yaml @@ -0,0 +1,18 @@ +name: test-snap-architectures-filtered-core22 +base: core22 +version: "1.0" +summary: Test snap for remote build +description: Test snap for remote build + +grade: stable +confinement: strict + +architectures: + - build-on: [amd64] + build-for: [amd64] + - build-on: [riscv64] + build-for: [riscv64] + +parts: + my-part: + plugin: nil diff --git a/tests/spread/core22/remote-build/task.yaml b/tests/spread/core22/remote-build/task.yaml index feee247997..3b2d9a08f2 100644 --- a/tests/spread/core22/remote-build/task.yaml +++ b/tests/spread/core22/remote-build/task.yaml @@ -8,6 +8,7 @@ environment: SNAP/all: all SNAP/no_architectures: no-architectures SNAP/architectures: architectures + SNAP/architectures_filtered: architectures-filtered SNAP/build_for_arg: build-for-arg SNAPCRAFT_REMOTE_BUILD_STRATEGY: disable-fallback CREDENTIALS_FILE: "$HOME/.local/share/snapcraft/launchpad-credentials" diff --git a/tests/spread/core24/remote-build/snaps/platforms-filtered/arguments.txt b/tests/spread/core24/remote-build/snaps/platforms-filtered/arguments.txt new file mode 100644 index 0000000000..c2bfcf76fd --- /dev/null +++ b/tests/spread/core24/remote-build/snaps/platforms-filtered/arguments.txt @@ -0,0 +1 @@ +--platform amd64 diff --git a/tests/spread/core24/remote-build/snaps/platforms-filtered/expected-snaps.txt b/tests/spread/core24/remote-build/snaps/platforms-filtered/expected-snaps.txt new file mode 100644 index 0000000000..95f57c66a3 --- /dev/null +++ b/tests/spread/core24/remote-build/snaps/platforms-filtered/expected-snaps.txt @@ -0,0 +1 @@ +test-snap-platforms-filtered-core24_1.0_amd64.snap diff --git a/tests/spread/core24/remote-build/snaps/platforms-filtered/snapcraft.yaml b/tests/spread/core24/remote-build/snaps/platforms-filtered/snapcraft.yaml new file mode 100644 index 0000000000..722045069a --- /dev/null +++ b/tests/spread/core24/remote-build/snaps/platforms-filtered/snapcraft.yaml @@ -0,0 +1,20 @@ +name: test-snap-platforms-filtered-core24 +base: core24 +version: "1.0" +summary: Test snap for remote build +description: Test snap for remote build + +grade: stable +confinement: strict + +platforms: + amd64: + build-on: [amd64] + build-for: [amd64] + riscv64: + build-on: [riscv64] + build-for: [riscv64] + +parts: + my-part: + plugin: nil diff --git a/tests/spread/core24/remote-build/task.yaml b/tests/spread/core24/remote-build/task.yaml index 152d3abec4..28cd41e958 100644 --- a/tests/spread/core24/remote-build/task.yaml +++ b/tests/spread/core24/remote-build/task.yaml @@ -12,6 +12,7 @@ environment: SNAP/platforms: platforms SNAP/no_platforms: no-platforms SNAP/platform_arg: platform-arg + SNAP/platforms_filtered: platforms-filtered SNAP/build_for_arg: build-for-arg CREDENTIALS_FILE: "$HOME/.local/share/snapcraft/launchpad-credentials" CREDENTIALS_FILE/new_credentials: "$HOME/.local/share/snapcraft/launchpad-credentials" diff --git a/tests/unit/commands/test_remote.py b/tests/unit/commands/test_remote.py index 0535031bd0..45ed63ffe6 100644 --- a/tests/unit/commands/test_remote.py +++ b/tests/unit/commands/test_remote.py @@ -618,14 +618,54 @@ def test_build_for_argument( mock_start_builds.assert_called_once_with(ANY, architectures=expected_build_fors) -def test_architecture_defined_twice_error( +@pytest.mark.parametrize( + ("archs", "expected_archs"), + [ + ("amd64", ["amd64"]), + ("riscv64", ["riscv64"]), + ("amd64,riscv64", ["amd64", "riscv64"]), + ], +) +def test_architectures_filter( + mocker, + snapcraft_yaml, + fake_services, + mock_confirm, + mock_remote_builder_fake_build_process, + archs, + expected_archs, +): + """Filter an 'architectures' key with '--build-for'.""" + snapcraft_yaml_dict = { + "base": "core22", + "architectures": [ + {"build-on": ["amd64"], "build-for": ["amd64"]}, + {"build-on": ["riscv64"], "build-for": ["riscv64"]}, + ], + } + snapcraft_yaml(**snapcraft_yaml_dict) + mocker.patch.object( + sys, + "argv", + ["snapcraft", "remote-build", "--build-for", archs], + ) + mock_start_builds = mocker.patch( + "craft_application.services.remotebuild.RemoteBuildService.start_builds" + ) + app = application.create_app() + app.run() + + mock_start_builds.assert_called_once_with(ANY, architectures=expected_archs) + + +def test_architectures_filter_error( capsys, mocker, snapcraft_yaml, fake_services, mock_confirm, ): - """Error if architectures are in the project metadata and as a build argument.""" + """Error if '--build-for' entirely filters the build plan.""" snapcraft_yaml_dict = { "base": "core22", "architectures": [{"build-on": ["riscv64"], "build-for": ["riscv64"]}], @@ -634,35 +674,72 @@ def test_architecture_defined_twice_error( mocker.patch.object( sys, "argv", - ["snapcraft", "remote-build", "--build-for", "riscv64"], + ["snapcraft", "remote-build", "--build-for", "arm64"], ) app = application.create_app() app.run() _, err = capsys.readouterr() + assert "No build matches the current execution environment." in err assert ( - "'--build-for' cannot be used when 'architectures' is in the snapcraft.yaml." - ) in err - assert ( - "Remove '--build-for' from the command line or remove 'architectures' in the snapcraft.yaml." + "Check the project's 'platforms' declaration, and the " + "'--platform' and '--build-for' parameters." ) in err -@pytest.mark.parametrize("base", const.CURRENT_BASES - {"core22", "devel"}) -@pytest.mark.parametrize("argument", ["--build-for", "--platform"]) -def test_platform_defined_twice_error( - base, - argument, +@pytest.mark.parametrize("arg", ["--build-for", "--platform"]) +@pytest.mark.parametrize( + ("archs", "expected_archs"), + [ + ("amd64", ["amd64"]), + ("riscv64", ["riscv64"]), + ("amd64,riscv64", ["amd64", "riscv64"]), + ], +) +def test_platforms_filter( + mocker, + snapcraft_yaml, + fake_services, + mock_confirm, + mock_remote_builder_fake_build_process, + arg, + archs, + expected_archs, +): + """Filter a 'platforms' key with '--build-for' or '--platform'.""" + snapcraft_yaml_dict = { + "base": "core24", + "platforms": { + "amd64": {"build-on": "amd64", "build-for": "amd64"}, + "riscv64": {"build-on": "riscv64", "build-for": "riscv64"}, + }, + } + snapcraft_yaml(**snapcraft_yaml_dict) + mocker.patch.object( + sys, + "argv", + ["snapcraft", "remote-build", arg, archs], + ) + mock_start_builds = mocker.patch( + "craft_application.services.remotebuild.RemoteBuildService.start_builds" + ) + app = application.create_app() + app.run() + + mock_start_builds.assert_called_once_with(ANY, architectures=expected_archs) + + +def test_platforms_filter_error( capsys, mocker, snapcraft_yaml, fake_services, mock_confirm, ): - """Error if platforms are in the project metadata and as a build argument.""" + """Error if '--build-for' entirely filters the build plan.""" snapcraft_yaml_dict = { - "base": base, + "base": "core24", "platforms": { "riscv64": {"build-on": "riscv64", "build-for": "riscv64"}, }, @@ -671,18 +748,17 @@ def test_platform_defined_twice_error( mocker.patch.object( sys, "argv", - ["snapcraft", "remote-build", argument, "riscv64"], + ["snapcraft", "remote-build", "--build-for", "arm64"], ) app = application.create_app() app.run() _, err = capsys.readouterr() + assert "No build matches the current execution environment." in err assert ( - f"{argument!r} cannot be used when 'platforms' is in the snapcraft.yaml." - ) in err - assert ( - f"Remove {argument!r} from the command line or remove 'platforms' in the snapcraft.yaml." + "Check the project's 'platforms' declaration, and the " + "'--platform' and '--build-for' parameters." ) in err diff --git a/tests/unit/test_application.py b/tests/unit/test_application.py index ea86a2b518..96510664a3 100644 --- a/tests/unit/test_application.py +++ b/tests/unit/test_application.py @@ -21,6 +21,7 @@ import sys from textwrap import dedent +import craft_application.launchpad import craft_application.remote import craft_cli import craft_parts.plugins @@ -704,11 +705,18 @@ def test_store_key_error(mocker, capsys): ) -def test_remote_build_error(mocker, capsys): +@pytest.mark.parametrize( + "error_class", + [ + craft_application.remote.RemoteBuildError, + craft_application.launchpad.LaunchpadError, + ], +) +def test_remote_build_error(mocker, capsys, error_class): """Catch remote build errors and include a documentation link.""" mocker.patch( "snapcraft.application.Application._run_inner", - side_effect=craft_application.remote.RemoteBuildError(message="test-error"), + side_effect=error_class("test-error"), ) return_code = application.main()