feat: add platforms and drop architectures for core24 #4630

mr-cal · 2024-03-04T18:50:06Z

Have you followed the guidelines for contributing?
Have you signed the CLA?
Have you successfully run tox run -m lint?
Have you successfully run tox run -e test-py310? (supported versions: py39, py310, py311, py312)

This PR is a draft.

Add platforms in core24
Drop architectures in core24

Similar to architectures, the platforms keyword is optional and defaults to building-on and building-for the host architecture.

As much as possible, I tried to follow charmcraft's and rockcraft's implementation of platform support.

(CRAFT-2535)

codecov · 2024-03-04T19:06:36Z

Codecov Report

Attention: Patch coverage is 98.98990% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 89.23%. Comparing base (a019fe7) to head (554d8c7).
Report is 24 commits behind head on feature/craft-application.

Files	Patch %	Lines
snapcraft/models/project.py	98.68%	1 Missing ⚠️

Additional details and impacted files

@@                      Coverage Diff                      @@
##           feature/craft-application    #4630      +/-   ##
=============================================================
+ Coverage                      89.14%   89.23%   +0.08%     
=============================================================
  Files                            331      335       +4     
  Lines                          22078    22323     +245     
=============================================================
+ Hits                           19682    19919     +237     
- Misses                          2396     2404       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mr-cal · 2024-03-04T20:18:25Z

@lengau or @tigarmo: could I get some early feedback of this draft? You can hide the code coverage annotations for now, I am missing a ton of unit tests.

tigarmo

Did a first review pass, thanks!

snapcraft/application.py

tigarmo · 2024-03-04T21:00:48Z

snapcraft/models/project.py

+        if len(build_for) > 1:
+            raise CraftValidationError(
+                str(
+                    f"Trying to build a rock for {build_for} "


(this refers to rock)

snapcraft/application.py

snapcraft/models/project.py

tests/unit/conftest.py

snapcraft/const.py

snapcraft/models/project.py

snapcraft/application.py

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

mr-cal · 2024-03-06T20:59:24Z

Sorry for the force-push, there were file conflicts. The already-reviewed commit has not been changed.

tigarmo · 2024-03-07T12:46:32Z

@mr-cal is this ready for review?

mr-cal · 2024-03-07T15:33:59Z

@mr-cal is this ready for review?

Not yet 🙃

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

mr-cal · 2024-03-07T17:32:21Z

This is now ready for review.

A few outstanding items:

Should the contents of meta/snap.yaml change from architectures to platforms?
~~I need to add a spread test for --platforms~~
What should be done for scenarios like this spread test? It has:

name: core2x
type: base
build-base: core22

Which means the effective base is core2x. Should that use architectures or platforms? Or should we update this test to be realistic (i.e change the snapcraft.yaml to name: core22 or name: core24)?

tigarmo · 2024-03-07T18:52:44Z

tests/spread/core24/grammar/snap/snapcraft.yaml

 grade: devel
 confinement: strict
-base: core22


I think this might have uncovered a new bug?

Or was this test in the core24 suite accidentally not updated to base: core24?

Ooh, you may be right: https://github.com/canonical/snapcraft/actions/runs/8195514975/job/22414086129?pr=4630#step:4:524

tests/unit/commands/test_expand_extensions.py

tests/unit/test_application.py

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

mr-cal · 2024-03-07T20:27:16Z

tests/spread/core24/platforms/snaps/platform-match` fails because snapcraft is not using the build-for from the build plan and is instead using the host architecture.

Snapcraft creates the correct build plan but it feels like the build-for isn't getting passed from craft-application to craft-parts. I haven't figured this out yet.

tigarmo · 2024-03-07T20:50:01Z

tests/spread/core24/platforms/snaps/platform-match` fails because snapcraft is not using the build-for from the build plan and is instead using the host architecture.

Snapcraft creates the correct build plan but it feels like the build-for isn't getting passed from craft-application to craft-parts. I haven't figured this out yet.

I think the problem is this: https://github.com/canonical/snapcraft/blob/main/snapcraft/application.py#L47
In the absence of a command-line build-for, that code is defaulting to the host arch (in this case amd64). I think the right solution here is to pass the filtered build plan to the package service, like craft-application does with the lifecycle service

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

mr-cal · 2024-03-07T22:14:54Z

Thank you! I'm not sure I would have figured that out on my own.

mr-cal · 2024-03-08T13:27:24Z

Looks like some spread tests are failing because there are a handful of get_build_for() calls in snapcraft/parts/lifecycle.py that assume the architectures keyword exists and has been filtered.

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

lengau

Fantastic work! This was a biggie

tigarmo · 2024-03-08T18:37:35Z

Some of the failures are like this:

2024-03-08T16:26:32.6531590Z 2024-03-08 16:26:32 Error executing google:ubuntu-22.04-64:tests/spread/plugins/craft-parts/python-symlinks:bare_provisioned (mar081610-674143) : 
2024-03-08T16:26:32.6533187Z -----
2024-03-08T16:26:32.6533878Z + cd bare-provisioned
2024-03-08T16:26:32.6534676Z + '[' -f expected-symlink ']'
2024-03-08T16:26:32.6535450Z + snapcraft prime
2024-03-08T16:26:32.6536663Z Bad snapcraft.yaml content:
2024-03-08T16:26:32.6538192Z - 'architectures' keyword is not supported for base 'bare'. Use 'platforms' keyword instead.

Looks like this validation should a) look at the build-base (probably use the get_effective_base() function if it's not) and b) only fail if the build-base is core24 (or devel I guess), instead of only accepting core22

cmatsuoka

After a long discussion we understood the issues and the platforms validation pattern used in this PR. Although it works, there is redundant and/or sub-optimal model validation that also exists in Rockcraft. These can be addressed in follow-up PRs for both Snapcraft and Rockcraft.

snapcraft/application.py

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

…work/CRAFT-2535-platforms

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

mr-cal · 2024-03-09T00:08:20Z

For the test failures for google:ubuntu-24.04-64:tests/spread/core24/grammar:arm64:

I think we need something like this in craft-application: canonical/craft-application@main...work/grammar-build-for . Maybe this should follow the same path as canonical/craft-application#259 now that there is a self._build_plan to use.

But even after that patch, snapcraft clean --destructive-mode fails with Multiple builds match the current platform, which I also think is a craft-application quandary.

Otherwise, the spread tests look good.

tigarmo · 2024-03-11T11:22:17Z

@mr-cal I think the problem with the snapcraft clean --destructive-mode is again the assumption that build-for is provided which we need to fix upstream in craft-application
my suggestion is to ignore that particular failure for this PR

mr-cal · 2024-03-11T15:31:53Z

Sounds good, I'll make an issue after

@mr-cal I think the problem with the snapcraft clean --destructive-mode is again the assumption that build-for is provided which we need to fix upstream in craft-application my suggestion is to ignore that particular failure for this PR

New issue in craft-application: canonical/craft-application#261

t4saha · 2024-06-21T05:07:05Z

@mr-cal Just migrated my app to core24. Platforms is declared only as amd64 and arm64 but remote build is trying to build for all platforms.
https://github.com/t4saha/stellarium-snap

sergiusens · 2024-06-21T10:59:31Z

@t4saha I created #4867 to track it in case you want to follow

mr-cal requested a review from lengau March 4, 2024 20:17

mr-cal marked this pull request as draft March 4, 2024 20:18

tigarmo reviewed Mar 4, 2024

View reviewed changes

lengau reviewed Mar 5, 2024

View reviewed changes

snapcraft/const.py Show resolved Hide resolved

lengau reviewed Mar 5, 2024

View reviewed changes

snapcraft/models/project.py Outdated Show resolved Hide resolved

lengau reviewed Mar 5, 2024

View reviewed changes

snapcraft/models/project.py Outdated Show resolved Hide resolved

lengau reviewed Mar 5, 2024

View reviewed changes

snapcraft/models/project.py Outdated Show resolved Hide resolved

lengau reviewed Mar 5, 2024

View reviewed changes

snapcraft/application.py Outdated Show resolved Hide resolved

lengau reviewed Mar 5, 2024

View reviewed changes

snapcraft/application.py Outdated Show resolved Hide resolved

mr-cal added 2 commits March 6, 2024 12:22

feat: add platforms and drop architectures for core24

e06d1e4

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

chore: finalize feature and feedback from review

a136703

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

mr-cal force-pushed the work/CRAFT-2535-platforms branch from 84dfa5c to a136703 Compare March 6, 2024 20:58

Merge branch 'feature/craft-application' into work/CRAFT-2535-platforms

96a1474

fix: change where default value for platforms is set

fc4400a

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

mr-cal marked this pull request as ready for review March 7, 2024 16:02

mr-cal requested review from tigarmo and lengau March 7, 2024 16:03

tests: update core24 spread test

0ef73c0

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

tigarmo reviewed Mar 7, 2024

View reviewed changes

mr-cal added 3 commits March 7, 2024 13:37

chore: revert test_expand_extensions unit test changes

f46277c

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

refactor: rename --build-for tests

c53a4b3

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

tests: add --platform tests

a4373fa

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

Merge branch 'feature/craft-application' into work/CRAFT-2535-platforms

6008554

fix: pass build_plan instead of build_for to package class

b5b4555

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

mr-cal added 2 commits March 8, 2024 09:38

fix: architectures is only supported for core22

f055209

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

style: linter fixes

ef44ad8

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

lengau approved these changes Mar 8, 2024

View reviewed changes

cmatsuoka approved these changes Mar 8, 2024

View reviewed changes

snapcraft/application.py Outdated Show resolved Hide resolved

mr-cal added 3 commits March 8, 2024 15:21

fix: pre=False for validating archs and platforms

6a69a45

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

refactor: move SnapcraftBuildPlanner to projects model module

0ebe713

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

Merge remote-tracking branch 'origin/feature/craft-application' into …

9ae96fc

…work/CRAFT-2535-platforms

This was referenced Mar 8, 2024

Refactor platform validation #4650

Open

Refactor platform validation canonical/rockcraft#502

Open

mr-cal added 2 commits March 8, 2024 15:53

fix: correct merge conflict with #4642

4d434ba

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

fix: another pydantic change...

554d8c7

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

tigarmo approved these changes Mar 11, 2024

View reviewed changes

sergiusens merged commit f771a4a into feature/craft-application Mar 11, 2024
7 of 12 checks passed

mr-cal mentioned this pull request Mar 11, 2024

Commands cannot always load projects with advanced grammar canonical/craft-application#261

Open

mr-cal mentioned this pull request Apr 19, 2024

feature/craft-application: implement get_build_plan() #4469

Closed

sergiusens mentioned this pull request Jun 21, 2024

Migrated my app to core24. Platforms is declared only as amd64 and arm64 but remote build is trying to build for all platforms. #4867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add platforms and drop architectures for core24 #4630

feat: add platforms and drop architectures for core24 #4630

mr-cal commented Mar 4, 2024 •

edited

Loading

codecov bot commented Mar 4, 2024 •

edited

Loading

mr-cal commented Mar 4, 2024 •

edited

Loading

tigarmo left a comment

tigarmo Mar 4, 2024

mr-cal commented Mar 6, 2024

tigarmo commented Mar 7, 2024

mr-cal commented Mar 7, 2024

mr-cal commented Mar 7, 2024 •

edited

Loading

tigarmo Mar 7, 2024

mr-cal Mar 7, 2024

mr-cal Mar 7, 2024

mr-cal commented Mar 7, 2024

tigarmo commented Mar 7, 2024 •

edited

Loading

mr-cal commented Mar 7, 2024

mr-cal commented Mar 8, 2024

lengau left a comment

tigarmo commented Mar 8, 2024

cmatsuoka left a comment

mr-cal commented Mar 9, 2024

tigarmo commented Mar 11, 2024

mr-cal commented Mar 11, 2024

t4saha commented Jun 21, 2024 •

edited

Loading

sergiusens commented Jun 21, 2024

feat: add platforms and drop architectures for core24 #4630

feat: add platforms and drop architectures for core24 #4630

Conversation

mr-cal commented Mar 4, 2024 • edited Loading

codecov bot commented Mar 4, 2024 • edited Loading

Codecov Report

mr-cal commented Mar 4, 2024 • edited Loading

tigarmo left a comment

Choose a reason for hiding this comment

tigarmo Mar 4, 2024

Choose a reason for hiding this comment

mr-cal commented Mar 6, 2024

tigarmo commented Mar 7, 2024

mr-cal commented Mar 7, 2024

mr-cal commented Mar 7, 2024 • edited Loading

tigarmo Mar 7, 2024

Choose a reason for hiding this comment

mr-cal Mar 7, 2024

Choose a reason for hiding this comment

mr-cal Mar 7, 2024

Choose a reason for hiding this comment

mr-cal commented Mar 7, 2024

tigarmo commented Mar 7, 2024 • edited Loading

mr-cal commented Mar 7, 2024

mr-cal commented Mar 8, 2024

lengau left a comment

Choose a reason for hiding this comment

tigarmo commented Mar 8, 2024

cmatsuoka left a comment

Choose a reason for hiding this comment

mr-cal commented Mar 9, 2024

tigarmo commented Mar 11, 2024

mr-cal commented Mar 11, 2024

t4saha commented Jun 21, 2024 • edited Loading

sergiusens commented Jun 21, 2024

mr-cal commented Mar 4, 2024 •

edited

Loading

codecov bot commented Mar 4, 2024 •

edited

Loading

mr-cal commented Mar 4, 2024 •

edited

Loading

mr-cal commented Mar 7, 2024 •

edited

Loading

tigarmo commented Mar 7, 2024 •

edited

Loading

t4saha commented Jun 21, 2024 •

edited

Loading