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 official Dockerfile #17

Merged
merged 12 commits into from
Apr 10, 2024
Merged

Add official Dockerfile #17

merged 12 commits into from
Apr 10, 2024

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Mar 26, 2024

Follow-up to #1.

I think building native binaries on docker is not the right approach. It's better to run on the jvm: easier build, faster runtime and compatible with linux arm64, which we don't support otherwise.

TODO:

cc @sethforprivacy

@pm47 pm47 requested a review from sstone March 26, 2024 20:25
@sethforprivacy
Copy link
Contributor

Will take a deeper look asap, but at first glance I love this approach!

@tsmith123
Copy link

tsmith123 commented Mar 26, 2024

I'm getting "ERROR: failed to solve: eclipse-temurin:17-jre-alpine: no match for platform in manifest: not found" running on an M2 Macbook when building image.

@pm47
Copy link
Member Author

pm47 commented Mar 26, 2024 via email

@tsmith123
Copy link

Changed both to 21 and it seems better but now I'm getting:

"ERROR: failed to solve: process "/bin/sh -c git clone --recursive --branch ${LIGHTNING_KMP_BRANCH} https://github.com/ACINQ/lightning-kmp . && test git rev-parse HEAD = ${LIGHTNING_KMP_COMMIT_HASH} || exit 1 && ./gradlew publishToMavenLocal -x dokkaHtml" did not complete successfully: exit code: 1"

@pm47
Copy link
Member Author

pm47 commented Mar 27, 2024

Rebased on master.

Changed both to 21 and it seems better but now I'm getting:
"ERROR: failed to solve: process "/bin/sh -c git clone --recursive --branch ${LIGHTNING_KMP_BRANCH} https://github.com/ACINQ/lightning-kmp . && test git rev-parse HEAD = ${LIGHTNING_KMP_COMMIT_HASH} || exit 1 && ./gradlew publishToMavenLocal -x dokkaHtml" did not complete successfully: exit code: 1"

@tsmith123 your issue should be fixed at 5630fa0.

@pm47 pm47 marked this pull request as ready for review March 27, 2024 13:50
Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

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

Building this docker image still won't work on macOS arm64 because of gradle/gradle#24875.
It could be fixed by using a glibc-based distro (ubuntu, ...) or maybe switching to a snapshot version of gradle 8.8 but I'm not sure it's worth it as macOS arm64 is not a production target (and we already publish a native phoenixd exe for macOS arm64).

@pm47
Copy link
Member Author

pm47 commented Mar 27, 2024

So @sstone should work on all 4 combinations linux/macos x64/arm64?

More portability is better for docker I think. Besides, ubuntu 21 + jdk21 seems tailor made for a bitcoin app 😄

@pm47
Copy link
Member Author

pm47 commented Mar 27, 2024

Actually went back to alpine for the final image. Best of both worlds or over the top?

@sethforprivacy
Copy link
Contributor

Dockerfile looks GTG to me, fantasticly simplified approach compared to building from source.

I'll switch to using this as well in mine (will keep my repo just so there can be a published image in GHCR/Dockerhub) with proper credit, of course. Thanks for all of the work on this @pm47!

@sethforprivacy
Copy link
Contributor

I have a few other minor improvements (none necessary, just tweaks and more explicit declaration of things like user:group pairings) in my PR on my own repo here, feel free to take anything that strikes your fancy, no credit etc. necessary.

sethforprivacy/phoenixd-docker#3

@sethforprivacy
Copy link
Contributor

Sorry for the message spam, but I actually just caught a breaking issue with the proposed Dockerfile. This Dockerfile doesn't properly set the file permissions for /phoenix which can sometimes cause the image to error out with a permissions issue when run.

See the fix here: sethforprivacy/phoenixd-docker@f3a63e0

@sethforprivacy
Copy link
Contributor

sethforprivacy commented Mar 27, 2024

@pm47 seems there is a build issue for ARMv8, can't get it to find a workable Kotlin version. Logs:

https://paste.sethforprivacy.com/?ddbbbdc28345c06d#4DPdDZvXT4CGRBJ7jo1CDnNTUB3Ynkvp5yuMRcMJ3iqX

Not sure if you can access it, but the full Github Action run is here: https://github.com/sethforprivacy/phoenixd-docker/actions/runs/8457575444/job/23169856878?pr=3

Edit: Nevermind, seems you can't build ARMv8 using ARMv8 due to Kotlin having no native aarch64 Linux support. Will find another way to distribute ARMv8 images then.

@pm47
Copy link
Member Author

pm47 commented Mar 27, 2024

Edit: Nevermind, seems you can't build ARMv8 using ARMv8 due to Kotlin having no native aarch64 Linux support. Will find another way to distribute ARMv8 images then.

On the contrary, it should work, that is the point of using a jvm build for docker instead of a native one. Seems like gradle tries to build the native version, which we are not going to use anyway.

Will look at your other changes, feel free to suggest changes directly on this PR too!

Copy link
Contributor

@sethforprivacy sethforprivacy left a comment

Choose a reason for hiding this comment

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

I think this encompasses all of my minor changes/improvements, feel free to take or leave any and all of them as you see fit :)

.docker/Dockerfile Show resolved Hide resolved
.docker/Dockerfile Show resolved Hide resolved
.docker/Dockerfile Outdated Show resolved Hide resolved
.docker/Dockerfile Outdated Show resolved Hide resolved
RUN apt-get update \
&& apt-get upgrade -y
RUN apt-get install -y --no-install-recommends bash git \
&& apt clean
Copy link
Contributor

Choose a reason for hiding this comment

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

Better apt cleanup:

Suggested change
&& apt clean
&& apt clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

.docker/Dockerfile Outdated Show resolved Hide resolved
.docker/Dockerfile Outdated Show resolved Hide resolved
Co-authored-by: Seth For Privacy <40500387+sethforprivacy@users.noreply.github.com>
@sstone
Copy link
Member

sstone commented Mar 28, 2024

Building phoenixd in docker is hard and there does not seem to be a way to make it work for linux/arm64 (which would be a real production target), even if issues with gradle on alpine are solved.
Why not remove the build step and have the docker file simply download the released JVM binaries and use them directly ? that would give us a simple docker file that would work on all targets the JVM version supports (including macos arm64/x64 and linux arm64/x64) ?

@sstone
Copy link
Member

sstone commented Mar 28, 2024

Building phoenixd in docker is hard and there does not seem to be a way to make it work for linux/arm64

@pm47 found a workaround (see #25)

@pm47
Copy link
Member Author

pm47 commented Mar 28, 2024

@sethforprivacy the build should pass on arm at 75247e4 (current master tip)

@sethforprivacy
Copy link
Contributor

@sethforprivacy the build should pass on arm at 75247e4 (current master tip)

Testing now! https://github.com/sethforprivacy/phoenixd-docker/actions/runs/8468177452/job/23200542411?pr=3

@sethforprivacy
Copy link
Contributor

@sethforprivacy the build should pass on arm at 75247e4 (current master tip)

Builds are working on ARMv8 now!

.docker/Dockerfile Outdated Show resolved Hide resolved
Co-authored-by: Seth For Privacy <40500387+sethforprivacy@users.noreply.github.com>
@pm47
Copy link
Member Author

pm47 commented Mar 28, 2024

Looking pretty good I think. Will update branch/version after next release

@tsmith123
Copy link

Not sure if this Dockerfile is in its final state yet but it doesn't work for me on an M2 Macbook.

image

@sethforprivacy
Copy link
Contributor

Not sure if this Dockerfile is in its final state yet but it doesn't work for me on an M2 Macbook.

image

This issue was fixed in master, so next release will include the fix (per #17 (comment)). It was discussed at length above if you want more details on it!

@pm47
Copy link
Member Author

pm47 commented Mar 29, 2024

@tsmith123 Please confirm you are getting this error when using the Dockerfile at commit f255a11.

@dnjooiopa
Copy link

@tsmith123 Please confirm you are getting this error when using the Dockerfile at commit f255a11.

I got the same error on an M1 MacBook.
Commit f255a11
image

@pm47
Copy link
Member Author

pm47 commented Apr 4, 2024

@tsmith123 @dnjooiopa Please try again at 1c3a7ab thanks!

@dnjooiopa
Copy link

dnjooiopa commented Apr 6, 2024

@tsmith123 @dnjooiopa Please try again at 1c3a7ab thanks!

Great! Docker is now successfully built on my M1 Mac.

Screenshot 2567-04-06 at 08 56 45

@pm47 pm47 merged commit 2964e34 into master Apr 10, 2024
@pm47 pm47 deleted the docker branch April 10, 2024 15:43
@sethforprivacy
Copy link
Contributor

Amazing work on this, thank you @pm47! I've merged a very similar Dockerfile with automatic builds via Github Actions, so anyone who wants to use a ready-built image for phoenixd can use it easily:

https://github.com/sethforprivacy/phoenixd-docker/pkgs/container/phoenixd

The latest v0.1.4 version is building now and will be available shortly.

vincenzopalazzo pushed a commit to vincenzopalazzo/phoenixd that referenced this pull request Nov 7, 2024
The Dockerfile builds on windows x86, linux x86/arm and mac x86/arm.

It uses a JVM build, which is simpler and more portable than native builds.

---------

Co-authored-by: Seth For Privacy <40500387+sethforprivacy@users.noreply.github.com>
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.

5 participants