fix(remote): parse comma-separated architectures

snapcraft/commands/remote.py

@@ -1,6 +1,6 @@
 # -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
 #
-# Copyright 2022-2023 Canonical Ltd.
+# Copyright 2022-2024 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
@@ -91,18 +91,17 @@ def fill_parser(self, parser: argparse.ArgumentParser) -> None:
         parser.add_argument(
             "--status", action="store_true", help="display remote build status"
         )
-        parser_target = parser.add_mutually_exclusive_group()
-        parser_target.add_argument(
+        parser.add_argument(
             "--build-on",
+            type=lambda arg: [arch.strip() for arch in arg.split(",")],
             metavar="arch",
-            nargs="+",
             help=HIDDEN,
         )
-        parser_target.add_argument(
+        parser.add_argument(
             "--build-for",
+            type=lambda arg: [arch.strip() for arch in arg.split(",")],
             metavar="arch",
-            nargs="+",
-            help="architecture to build for",
+            help="comma-separated list of architectures to build for",
         )
         parser.add_argument(
             "--build-id", metavar="build-id", help="specific build id to retrieve"
@@ -141,7 +140,6 @@ def run(self, parsed_args: argparse.Namespace) -> None:
 
         if parsed_args.build_on:
             emit.message("Use --build-for instead of --build-on")
-            parsed_args.build_for = parsed_args.build_on
 
         if not parsed_args.launchpad_accept_public_upload and not confirm_with_user(
             _CONFIRMATION_PROMPT
@@ -381,11 +379,24 @@ def _determine_architectures(self) -> List[str]:
         The build architectures can be set via the `--build-on` parameter or determined
         from the build-on architectures listed in the project's snapcraft.yaml.
 
+        To retain backwards compatibility, `--build-for` can also be used to
+        set the architectures.
+
         :returns: A list of architectures.
 
         :raises SnapcraftError: If `--build-on` was provided and architectures are
         defined in the project's snapcraft.yaml.
+        :raises SnapcraftError: If `--build-on` and `--build-for` are both provided.
         """
+        # argparse's `add_mutually_exclusive_group()` cannot be used because
+        # ArgumentParsingErrors executes the legacy remote-builder before this module
+        # can decide if the project is allowed to use the legacy remote-builder
+        if self._parsed_args.build_on and self._parsed_args.build_for:
+            raise SnapcraftError(
+                # use the same error as argparse produces for consistency
+                "Error: argument --build-for: not allowed with argument --build-on"
+            )
+
         project_architectures = self._get_project_build_on_architectures()
         if project_architectures and self._parsed_args.build_for:
             raise SnapcraftError(
@@ -395,6 +406,8 @@ def _determine_architectures(self) -> List[str]:
 
         if project_architectures:
             archs = project_architectures
+        elif self._parsed_args.build_on:
+            archs = self._parsed_args.build_on
         elif self._parsed_args.build_for:
             archs = self._parsed_args.build_for
         else:

tests/unit/commands/test_remote.py

@@ -1,6 +1,6 @@
 # -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
 #
-# Copyright 2022-2023 Canonical Ltd.
+# Copyright 2022-2024 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
@@ -152,6 +152,43 @@ def test_command_accept_upload(
     mock_run_new_or_fallback_remote_build.assert_called_once()
 
 
+@pytest.mark.parametrize(
+    "create_snapcraft_yaml", CURRENT_BASES | LEGACY_BASES, indirect=True
+)
+@pytest.mark.usefixtures(
+    "create_snapcraft_yaml", "mock_confirm", "use_new_remote_build"
+)
+def test_command_new_build_arguments_mutually_exclusive(capsys, mocker):
+    """`--build-for` and `--build-on` are mutually exclusive in the new remote-build."""
+    mocker.patch.object(
+        sys,
+        "argv",
+        ["snapcraft", "remote-build", "--build-on", "amd64", "--build-for", "arm64"],
+    )
+
+    cli.run()
+
+    _, err = capsys.readouterr()
+    assert "Error: argument --build-for: not allowed with argument --build-on" in err
+
+
+@pytest.mark.parametrize(
+    "create_snapcraft_yaml", LEGACY_BASES | {"core22"}, indirect=True
+)
+@pytest.mark.usefixtures("create_snapcraft_yaml", "mock_confirm")
+def test_command_legacy_build_arguments_not_mutually_exclusive(mocker, mock_run_legacy):
+    """`--build-for` and `--build-on` are not mutually exclusive for legacy."""
+    mocker.patch.object(
+        sys,
+        "argv",
+        ["snapcraft", "remote-build", "--build-on", "amd64", "--build-for", "arm64"],
+    )
+
+    cli.run()
+
+    mock_run_legacy.assert_called_once()
+
+
 @pytest.mark.parametrize(
     "create_snapcraft_yaml", CURRENT_BASES | LEGACY_BASES, indirect=True
 )
@@ -710,13 +747,56 @@ def test_determine_architectures_host_arch(mocker, mock_remote_builder):
     )
 
 
+@pytest.mark.parametrize("build_flag", ["--build-for", "--build-on"])
+@pytest.mark.parametrize(
+    ("archs", "expected_archs"),
+    [
+        ("amd64", ["amd64"]),
+        ("amd64,arm64", ["amd64", "arm64"]),
+        ("amd64,amd64,arm64 ", ["amd64", "amd64", "arm64"]),
+    ],
+)
+@pytest.mark.parametrize(
+    "create_snapcraft_yaml", CURRENT_BASES | LEGACY_BASES, indirect=True
+)
+@pytest.mark.usefixtures(
+    "create_snapcraft_yaml", "mock_confirm", "use_new_remote_build"
+)
+def test_determine_architectures_provided_by_user_duplicate_arguments(
+    build_flag, archs, expected_archs, mocker, mock_remote_builder
+):
+    """Argparse should only consider the last argument provided for build flags."""
+    mocker.patch.object(
+        sys,
+        "argv",
+        # `--build-{for|on} armhf` should get silently ignored by argparse
+        ["snapcraft", "remote-build", build_flag, "armhf", build_flag, archs],
+    )
+
+    cli.run()
+
+    mock_remote_builder.assert_called_with(
+        app_name="snapcraft",
+        build_id=None,
+        project_name="mytest",
+        architectures=expected_archs,
+        project_dir=Path(),
+        timeout=0,
+    )
+
+
+@pytest.mark.parametrize("build_flag", ["--build-for", "--build-on"])
 @pytest.mark.parametrize(
-    ("args", "expected_archs"),
+    ("archs", "expected_archs"),
     [
-        (["--build-for", "amd64"], ["amd64"]),
-        (["--build-for", "amd64", "arm64"], ["amd64", "arm64"]),
+        ("amd64", ["amd64"]),
+        ("amd64,arm64", ["amd64", "arm64"]),
+        ("amd64, arm64", ["amd64", "arm64"]),
+        ("amd64,arm64 ", ["amd64", "arm64"]),
+        ("amd64,arm64,armhf", ["amd64", "arm64", "armhf"]),
+        (" amd64 , arm64 , armhf ", ["amd64", "arm64", "armhf"]),
         # launchpad will accept and ignore duplicates
-        (["--build-for", "amd64", "amd64"], ["amd64", "amd64"]),
+        (" amd64 , arm64 , arm64 ", ["amd64", "arm64", "arm64"]),
     ],
 )
 @pytest.mark.parametrize(
@@ -726,10 +806,10 @@ def test_determine_architectures_host_arch(mocker, mock_remote_builder):
     "create_snapcraft_yaml", "mock_confirm", "use_new_remote_build"
 )
 def test_determine_architectures_provided_by_user(
-    args, expected_archs, mocker, mock_remote_builder
+    build_flag, archs, expected_archs, mocker, mock_remote_builder
 ):
     """Use architectures provided by the user."""
-    mocker.patch.object(sys, "argv", ["snapcraft", "remote-build"] + args)
+    mocker.patch.object(sys, "argv", ["snapcraft", "remote-build", build_flag, archs])
 
     cli.run()