Provide support to run inside Docker #4

jurgenhaas · 2021-03-03T10:21:08Z

This PR provides:

New files

docker-entrypoint.sh
Dockerfile
mdshow.sh: script to run on the host to start the Docker container and run mdshow commands.

Changes in assets/pandoc/templates/default.revealjs

Get theme CSS from .mdshow/theme/... instead of $revealjs-url$/theme/... because that way, custom themes that also provide additional assets and not only a css file are supported.

Changes in mdshow

Switched from yarn to npm
Changed config path from homedir to /opt because otherwise we run into permission issues as the host user is unknown inside the container
Call gulp css-themes once in sync-custom-themes to get a potential custom theme being compiled initially and not only after updates
Switch arguments in $(MDSHOW_CONFIG)/reveal.js/dist/theme/%: $(MDSHOW_CONFIG)/theme/%/assets and also use copy instead of link.
Add new argument --chrome-arg="--no-sandbox" to PDF creation as the chrome process wouldn't launch otherwise.

jceb · 2021-03-03T17:41:44Z

Does the non-Docker version still work?

jurgenhaas · 2021-03-03T19:14:59Z

I haven't tried, but it should, the changes are minor with regard to that.

jurgenhaas · 2021-03-04T07:06:09Z

@jceb Just thinking, we could of course add some context checking to the make script to keep it at its original behaviour on the host and do the docker specific stuff inside.

jceb · 2021-03-03T17:42:49Z

assets/pandoc/templates/default.revealjs

@@ -24,9 +24,9 @@ $endif$
  <!--   $styles.html()$ -->
  <!-- </style> -->
 $if(theme)$
-  <link rel="stylesheet" href="$revealjs-url$/theme/$theme$.css" id="theme">
+  <link rel="stylesheet" href=".mdshow/theme/$theme$/assets/$theme$.css" id="theme">


This looks like an incompatible change, same below.

Well, even on a host based system I'd suggest to grab assets from the config path rather than from the distro path, because I wouldn't want to copy custom assets like a custom theme into the distro directory. So yes, this changes the location for the assets, but that should work both inside the container and on the host likewise.

jceb · 2021-03-03T17:44:42Z

Dockerfile

+                autoconf automake curl g++ gcc jq make npm rsync unzip \
+                ca-certificates fonts-liberation libappindicator3-1 libasound2 libatk-bridge2.0-0 libatk1.0-0 \
+                libc6 libcairo2 libcups2 libdbus-1-3 libexpat1 libfontconfig1 libgbm1 libgcc1 libglib2.0-0 \
+                libgtk-3-0 libnspr4 libnss3 libpango-1.0-0 libpangocairo-1.0-0 libstdc++6 libx11-6 libx11-xcb1 \


What are all these x11 libraries needed for?

They are required for the PDF creation.

jceb · 2021-03-03T17:46:39Z

docker-entrypoint.sh

@@ -0,0 +1,30 @@
+#!/bin/sh
+set -e


Please add set -u as well.

That would break the test below, which verifies if the DOCKER_HOST variable is available, so we can't add this here.

docker-entrypoint.sh

jceb · 2021-03-03T17:52:29Z

mdshow

@@ -1,12 +1,13 @@
 #!/usr/bin/make -f
 # TODO: newer versions of env support this: !/usr/bin/env -S make -f
+# NOTE Jürgen Haas: switched from yarn to npm, because yarn does not install node_modules/node-jp/bin


Strange, it does for me on the desktop. Why wouldn't it work in docker? Maybe use a nodeJS docker image instead of ubuntu? I'm a big fan of Alpine which is much smaller than ubuntu images.

I've had that issue on the desktop before and it was most likely because of missing dependencies. So I tried npm instead and that worked. I have no intention to spend more time getting yarn to work as it works just fine with npm.

I use Alpine normally as well, but started here with Ubuntu because I wanted to replicate my hosts environment. For now, I wouldn't want to spend more time trying to resolve dependencies again, just to save a few bytes - although I otherwise agree with your statement.

jceb · 2021-03-03T18:27:17Z

mdshow

 # Author: Jan Christoph Ebersbach <jceb@e-jc.de>
 # Copyright (c) 2020 Jan Christoph Ebersbach
 # License: Apache-2.0

 SHELL := /bin/bash

-MDSHOW_CONFIG = ${HOME}/.config/mdshow
+MDSHOW_CONFIG = /opt/mdshow


This is an incompatible change when mdshow runs outside docker. Could we mitigate this by adding another variable that's overridden within docker, something along the lines of:

CONFIG_PATH = ${HOME}/.config MDSHOW_CONFIG = $(CONFIG_PATH)/mdshow

And then call make with make <target> CONFIG_PATH=/opt

Good idea, I've added the CONFIG_PATH variable to the mdshow script so that it'll always be used.

jceb · 2021-03-03T18:28:25Z

mdshow

@@ -137,6 +138,7 @@ sass: $(MDSHOW_CONFIG)/reveal.js \

 .sync-custom-themes: $(wildcard $(MDSHOW_CONFIG)/theme/*/source/*.scss)
 	@[ -n "$^" ] && rsync -u $^ $(MDSHOW_CONFIG)/reveal.js/css/theme/source/ || true
+	@[ -n "$^" ] && cd $(MDSHOW_CONFIG)/reveal.js && gulp css-themes || true


Why call gulp manually here, shouldn't it be called by gulp-watch automatically?

During initial startup, the gulp watch is too late to compile the custom theme which doesn't come with a css but only the SCSS file.

jceb · 2021-03-03T18:33:33Z

docker-entrypoint.sh

@@ -0,0 +1,30 @@
+#!/bin/sh


I haven't fully wrapped my head around this script. What is it good for? Why not use mdshow as entrypoint?

This way you can execute other commands in the container too if you wanted to.

jceb · 2021-03-03T18:42:58Z

mdshow.sh

@@ -0,0 +1,7 @@
+#!/bin/bash


Could we turn this script into a mdshow Makefile target, e.g. ? This way the user always calls mdshow instead of a separate command.

How about we make the mdshow Makefile aware of docker and add a DOCKER variable. When it's set it executes the Makefile targets in the docker container. A call would look like this mdshow pdf DOCKER=1. Just a few targets would need to capture and react to it. The majority of the targets wouldn't need to know about it. This could be extended one step further by setting the variable through the name of the command, e.g. if the command is called dmdshow then DOCKER=1 is set.

The makefile is currently completely encapsulated in the container and not accessible from the host. This script file is not even required, it's just a shortcut. People can use this or have their own trigger command in the host. But at least, this is the only thing that needs to b distributed and I prefer that to be as small as possible. Everything else is loaded automatically as t is contained in the docker image.

jceb · 2021-03-03T18:44:42Z

mdshow.sh

+  THEME=--volume=${MDSHOW_THEME_PATH}:/opt/mdshow/theme/$(basename ${MDSHOW_THEME_PATH})
+fi
+
+docker run --user=$(id -u) --rm -it --net=host ${THEME} --volume=$(pwd):/mdshow --workdir=/mdshow registry.lakedrops.com/docker/mdshow mdshow $@


One last thing, how does the docker integration handle multiple parallel executions of the mdshow command? The default mdshow command takes a port to separate multiple executions.

That works with the Docker version too, the same PORT argument can be used for this in that context as well.

jceb · 2021-03-21T19:16:21Z

@jurgenhaas hey, sorry for the delay. I didn't realize that I have to request a review. So here are the comments.

jurgenhaas · 2021-03-27T11:19:03Z

@jceb worked on your review and committed the changes.

Provide support to run inside Docker

2d48bd2

jurgenhaas mentioned this pull request Mar 3, 2021

Make it available as Docker image #3

Open

jceb requested changes Mar 21, 2021

View reviewed changes

jurgenhaas added 2 commits March 27, 2021 12:13

Incorporate changes from @jceb's review

15bc0f8

Incorporate changes from @jceb's review

bdd8438

jurgenhaas closed this May 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide support to run inside Docker #4

Provide support to run inside Docker #4

jurgenhaas commented Mar 3, 2021

jceb commented Mar 3, 2021

jurgenhaas commented Mar 3, 2021

jurgenhaas commented Mar 4, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb Mar 3, 2021

jurgenhaas Mar 27, 2021

jceb commented Mar 21, 2021

jurgenhaas commented Mar 27, 2021

Provide support to run inside Docker #4

Provide support to run inside Docker #4

Conversation

jurgenhaas commented Mar 3, 2021

jceb commented Mar 3, 2021

jurgenhaas commented Mar 3, 2021

jurgenhaas commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jceb commented Mar 21, 2021

jurgenhaas commented Mar 27, 2021