Skip to content

Take two: Make local/docker implementation platform-aware#305

Merged
jabrown85 merged 3 commits into
buildpacks:mainfrom
bojidar-bg:l1599-local-platform
Apr 15, 2026
Merged

Take two: Make local/docker implementation platform-aware#305
jabrown85 merged 3 commits into
buildpacks:mainfrom
bojidar-bg:l1599-local-platform

Conversation

@bojidar-bg

@bojidar-bg bojidar-bg commented Feb 17, 2026

Copy link
Copy Markdown
Contributor

Second attempt at #302 after buildpacks/lifecycle#1605.

Poking around the failed to export: saving image: failed to fetch base layers: error, I discovered that:

  1. The ID returned by docker image inspect --platform ... i useless for use with other commands. (See also c8d: unable to reference platform-specific images by ID moby/moby#49995) Hence, we run both a platform-specific inspect for the layer info and a platform-agnostic inspect for the image ID.
  2. ImageHistory also has a --platform argument. Passing in the same platform as used for inspecting the image seems prudent.
  3. ensureLayers calls ImageSave which also has a --platform argument. We need to pass that, as otherwise we will try to load layers for the wrong architecture.
  4. ImageLoad and ImageRemove also have platform arguments. ImageLoad is going to autodetect the correct platform when we pass it with the tar archive; ImageRemove deleting the other platforms under the same tag is probably a good thing.

I tested locally with creating an linux/amd64 builder from a lifecycle which includes the new changes.

Exported image arch. Old lifecycle New lifecycle (with this PR)
Invoke amd64 builder with docker, amd64 host linux/amd64 linux/amd64
Invoke amd64 builder with pack CLI, amd64 host linux/amd64 linux/amd64
Invoke amd64 builder with docker, arm64 host linux/arm64 - wrong linux/amd64
Invoke amd64 builder with pack CLI, arm64 host linux/arm64 - wrong linux/amd64

@bojidar-bg bojidar-bg changed the title Take two: Pass make local/docker implementation platform-aware Take two: Make local/docker implementation platform-aware Feb 18, 2026
@bojidar-bg
bojidar-bg force-pushed the l1599-local-platform branch 2 times, most recently from 7154d6b to 045de7a Compare February 19, 2026 13:18
@codecov

codecov Bot commented Feb 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.36735% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.66%. Comparing base (ee231a0) to head (c36b3eb).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #305      +/-   ##
==========================================
+ Coverage   34.06%   34.66%   +0.61%     
==========================================
  Files          39       39              
  Lines        3301     3359      +58     
==========================================
+ Hits         1124     1164      +40     
- Misses       1982     2000      +18     
  Partials      195      195              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bojidar-bg
bojidar-bg force-pushed the l1599-local-platform branch from 045de7a to c9be91c Compare April 6, 2026 13:11
@bojidar-bg
bojidar-bg marked this pull request as ready for review April 6, 2026 13:13
@bojidar-bg
bojidar-bg requested a review from a team as a code owner April 6, 2026 13:13
Signed-off-by: Bozhidar Marinov <bozhidar.marinov1@digits.schwarz>
@bojidar-bg
bojidar-bg force-pushed the l1599-local-platform branch from c9be91c to 1584d5b Compare April 6, 2026 13:19
@bojidar-bg

bojidar-bg commented Apr 6, 2026

Copy link
Copy Markdown
Contributor Author

Dropped the ball on this for a while, but finally got around to polishing it up. Tested with a lifecycle built with the PRed version of imgutils on a few different platforms, and it seemed to be working. 😃

(Unsure if the windows failure is related to my changes.)

Comment thread local/new.go
}

func defaultPlatform(dockerClient DockerClient) (imgutil.Platform, error) {
func defaultPlatform(dockerClient DockerClient) (imgutil.Platform, bool, error) {

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.

this looks good - do you think we can get some tests with mocks? The new platform aware paths look right but tests mocking out the api version to confirm the called methods would make me feel a bit better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I ended up adding a test job that uses containerd in the workflow, plus a test which only gets called in that job and should exercise the platform-aware code paths.

@bojidar-bg
bojidar-bg force-pushed the l1599-local-platform branch from 038b1b4 to f7c1b46 Compare April 15, 2026 12:42
Comment thread .github/workflows/test.yaml Outdated
GitHub actions currently ships an old version of Docker without support for multi-platform images or the containerd storage backend.

Signed-off-by: Bozhidar Marinov <bozhidar.marinov1@digits.schwarz>
Signed-off-by: Bozhidar Marinov <bozhidar.marinov1@digits.schwarz>
@bojidar-bg
bojidar-bg force-pushed the l1599-local-platform branch from f7c1b46 to c36b3eb Compare April 15, 2026 14:21
@jabrown85

Copy link
Copy Markdown
Contributor

Thanks for this! I'll try and bump lifecycle to this commit and see how that lands.

@jabrown85
jabrown85 merged commit 73856e6 into buildpacks:main Apr 15, 2026
6 checks passed
@bojidar-bg

Copy link
Copy Markdown
Contributor Author

Let's hope it has better luck than last time! 😅 😅

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.

2 participants