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

Remove install-multilib.sh #91

Merged
merged 1 commit into from
May 3, 2023

Conversation

lgrosz
Copy link
Contributor

@lgrosz lgrosz commented Apr 13, 2023

Using buildarg TARGETPLATFORM to install multilib where necessary.

I've been thinking about trying to get some cross architecture containers going and would like to fixup the build and deploy scripts and the GitHub workflow to better allow for that.

I chose to use TARGETPLATFORM as a build argument as docker buildx provides it by default, which seems to be the only current option to get build cross-architecture containers. Since this project is only built for the linux/amd64 platform right now, I added that as the only option in the workflow job matrix.

Using buildarg TARGETPLATFORM to install multilib where necessary.
Copy link
Member

@yoctopidg3 yoctopidg3 left a comment

Choose a reason for hiding this comment

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

Acking this. I want to test this a bit, but it looks reasonable. @moto-timo

@moto-timo
Copy link
Member

Tested local ./buildall.sh and ran into an unrelated debian-9 failure. The CI build for the PR was green. Looks like a reasonable, sensible change.

Would be nice to also test on arm64 host, but my quick test on an M1 Macbook uncovered issues with the find command differences compared to Linux :(

Perhaps I'll spin up an arm64 VM in the cloud and test it there.

@@ -35,6 +34,7 @@ baseimage=`grep FROM Dockerfile | sed -e 's/FROM //'`
${ENGINE_CMD} pull $baseimage

${ENGINE_CMD} build \
--build-arg TARGETPLATFORM=$TARGETPLATFORM \
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'm not sure this is necessary. When I use a regular docker build, this gets defined.

from ubuntu:20.04

ARG TARGETPLATFORM
RUN echo $TARGETPLATFORM
$ docker --version
Docker version 20.10.12, build e91ed57

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This extends to the matrix additions and the -builder image below as well.

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 am not sure if podman or other supported container platforms do the same.

Copy link
Member

Choose a reason for hiding this comment

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

FROM ubuntu:20.04

RUN env
HOSTNAME=<foo>
HOME=/root
PATH=<...>
PWD=/

So looks like docker implementations are not consistent... I had to pass in --build-arg TARGETPLATFORM=$(uname -m) to have something defined.

Copy link
Contributor Author

@lgrosz lgrosz Apr 28, 2023

Choose a reason for hiding this comment

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

What if you added ARG TARGETPLATFORM in the Dockerfile too?

If you were to use docker buildx build does that define TARGETPLATFORM? Since the build script supports other container platforms we probably couldn't remove it anyways since I don't think those platforms have the buildx command.

@moto-timo
Copy link
Member

Just for grins:
Build on Ampere arm64 failed for alma-9, centos-7, debian-9, fedora-34 and fedora-35. We should really be bumping to only fedora-36,37,38 based on EOL, so that can be ignored. But this is promising nonetheless.

Copy link
Member

@yoctopidg3 yoctopidg3 left a comment

Choose a reason for hiding this comment

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

Approved. Debian-9 should be dropped anyhow as it's EOL.

Copy link
Member

@moto-timo moto-timo 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 enough to merge. If we need to tweak it later, so be it. Since debian-9 is EOL, we will ignore that failure and merge.

@moto-timo moto-timo merged commit 71e581a into crops:master May 3, 2023
15 checks passed
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.

None yet

3 participants