-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: use craft-platforms for build plans #1894
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,11 +29,13 @@ | |
cast, | ||
) | ||
|
||
import craft_platforms | ||
import pydantic | ||
import pydantic.v1 | ||
from craft_application import errors, models, util | ||
from craft_application.util import safe_yaml_load | ||
from craft_cli import CraftError, emit | ||
from craft_platforms import charm | ||
from craft_providers import bases | ||
from pydantic import dataclasses | ||
from typing_extensions import Self | ||
|
@@ -345,38 +347,28 @@ def get_build_plan(self) -> list[models.BuildInfo]: | |
|
||
if self.platforms is None: | ||
raise CraftError("Must define at least one platform.") | ||
build_infos = [] | ||
for platform_name, platform in self.platforms.items(): | ||
if platform is None: | ||
if platform_name not in const.SUPPORTED_ARCHITECTURES: | ||
raise CraftError( | ||
f"Invalid platform {platform_name}.", | ||
details="A platform name must either be a valid architecture name or the " | ||
"platform must specify one or more build-on and build-for architectures.", | ||
) | ||
build_infos.append( | ||
models.BuildInfo( | ||
platform_name, | ||
build_on=platform_name, | ||
build_for=platform_name, | ||
base=base, | ||
) | ||
) | ||
else: | ||
# TODO: this should go to craft-platforms, so silence mypy for now. | ||
for build_on in platform.build_on: # type: ignore[union-attr] | ||
build_infos.extend( | ||
[ | ||
models.BuildInfo( | ||
platform_name, | ||
build_on=str(build_on), | ||
build_for=str(build_for), | ||
base=base, | ||
) | ||
for build_for in platform.build_for # type: ignore[union-attr] | ||
] | ||
) | ||
return build_infos | ||
platforms = cast( | ||
# https://github.com/canonical/craft-platforms/issues/43 | ||
craft_platforms.Platforms, # pyright: ignore[reportPrivateImportUsage] | ||
{ | ||
name: (platform.marshal() if platform else None) | ||
for name, platform in self.platforms.items() | ||
}, | ||
) | ||
build_infos = charm.get_platforms_charm_build_plan( | ||
base=self.base, | ||
build_base=self.build_base, | ||
platforms=platforms, | ||
) | ||
return [ | ||
models.BuildInfo( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: maybe craft-application's BuildInfo should have a staticmethod to convert a craft-platform's BuildInfo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both of your comments here are things I'd like to address in canonical/craft-application#391 :-) |
||
platform=info.platform, | ||
build_on=str(info.build_on), | ||
build_for=str(info.build_for), | ||
base=base, | ||
) | ||
for info in build_infos | ||
] | ||
|
||
|
||
class CharmcraftProject(models.Project, metaclass=abc.ABCMeta): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just me rambling but it bothers me a bit that we have to do this 'round-trip' where we unmarshal the project, which includes validation (necessary) and possibly transformation (dangerous), and then re-marshal to pass to craft-platforms. This makes the assembling of the build plan a little different between the application and systems that just use craft-platforms (like launchpad), and makes us still need "all" of this code here.
I don't have any good suggestions to address this though; maybe the base BuildPlanner class should retain the original values of base, build-base and platforms somewhere?