Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Fix build rules for image.mk #235

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix build rules for image.mk #235

wants to merge 1 commit into from

Conversation

tamalsaha
Copy link

@tamalsaha tamalsaha commented Jun 27, 2023

Description of your changes

We are not using xpkg instead using the img.mk build rules to package our images. This pr makes the minimal changes to fix the build rules and build multi-arch image using docker manifest command.

Fixes #

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

I was able to use my fork to build and publish images

https://github.com/kubeform/provider-gcp/actions/runs/5395921949/jobs/9798952056

@pedjak
Copy link

pedjak commented Jul 4, 2023

Thanks for your contribution. Could you please share a bit more details why manifest-tool got replaced with docker manifest? The later is marked experimental, is there any issues with manifest-tool?

@tamalsaha
Copy link
Author

tamalsaha commented Jul 5, 2023

manifest-tool command did not work on Mac M1 laptops. docker manifest command has existed since 2018. I don't know why it is still marked as experimental but this is pretty much what everyone uses these days since it is part of the docker already. You can see the history of the commands here: https://github.com/estesp/manifest-tool#history

Usage in k/k:

https://github.com/kubernetes/kubernetes/blob/master/build/pause/Makefile#L83

@jbw976
Copy link
Member

jbw976 commented Jul 5, 2023

Would it be possible to share some more details here @tamalsaha on why manifest-tool didn't work on M1 laptops? What was the error you saw and how can you reproduce it? a lot of us have M1 laptops as well and haven't seen an issue when building ourselves, so understanding the root cause here would be helpful :)

@tamalsaha
Copy link
Author

tamalsaha commented Jul 5, 2023

There is no darwin-arm64 version offered by the manifest-tools project. https://github.com/estesp/manifest-tool/releases/tag/v1.0.3

So, curl failed.

@jbw976
Copy link
Member

jbw976 commented Jul 5, 2023

Thank you - any further details you can provide about how to reproduce this though? would you be able to fill out details in the "How has this code been tested" section of the PR body?

@tamalsaha
Copy link
Author

tamalsaha commented Jul 5, 2023

any further details you can provide about how to reproduce this though?

Just try from any Mac M1 laptop.

How has this code been tested

Done!

Comment on lines +243 to +244
do.build.image.%:
@$(MAKE) -C $(IMAGE_DIR)/$* IMAGE_PLATFORMS=$(IMAGE_PLATFORM) IMAGE=$(BUILD_REGISTRY)/$*-$(ARCH) img.build
Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines +89 to +90
IMAGE_PLATFORMS_LIST := $(subst _,/,$(filter linux_%,$(PLATFORMS)))
IMAGE_PLATFORM := $(subst _,/,$(PLATFORM))
Copy link
Author

Choose a reason for hiding this comment

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

@jbw976
Copy link
Member

jbw976 commented Jul 6, 2023

Just try from any Mac M1 laptop

Try what exactly? 😇 I'm hoping you can share more precise repro steps here, e.g. the specific commands to run to repro this and where to run them from. Thank you! 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants