launchpad-reviewers team mailing list archive

[Quentin Debhi <mp+474016@xxxxxxxxxxxxxxxxxx>]

thanks for the very thorough review, i hope i made sense in my explanation

if the e2e tests at the end end up not working, i'll go back to copy pasting the older parsers
but atm, this feels like this parser respects the current expectations of recent *craft yaml's more than the previous ones (or at least tries to ^^')

Diff comments:

> diff --git a/lib/lp/crafts/adapters/buildarch.py b/lib/lp/crafts/adapters/buildarch.py
> new file mode 100644
> index 0000000..62dbe91
> --- /dev/null
> +++ b/lib/lp/crafts/adapters/buildarch.py
> @@ -0,0 +1,229 @@
> +# Copyright 2024 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__all__ = [
> +    "determine_instances_to_build",
> +]
> +
> +
> +import json
> +import re
> +
> +
> +class CraftBasesParserError(Exception):
> +    """Base class for all exceptions in this module."""
> +
> +
> +class MissingPropertyError(CraftBasesParserError):
> +    """Error for when an expected property is not present in the YAML."""
> +
> +    def __init__(self, prop, msg=None):
> +        if msg is None:
> +            msg = f"Craft specification is missing the {prop!r} property"
> +        super().__init__(msg)
> +        self.property = prop
> +
> +
> +class BadPropertyError(CraftBasesParserError):
> +    """Error for when a YAML property is malformed in some way."""
> +
> +
> +class CraftBase:
> +    """A single base in sourcecraft.yaml."""
> +
> +    def __init__(self, name, channel, architectures=None):
> +        self.name = name
> +        if not isinstance(channel, str):
> +            raise BadPropertyError(
> +                "Channel {!r} is not a string (missing quotes?)".format(
> +                    channel
> +                )
> +            )
> +        self.channel = channel
> +        self.architectures = architectures
> +
> +    @classmethod
> +    def from_dict(cls, properties):
> +        """Create a new base from a dict."""
> +        try:
> +            name = properties["name"]
> +        except KeyError:
> +            raise MissingPropertyError("name")
> +        try:
> +            channel = properties["channel"]
> +        except KeyError:
> +            raise MissingPropertyError("channel")
> +        return cls(
> +            name=name,
> +            channel=channel,
> +            architectures=properties.get("architectures"),
> +        )
> +
> +    def __eq__(self, other):
> +        return (
> +            self.name == other.name
> +            and self.channel == other.channel
> +            and self.architectures == other.architectures
> +        )
> +
> +    def __ne__(self, other):
> +        return not self == other
> +
> +    def __hash__(self):
> +        return hash((self.name, self.channel, tuple(self.architectures)))
> +
> +    def __str__(self):
> +        return "{} {} {}".format(
> +            self.name, self.channel, json.dumps(self.architectures)
> +        )
> +
> +
> +class CraftBaseConfiguration:
> +    """A configuration in sourcecraft.yaml."""
> +
> +    def __init__(self, build_on, build_for=None):
> +        self.build_on = build_on
> +        self.build_for = list(build_on) if build_for is None else build_for
> +
> +    @classmethod
> +    def from_dict(
> +        cls, sourcecraft_data, supported_arches, requested_architectures
> +    ):
> +        base = sourcecraft_data.get("base")
> +        if not base:

this will apply for all other comments, but `sourcecraft.yaml` does not have a reference yet
but according to Dariusz, it is similar to the one for `rockcraft.yaml`
https://documentation.ubuntu.com/rockcraft/en/latest/reference/rockcraft.yaml/

following strictly what i see in there, it says:
```
base
Type: One of ubuntu@20.04 | ubuntu@22.04 | ubuntu@24.04 | bare
Required: Yes

build-base
Type: One of ubuntu@20.04 | ubuntu@22.04 | ubuntu@24.04 | devel
Required: Yes, if base is bare
 - otherwise it is optional and defaults to the value of base
```

so it might be a shortcut to check `build-base` first, but i'd say this is correct this way?

> +            raise MissingPropertyError("base")
> +
> +        if isinstance(base, str):
> +            if base == "bare" and "build-base" not in sourcecraft_data:
> +                raise MissingPropertyError(
> +                    "build-base",
> +                    "If base is 'bare', then build-base must be specified",
> +                )
> +            if base == "bare":
> +                base = sourcecraft_data["build-base"]
> +            # Expected short-form value looks like 'ubuntu@24.04'
> +            match = re.match(r"(.+)@(.+)", base)
> +            if not match:
> +                raise BadPropertyError(
> +                    f"Invalid value for base '{base}'. Expected value should "
> +                    "be like 'ubuntu@24.04'"
> +                )
> +            base_name, base_channel = match.groups()
> +
> +        platforms = sourcecraft_data.get("platforms")
> +        if not platforms:
> +            raise MissingPropertyError("platforms")
> +
> +        configs = []
> +        supported_arch_names = {
> +            das.architecturetag for das in supported_arches
> +        }
> +
> +        for platform, configuration in platforms.items():
> +            if (
> +                requested_architectures
> +                and platform not in requested_architectures

i don't really know at which level the `requested_architectures` is defined, but that is not inside the yaml file

this would be added somewhere else (a recipe i think?) and it would be intersected with the yaml file when requesting the build (this parameter defaults to None)

you can look at this particular test for rocks:
`test_requestBuildsFromJob_architectures_parameter()` inside `lib/lp/rocks/tests/test_rockrecipe.py`

TLDR, they don't have to define both
ig this would be used when someone wants to exclude a certain architecture from their build request when they cannot edit the yaml file itself

> +            ):
> +                continue
> +
> +            build_on = []
> +            build_for = None
> +
> +            if configuration:
> +                if "build-for" in configuration:
> +                    build_for = configuration["build-for"]
> +                    if isinstance(build_for, list):
> +                        if len(build_for) != 1:
> +                            raise BadPropertyError(
> +                                f"build-for must be a single string or a list "

makes sense, added it

> +                                f"with exactly one element for platform "
> +                                f"{platform}"
> +                            )
> +                        build_for = build_for[0]
> +                    if build_for not in supported_arch_names:
> +                        raise BadPropertyError(
> +                            f"Unsupported architecture {build_for} in "
> +                            f"platform {platform}"
> +                        )
> +                elif platform not in supported_arch_names:

the `configuration` is `build-on` and `build-for` fields
and this is what the reference says:

```
platforms.<entry>.build-on
Type: list[string]
Required: Yes, if build-for is specified or if <entry> is not a supported architecture name.

Host architectures where the rock can be built. Defaults to <entry> if that is a valid, supported architecture name.


platforms.<entry>.build-for
Type: string | list[string]
Required: Yes, if <entry> is not a supported architecture name.

Target architecture the rock will be built for. Defaults to <entry> that is a valid, supported architecture name.
```

TLDR, we allow to omit them since they get defaulted to whatever the platform is (if it's valid)
and the code was doing exactly that:
```py
if "build-for" in configuration`:
...
elif platform not in supported_arch_names:
```

that still needed some arrangements so i fixed it

> +                    raise MissingPropertyError(
> +                        f"build-for is required for unsupported architecture "
> +                        f"{platform}"
> +                    )
> +
> +                if "build-on" in configuration:
> +                    build_on = configuration["build-on"]
> +                    if isinstance(build_on, str):
> +                        build_on = [build_on]
> +                    for arch in build_on:
> +                        if arch not in supported_arch_names:
> +                            raise BadPropertyError(
> +                                f"Unsupported architecture {arch} in platform "
> +                                f"{platform}"
> +                            )
> +                elif platform not in supported_arch_names:
> +                    raise MissingPropertyError(
> +                        f"build-on is required for unsupported architecture "
> +                        f"{platform}"
> +                    )
> +
> +                for arch in build_on:
> +                    if arch not in supported_arch_names:
> +                        raise BadPropertyError(
> +                            f"Unsupported architecture {arch} in platform "
> +                            f"{platform}"
> +                        )
> +
> +            if not build_on and platform in supported_arch_names:
> +                build_on = [platform]
> +
> +            if build_for is None:
> +                build_for = (
> +                    platform if platform in supported_arch_names else None
> +                )
> +
> +            if build_for is None:

fixed

> +                raise BadPropertyError(
> +                    f"Unable to determine build-for for platform {platform}"
> +                )
> +
> +            build_on = [
> +                CraftBase(base_name, base_channel, architecture)
> +                for architecture in build_on
> +            ]
> +            build_for = CraftBase(base_name, base_channel, build_for)
> +
> +            configs.append(cls(build_on, [build_for]))
> +
> +        return configs
> +
> +
> +def determine_instances_to_build(
> +    sourcecraft_data, supported_arches, requested_architectures=None
> +):
> +    """Return a list of instances to build based on sourcecraft.yaml.
> +
> +    :param sourcecraft_data: A parsed sourcecraft.yaml.
> +    :param supported_arches: An ordered list of all `DistroArchSeries` that
> +        we can create builds for. Note that these may span multiple
> +        `DistroSeries`.
> +    :return: A list of `DistroArchSeries`.
> +    """
> +    configs = CraftBaseConfiguration.from_dict(
> +        sourcecraft_data, supported_arches, requested_architectures
> +    )
> +
> +    instances = []
> +    for config in configs:
> +        for build_on in config.build_on:
> +            for das in supported_arches:
> +                if (

idk if the order is important, but if it is we can use a simple dict now (order of insertion is preserved)
sets are unordered in python

so out of consistency with the others, i'll use a dict for now, thanks ^^

> +                    das.distroseries.distribution.name == build_on.name
> +                    and build_on.channel
> +                    in (das.distroseries.name, das.distroseries.version)
> +                    and das.architecturetag in build_on.architectures
> +                ):
> +                    instances.append(das)
> +                    break
> +
> +    return instances


-- 
https://code.launchpad.net/~ruinedyourlife/launchpad/+git/launchpad/+merge/474016
Your team Launchpad code reviewers is requested to review the proposed merge of ~ruinedyourlife/launchpad:add-sourcecraft-yaml-parser into launchpad:master.