-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Server: Switch to node:16-bullseye and add python to the image to support building on non amd64 platforms #5202
Conversation
I don't know what problem is being solved. Also if multiple fixes are included they should be split into multiple pull requests, with link to the bug that's being fixed. |
This was needed to be able to get it working on armv7 architecture .. I could try to strip it down a little bit .. I just dislike having multiple related PRs like that :) The tl;dr of the change is that its switching to an ubuntu focal based base image rather than a debian buster one .. focal has a lot of updated packages compared to buster (and buster is the current "stable" debian so there is no straightforward upgrade path there). Let me see if I can re-explain ... The reason your current Dockerfile works is because a few of the node packages download pre-compiled binary libraries for "most" platforms .. I think the biggest offender there was So .. the core reason that I needed to switch to ubuntu:focal is because of the glibc version that comes with buster .. Issue 1: need the equivalent of Solution:
Issue 2: need python to install sqlite libs from source .. because again there is no armv7 pre-compiled version available Solution: Install python .. Technically at this point this could be ok .. and I could revert the PR to this state .. Bonus Issue: the way apt was being run is against Dockerfile best practices. Its recommended that apt installs are all one RUN line with apt-get update, install, and a cache clean all in one docker layer. https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run Solution: clean up the |
If you would like me to run a build again on armv7 .. just to document the original build issue .. I could .. I mean I opened this PR so I can help have the mainline support armv7 properly .. so I don't have to maintain my own branch .. I would think that supporting armv7 may be very nice for this project since hobbyists may want to run the server on their RPi devices :) |
Original Build LogStep 21/36 : RUN npm run bootstrap ---> Running in 2b06b2ab3001
lerna notice cli v3.22.1
make: Entering directory '/home/joplin/packages/tools/node_modules/sharp/build' lerna ERR! npm install stderr: npm ERR! code ELIFECYCLE npm ERR! A complete log of this run can be found in: lerna ERR! npm install exited 1 in '@joplin/tools' npm ERR! A complete log of this run can be found in: The main issue there is:
in the node:12 image glibc 2.24 is used:
And .. the required libvips is not available:
|
Basically once I switched out the base image and added node back in .. it complained about missing python to install the sqlite lib .. so I added that .. |
Ok thanks for explaining. What I don't understand is that, for me, the point of a Docker image is a reproducible environment that should work wherever it's installed, but in your case it seems libvips is missing for some reasons. And if it's missing can't it just be added with "apt-get"? |
Correct, you could if the correct version was available on debian buster .. but as you see here:
The last available version if 8.4.5 .. and the node libs need 8.9.1 |
debian buster has very very conservative versions of libraries .. it often is quite troublesome to get recent things for it .. thus the switch to ubuntu focal which is the current long term supported ubuntu (also debian based at its core, but much more updated) |
And if we use this image, does it mean the server will now be compatible with ARM? (as mentioned in this issue - #4937) Also any reason why it's called "buildpack-deps:focal" instead of something clearer like "ubuntu:focal"? |
using this image will "allow" it to support ARM .. but you still have to update your dockerhub publishing pipeline to build an armv7 (and possibly some more like armv8, etc) architecture versions of the image ..
see: https://hub.docker.com/_/buildpack-deps
The In fact the previous |
Right, but what about buster-backports? |
I split out the one small tweak to its own PR so at least that can go through uncontested .. I'm having a look at your build scripts and seeing how I can easily add proper multi platform support in .. so you actually publish armv7 (and maybe arm64 while we're at it) packages to dockerhub .. I'll try the buster-backports path if possible .. |
Eh no it doesn't look like buster-backports has libvips or libc .. so I think my original approach is kind of the only way here .. |
I bumped node to 16 in this PR to match the version upstream |
This overlaps with #5337 so let's mere that one first and then I'll clean this up |
Dockerfile.server
Outdated
@@ -1,9 +1,21 @@ | |||
# https://versatile.nl/blog/deploying-lerna-web-apps-with-docker | |||
|
|||
FROM node:16 | |||
FROM buildpack-deps:focal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we use node:16, doesn't it have the dependencies that you need? If it doesn't can't we just manually install them on top of node:16? I'd rather use an image that specifically for Node if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm .. 16 is still based on buster .. but there is 16-bullseye .. so let check if that would do it ..
f83a402
to
8d9183c
Compare
Yup, switching to 16-bullseye did it .. I cleaned this up a bit .. since there is a bit of an overlap with that other vim related PR .. this will probably need a rebase once that is merged .. but this is probably the smallest diff now to make this build properly on non amd64 |
Other than possible merge conflicts once the other PR is merged .. is there any concerns left @laurent22 ? |
Thanks @piotrb, that looks good I think. Did you have the opportunity to build that image, and how does it compare to the previous one in terms of size? |
I have some screenshots in the other pr .. but the short of it is: The updated build generates 3 images now (one for each arch). amd64 turned out to be ~719megs So on average all the images are a bit smaller I'd say if you're seriously concerned about image size we should be swapping over to alpine if possible .. debian/ubuntu based images are naturally going to be larger. For comparison .. Your node deps are pretty deep .. I'm not sure if you need all that you include in the server package .. it just feels like a lot of deps for seemingly something relatively simple .. a backend server to sync notes ;) .. why does it need to know about renderers and gfm and all that stuff? (that's just an aside, but I'm sure the node_modules folder is pretty massive inside the image .. since the difference between the ~340megs and the final ~720 megs .. is going to be any packages you install .. so that's almost 500 megs of node packages and source code ;) |
This uses newer libs which are needed to build some of the dependencies which are not available pre-built on non-amd64 architectures. Also to instal sqlite from source python seems to be needed.
Alright I rebased this PR so it doesn't conflict with the main branch .. its ready to be merged (if you'd like to to potentially take a stab at slimming things down, let's do that outside this PR please) |
Reducing the Node dependencies would probably require splitting the lib package into sub-packages (not an option) or using some tree shacking algorithm. That might remove the sync target related code in particular which is not needed. As for Markdown rendering, it's for the published notes which need to be rendered to HTML |
ok let's merge and I will try the next release with this. Thanks for the update! |
The issue is that node:12 (which uses buildpack-deps:buster) uses quite old libs, among them glibc ..
Now a bunch of the node deps can pull down prebuild libs (especially this was an issue with sqlite) ..
But on armv7 (ie raspberry pi devices) these prebuilt libraries do not exist, and building them from source
requires newer libraries.
Also to instal sqlite from source python seems to be needed.
Also took the opportunity to apply some best practices and merged the layers for the apt installs and cleaned the caches afterwards.
This may be a bit of a controversial switch to use node-build, but unfortunately there doesn't seem to be official ubuntu based node base images to use instead of node:12 .. so this may need to be it .. alternate thoughts are welcome :)