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 a Windows platform native image (WCOW) #40

Closed
wants to merge 35 commits into from

Conversation

gesellix
Copy link
Contributor

@gesellix gesellix commented Jul 30, 2022

This adds a Windows image to the build, and to the published multi-arch image.

The Windows build ignores the Makefile, though. Please give some feedback if you'd prefer a more future-proof approach. I'd personally suggest to get rid of the Makefile and stick to the Dockerfiles. Another option would be to perform a cross-platform build using that Makefile and publishing the binaries for each platform as a package. Those binaries could then be used in the Dockerfiles.

I would also suggest to move the Linux Dockerfile to a subdirectory linux, so that it becomes more obvious that there are dedicated Dockerfiles for both Linux and Windows.

Due to docker/buildx#176 and docker/build-push-action#18 we have to create the Windows image manually, and add it to the multi-arch manifest in another step. I've tested the workflow applied here for one of my own images, see https://github.com/gesellix/go-npipe/releases/tag/v2022-07-31T14-30-00.

Edit: the current approach is based on https://lippertmarkus.com/2021/11/30/win-multiarch-img-lin/.

Relates to #35
Relates to testcontainers/testcontainers-java#5621

uses: docker/setup-qemu-action@v1
with:
uses: docker/setup-qemu-action@v2
with:
image: tonistiigi/binfmt:qemu-v6.0.0-12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this one required? I'm not sure whether dependabot would notify about updates for that image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version pinning was required for linux/s390x builds, see #26.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the version pinning, because the linux/s390x builds would fail otherwise... although the version pinning had been introduces due to the same error which appears now 🤪

image: tonistiigi/binfmt:qemu-v6.0.0-12
- name: Set up Docker Buildx
id: buildx
uses: docker/setup-buildx-action@v1
uses: docker/setup-buildx-action@v2
with:
version: v0.6.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this one required? I'm not sure whether dependabot would notify about updates for that version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version pinning was required for linux/s390x builds, see #26.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the version pinning, because the linux/s390x builds would fail otherwise... although the version pinning had been introduces due to the same error which appears now 🤪

Dockerfile Outdated Show resolved Hide resolved
windows/Dockerfile Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@kiview
Copy link
Member

kiview commented Aug 8, 2022

@gesellix
Copy link
Contributor Author

gesellix commented Aug 8, 2022

Woah, what are these issues on CI?

local error: tls: bad record MAC might be a temporary error... very strange. I think retrying the workflows is the best we can do. Not sure if such an error could be an indication of a bad actor tampering with the network. There's a chance that the Golang proxy has issues?

kiview
kiview previously approved these changes Aug 9, 2022
windows/Dockerfile Outdated Show resolved Hide resolved
@kiview
Copy link
Member

kiview commented Aug 9, 2022

AFAIK GHA uses Cygwin and bash for Windows when executing scripts, so that is something to keep in mind if we want to continue to make use of Make. However, the Makefile is not used anymore after all, or am I missing something?

@kiview
Copy link
Member

kiview commented Aug 9, 2022

Btw., did you already rebase on #28?

@gesellix
Copy link
Contributor Author

gesellix commented Aug 9, 2022

@kiview I'm not sure if you (maintainers) would prefer using the Makefile for non-containerized builds. I have deleted it now as suggestion, but if you'd prefer to keep it, just leave a note :)

This branch is now rebased on the current master branch. I also moved the Linux-Dockerfile to a subdirectory "linux".

Are you going to squash the commits? If not, I can certainly cleanup a bit.

@kiview
Copy link
Member

kiview commented Aug 9, 2022

We always merge with squash, so don't worry about the actual commit history 🙂
@bsideup Do you have a need for the Makefile?

windows/Dockerfile Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile.windows Show resolved Hide resolved
Dockerfile.windows Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile.windows Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gesellix
Copy link
Contributor Author

gesellix commented Dec 2, 2022

if you agree, I'm updating the description to the latest state of the PR (for discoverability of the real changes), and once approved, we merge and release as v0.4.0

@mdelapenya yes, please, feel free to take over! :)

- name: Run Ryuk on Windows
if: ${{ matrix.os == 'windows-2022' }}
run: docker run -v //./pipe/docker_engine://./pipe/docker_engine testcontainers/ryuk:${RYUK_VERSION}
continue-on-error: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this line still required?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we need to change it slightly, but we would like to keep something like this to test the built image. Due to the complexity and amount of combinations an error sneaked in. The linux/amd64 and windows/amd64 contained the ARM binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also think that the checks are required and make sense. I only asked for the "continue on error" line, which is at the very end of the workflow and doesn't have any effect (?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right. continue-on-error is not necessary. We will adjust it with the update that fixes the binaries. Unfortunately, for Windows more changes are necessary due to: actions/runner-images#6688.

@mdelapenya
Copy link
Contributor

@gesellix Hi 👋 We are thinking about revisiting this PR in order to get the windows container for Ryuk. We explored the options with goreleaser, although with very few success, as the ability to merge manifests coming from different builders is a Pro feature.

Said that, I think it would be very beneficial if you and I can sync, probably in a zoom call or slack huddle, so we can make this PR into Ryuk. Wdyt?

@gesellix
Copy link
Contributor Author

gesellix commented Mar 2, 2023

Hey @mdelapenya :) I'm going to contact you in Slack!

@mdelapenya
Copy link
Contributor

mdelapenya commented Mar 14, 2023

We have a really productive online session today, where we were talking on how to unblock this issue after the recent changes in the build system of Ryuk and the failed intents of using goreleaser to build the app without using the Pro version of goreleaser.

As a summary:

  • @gesellix shared is use case, where Ryuk needs to run on Windows for certain services he build
  • goreleaser has demonstrated to be a powerful tool to build Go applications, but without the Pro license it's not possible to split the build for linux/windows and merge them into a single image
  • I'm closing this PR as @gesellix will send a fresh new one, using https://github.com/docker-client/echo-server/blob/main/.github/workflows/release.yml as example to build the multi-OS image
  • @kiview suggested starting with one Windows version (one of the latest or the most used version), and once we have it working make progress with more versions (2019, 2022...). For that we probably need to update the GH workflow to use a matrix for windows and adjust/create the Dockerfile for Windows, using a build arg for the BASE version.
  • @mdelapenya will pair with @gesellix for the final review
  • windows users and we are happy 😄

Therefore, I'm closing this PR waiting for the new one. Thanks all for participating in this healthy discussion 👏

@HofmeisterAn
Copy link
Contributor

I would like to mention that our cross-platform build essentially worked, but we encountered issues while publishing the multi-arch manifest to Docker Hub. The pipeline did not merge the manifest, instead it overwrote previously published manifests.

@gesellix
Copy link
Contributor Author

I've restarted to work on the multi-arch manifest: gesellix#2

About the goreleaser images and using their manifests: I think that should work somehow, it all comes down to using explicit image references (can you share/link how goreleaser names all the published variants?), collecting their manifests, checking/annotating the os.version of the Windows variants, and then merging manifests.

I've tried to extract the "merger" step into a single job, maybe it can be refactored to a custom action with all the images/manifests as input and the desired multi-arch image name as output.

@HofmeisterAn
Copy link
Contributor

Its been a while, but I think this are the latest changes we tested. The names are probably the value of image_templates. Does that help?

@gesellix
Copy link
Contributor Author

I'll try (probably not before next week), thanks for the link!

@narcis-ro
Copy link

Hey guys. Are you still planning to merge either versions? Kind regards

@gesellix
Copy link
Contributor Author

I've rebased the code at gesellix#2 to the current upstream (this repo). Let's see, if the workflows still work, which would also be a check, whether the concept stands the test of time :)

@narcis-ro
Copy link

Thanks for the quick reply @gesellix
Would be nice to finally have this option for windows

@gesellix
Copy link
Contributor Author

... something seems to be broken on the GitHub's side of Action Runners. I'll check later/tomorrow. Multi-Arch images should appear at https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk (with the older one at https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk/102573888?tag=wip).

@gesellix
Copy link
Contributor Author

See https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk/182452091?tag=20240222.1 with a multi-arch image available for the proof of concept.

Docker image name: ghcr.io/gesellix/moby-ryuk:20240222.1

@gesellix
Copy link
Contributor Author

A clean pull request is open for discussion at #110

@narcis-ro
Copy link

PR was merged. When is it gonna be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants