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

Docker image workflow (publish on ghcr.io) #14230

Merged
merged 25 commits into from Mar 24, 2024

Conversation

AFCMS
Copy link
Contributor

@AFCMS AFCMS commented Jan 8, 2024

Should fix #14164

Follows the official Docker documentation to build image using GitHub Actions workflows.

Official Actions by Docker are used.

Links:

Tags are automatically determined by the Docker metadata Action.
You can find how its done here

Please note that the build of the arm64 image takes a much longer time than the amd64 one. This is due to the use of QEMU for the build process, following the Docker documentation. It may be possible to not require the use of QEMU by modifying the Dockerfile but I don't have the knowledge to do so.

To do

This PR is a Work in Progress.

  • Basic stuff
  • Test if build works (on fork)
  • Is opencontainers metadata correct for the images?
  • Should the workflow use a matrix strategy for the architectures? No more arm64
  • Remove old image building workflow?
  • Test running --version in CI
  • More architectures? (alpine supported architectures) Or remove arm64? Require cross-compilation support in Dockerfile.

How to test

Merge the branch into one of your forks of Minetest, see the workflow run, see the container in the package section of the repository. See the two architectures. Pull the image and see if it runs.

In my case:

@AFCMS AFCMS marked this pull request as draft January 8, 2024 16:50
@sfan5
Copy link
Member

sfan5 commented Jan 8, 2024

Please note that the build of the arm64 image takes a much longer time than the amd64 one. This is due to the use of QEMU for the build process, following the Docker documentation. It may be possible to not require the use of QEMU by modifying the Dockerfile but I don't have the knowledge to do so.

The use of qemu cannot be avoided since Github does not provide any ARM runners. I'd simply drop it.

@wsor4035 wsor4035 added the @ Build CMake, build scripts, official builds, compiler and linker errors label Jan 8, 2024
@AFCMS
Copy link
Contributor Author

AFCMS commented Jan 10, 2024

The use of qemu cannot be avoided since Github does not provide any ARM runners. I'd simply drop it.

Isn't arm support interesting enough for raspberry pi users? The CI is basically free so my biggest concern was the longer time for the CI to finish in stuff like PRs. That could perhaps be improved by using a matrix strategy so the two builds are visually separated.

@Zughy Zughy added the Bugfix 🐛 PRs that fix a bug label Jan 22, 2024
@AFCMS
Copy link
Contributor Author

AFCMS commented Jan 26, 2024

Docker support cross compilation in the image build process, but it requires modifications of the Dockerfile.

https://docs.docker.com/build/guide/multi-platform/#build-using-cross-compilation

GCC is also not straightforward to setup for cross-compilation, so I think its better to just drop the QEMU based arm build and later think about allowing cross-compilation in the the Dockerfile.

@AFCMS AFCMS changed the title Docker image workflow (publish on ghcr.io, amd64 + arm64) Docker image workflow (publish on ghcr.io) Feb 12, 2024
@AFCMS
Copy link
Contributor Author

AFCMS commented Feb 12, 2024

I dropped the arm64 support. IMO it would be a good thing to support it, but it should be done with cross-compilation support in the Dockerfile.

@AFCMS
Copy link
Contributor Author

AFCMS commented Mar 4, 2024

On the topic of cross compilation, this seems to be a pretty useful tool:

https://github.com/tonistiigi/xx

.github/workflows/docker_image.yml Show resolved Hide resolved
.github/workflows/docker_image.yml Outdated Show resolved Hide resolved
.github/workflows/docker_image.yml Show resolved Hide resolved
@sfan5 sfan5 added the Concept approved Approved by a core dev: PRs welcomed! label Mar 4, 2024
@AFCMS AFCMS requested a review from sfan5 March 11, 2024 15:09
@AFCMS AFCMS marked this pull request as ready for review March 11, 2024 20:24
.github/workflows/docker_image.yml Outdated Show resolved Hide resolved
.github/workflows/docker_image.yml Outdated Show resolved Hide resolved
.github/workflows/docker_image.yml Outdated Show resolved Hide resolved
AFCMS and others added 3 commits March 12, 2024 19:32
@sfan5
Copy link
Member

sfan5 commented Mar 12, 2024

Hmm, now it doesn't run for the PR anymore?
or is that because we just added it?

@AFCMS
Copy link
Contributor Author

AFCMS commented Mar 12, 2024

@AFCMS
Copy link
Contributor Author

AFCMS commented Mar 13, 2024

I have tried adding back the kubernetes.yml file, but my IDE shows some errors

image

I never used Kubernetes and don't have the knowledge to fix those. I think I will just drop it (could be added back later if needed).

@AFCMS
Copy link
Contributor Author

AFCMS commented Mar 13, 2024

I still need to fix the image metadata

@AFCMS
Copy link
Contributor Author

AFCMS commented Mar 13, 2024

I have put LGPL-2.1-only as SPDX license identifier for the Docker image metadata.

Is it fine or should I go with the LGPL-2.1-or-later ?

For reference: https://spdx.org/licenses

@AFCMS AFCMS requested a review from sfan5 March 13, 2024 21:34
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@AFCMS AFCMS requested a review from sfan5 March 14, 2024 19:25
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
AFCMS and others added 4 commits March 15, 2024 16:52
Co-authored-by: sfan5 <sfan5@live.de>
Co-authored-by: sfan5 <sfan5@live.de>
Co-authored-by: sfan5 <sfan5@live.de>
@AFCMS AFCMS requested a review from sfan5 March 15, 2024 15:58
README.md Outdated Show resolved Hide resolved
@rubenwardy
Copy link
Member

Looks good apart from that

@AFCMS AFCMS requested a review from rubenwardy March 24, 2024 17:12
Copy link
Member

@rubenwardy rubenwardy left a comment

Choose a reason for hiding this comment

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

Looks good

@AFCMS
Copy link
Contributor Author

AFCMS commented Mar 24, 2024

On a side note after merge the package should be set as public in it's settings. At least on my account it's private by default and only visible to the repository maintainers (it's confusing IMO, there is no clear indication that it's private in the maintainers view).

@sfan5 sfan5 merged commit 5a27c05 into minetest:master Mar 24, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug @ Build CMake, build scripts, official builds, compiler and linker errors Concept approved Approved by a core dev: PRs welcomed! >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker CI pipeline has been broken for over a year
5 participants