Docker build now fails with what look like latest commits related to xterm-addon-ligatures

[exinknighthigh]

The current xtermjs code (commit 01432a3) in a Docker container on Windows (Docker For Windows) fails when doing a build on the stock DockerFile: docker build -t stockxtermjs .

The problems seems related to the xterm-addon-ligatures

Error:

Step 9/9 : RUN npm run build && npm run test
---> Running in 92bc2fd18903

xterm@4.7.0 build /usr/src/app
tsc -b ./tsconfig.all.json

addons/xterm-addon-ligatures/src/font.ts(6,29): error TS2307: Cannot find module 'font-finder' or its corresponding type declarations.
addons/xterm-addon-ligatures/src/font.ts(7,32): error TS2307: Cannot find module 'font-ligatures' or its corresponding type declarations.
addons/xterm-addon-ligatures/src/index.ts(7,22): error TS2307: Cannot find module 'font-ligatures' or its corresponding type declarations.
addons/xterm-addon-ligatures/src/index.ts(75,14): error TS2347: Untyped function calls may not accept type arguments.
addons/xterm-addon-ligatures/src/index.ts(76,9): error TS7006: Parameter 'range' implicitly has an 'any' type.
addons/xterm-addon-ligatures/src/index.test.ts(7,24): error TS2307: Cannot find module 'sinon' or its corresponding type declarations.
addons/xterm-addon-ligatures/src/index.test.ts(9,29): error TS2307: Cannot find module 'font-finder' or its corresponding type declarations.
addons/xterm-addon-ligatures/src/index.test.ts(10,32): error TS2307: Cannot find module 'font-ligatures' or its corresponding type declarations.
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! xterm@4.7.0 build: tsc -b ./tsconfig.all.json
npm ERR! Exit status 2
npm ERR!
npm ERR! Failed at the xterm@4.7.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR! /root/.npm/_logs/2020-07-10T02_46_18_037Z-debug.log
The command '/bin/sh -c npm run build && npm run test' returned a non-zero code: 2

Details

• Browser and browser version: Any
• OS version: Windows 10 (Docker For Windows)
• xterm.js version: 4.7.0

Steps to reproduce

This error should easily reproduced by pulling the latest code and simply attempting a docker build command in Docker For Windows. Whether it can be reproduced on Docker running on a Linux host is unknown.

[Tyriar]

Step 9/9 : RUN npm run build && npm run test

Can you check the output of the yarn step? It looks like dependencies aren't getting installed?

[exinknighthigh]

I'm not sure where I would find that, especially in the context of a Docker container that runs for about 6 seconds and then fails with the above error. Do you mean the npm debug log? That I've attached.

2020-07-15T02_01_19_686Z-debug.log

Here are some things I have discovered:

• I can recreate the problem on 2 different machines on 2 different networks. One is running Docker for Windows and the other Docker on Linux.

• Starting with xtermjs 4.0 I'm unable to actually do a docker build or docker-compose up without running into an npm ERR
  as a function of CMD npm run build && npm run start

• Version 3.14.5 works fine.

Am I the only one reporting Docker build problems with 4.0 and above?

[Tyriar]

@exinknighthigh the entry point conditionally runs npm install:

xterm.js/Dockerfile

Line 8 in ab63b51

ENTRYPOINT ["/bin/bash", "-c", "if [[ ! -d node_modules ]]; then npm install; fi; exec \"${@:0}\";"]

Maybe that never runs, I think ideally we would just run that regardless so everything is contained, especially now that there are native dependencies in xterm-addon-ligatures.

Am I the only one reporting Docker build problems with 4.0 and above?

I know there are other users but no one in the team uses the dockerfile anymore so we typically rely on external contributors to fix it.

[exinknighthigh]

Tyriar - Thanks for the feedback.

The problem is not the ENTRYPOINT

The problem is that those ligatures specific modules occur as dependencies and devDependencies in the xterm-addons-ligatures package.json, not the root package.json

The DockerFile is only doing a RUN npm install based on the root package.json

It never picks up those other dependencies.

I can fix the problem in the DockerFile by doing another RUN npm install on the ligatures package.json or I can fix it by moving those dependencies into the root package.json. I'm not sure either approach is particularly clean.

[exinknighthigh]

Also, in the context of docker build, 5 of the ligatures tests are failing due to latency. I had to remove these to get the build to succeed:

5 failing

1. xterm-addon-ligatures
   returns the correct set of ranges once the font has loaded:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/usr/src/app/addons/xterm-addon-ligatures/out/index.test.js)
   at listOnTimeout (internal/timers.js:549:17)
   at processTimers (internal/timers.js:492:7)

2. xterm-addon-ligatures
   handles quoted font names:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/usr/src/app/addons/xterm-addon-ligatures/out/index.test.js)
   at listOnTimeout (internal/timers.js:549:17)
   at processTimers (internal/timers.js:492:7)

3. xterm-addon-ligatures
   falls back to later fonts if earlier ones are not present:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/usr/src/app/addons/xterm-addon-ligatures/out/index.test.js)
   at listOnTimeout (internal/timers.js:549:17)
   at processTimers (internal/timers.js:492:7)

4. xterm-addon-ligatures
   uses the current font value:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/usr/src/app/addons/xterm-addon-ligatures/out/index.test.js)
   at listOnTimeout (internal/timers.js:549:17)
   at processTimers (internal/timers.js:492:7)

5. xterm-addon-ligatures
   allows multiple terminal instances that use different fonts:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/usr/src/app/addons/xterm-addon-ligatures/out/index.test.js)
   at listOnTimeout (internal/timers.js:549:17)
   at processTimers (internal/timers.js:492:7)

[exinknighthigh]

So I was able to solve all these problems be rewriting the Dockerfile. In rewriting, I think there is a bigger issue, though, The Dockerfile REALLY needs love, particularly from a security standpoint. Out of the box, xtermjs runs under root. At the very least, I would include comments in the file about how to make it multi stage and run under a non root user with limited access, especially given the nature of this project. I'm happy to provide an example. Alternatively, if the Dockerfile and docker-compose.yml isn't getting maintained, you might want to just deprecate them. In any case, thanks for all your hard work.

[Tyriar]

The Dockerfile REALLY needs love, particularly from a security standpoint

I don't think this was ever meant to be a secure dockerfile, more as a convenient way to develop in a dockerfile this is why node_modules is shared for example. Running with limited access sounds like a good idea though.

[jerch]

Imho the docker thingy is currently unmaintained and, if remember correctly, we already had the discussion whether to keep it at all. We voted for keeping it while it works, but ofc this is not the best idea to deal with it. It might break anytime and even worse, imho we dont even have a test stage for it, thus it might break silently, doh.

@exinknighthigh
Yes root in docker is dangerous, if you can clean up the docker file to work properly with higher security in mind, that would be really helpful. But further note that none of us develops with docker in mind (@Tyriar correct me if I am wrong), so it might fall short again right after. Unless you want to keep an eye on its maintenance, I'd say we should think about removing it from the official repo.

[Tyriar]

@jerch I think you're right, we should pull it out so we don't have to worry about supporting these types of requests.