Merge pull request #389 from marp-team/dont-rely-is-docker-env

Improve Docker detection for better Chromium execution

Author: yhatt

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Fixed
 
 - Prevent outputting a warning about `CHROME_PATH` env if fallbacked to Edge ([#388](https://github.com/marp-team/marp-cli/pull/388))
+- Improve Docker detection for better Chromium execution within general images ([#389](https://github.com/marp-team/marp-cli/pull/389))
 
 ## v1.4.0 - 2021-08-29
 

Dockerfile

@@ -24,7 +24,7 @@ RUN addgroup -S marp && adduser -S -g marp marp \
     && chown -R marp:marp /home/marp
 
 USER marp
-ENV IS_DOCKER true
+ENV CHROME_PATH /usr/bin/chromium-browser
 
 WORKDIR /home/marp/.cli
 COPY --chown=marp:marp . /home/marp/.cli/

jest.config.js

@@ -3,6 +3,7 @@ const { jsWithBabel } = require('ts-jest/presets')
 const esModules = [
   'ansi-regex',
   'array-union',
+  'is-docker',
   'globby',
   'os-locale',
   'slash',

package.json

@@ -100,6 +100,7 @@
     "get-stdin": "^9.0.0",
     "globby": "^12.0.1",
     "image-size": "^1.0.0",
+    "is-docker": "^3.0.0",
     "jest": "^27.0.6",
     "jest-junit": "^12.2.0",
     "nanoid": "^3.1.25",

src/config.ts

@@ -11,6 +11,7 @@ import { keywordsAsArray } from './engine/meta-plugin'
 import { error } from './error'
 import { TemplateOption } from './templates'
 import { Theme, ThemeSet } from './theme'
+import { isOfficialImage } from './utils/docker'
 
 type Overwrite<T, U> = Omit<T, Extract<keyof T, keyof U>> & U
 
@@ -105,9 +106,9 @@ export class MarpCLIConfig {
     const preview = (() => {
       const p = this.args.preview ?? this.conf.preview ?? false
 
-      if (p && process.env.IS_DOCKER) {
+      if (p && isOfficialImage()) {
         warn(
-          `Preview window cannot show in an official docker image. Preview option was ignored.`
+          `Preview window cannot show within an official docker image. Preview option was ignored.`
         )
         return false
       }

src/converter.ts

@@ -18,6 +18,7 @@ import templates, {
   TemplateResult,
 } from './templates/'
 import { ThemeSet } from './theme'
+import { isOfficialImage } from './utils/docker'
 import {
   generatePuppeteerDataDirPath,
   generatePuppeteerLaunchArgs,
@@ -472,10 +473,10 @@ export class Converter {
       )
 
       // Snapd Chromium cannot access from sandbox container to user-land `/tmp`
-      // directory so create tmp file to home directory if in Linux. (There is
-      // an exception for an official docker image)
+      // directory so always create tmp file to home directory if in Linux.
+      // (There is an exception for an official docker image)
       return baseFile.saveTmpFile({
-        home: process.platform === 'linux' && !process.env.IS_DOCKER,
+        home: process.platform === 'linux' && !isOfficialImage(),
         extension: '.html',
       })
     })()

src/marp-cli.ts

@@ -9,6 +9,7 @@ import { File, FileType } from './file'
 import { Preview, fileToURI } from './preview'
 import { Server } from './server'
 import templates from './templates'
+import { isOfficialImage } from './utils/docker'
 import { resetExecutablePath } from './utils/puppeteer'
 import version from './version'
 import watcher, { Watcher, notifier } from './watcher'
@@ -104,7 +105,7 @@ export const marpCli = async (
         preview: {
           alias: 'p',
           describe: 'Open preview window',
-          hidden: !!process.env.IS_DOCKER,
+          hidden: isOfficialImage(),
           group: OptionGroup.Basic,
           type: 'boolean',
         },

src/utils/docker.ts

@@ -0,0 +1,4 @@
+import _isDocker from 'is-docker'
+
+export const isDocker = () => isOfficialImage() || _isDocker()
+export const isOfficialImage = () => !!process.env.MARP_USER

src/utils/puppeteer.ts

@@ -3,6 +3,7 @@ import path from 'path'
 import puppeteer from 'puppeteer-core'
 import { warn } from '../cli'
 import { CLIErrorCode, error } from '../error'
+import { isDocker } from '../utils/docker'
 import { findChromeInstallation } from './chrome-finder'
 import { findEdgeInstallation } from './edge-finder'
 import { isWSL, resolveWindowsEnv } from './wsl'
@@ -30,11 +31,11 @@ export const generatePuppeteerLaunchArgs = () => {
   const args = new Set<string>(['--export-tagged-pdf'])
 
   // Docker environment and WSL environment need to disable sandbox. :(
-  if (process.env.IS_DOCKER || isWSL()) args.add('--no-sandbox')
+  if (isDocker() || isWSL()) args.add('--no-sandbox')
 
   // Workaround for Chrome 73 in docker and unit testing with CircleCI
   // https://github.com/GoogleChrome/puppeteer/issues/3774
-  if (process.env.IS_DOCKER || process.env.CI)
+  if (isDocker() || process.env.CI)
     args.add('--disable-features=VizDisplayCompositor')
 
   // Enable DocumentTransition API
@@ -44,15 +45,10 @@ export const generatePuppeteerLaunchArgs = () => {
   if (executablePath === false) {
     let findChromeError: Error | undefined
 
-    if (process.env.IS_DOCKER) {
-      // Use already known path within Marp CLI official Docker image
-      executablePath = '/usr/bin/chromium-browser'
-    } else {
-      try {
-        executablePath = findChromeInstallation()
-      } catch (e) {
-        if (e instanceof Error) findChromeError = e
-      }
+    try {
+      executablePath = findChromeInstallation()
+    } catch (e) {
+      if (e instanceof Error) findChromeError = e
     }
 
     if (!executablePath) {

test/marp-cli.ts

@@ -20,6 +20,7 @@ import {
 import { Preview } from '../src/preview'
 import { Server } from '../src/server'
 import { ThemeSet } from '../src/theme'
+import * as docker from '../src/utils/docker'
 import * as version from '../src/version'
 import { Watcher } from '../src/watcher'
 
@@ -185,10 +186,9 @@ describe('Marp CLI', () => {
         })
 
         describe('when CLI is running in an official Docker image', () => {
-          beforeEach(() => (process.env.IS_DOCKER = '1'))
-          afterEach(() => delete process.env.IS_DOCKER)
-
           it('does not output help about --preview option', async () => {
+            jest.spyOn(docker, 'isOfficialImage').mockImplementation(() => true)
+
             expect(await run()).toBe(0)
             expect(error).toHaveBeenCalledWith(
               expect.not.stringContaining('--preview')
@@ -330,10 +330,8 @@ describe('Marp CLI', () => {
         })
 
         describe('when CLI is running in an official Docker image', () => {
-          beforeEach(() => (process.env.IS_DOCKER = '1'))
-          afterEach(() => delete process.env.IS_DOCKER)
-
           it('ignores --preview option with warning', async () => {
+            jest.spyOn(docker, 'isOfficialImage').mockImplementation(() => true)
             const warn = jest.spyOn(cli, 'warn').mockImplementation()
 
             await run()
@@ -897,11 +895,10 @@ describe('Marp CLI', () => {
       })
 
       describe('when CLI is running in an official Docker image', () => {
-        beforeEach(() => (process.env.IS_DOCKER = '1'))
-        afterEach(() => delete process.env.IS_DOCKER)
-
         it('ignores --preview option with warning', async () => {
+          jest.spyOn(docker, 'isOfficialImage').mockImplementation(() => true)
           await marpCli([onePath, '--preview', '--no-output'])
+
           expect(Preview.prototype.open).not.toHaveBeenCalled()
           expect(warn).toHaveBeenCalledWith(
             expect.stringContaining('Preview option was ignored')

test/utils/puppeteer.ts

@@ -15,6 +15,9 @@ const puppeteer = (): typeof import('../../src/utils/puppeteer') =>
 const chromeFinder = (): typeof import('../../src/utils/chrome-finder') =>
   require('../../src/utils/chrome-finder')
 
+const docker = (): typeof import('../../src/utils/docker') =>
+  require('../../src/utils/docker')
+
 const edgeFinder = (): typeof import('../../src/utils/edge-finder') =>
   require('../../src/utils/edge-finder')
 
@@ -117,17 +120,15 @@ describe('#generatePuppeteerLaunchArgs', () => {
     }
   })
 
-  it('uses specific settings if running in the official Docker image', () => {
-    try {
-      process.env.IS_DOCKER = 'true'
+  it('uses specific settings if running within a Docker container', () => {
+    jest.spyOn(docker(), 'isDocker').mockImplementation(() => true)
+    jest
+      .spyOn(chromeFinder(), 'findChromeInstallation')
+      .mockImplementation(() => '/usr/bin/chromium')
 
-      const args = puppeteer().generatePuppeteerLaunchArgs()
-      expect(args.executablePath).toBe('/usr/bin/chromium-browser')
-      expect(args.args).toContain('--no-sandbox')
-      expect(args.args).toContain('--disable-features=VizDisplayCompositor')
-    } finally {
-      delete process.env.IS_DOCKER
-    }
+    const args = puppeteer().generatePuppeteerLaunchArgs()
+    expect(args.args).toContain('--no-sandbox')
+    expect(args.args).toContain('--disable-features=VizDisplayCompositor')
   })
 
   it("ignores puppeteer's --disable-extensions option if defined CHROME_ENABLE_EXTENSIONS environment value", () => {

yarn.lock

@@ -4203,6 +4203,11 @@ is-docker@^2.0.0:
   resolved "https://registry.yarnpkg.com/is-docker/-/is-docker-2.2.1.tgz#33eeabe23cfe86f14bde4408a02c0cfb853acdaa"
   integrity sha512-F+i2BKsFrH66iaUFc0woD8sLy8getkwTwtOBjvs56Cx4CgJDeKQeqfz8wAYiSb8JOprWhHH5p77PbmYCvvUuXQ==
 
+is-docker@^3.0.0:
+  version "3.0.0"
+  resolved "https://registry.yarnpkg.com/is-docker/-/is-docker-3.0.0.tgz#90093aa3106277d8a77a5910dbae71747e15a200"
+  integrity sha512-eljcgEDlEns/7AXFosB5K/2nCM4P7FQPkGc/DWLy5rmFEWvZayGrik1d9/QIY5nJ4f9YsVvBkA6kJpHn9rISdQ==
+
 is-expression@^4.0.0:
   version "4.0.0"
   resolved "https://registry.yarnpkg.com/is-expression/-/is-expression-4.0.0.tgz#c33155962abf21d0afd2552514d67d2ec16fd2ab"