Skip to content
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

Add "--platform" to GHA generate script #81

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

tianon
Copy link
Member

@tianon tianon commented Sep 14, 2023

I discovered this was missing when I created a riscv64-only image and it then failed to build on amd64. 😅

It should be totally harmless on most repos since I don't think anyone but me using this script has images that don't support one of either amd64 or windows-amd64, which will automatically be preferred. 🙈

@codecov-commenter
Copy link

Codecov Report

Merging #81 (0e6bdaa) into master (ca308fd) will not change coverage.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master      #81   +/-   ##
=======================================
  Coverage   73.10%   73.10%           
=======================================
  Files           7        7           
  Lines         714      714           
=======================================
  Hits          522      522           
  Misses        162      162           
  Partials       30       30           

@tianon tianon marked this pull request as draft September 14, 2023 23:24
@tianon
Copy link
Member Author

tianon commented Sep 14, 2023

https://github.com/tianon/dockerfiles/actions/runs/6190609199/job/16807227669 is an example failing job

tianon/dockerfiles@ee0ab6c is where I've swapped in this commit for testing

https://github.com/tianon/dockerfiles/actions/runs/6191900925 is the resulting failing job that's now failing in a more amusing way ("exec format error", because apparently I had erroneously assumed that GHA set up QEMU already by default 😂)

@tianon
Copy link
Member Author

tianon commented Sep 14, 2023

So, I guess for things we can't build natively we can either:

  1. skip them entirely; or
  2. add "install emulation hooks" code to the prepare section for them

At this point, I think I might be leaning towards the former, but I don't have a problem with doing the latter instead. Thoughts, @yosifkit? (either way, this really doesn't affect anyone I'm aware of except for me, lolsob)

@tianon
Copy link
Member Author

tianon commented Sep 14, 2023

I guess option 3. is "both" and we make it environment-variable-configurable.

tianon added a commit to tianon/dockerfiles that referenced this pull request Sep 15, 2023
tianon added a commit to tianon/dockerfiles that referenced this pull request Sep 15, 2023
Copy link
Member

@yosifkit yosifkit left a comment

Choose a reason for hiding this comment

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

This is fine as-is and I want #80 to build on top of it.

@tianon
Copy link
Member Author

tianon commented Oct 24, 2023

As is this doesn't actually solve my problem, but that can be a follow-up, I guess. Just need some input on which way to solve it 😅

@yosifkit
Copy link
Member

I lean toward skipping as well. But then I feel like the most rational test (checking dpkg --print-architecture) to know what to skip would not allow a user to override it by pre-installing binfmt-support and qemu-user-static. Is there a better test?

Alternatively, I am leaning toward just auto installing binfmt/qemu if the provided arch does not match dpkg. This might initially only be used by your repo, but it could give us opportunity to test some of the official images on alternative arches.

@yosifkit yosifkit marked this pull request as ready for review October 25, 2023 16:35
@yosifkit yosifkit merged commit 07ad706 into docker-library:master Oct 25, 2023
4 checks passed
@tianon tianon deleted the github-actions-platform branch October 26, 2023 04:26
tianon added a commit to tianon/dockerfiles that referenced this pull request Dec 1, 2023
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.

3 participants