launchpad-reviewers team mailing list archive

[Alex Lowe <mp+483337@xxxxxxxxxxxxxxxxxx>]

Review: Approve

Looks great, thanks!

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):
> +            base = f"{base['name']}@{base['channel']}"
> +
> +        # Generate exhaustive build plan
> +        try:
> +            exhaustive_build_plan = charm.get_platforms_charm_build_plan(
> +                base=base,
> +                platforms=platforms,
> +                build_base=build_base,
> +            )
> +
> +        except (CraftPlatformsError, ValueError) as e:

Thanks!

> +            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
> +                    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.


-- 
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.