launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32339
Re: [Merge] ~alvarocs/launchpad:charms-craft-platforms into launchpad:master
All comments addressed :). All fixed except for the ValueError one, which may be a bug in craft-platforms
Diff comments:
> diff --git a/lib/lp/charms/adapters/buildarch.py b/lib/lp/charms/adapters/buildarch.py
> index 66bc20e..7a63117 100644
> --- a/lib/lp/charms/adapters/buildarch.py
> +++ b/lib/lp/charms/adapters/buildarch.py
> @@ -219,18 +156,139 @@ def determine_instances_to_build(
> charmcraft.yaml does not explicitly declare any bases.
> :return: A list of `DistroArchSeries`.
> """
> +
> + def _check_duplicate_run_on(configs):
> + """Ensure that multiple `run-on` items don't overlap;
> + this is ambiguous and forbidden by charmcraft.
> +
> + :param configs: List of CharmBaseConfiguration objects
> + :raises DuplicateRunOnError if any architecture appears in
> + multiple run-on configurations
> + """
> +
> + run_ons = Counter()
> + for config in configs:
> + run_ons.update(config.run_on)
> + duplicates = {config for config, count in run_ons.items() if count > 1}
> + if duplicates:
> + raise DuplicateRunOnError(duplicates)
> +
> + def _process_configs_to_instances(configs, supported_arches):
> + """Convert base configurations to buildable instances.
> +
> + Filters configurations to only include supported architectures and
> + distro series.
> +
> + :param configs: List of CharmBaseConfiguration objects
> + :param supported_arches: List of supported DistroArchSeries
> + :return: OrderedDict of filtered DistroArchSeries instances
> + """
> +
> + _check_duplicate_run_on(configs)
> + instances = OrderedDict()
> + for config in configs:
> + # Charms are allowed to declare that they build on architectures
> + # that Launchpad doesn't currently support (perhaps they're
> + # upcoming, or perhaps they used to be supported). We just ignore
> + # those.
> + for build_on in config.build_on:
> + for das in supported_arches:
> + if das.distroseries.distribution.name != build_on.name:
> + continue
> + if build_on.channel not in (
> + das.distroseries.name,
> + das.distroseries.version,
> + ):
> + continue
> + if build_on.architectures is None:
> + # Build on all supported architectures for the
> + # requested series.
> + instances[das] = None
> + elif das.architecturetag in build_on.architectures:
> + # Build on the first matching supported architecture
> + # for the requested series.
> + instances[das] = None
> + break
> + else:
> + continue
> + break
> + return instances
> +
> from lp.charms.model.charmrecipe import is_unified_format
>
> + # 1) Charm with 'bases' format
> bases_list = charmcraft_data.get("bases")
> -
> if bases_list:
> configs = [
> CharmBaseConfiguration.from_dict(item) for item in bases_list
> ]
> + instances = _process_configs_to_instances(configs, supported_arches)
> + instances_to_build = [(None, das) for das in instances.keys()]
> + return instances_to_build
> +
> + # 2) Charm with unified format
> elif is_unified_format(charmcraft_data):
> - configs = UnifiedCharmBaseConfiguration.from_dict(
> - charmcraft_data, supported_arches
> - )
> +
> + base = charmcraft_data["base"]
> + build_base = charmcraft_data.get("build-base", None)
> + platforms = charmcraft_data.get("platforms", None)
> + # Reformat base dict style
> + if isinstance(base, dict):
Raised an error when base is a dict
> + base = f"{base['name']}@{base['channel']}"
> +
> + # Generate exhaustive build plan
> + try:
> + exhaustive_build_plan = charm.get_platforms_charm_build_plan(
Implemented get_build_plan
> + base=base,
> + platforms=platforms,
> + build_base=build_base,
> + )
> +
> + except (CraftPlatformsError, ValueError) as e:
Yes, for a couple of tests I got ValueError, so I added it to the except too. You can see the logs here: https://pastebin.canonical.com/p/Zp3JxqqSDM/, and these are the tests: https://pastebin.canonical.com/p/GS44fjgGFH/
> + message = getattr(e, "message", str(e))
> + resolution = getattr(e, "resolution", None)
> + raise CraftPlatformsBuildPlanError(
> + f"Failed to compute the build plan for base={base}, "
> + f"build base={build_base}, platforms={platforms}: {message}",
> + resolution=resolution,
> + )
> + # Filter exhaustive build plan
> + filtered_plan = []
> + for info in exhaustive_build_plan:
> + for das in supported_arches:
> + # Compare DAS-BuildInfo and append if match
> + if (
> + das.distroseries.distribution.name
> + == info.build_base.distribution
> + and info.build_base.series
updated to info.build_base.series == das.distroseries.version
> + in (das.distroseries.name, das.distroseries.version)
> + and das.architecturetag == info.build_on.value
> + ):
> + filtered_plan.append((info, das))
> + break
> + # Group by platform
> + platform_plans = {}
> + for info, das in filtered_plan:
> + platform_plans.setdefault(info.platform, []).append((info, das))
> + # Pick one BuildInfo per platform
> + instances_to_build = []
> + for _platform, pairs in platform_plans.items():
> + # One way of building for that platform, i.e one (info, das)
> + if len(pairs) == 1:
> + instances_to_build.append(pairs[0])
> + continue
> + # More than one way of building for that platform
> + for info, das in pairs:
> + # Pick the native build
> + if info.build_on == info.build_for:
> + instances_to_build.append((info, das))
> + break
> + # Pick first one if none are native
> + else:
> + instances_to_build.append(pairs[0])
> + return instances_to_build
> +
> + # 3) Charms with no bases specified
> else:
> # If no bases are specified, build one for each supported
> # architecture for the default series.
> diff --git a/lib/lp/charms/adapters/tests/test_buildarch.py b/lib/lp/charms/adapters/tests/test_buildarch.py
> index a87528d..d14ca60 100644
> --- a/lib/lp/charms/adapters/tests/test_buildarch.py
> +++ b/lib/lp/charms/adapters/tests/test_buildarch.py
> @@ -399,6 +390,178 @@ class TestDetermineInstancesToBuild(WithScenarios, TestCaseWithFactory):
> ],
> },
> ),
> + # Unified format scenarios
> + (
> + "unified single platform",
> + {
> + "bases": None,
> + "base": "ubuntu@20.04",
> + "platforms": {
> + "ubuntu-amd64": {
> + "build-on": ["amd64"],
> + "build-for": ["amd64"],
> + }
> + },
> + "expected": [("20.04", "amd64")],
> + },
> + ),
> + (
> + "unified multi-platforms",
> + {
> + "bases": None,
> + "base": {"name": "ubuntu", "channel": "20.04"},
> + "platforms": {
> + "ubuntu-amd64": {
> + "build-on": ["amd64"],
> + "build-for": ["amd64"],
> + },
> + "ubuntu-arm64": {
> + "build-on": ["arm64"],
> + "build-for": ["arm64"],
> + },
> + },
> + "expected": [("20.04", "amd64"), ("20.04", "arm64")],
> + },
> + ),
> + (
> + "unified mismatched platform base",
> + {
> + "bases": None,
> + "base": "ubuntu@20.04",
> + "platforms": {
> + "ubuntu:18.04": {
Right, I'll remove this test so that it does not break in the future
> + "build-on": ["amd64"],
> + "build-for": ["amd64"],
> + },
> + },
> + "expected_exception": MatchesException(
> + CraftPlatformsBuildPlanError,
> + r"Failed to compute the build plan for base=.+",
> + ),
> + },
> + ),
> + (
> + "unified build-for all single platform",
> + {
> + "bases": None,
> + "base": "ubuntu@20.04",
> + "platforms": {
> + "ubuntu-amd64": {
> + "build-on": ["amd64"],
> + "build-for": ["all"],
> + },
> + },
> + "expected": [("20.04", "amd64")],
> + },
> + ),
> + (
> + "unified multiple build-for all platforms (invalid)",
> + {
> + "bases": None,
> + "base": "ubuntu@20.04",
> + "platforms": {
> + "ubuntu-amd64": {
> + "build-on": ["amd64"],
> + "build-for": ["all"],
> + },
> + "ubuntu-arm64": {
> + "build-on": ["arm64"],
> + "build-for": ["all"],
> + },
> + },
> + "expected": [("20.04", "amd64"), ("20.04", "arm64")],
> + },
> + ),
> + (
> + "unified without platforms, builds for all allowed archs",
> + {
> + "bases": None,
> + "base": "ubuntu@20.04",
> + "platforms": None,
> + "expected": [
> + ("20.04", "amd64"),
> + ("20.04", "arm64"),
> + ("20.04", "riscv64"),
> + ],
> + },
> + ),
> + (
> + "unified without supported series",
> + {
> + "bases": None,
> + "base": "ubuntu@22.04",
> + "platforms": {
> + "ubuntu-amd64": {
> + "build-on": ["amd64"],
> + "build-for": ["amd64"],
> + },
> + },
> + "expected": [],
> + },
> + ),
> + (
> + "unified invalid platform name",
> + {
> + "bases": None,
> + "base": "ubuntu@20.04",
> + "platforms": {"not-a-valid-arch": None},
> + "expected_exception": MatchesException(
> + CraftPlatformsBuildPlanError,
> + r"Failed to compute the build plan for base=.+",
> + ),
> + },
> + ),
> + (
> + "unified missing build-on",
> + {
> + "bases": None,
> + "base": "ubuntu@20.04",
> + "platforms": {
> + "ubuntu-amd64": {
> + "build-on": [],
> + "build-for": ["amd64"],
> + },
> + },
> + "expected": [],
> + },
> + ),
> + (
> + "unified cross-compiling combinations, taking the first one",
> + {
> + "bases": None,
> + "base": "ubuntu@20.04",
> + "platforms": {
> + "ubuntu-cross": {
> + "build-on": ["amd64", "arm64"],
> + "build-for": ["riscv64"],
> + },
> + },
> + "expected": [("20.04", "amd64")],
> + },
> + ),
> + (
> + "unified base only with unsupported series",
> + {
> + "bases": None,
> + "base": "ubuntu@99.99",
> + "platforms": None,
> + "expected": [],
> + },
> + ),
> + # No bases specified scenario
> + (
> + "no bases specified",
> + {
> + "bases": None,
> + "base": None,
> + "platforms": None,
> + "expected": [
> + ("20.04", "amd64"),
> + ("20.04", "arm64"),
> + ("20.04", "riscv64"),
> + ],
> + },
> + ),
> ]
>
> def test_parser(self):
--
https://code.launchpad.net/~alvarocs/launchpad/+git/launchpad/+merge/483337
Your team Launchpad code reviewers is requested to review the proposed merge of ~alvarocs/launchpad:charms-craft-platforms into launchpad:master.
References
