Skip to content

feat: allow devel in multi-base charm builds - #217

Open
lengau with Copilot wants to merge 10 commits into
mainfrom
copilot/allow-devel-in-multi-base-builds
Open

feat: allow devel in multi-base charm builds#217
lengau with Copilot wants to merge 10 commits into
mainfrom
copilot/allow-devel-in-multi-base-builds

Conversation

Copilot AI commented Apr 21, 2026

Copy link
Copy Markdown
Contributor
  • Explore codebase and understand the issue
  • Fix _validate_base_definition to allow devel series in build-on entries
  • Fix get_platforms_charm_build_plan to use per-entry build base when present
  • Add test cases for the new devel build-on functionality
  • Update the hypothesis fuzz test filter to allow valid devel build-on cases
  • Reject mixed devel+stable build-on (consistent with rejecting two different stable bases)
  • Fix Hypothesis filter bugs: vectorise build-on/build-for scalars, reject mixed devel+stable within filter
  • Fix linting issues: use Set[Optional[str]] for Python 3.8 compat, fix ruff format violations
  • Reject devel+base-less mixing in build-on (base-less entries inherit a stable top-level base)
  • Run tests (545/545 pass)

Copilot AI linked an issue Apr 21, 2026 that may be closed by this pull request
Copilot AI and others added 2 commits April 21, 2026 20:33
Allow `devel` (or `ubuntu@devel`, `opensuse@devel`, etc.) in the
`build-on` entries of multi-base platform definitions, even when
`build-for` uses a different stable base.

For example, this is now valid:

  platforms:
    plucky:
      build-on: [devel:amd64]
      build-for: [ubuntu@24.04:amd64]

Changes:
- `_validate_base_definition`: Separate build-on and build-for base
  collection. Exclude devel-series `build-on` entries from the
  base-consistency check.
- `get_platforms_charm_build_plan` product loop: Use the `build-on`
  entry's base as `build_base` when present (e.g. `ubuntu@devel` from
  `devel:amd64`), falling back to the platform-level `distro_base`.
- Add four new parameterized test cases covering devel build-on
  scenarios.
- Update hypothesis fuzz test filter to allow valid devel build-on
  entries.

Agent-Logs-Url: https://github.com/canonical/craft-platforms/sessions/598be95c-71f9-48a0-9e52-a1ccc44951b3

Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>
Copilot AI changed the title [WIP] Allow devel in multi-base builds Allow devel in multi-base charm builds Apr 21, 2026
Copilot AI requested a review from lengau April 21, 2026 20:35
Comment thread tests/unit/charm/test_build.py Outdated
Mixing a devel-series entry (e.g. `devel:amd64`) with a stable-series
entry (e.g. `ubuntu@24.04:arm64`) in the same `build-on` list is now
rejected, consistent with the existing rule that two different stable
bases cannot coexist in `build-on`.

- `_validate_base_definition`: track devel vs. non-devel build-on
  entries separately and raise `InvalidMultiBaseError` if both are
  present with explicit bases.
- Move `multi-base-devel-and-stable-build-on` from the success
  parametrize list to the `test_build_plans_bad_base` list.

Agent-Logs-Url: https://github.com/canonical/craft-platforms/sessions/1b3802d9-4240-4773-9111-640e9e5c43e0

Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the charm build planner to allow devel (and *@devel) bases in build-on entries for multi-base platform definitions, so the build environment can differ from the stable build-for base while still enforcing consistency rules for non-devel bases.

Changes:

  • Relax _validate_base_definition to ignore devel-series bases when checking build-on/build-for base consistency, while still rejecting mixed devel + explicit stable bases within build-on.
  • In get_platforms_charm_build_plan, use the per-build-on entry base (when present) as BuildInfo.build_base.
  • Add/adjust unit tests and Hypothesis fuzz filtering to cover/permit devel build-on scenarios.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
craft_platforms/charm/_build.py Allows devel-series build-on bases to differ from build-for, and propagates per-entry build base into BuildInfo.
tests/unit/charm/test_build.py Adds explicit unit cases for devel build-on and updates fuzz-test filtering to include devel forms.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit/charm/test_build.py Outdated
The filter lambda had three issues:
1. `p["build-on"]` could be a scalar string from `platform_dict()`;
   iterating over it yielded characters, not build-on entries.
2. `p["build-for"][0]` gave the first *character* when `build-for` was
   a scalar string, not the first entry.
3. The filter allowed mixed devel + non-devel-with-explicit-base in
   `build-on`, but `_validate_base_definition` now rejects that.

Replace the lambda with a `_is_valid_multi_base_platform_dict` helper
that correctly vectorises both fields and enforces the same rules as
the validator: either all build-on entries match the build-for base,
or they are devel-series only (no mixing devel with explicit stable
bases).

Agent-Logs-Url: https://github.com/canonical/craft-platforms/sessions/53907464-0c84-44e1-9a54-c9720be85811

Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>
@lengau

lengau commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

@copilot fix the linting issues.

@lengau lengau changed the title Allow devel in multi-base charm builds feat: allow devel in multi-base charm builds Apr 21, 2026
- Use `Set[Optional[str]]` instead of `set[str | None]` for Python 3.8
  compatibility (fixes ty and mypy errors on lines 96 and 120)
- Add `Set` to the `typing` imports
- Reformat `parse_base_and_architecture` assignment and ternary
  `build_base=` expression in _build.py to satisfy ruff format
- Reformat `build_fors = (...)` assignment and list comprehension
  condition in test_build.py to satisfy ruff format

Agent-Logs-Url: https://github.com/canonical/craft-platforms/sessions/8b45bd49-6831-4ba9-b5ed-f7d45cb14d3f

Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>

Copilot AI commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix the linting issues.

Fixed in 542fbb6:

  • set[str | None]Set[Optional[str]] (added Set to typing imports) to fix ty/mypy Python 3.8 compatibility errors on lines 96 and 120
  • Reformatted the parse_base_and_architecture call and build_base= ternary in _build.py to satisfy ruff format
  • Reformatted build_fors = (...) and the list comprehension condition in test_build.py to satisfy ruff format

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates charm multi-base build planning/validation to allow devel series entries in build-on, and adjusts build plan generation and fuzz tests accordingly.

Changes:

  • Allow devel/ubuntu@devel in build-on entries during base validation (with constraints).
  • Use per-entry build-on base (when present) as the build_base in the generated charm build plan.
  • Add unit tests and update the Hypothesis fuzz-test filter to admit valid devel build-on cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
craft_platforms/charm/_build.py Updates base validation to treat devel build-on specially and uses per-entry build-on base when generating BuildInfo.
tests/unit/charm/test_build.py Adds targeted unit tests for devel build-on and improves Hypothesis filtering to match the updated validation rules.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread craft_platforms/charm/_build.py
Extend the devel/non-devel mixing check in `_validate_base_definition` to
also reject base-less `build-on` entries (which inherit a stable top-level
base) when devel entries are present.  Previously only entries with an
explicit non-devel base were caught; now any non-devel entry — including
bare architecture strings like "amd64" — triggers `InvalidMultiBaseError`
when mixed with a devel entry.

- Remove `has_non_devel_base_build_on`; use `non_devel_build_on_bases`
  (which already records `None` for base-less entries) as the guard.
- Update `_is_valid_multi_base_platform_dict` fuzz-test filter to match the
  same rule: reject whenever devel and non-devel entries coexist in build-on.
- Add `devel-and-base-less-mixed-build-on` test case covering the scenario
  described by the reviewer.

Agent-Logs-Url: https://github.com/canonical/craft-platforms/sessions/7f809be5-d846-4c1f-9208-2a38e09e4062

Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>
@lengau
lengau marked this pull request as ready for review June 15, 2026 17:51
@lengau
lengau requested a review from a team July 2, 2026 18:22

@bepri bepri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a comment about tests

return True


def _is_valid_multi_base_platform_dict(p):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels complex enough that it deserves its own test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this is a pretty complex helper function for a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow devel in multi-base builds

5 participants