Skip to content

Commit

Permalink
Merge pull request #463 from marp-team/resolving-app-chrome-path-in-mac
Browse files Browse the repository at this point in the history
Add CHROME_PATH resolver for .app directory in Mac
  • Loading branch information
yhatt authored Jul 20, 2022
2 parents 3e631cb + 7155b4d commit e072ef7
Show file tree
Hide file tree
Showing 11 changed files with 187 additions and 30 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## [Unreleased]

### Added

- macOS: Auto detection of executable path when `CHROME_PATH` env has pointed `.app` directory ([#460](https://github.com/marp-team/marp-cli/issues/460), [#463](https://github.com/marp-team/marp-cli/pull/463))

### Changed

- Docker image: Set `PATH` env to the project directory ([#462](https://github.com/marp-team/marp-cli/pull/462) by [@rhtenhove](https://github.com/rhtenhove))
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
"eslint-plugin-import": "^2.26.0",
"eslint-plugin-jest": "^26.5.1",
"express": "^4.18.1",
"fast-plist": "^0.1.2",
"get-stdin": "^9.0.0",
"globby": "^13.1.1",
"image-size": "^1.0.1",
Expand Down
6 changes: 4 additions & 2 deletions src/converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,9 @@ export class Converter {
if (this.options.baseUrl) return this.options.baseUrl

if (isFile(f) && type !== ConvertType.html) {
return isChromeInWSLHost(generatePuppeteerLaunchArgs().executablePath)
return isChromeInWSLHost(
(await generatePuppeteerLaunchArgs()).executablePath
)
? `file:${await resolveWSLPathToHost(f.absolutePath)}`
: f.absoluteFileScheme
}
Expand Down Expand Up @@ -597,7 +599,7 @@ export class Converter {

private static async runBrowser({ timeout }: { timeout?: number }) {
if (!Converter.browser) {
const baseArgs = generatePuppeteerLaunchArgs()
const baseArgs = await generatePuppeteerLaunchArgs()

Converter.browser = await launchPuppeteer({
...baseArgs,
Expand Down
2 changes: 1 addition & 1 deletion src/preview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export class Preview extends (EventEmitter as new () => TypedEmitter<Preview.Eve
}

private async launch(): Promise<Preview.Window> {
const baseArgs = generatePuppeteerLaunchArgs()
const baseArgs = await generatePuppeteerLaunchArgs()

this.puppeteerInternal = await launchPuppeteer({
...baseArgs,
Expand Down
68 changes: 64 additions & 4 deletions src/utils/chrome-finder.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,28 @@
import fs from 'fs'
import path from 'path'
import {
darwinFast,
linux,
win32,
wsl,
} from 'chrome-launcher/dist/chrome-finder'
import { parse as parsePlist } from 'fast-plist'
import { isWSL } from './wsl'

const macAppDirectoryMatcher = /.app\/?$/

// A lightweight version of Launcher.getFirstInstallation()
// https://github.com/GoogleChrome/chrome-launcher/blob/30755cde8627b7aad6caff1594c9752f80a39a4d/src/chrome-launcher.ts#L189-L192
export const findChromeInstallation = () => {
export const findChromeInstallation = async () => {
// 'wsl' platform will resolve Chrome from Windows. In WSL 2, Puppeteer cannot
// communicate with Chrome spawned in the host OS so should follow the
// original platform ('linux') if CLI was executed in WSL 2.
const platform = isWSL() === 1 ? 'wsl' : process.platform

const installations = (() => {
const installations = await (async () => {
switch (platform) {
/* istanbul ignore next: CI is not testing against darwin */
case 'darwin':
return [darwinFast()]
return await withNormalizedChromePathForDarwin(() => [darwinFast()])
case 'linux':
return linux()
case 'win32':
Expand All @@ -33,3 +37,59 @@ export const findChromeInstallation = () => {

return installations[0]
}

/**
* Run callback with modified CHROME_PATH env variable if it has been pointed to
* the ".app" directory instead of the executable binary for Darwin.
*/
export const withNormalizedChromePathForDarwin = async <T>(
callback: () => T
): Promise<T> => {
const originalChromePath = Object.prototype.hasOwnProperty.call(
process.env,
'CHROME_PATH'
)
? process.env.CHROME_PATH
: undefined

if (
originalChromePath !== undefined &&
macAppDirectoryMatcher.test(originalChromePath)
) {
try {
const appDirStat = await fs.promises.stat(originalChromePath)

if (appDirStat.isDirectory()) {
const manifestPath = path.join(
originalChromePath,
'Contents',
'Info.plist'
)
const manifestBody = await fs.promises.readFile(manifestPath)
const manifest = parsePlist(manifestBody.toString())

if (
manifest.CFBundlePackageType == 'APPL' &&
manifest.CFBundleExecutable
) {
process.env.CHROME_PATH = path.join(
originalChromePath,
'Contents',
'MacOS',
manifest.CFBundleExecutable
)
}
}
} catch (e) {
// ignore
}
}

try {
return await callback()
} finally {
if (originalChromePath !== undefined) {
process.env.CHROME_PATH = originalChromePath
}
}
}
4 changes: 2 additions & 2 deletions src/utils/puppeteer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export const generatePuppeteerDataDirPath = async (
return dataDir
}

export const generatePuppeteerLaunchArgs = () => {
export const generatePuppeteerLaunchArgs = async () => {
const args = new Set<string>(['--export-tagged-pdf', '--test-type'])

// Docker environment and WSL environment need to disable sandbox. :(
Expand All @@ -87,7 +87,7 @@ export const generatePuppeteerLaunchArgs = () => {
let findChromeError: Error | undefined

try {
executablePath = findChromeInstallation()
executablePath = await findChromeInstallation()
} catch (e: unknown) {
if (isError(e)) findChromeError = e
}
Expand Down
Empty file.
26 changes: 26 additions & 0 deletions test/utils/_mac_bundles/Valid.app/Contents/Info.plist
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>CFBundleDisplayName</key>
<string>Valid app</string>
<key>CFBundleExecutable</key>
<string>Valid app</string>
<key>CFBundleIdentifier</key>
<string>com.example.valid-app</string>
<key>CFBundleInfoDictionaryVersion</key>
<string>6.0</string>
<key>CFBundleName</key>
<string>Valid app</string>
<key>CFBundlePackageType</key>
<string>APPL</string>
<key>CFBundleShortVersionString</key>
<string>1.0.0</string>
<key>CFBundleVersion</key>
<string>1.0.0</string>
<key>LSApplicationCategoryType</key>
<string>public.app-category.utilities</string>
<key>LSMinimumSystemVersion</key>
<string>11.0.0</string>
</dict>
</plist>
Empty file.
89 changes: 74 additions & 15 deletions test/utils/puppeteer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,32 +97,32 @@ describe('#generatePuppeteerDataDirPath', () => {
})

describe('#generatePuppeteerLaunchArgs', () => {
it('finds out installed Chrome through chrome finder', () => {
it('finds out installed Chrome through chrome finder', async () => {
const getFirstInstallation = jest
.spyOn(chromeFinder(), 'findChromeInstallation')
.mockImplementation(() => '/usr/bin/chromium')
.mockResolvedValue('/usr/bin/chromium')

const args = puppeteerUtils().generatePuppeteerLaunchArgs()
const args = await puppeteerUtils().generatePuppeteerLaunchArgs()
expect(args.executablePath).toBe('/usr/bin/chromium')
expect(getFirstInstallation).toHaveBeenCalledTimes(1)

// Cache found result
puppeteerUtils().generatePuppeteerLaunchArgs()
await puppeteerUtils().generatePuppeteerLaunchArgs()
expect(getFirstInstallation).toHaveBeenCalledTimes(1)
})

it('finds out installed Edge as the fallback if not found Chrome', () => {
it('finds out installed Edge as the fallback if not found Chrome', async () => {
jest.spyOn(chromeFinder(), 'findChromeInstallation').mockImplementation()
jest
.spyOn(edgeFinder(), 'findEdgeInstallation')
.mockImplementation(() => '/usr/bin/msedge')

const args = puppeteerUtils().generatePuppeteerLaunchArgs()
const args = await puppeteerUtils().generatePuppeteerLaunchArgs()
expect(args.executablePath).toBe('/usr/bin/msedge')
expect(edgeFinder().findEdgeInstallation).toHaveBeenCalledTimes(1)
})

it('throws CLIError with specific error code if not found executable path', () => {
it('throws CLIError with specific error code if not found executable path', async () => {
const warn = jest.spyOn(console, 'warn').mockImplementation()

jest
Expand All @@ -132,7 +132,7 @@ describe('#generatePuppeteerLaunchArgs', () => {
})
jest.spyOn(edgeFinder(), 'findEdgeInstallation').mockImplementation()

expect(puppeteerUtils().generatePuppeteerLaunchArgs).toThrow(
await expect(puppeteerUtils().generatePuppeteerLaunchArgs).rejects.toThrow(
new (CLIError())(
'You have to install Google Chrome, Chromium, or Microsoft Edge to convert slide deck with current options.',
CLIErrorCode.NOT_FOUND_CHROMIUM
Expand All @@ -143,40 +143,99 @@ describe('#generatePuppeteerLaunchArgs', () => {
)
})

it('uses custom executable path if CHROME_PATH environment value was defined as executable path', () => {
it('uses custom executable path if CHROME_PATH environment value was defined as executable path', async () => {
jest.dontMock('../../src/utils/chrome-finder')

try {
process.env.CHROME_PATH = path.resolve(__dirname, '../../marp-cli.js')

const args = puppeteerUtils().generatePuppeteerLaunchArgs()
const args = await puppeteerUtils().generatePuppeteerLaunchArgs()
expect(args.executablePath).toBe(process.env.CHROME_PATH)
} finally {
delete process.env.CHROME_PATH
}
})

it('uses specific settings if running within a Docker container', () => {
it('uses specific settings if running within a Docker container', async () => {
jest.spyOn(docker(), 'isDocker').mockImplementation(() => true)
jest
.spyOn(chromeFinder(), 'findChromeInstallation')
.mockImplementation(() => '/usr/bin/chromium')
.mockResolvedValue('/usr/bin/chromium')

const args = puppeteerUtils().generatePuppeteerLaunchArgs()
const args = await puppeteerUtils().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", () => {
it("ignores puppeteer's --disable-extensions option if defined CHROME_ENABLE_EXTENSIONS environment value", async () => {
try {
process.env.CHROME_ENABLE_EXTENSIONS = 'true'

const args = puppeteerUtils().generatePuppeteerLaunchArgs()
const args = await puppeteerUtils().generatePuppeteerLaunchArgs()
expect(args.ignoreDefaultArgs).toContain('--disable-extensions')
} finally {
delete process.env.CHROME_ENABLE_EXTENSIONS
}
})

describe('with CHROME_PATH env in macOS', () => {
let originalPlatform: string | undefined

beforeEach(() => {
originalPlatform = process.platform
Object.defineProperty(process, 'platform', { value: 'darwin' })

jest.dontMock('../../src/utils/chrome-finder')

// Set Edge fallback as a sentinel
jest
.spyOn(edgeFinder(), 'findEdgeInstallation')
.mockImplementation(() => '/usr/bin/msedge')
})

afterEach(() => {
if (originalPlatform !== undefined) {
Object.defineProperty(process, 'platform', { value: originalPlatform })
}
originalPlatform = undefined

delete process.env.CHROME_PATH
})

it('does not apply custom path if CHROME_PATH was undefined', async () => {
process.env.CHROME_PATH = undefined

const args = await puppeteerUtils().generatePuppeteerLaunchArgs()
expect(args.executablePath).toBeTruthy()
})

it('tries to resolve the executable binary if CHROME_PATH was pointed to valid app bundle', async () => {
const validAppPath = path.resolve(__dirname, './_mac_bundles/Valid.app')
const validAppExecutable = path.resolve(
__dirname,
'./_mac_bundles/Valid.app/Contents/MacOS/Valid app'
)

process.env.CHROME_PATH = validAppPath

const args = await puppeteerUtils().generatePuppeteerLaunchArgs()
expect(args.executablePath).toBe(validAppExecutable)
expect(process.env.CHROME_PATH).toBe(validAppPath)
})

it('fallbacks to an original custom path if CHROME_PATH was pointed to invalid app bundle', async () => {
const invalidAppPath = path.resolve(
__dirname,
'./_mac_bundles/Invalid.app'
)

process.env.CHROME_PATH = invalidAppPath

const args = await puppeteerUtils().generatePuppeteerLaunchArgs()
expect(args.executablePath).toBe(invalidAppPath)
expect(process.env.CHROME_PATH).toBe(invalidAppPath)
})
})
})

describe('#launchPuppeteer', () => {
Expand Down
17 changes: 11 additions & 6 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3468,6 +3468,11 @@ fast-levenshtein@^2.0.6, fast-levenshtein@~2.0.6:
resolved "https://registry.yarnpkg.com/fast-levenshtein/-/fast-levenshtein-2.0.6.tgz#3d8a5c66883a16a30ca8643e851f19baa7797917"
integrity sha512-DCXu6Ifhqcks7TZKY3Hxp3y6qphY5SJZmrWMDrKcERSOXWQdMhU9Ig/PYrzyw/ul9jOIyh0N4M0tbC5hodg8dw==

fast-plist@^0.1.2:
version "0.1.2"
resolved "https://registry.yarnpkg.com/fast-plist/-/fast-plist-0.1.2.tgz#a45aff345196006d406ca6cdcd05f69051ef35b8"
integrity sha512-2HxzrqJhmMoxVzARjYFvkzkL2dCBB8sogU5sD8gqcZWv5UCivK9/cXM9KIPDRwU+eD3mbRDN/GhW8bO/4dtMfg==

fast-safe-stringify@^2.1.1:
version "2.1.1"
resolved "https://registry.yarnpkg.com/fast-safe-stringify/-/fast-safe-stringify-2.1.1.tgz#c406a83b6e70d9e35ce3b30a81141df30aeba884"
Expand Down Expand Up @@ -5404,9 +5409,9 @@ mkdirp@^1.0.4, mkdirp@~1.0.4:
integrity sha512-vVqVZQyf3WLx2Shd0qJ9xuvqgAyKPLAiqITEtqW0oIUjzo3PePDd6fW9iFz30ef7Ysp/oiWqbhszeGWW2T6Gzw==

moment@~2.29.3:
version "2.29.3"
resolved "https://registry.yarnpkg.com/moment/-/moment-2.29.3.tgz#edd47411c322413999f7a5940d526de183c031f3"
integrity sha512-c6YRvhEo//6T2Jz/vVtYzqBzwvPT95JBQ+smCytzf7c50oMZRsR/a4w88aD34I+/QVSfnoAnSBFPJHItlOMJVw==
version "2.29.4"
resolved "https://registry.yarnpkg.com/moment/-/moment-2.29.4.tgz#3dbe052889fe7c1b2ed966fcb3a77328964ef108"
integrity sha512-5LC9SOxjSc2HF6vO2CyuTDNivEdoz2IvyJJGj6X8DJ0eFyfszE0QiEd+iXmBvUP3WHxSjFH/vIsA0EN00cgr8w==

[email protected]:
version "2.0.0"
Expand Down Expand Up @@ -7539,9 +7544,9 @@ terminal-link@^2.0.0:
supports-hyperlinks "^2.0.0"

terser@^5.0.0:
version "5.14.0"
resolved "https://registry.yarnpkg.com/terser/-/terser-5.14.0.tgz#eefeec9af5153f55798180ee2617f390bdd285e2"
integrity sha512-JC6qfIEkPBd9j1SMO3Pfn+A6w2kQV54tv+ABQLgZr7dA3k/DL/OBoYSWxzVpZev3J+bUHXfr55L8Mox7AaNo6g==
version "5.14.2"
resolved "https://registry.yarnpkg.com/terser/-/terser-5.14.2.tgz#9ac9f22b06994d736174f4091aa368db896f1c10"
integrity sha512-oL0rGeM/WFQCUd0y2QrWxYnq7tfSuKBiqTjRPWrRgB46WD/kiwHwF8T23z78H6Q6kGCuuHcPB+KULHRdxvVGQA==
dependencies:
"@jridgewell/source-map" "^0.3.2"
acorn "^8.5.0"
Expand Down

0 comments on commit e072ef7

Please sign in to comment.