dogecoin / docker

Dogecoin Core on Docker
MIT License
27 stars 18 forks source link

refactor: removed dogecoin-qt #35

Closed xanimo closed 2 years ago

xanimo commented 2 years ago

removed dogecoin-qt from Dockerfile and entrypoint.py since it's not supported and wasting resources.

refined Dockerfile by extracting the specific files we need from our archive and copying each while setting appropriate permissions and owner:group in each COPY.

removed unnecessary WORKDIR /tmp since we are only copying specific files from verify build stage, thus we have nothing to rm -rf *

xanimo commented 2 years ago

removed dogecoin-qt from Dockerfile and entrypoint.py since it's not supported and wasting resources.

refined Dockerfile by extracting the specific files we need from our archive and copying each while setting appropriate permissions and owner:group in each COPY.

removed unnecessary WORKDIR /tmp since we are only copying specific files from verify build stage, thus we have nothing to rm -rf *

in a sub optimal fashion of not submitting PR's with atomic commits, i forgot to mention changing python3 to python3-minimal and adding .gitignore and .dockerignore files as placeholders to remind us that they're excellent assets to reduce overhead on system resources in particular build context archives every time we run build.

this reduces the linux/amd64 image for instance from 91.22 MB: https://hub.docker.com/layers/xanimo/1.14.5-dogecoin/bullseye/images/sha256-aa8fc7d558018725d2587daf45156db1412e50eec65bf23711274f2a4d172136?context=explore

to 57.11 MB without removing WORKDIR /tmp and COPY ...tar.gz ./: https://hub.docker.com/layers/xanimo/1.14.5-dogecoin/no-qt/images/sha256-e6256b9c177e45dfdcd191cb332a1eabce931b475ff6104e8ba4d28880dbb5b9?context=explore

to 47.62 MB in it's current state as reflected in this PR: https://hub.docker.com/layers/xanimo/1.14.5-dogecoin/headless/images/sha256-fde519015d6bce653dd7619c3f543be9bd4665a36d4174ddefb32acde512d99a?context=explore

patricklodder commented 2 years ago

Re: whether we want QT.

There is no dogecoin-qt for ARM architectures for dogecoin 1.14.5. So we can only provide it on a subset of images right now, I'm not really a fan.

It should be possible to containerize graphical application (did you tried it ?), by potentially sharing the window system of the host. I saw some articles related to it, I tried briefly few months ago but failed, I didn't go really far.

I'd expect that we'd need to include some X11 libraries in the docker image for that to work and that significantly increases attack/maintenance surface. I'd recommend that if there is a demand for this, to build it in a separate variant rather than in the main image? As a node operator, I would not want my dogecoind deployment to be bloated with X11 stuff.

xanimo commented 2 years ago

Need some changes:

1. Please put changes to `*ignore` files in a separate pull request, has nothing to do with removing QT

2. Please further separate this into 2 proposals:

   1. removal of `dogecoin-qt` - this is easy to approve
   2. changing the way we copy the files into the final container - this needs much more discussion and I have a lot of comments on that particular code.

done

xanimo commented 2 years ago

Re: whether we want QT.

There is no dogecoin-qt for ARM architectures for dogecoin 1.14.5. So we can only provide it on a subset of images right now, I'm not really a fan.

It should be possible to containerize graphical application (did you tried it ?), by potentially sharing the window system of the host. I saw some articles related to it, I tried briefly few months ago but failed, I didn't go really far.

I'd expect that we'd need to include some X11 libraries in the docker image for that to work and that significantly increases attack/maintenance surface. I'd recommend that if there is a demand for this, to build it in a separate variant rather than in the main image? As a node operator, I would not want my dogecoind deployment to be bloated with X11 stuff.

i personally prefer headless operation over gui so i share your sentiment. i think we should potentially support this in the future but at the moment i'm not interested in maintaining or developing such an image.

xanimo commented 2 years ago

A good opportunity to speak about what we want to do regarding dogecoin-qt.

It should be possible to containerize graphical application (did you tried it ?), by potentially sharing the window system of the host. I saw some articles related to it, I tried briefly few months ago but failed, I didn't go really far.

But it will probably not be the main use and be really a really specific feature. I didn't see the dogecoin-qt size, which is ~30MB on amd64. As you show, removing this executable divide almost the size of the image by 2, which is really significant.

Idk if adding GUI worth it for a Dogecoin Core image, if we could get rid of it for now and eventually if there is some demand in the future think about adding it ? Maybe it could be also a qt tag/image, to be able to provide dogecoin-qt through docker with keeping the benefit of a small size image only with other executables as main image.

i can't re-request a review from you so pinging you here. my thoughts on this are here: https://github.com/dogecoin/docker/pull/35#issuecomment-991796527

patricklodder commented 2 years ago

Blocked by #34 as that removes manpages but adds some dogecoin-qt

xanimo commented 2 years ago

Blocked by #34 as that removes manpages but adds some dogecoin-qt

removed copying manpages and ref of dogecoin-qt in entrypoint.

patricklodder commented 2 years ago

@AbcSxyZ could you please let us know if you:

  1. Agree with the concept/direction of this change (much like you proposed yourself in your last review comment, this PR implies that a dogecoin-qt deployment must be a separate image)
  2. Agree with the implementation done here?

Thanks 🙏