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

build from linuxserver/beets with latest modules #40

Closed
wants to merge 3 commits into from
Closed

build from linuxserver/beets with latest modules #40

wants to merge 3 commits into from

Conversation

w1ldg00se
Copy link
Contributor

Hi,
As you mentioned in issue #39 the dockerfile doesn't build any more because the cryptography module now requires rust.
Also i've seen the efforts made in pull request #34 to base the image off the linuxserver/beets image.

So i've created a dockerfile which builds off the linuxserver/beets image, installs rust and then installs betanin.
Betanin now runs a user abc, as this is the same user beets uses in the linuxserver/beets image.
In the setup.py you configured a specific version for every package, but that thows errors on installation so i changed everything to automatically use the newest version (except for werkzeug and SQLAlchemy, as their newest versions aren't compatible with Flask).
The linuxserver/beets image configures beets to use the directory /config for beet's configuration files. So i added the volumes /betanin/data and /betanin/config for betanin's configuration. I made it fully backward compatible to the old volumes (/root/.config/betanin, /root/.config/beets and /root/.local/share/betanin) by dynamically choosing the correct path in paths.py and fixing permissions and symlinks in the new startup script root/etc/services.d/betanin/run.
Betanin is now installed from source by copying this whole repository inside the image and installing it from there. In case you absolutely want to install it from the python repositories (like it was before), i could change that but you would need to update paths.py as it wouldn't successfully install otherwise.
The container is 616.5 MB, there may be improvement by uninstalling rust but other than that i tried to make it as small as possible.

There are 2 things that still break backward compatibility and i haven't found a solution for them:

  • UID/GID environment variables: All linuxserver images use PUID and PGID, so all users needs to change that.
  • CMD /_docker_entry: This CMD line in the dockerfile and the script got removed (because it now uses the script root/etc/services.d/betanin/run which is the way services are started in all images that use s6-overlay). But even though it got removed docker somehow saves that and tries to start the script, fails and kills the container. So all users need to delete and recreate their betanin container (as simple as docker-compose down && docker-compose up -d when using compose).

@w1ldg00se
Copy link
Contributor Author

I think the UID/GID->PUID/PGID issue can be resolved by editing the file /etc/cont-init.d/10-adduser and just using PUID/PGID as a fallback if UID/GID aren't set, here's a link to the original file:
https://github.com/linuxserver/docker-baseimage-alpine/blob/master/root/etc/cont-init.d/10-adduser.

And i just saw in the s6-overlay's readme (https://github.com/just-containers/s6-overlay) that it supports the use of the CMD command to start an application, so we wouldn't need the new run file when the code that corrects permissions and symlinks goes into a new script in /etc/cont-init.d. This way we could still use the _docker_entry script and nobody needs to recreate the existing docker container. It would just need a small change in _docker_entry to start betanin as user abc.

@sentriz: Would you mind giving me some feedback to this pull request? I want to know how likely you accept this pull request so i know if it's worth to invest time fixing the above issues.

@sentriz
Copy link
Owner

sentriz commented Apr 18, 2021

hi! thanks very much for your efforts here :) and sorry for the late reply

in general I like it. though it's a shame rust is now a dependency in the build but it's not a huge issue I suppose.
also, if this will likely end up being a breaking change for docker users, then it may be better to just go the whole hog:

  • add a notice to readme explaining new UID/GID varibles, new mounts etc
  • then remove backwards compatibility logic for paths like first_existing_mount, etc

what are your thoughts on that?

(also yeah removing rust/rustup etc after building in the dockerfile would be cool 👍 )

thanks!

@w1ldg00se
Copy link
Contributor Author

Hi! Sorry for the delay of over a month, was busy with other life things like finding a new job.

I just made a commit which fixes the 2 open issues here:

  • The UID/GID environment variables are used as a fallback when PUID/PGID aren't set. The Dockerfile just patches the script /etc/cont-init.d/10-adduser which already exists in the image.
  • The CMD command is back and it points to the same path as before, so nobody should have to recreate their existing container. The permissions and symlinks have been moved to /etc/cont-init.d/20-permissions.

Now there shouldn't be any breaking change for existing users. And there shouldn't be any performance issues except maybe a few milliseconds while starting the container.

Rust gets now uninstalled as it's only a build-dependency of cryptograpy, this saves ~175MB of disk space.

Oh and the linuxserver/beets image is broken since over a week because alpine got a new python version 3.8.10 which is not compatible with the last release of beets (v.1.4.9 which was released in 2019). More details see linuxserver/docker-beets#80. I first tried to use the last working image linuxserver/beets:1.4.9-ls94 but we need to install python-dev in there (because some node.js dependency needs it) and sadly in alpine it isn't possible to just install an older version of a package (which would work in arch or ubuntu) so it automatically installs the latest python-dev package which in turn automatically updates python to the latest version.
So the only working fix was to install the master version of beets which is compatible with this python version since 10 months. I added the link to the above issue in the Dockerfile, so lines 22+23 could be removed once the linuxserver/beets image works.

@sentriz
Copy link
Owner

sentriz commented May 24, 2021

hi! thank you for the updates on this :)

also I forgot to mention, a while ago when I first ran into the cryptography build issue I did my own attempt at fixing it while basing off of lsio/beets

and the diff is not too big (based on master)

diff --git a/Dockerfile b/Dockerfile
index ca98e50..c4c712d 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -1,21 +1,28 @@
-FROM python:3.6.6-alpine3.6
-LABEL maintainer="Senan Kelly <senan@senan.xyz>"
+FROM node:12.22.1-stretch-slim AS frontend-builder
+RUN apt update -qq
+RUN apt install -y -qq build-essential python
+WORKDIR /src
+COPY betanin_client/ .
+RUN npm install
+RUN PRODUCTION=true npm run-script build
 
-COPY \
-  ./requirements-docker.txt \
-  ./_docker_entry \
-  /
+
+FROM linuxserver/beets
+ENV HOME=/root
+RUN apk add --no-cache sudo build-base libev libffi-dev openssl-dev python3-dev
+WORKDIR /src
+COPY . .
+COPY --from=frontend-builder /src/dist/ /src/betanin_client/dist/
 RUN \
-  apk add --no-cache libev build-base libffi-dev sudo && \
-  pip --no-cache-dir install -U \
-  betanin -r /requirements-docker.txt && \
-  apk del build-base
+    /usr/bin/pip3 install --no-cache-dir --user . && \
+    apk del build-base libffi-dev openssl-dev python3-dev
+
 VOLUME /root/.local/share/betanin/
 VOLUME /root/.config/betanin/
 VOLUME /root/.config/beets/
 
-ENV PYTHONUNBUFFERED=1
-ENV PYTHONWARNINGS="ignore:Unverified HTTPS request"
 ENV UID=0
 ENV GID=0
-CMD [ "/_docker_entry" ]
+ENV PYTHONUNBUFFERED=1
+ENV PYTHONWARNINGS="ignore:Unverified HTTPS request"
+ENTRYPOINT [ "/src/docker-entry" ]
diff --git a/_docker_entry b/docker-entry
similarity index 78%
rename from _docker_entry
rename to docker-entry
index cc9a514..f8e7839 100755
--- a/_docker_entry
+++ b/docker-entry
@@ -1,5 +1,9 @@
 #!/bin/sh
 
+set -e
+
+export PATH=$HOME/.local/bin:$PATH
+
 [[ "$UID" -eq 0 ]] && exec betanin
 
 adduser -D -h ~root -u $UID -g $GID betanin
diff --git a/requirements-docker.txt b/requirements-docker.txt
deleted file mode 100644
index bced152..0000000
--- a/requirements-docker.txt
+++ /dev/null
@@ -1,4 +0,0 @@
-discogs-client
-beets-noimport
-pyacoustid
-pylast
diff --git a/setup.py b/setup.py
index 351d71f..a5475ac 100644
--- a/setup.py
+++ b/setup.py
@@ -19,6 +19,7 @@ ENTRY_POINTS = {
     ]
 }
 REQUIREMENTS = [
+    "cryptography==3.3.2",
     "beets",
     "apprise",
     "alembic==1.4.3",

this diff would completely overwrite the upstreams image's entrypoint to I think there would be no need to edit service files, etc
also it would remain completely backwards compatible like paths, volumes, UID/GID. basically only using upstreams images for the beet plugins and deps. also no need to install rust by pinning an old version of cryptography
(also as you mentioned in this version would probably need to install the latest beets too)

however, do you see any cons to this approach? are there benefits to using the lsio init system, or using the latest version cryptography that I'm missing? I'm still very open to your PR here

PS the angle I'm coming from here is that these days my time is limited with a full time job. and my preference would be to keep things really simple like

  • pinning deps in the hopes that things break less when coming back and trying to build the project
  • overwriting lsio's entrypoint so that betanin things break less if upstream decides to change the layout of the image

interested to hear your thoughts, thanks!

@sentriz sentriz force-pushed the master branch 9 times, most recently from ddc1eed to f7d0493 Compare September 5, 2021 22:28
@sentriz
Copy link
Owner

sentriz commented Sep 6, 2021

hello! thanks very much for this but closing for now. please see #46

@sentriz sentriz closed this Sep 6, 2021
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.

3 participants