-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: make build plans for snaps with platforms #21
Conversation
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.
Looking good! I found some missing corner cases for some of snapcraft's esoteric base/build-base definitions
3e0eaa2
to
e8477bc
Compare
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.
Minor feedback on type: snapd
and the docs but it looks good! Nice job catching the complexities of base/build-base combinations.
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.
One more thought - base: devel
should be an error, I don't think it is:
def test_base_devel_error():
"""'base: devel' raises an error."""
build_plan = _build.get_platforms_snap_build_plan(
base="devel",
platforms={"amd64": None},
build_base=None,
snap_type="app",
)
@mr-cal I've added the devel error test - it simply calls the base unknown or invalid. |
Re-requesting reviews from @mr-cal and @cmatsuoka because I've reworked this following our discussion |
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.
Looks great!
Note to self: With this latest design, we are moving some of the validation to snapcraft because craft-platforms will accept arbitrary base names. For example, snapcraft is responsible for raising an error for |
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.
Nice, this seems to follow all the rules established for snap bases and build-bases.
Can you add a PR description? |
38eb412
to
9be2ee9
Compare
tox
?Adds a function to make build plans for snaps that use the
platforms
keyword.Fixes #5
Requires #39
CRAFT-3008