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

multi-architecture implementation #14

Merged
merged 3 commits into from
Nov 29, 2021
Merged

multi-architecture implementation #14

merged 3 commits into from
Nov 29, 2021

Conversation

AbcSxyZ
Copy link
Contributor

@AbcSxyZ AbcSxyZ commented Nov 20, 2021

fix #17.

Beginning of an implementation for multi-architecture support using buildx. Remove security check, to rely on #6.

I'm able to build images for those for architecture : amd64, arm64, armv7 & i386.
Here for Docker hub example.

It may be possible to use dpkg or TARGETPLATFORM to find architectures. It seems to me that TARGETPLATFORM is easier to use with buildx, but won't work if used directly with docker build.
dpkg would need few more operations (install of dpkg-dev, set manually variables before a dirtier mapping), but would work with docker build. Docker official images are using dpkg.

This version is cleaner for pushing to Docker Hub, but may be interesting to let the user use the image manually using dpkg. Any opinion ?

It's using Github Action with configuration defined in #15 to publish image on Docker Hub

Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

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

Noted some things I noticed when testing this inline.

1.14.4/x86_64-bionic/Dockerfile Outdated Show resolved Hide resolved
cp share/man/man1/*.1 /usr/share/man/man1 && \
cp bin/dogecoin* /usr/local/bin && \
chown ${USER}:${USER} /usr/local/bin/dogecoin* && \
chmod 4555 /usr/local/bin/dogecoin* && \
rm -rf /tmp/*

COPY docker-entrypoint.py /usr/local/bin/docker-entrypoint
COPY --chmod=500 docker-entrypoint.py /usr/local/bin/docker-entrypoint
Copy link
Member

Choose a reason for hiding this comment

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

Step 12/17 : COPY --chmod=500 docker-entrypoint.py /usr/local/bin/docker-entrypoint
the --chmod option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled

Are we sure we want that dependency?

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 didn't know chmod was specific to buildx. Depend on the previous question about buildx vs dpkg.

Copy link
Member

Choose a reason for hiding this comment

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

It requires buildkit, which is not enabled by default everywhere. I think we can use RUN chmod 500 too?

Copy link
Contributor Author

@AbcSxyZ AbcSxyZ Nov 24, 2021

Choose a reason for hiding this comment

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

Sure. I would just recommend to use chmod within COPY if the image is only deployed/buildable with buildx. Otherwise, in RUN statement of course.

Copy link
Member

Choose a reason for hiding this comment

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

Why do you recommend it? Reduction of build steps or is there more to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduction of build steps

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Could you please change that then? Let's keep the extra build step until buildkit is the default build process everywhere (currently it's only default on docker-desktop.)

Copy link
Contributor Author

@AbcSxyZ AbcSxyZ Nov 28, 2021

Choose a reason for hiding this comment

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

Rn, because we are using TARGETPLATFORM, someone who will build this Dockerfile will need buildx.

In any case not a big change, but don't you want to do this edit when/if we're having a solution to enable docker build command ?

If we want to go with docker build, I suppose we should use dpkg. I think it's a matter of choice between having a more concise Dockerfile using buildx solution, where people will use this image in their own Dockerfile or with the cli and won't build it manually. Or we can get something more verbose, possibly a dirtier mapping, but easily buildable with dpkg.

I like this idea of using docker build, the solution may be not that bad, but it's the "simpler" path here.
Let's continue in #17.

Copy link
Member

Choose a reason for hiding this comment

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

I can do docker build --build-arg TARGETPLATFORM=linux/amd64 . right now to build this but it will fail on this instruction if one doesn't use buildkit by default.

The reason why I am asking to make the separate RUN chmod 500 /usr/local/bin/docker-entrypoint for the time being is to retain some form of backward compatibility for now, until buildkit is the only supported method. Since this does not affect the outcome - it only costs a trivial build step - this de-optimization helps portability until a later time that the build system is based on buildkit everywhere...

I have no way to build this without buildkit for this issue, whereas for the first issue I mentioned there is a workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's supersede this design with work from #17 :)

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Nov 29, 2021

Add implementation from issue #17 for cross architecture using dpkg --print-architecture instead of TARGETPLATFORM prepopulated variable.

@patricklodder patricklodder changed the title multi-architecture implementation for buildx multi-architecture implementation Nov 29, 2021
Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

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

ACK.

Note: This (dpkg) works for all debian-based distros, so we should have no issues for now.

@patricklodder patricklodder merged commit ce790d9 into dogecoin:main Nov 29, 2021
@AbcSxyZ AbcSxyZ deleted the multi-arch branch December 4, 2021 19:14
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.

feat: Support non-complicated x86_64 builds.
3 participants