From ae8824527c73a4e350bf2ab1a06f4b8999e5330e Mon Sep 17 00:00:00 2001 From: argv-minus-one Date: Thu, 18 Jul 2019 21:30:47 -0700 Subject: [PATCH 1/3] Catch pipeline errors during tests. Previously, if the appdmg pipeline errored out during one of the tests, the error was swallowed and the test simply timed out after 60 seconds. Now, the test fails with the actual error. --- test/api.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/api.js b/test/api.js index eff851c..ad4bd83 100644 --- a/test/api.js +++ b/test/api.js @@ -21,6 +21,8 @@ function runAppdmg (opts, verify, cb) { progressCalled++ }) + ee.on('error', cb) + ee.on('finish', function () { try { assert.strictEqual(progressCalled, STEPS * 2) From 08c1ab849080cbfd9e2b0444ad96afcb9b051885 Mon Sep 17 00:00:00 2001 From: argv-minus-one Date: Fri, 19 Jul 2019 20:55:28 -0700 Subject: [PATCH 2/3] Add support for pausing, aborting, and promisifying appdmg execution. --- README.md | 50 ++++++++++++++++ lib/appdmg.js | 6 +- lib/pipeline.js | 54 ++++++++++++++++- test/api.js | 154 +++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 257 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 46a1320..ddbe492 100644 --- a/README.md +++ b/README.md @@ -139,6 +139,56 @@ const ee = appdmg({ }); ``` +The object returned from the `appdmg` function also has these methods and properties: + +### ee.waitFor(promise) + +Pauses execution until the given `Promise` completes. If the promise rejects, then the appdmg run is aborted. This lets you do custom asynchronous work on the disk image while it's being built. + +For example, suppose your disk image will contain a folder called “Super Secret Folder”, which you want to be hidden from the Finder. Here's how to do it, using the Xcode command-line tools: + +```javascript +const appdmg = require('appdmg'); +const execa = require('execa'); +const path = require('path'); + +const ee = appdmg({ + // appdmg options go here +}); + +async function hideSecretFolder () { + // Use the SetFile program (it comes with Xcode) to hide `Super Secret Folder` from the Finder. + await execa('SetFile', [ + '-a', + 'V', + path.join(ee.temporaryMountPath, 'Super Secret Folder') + ]); +} + +ee.on('progress', info => { + if (info.type === 'step-begin' && info.title === 'Unmounting temporary image') { + ee.waitFor(hideSecretFolder()); + // appdmg will now wait, until hideSecretFolder() is finished, before unmounting the temporary image. + } +}) +``` + +### ee.abort(err) + +Abort the appdmg run with `err` as the reason. It must be a truthy value, preferably an `Error`. + +### ee.asPromise + +A `Promise` that completes when appdmg is finished. + +### ee.temporaryImagePath + +Path to the temporary disk image. This is a writable disk image that appdmg creates and mounts while it's working. + +### ee.temporaryMountPath + +Path where the temporary disk image is currently mounted. This property is set when it's mounted, and deleted when it's unmounted. + ## OS Support Currently the only supported os is Mac OS X. diff --git a/lib/appdmg.js b/lib/appdmg.js index 1cef8fd..8c8ae3d 100644 --- a/lib/appdmg.js +++ b/lib/appdmg.js @@ -209,10 +209,11 @@ module.exports = exports = function (options) { if (err) return next(err) pipeline.addCleanupStep('unlink-temporary-image', 'Removing temporary image', function (next) { + delete pipeline.temporaryImagePath fs.unlink(temporaryImagePath, next) }) - global.temporaryImagePath = temporaryImagePath + global.temporaryImagePath = pipeline.temporaryImagePath = temporaryImagePath next(null) }) }) @@ -226,10 +227,11 @@ module.exports = exports = function (options) { if (err) return next(err) pipeline.addCleanupStep('unmount-temporary-image', 'Unmounting temporary image', function (next) { + delete pipeline.temporaryMountPath hdiutil.detach(temporaryMountPath, next) }) - global.temporaryMountPath = temporaryMountPath + global.temporaryMountPath = pipeline.temporaryMountPath = temporaryMountPath next(null) }) }) diff --git a/lib/pipeline.js b/lib/pipeline.js index edba674..c7f1d7b 100644 --- a/lib/pipeline.js +++ b/lib/pipeline.js @@ -9,11 +9,23 @@ class Pipeline extends EventEmitter { this.steps = [] this.totalSteps = 0 this.currentStep = 0 + this._waitQueue = [] this.cleanupList = [] this.cleanupStore = {} } + async _wait () { + // Drain the waitingFor queue. Wait on every promise that gets added to the queue. Only return once there are no promises left in the queue. + let waitOn + + // Although assignment expressions are normally prohibited by StandardJS, the only other way I know of to write this is with a while (true) loop, which is dangerous. + // eslint-disable-next-line no-cond-assign + while (waitOn = this._waitQueue.pop()) { + await waitOn + } + } + _progress (obj) { obj.current = this.currentStep obj.total = this.totalSteps @@ -32,18 +44,18 @@ class Pipeline extends EventEmitter { }) } else { this._progress({ type: 'step-end', status: 'ok' }) - this[nextAction](cb) + this._wait().then(() => this[nextAction](cb), cb) } } next.skip = () => { this._progress({ type: 'step-end', status: 'skip' }) - this[nextAction](cb) + this._wait().then(() => this[nextAction](cb), cb) } this.currentStep++ this._progress({ type: 'step-begin', title: step.title }) - step.fn(next) + this._wait().then(() => step.fn(next), next) } addStep (title, fn) { @@ -98,8 +110,10 @@ class Pipeline extends EventEmitter { process.nextTick(() => { this._run((err) => { if (err) { + this._completed = { err } this.emit('error', err) } else { + this._completed = true this.emit('finish') } }) @@ -107,6 +121,40 @@ class Pipeline extends EventEmitter { return this } + + waitFor (promise) { + this._waitQueue.push(promise) + + // Suppress unhandled promise rejection warnings. Rejections will be handled later. + promise.catch(() => {}) + } + + abort (err) { + this.waitFor(Promise.reject(err)) + } + + get asPromise () { + let { _asPromise: p } = this + + if (!p) { + const { _completed: c } = this + + if (c === true) { + p = Promise.resolve() + } else if (typeof c === 'object' && 'err' in c) { + p = Promise.reject(c.err) + } else { + p = new Promise((resolve, reject) => { + this.once('finish', resolve) + this.once('error', reject) + }) + } + + this._asPromise = p + } + + return p + } } module.exports = Pipeline diff --git a/test/api.js b/test/api.js index ad4bd83..fcd0a05 100644 --- a/test/api.js +++ b/test/api.js @@ -2,6 +2,7 @@ 'use strict' +const execa = require('execa') const fs = require('fs') const path = require('path') const temp = require('fs-temp') @@ -13,7 +14,7 @@ const visuallyVerifyImage = require('./lib/visually-verify-image') const STEPS = 22 -function runAppdmg (opts, verify, cb) { +function runAppdmg (opts, verify, cb, extra) { let progressCalled = 0 const ee = appdmg(opts) @@ -34,6 +35,10 @@ function runAppdmg (opts, verify, cb) { const expected = path.join(__dirname, verify.visually) visuallyVerifyImage(opts.target, verify.title, expected, cb) }) + + if (extra) { + extra(ee) + } } describe('api', function () { @@ -45,7 +50,14 @@ describe('api', function () { }) afterEach(function () { - fs.unlinkSync(targetPath) + try { + fs.unlinkSync(targetPath) + } catch (err) { + if (err.code !== 'ENOENT') { + throw err + } + } + fs.rmdirSync(path.dirname(targetPath)) }) @@ -181,4 +193,142 @@ describe('api', function () { runAppdmg(opts, verify, done) }) + + it('pauses pipeline execution when told to', function (done) { + this.timeout(60000) // 1 minute + + const opts = { + target: targetPath, + basepath: path.join(__dirname, 'assets'), + specification: { + title: 'Test Title', + icon: 'TestIcon.icns', + background: 'TestBkg.png', + contents: [ + { x: 448, y: 344, type: 'link', path: '/Applications', name: 'System Apps' }, + { x: 192, y: 344, type: 'file', path: 'TestApp.app', name: 'My Nice App.app' }, + { x: 512, y: 128, type: 'file', path: 'TestDoc.txt', name: 'Documentation.txt' }, + { x: 0, y: 0, type: 'file', path: path.join('TestApp.app', 'Contents', 'Resources'), name: 'Super Secret Folder' } + ] + } + } + + const verify = { + format: 'UDZO', + title: 'Test Title', + visually: 'accepted-3.png' + } + + runAppdmg(opts, verify, done, ee => { + async function hideSecretFolder () { + // Sleep for 0.5 seconds, to verify that the disk image isn't unmounted before this promise resolves. + await new Promise(resolve => setTimeout(resolve, 500)) + + await execa('SetFile', [ + '-a', + 'V', + path.join(ee.temporaryMountPath, 'Super Secret Folder') + ]) + } + + ee.on('progress', info => { + if (info.type === 'step-begin' && info.title === 'Unmounting temporary image') { + ee.waitFor(hideSecretFolder()) + } + }) + }) + }) + + it('aborts pipeline execution when told to', function (done) { + this.timeout(5000) // 5 seconds + + const opts = { + target: targetPath, + source: path.join(__dirname, 'assets', 'appdmg.json') + } + + const ee = appdmg(opts) + const err = new Error('test error') + + ee.on('progress', () => { + ee.abort(err) + }) + + ee.on('finish', () => { + done(new Error('Pipeline execution did not abort')) + }) + + ee.on('error', _err => { + if (err === _err) { + if (fs.existsSync(targetPath)) { + done(new Error('Pipeline execution was aborted, but it created a disk image file anyway')) + } else { + done() + } + } else { + done(new Error(`Pipeline execution was aborted with wrong error: ${_err}`)) + } + }) + }) + + for (const getPromiseAfterEnd of [false, true]) { + it(`resolves asPromise when done${getPromiseAfterEnd ? ', even after the fact' : ''}`, function (done) { + this.timeout(30000) // 30 seconds + + const opts = { + target: targetPath, + source: path.join(__dirname, 'assets', 'appdmg.json') + } + + const ee = appdmg(opts) + + if (getPromiseAfterEnd) { + ee.on('error', done) + ee.on('finish', () => { + ee.asPromise.then(done, done) + }) + } else { + ee.asPromise.then(done, done) + } + }) + + it(`rejects asPromise when aborted${getPromiseAfterEnd ? ', even after the fact' : ''}`, function (done) { + this.timeout(5000) // 5 seconds + + const opts = { + target: targetPath, + source: path.join(__dirname, 'assets', 'appdmg.json') + } + + const ee = appdmg(opts) + + const err = new Error('test error') + + ee.on('progress', info => { + if (info.type === 'step-begin' && info.title === 'Creating temporary image') { + ee.abort(err) + } + }) + + function getAndCheckPromise () { + ee.asPromise.then( + () => done(new Error('appdmg().asPromise should have rejected, but didn\'t')), + _err => { + if (err === _err) { + done() + } else { + done(new Error(`appdmg().asPromise rejected with wrong error: ${_err}`)) + } + } + ) + } + + if (getPromiseAfterEnd) { + ee.on('error', getAndCheckPromise) + ee.on('finish', getAndCheckPromise) + } else { + getAndCheckPromise() + } + }) + } }) From 388a8533677980d4920ce2cce81fd007ae3f21ba Mon Sep 17 00:00:00 2001 From: argv-minus-one Date: Fri, 26 Jul 2019 15:10:17 -0700 Subject: [PATCH 3/3] Some fixes for pausing/aborting pipelines: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Document Pipeline#hasErrored. Set it to true before emitting step error events, rather than after, so that event handlers know that appdmg has entered an error state. • Make Pipeline#abort() do nothing if hasErrored is already true. • When the pipeline is waiting on multiple promises, wait for all of them, even if some of them reject. That way, no one gets surprised by appdmg starting cleanup too soon. • Don't throw rejections (just log them) from waitFor'd promises when hasErrored is already true. Prevents infinite loops. --- README.md | 10 +++++++++- lib/pipeline.js | 29 +++++++++++++++++++++++++---- test/api.js | 31 +++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ddbe492..680c37e 100644 --- a/README.md +++ b/README.md @@ -141,6 +141,12 @@ const ee = appdmg({ The object returned from the `appdmg` function also has these methods and properties: +### ee.hasErrored + +This property is initially `false`. It becomes `true` when appdmg encounters an error, and is cleaning up. + +When `hasErrored` is `true`, avoid doing anything in an event handler that could throw. Doing so will prevent appdmg from cleaning up after an error (unmounting the temporary disk image, deleting it, and so on). + ### ee.waitFor(promise) Pauses execution until the given `Promise` completes. If the promise rejects, then the appdmg run is aborted. This lets you do custom asynchronous work on the disk image while it's being built. @@ -166,7 +172,7 @@ async function hideSecretFolder () { } ee.on('progress', info => { - if (info.type === 'step-begin' && info.title === 'Unmounting temporary image') { + if (!ee.hasErrored && info.type === 'step-begin' && info.title === 'Unmounting temporary image') { ee.waitFor(hideSecretFolder()); // appdmg will now wait, until hideSecretFolder() is finished, before unmounting the temporary image. } @@ -177,6 +183,8 @@ ee.on('progress', info => { Abort the appdmg run with `err` as the reason. It must be a truthy value, preferably an `Error`. +This method has no effect if appdmg has already encountered an error (indicated by `hasErrored` being `true`). + ### ee.asPromise A `Promise` that completes when appdmg is finished. diff --git a/lib/pipeline.js b/lib/pipeline.js index c7f1d7b..f5a2887 100644 --- a/lib/pipeline.js +++ b/lib/pipeline.js @@ -10,6 +10,7 @@ class Pipeline extends EventEmitter { this.totalSteps = 0 this.currentStep = 0 this._waitQueue = [] + this.hasErrored = false this.cleanupList = [] this.cleanupStore = {} @@ -17,12 +18,30 @@ class Pipeline extends EventEmitter { async _wait () { // Drain the waitingFor queue. Wait on every promise that gets added to the queue. Only return once there are no promises left in the queue. - let waitOn + let waitOn, lastError // Although assignment expressions are normally prohibited by StandardJS, the only other way I know of to write this is with a while (true) loop, which is dangerous. // eslint-disable-next-line no-cond-assign while (waitOn = this._waitQueue.pop()) { - await waitOn + try { + await waitOn + } catch (err) { + // Wait for all queued promises, not just until one of them rejects. Once the queue is empty, throw the last error (that isn't null or undefined), and log all other errors. That way, no one gets surprised by appdmg starting cleanup too soon. + if (lastError != null) { + console.error(lastError) + } + + lastError = err + } + } + + if (lastError != null) { + if (this.hasErrored) { + // Don't throw at all if appdmg is already cleaning up from an error. + console.error(lastError) + } else { + throw lastError + } } } @@ -36,8 +55,8 @@ class Pipeline extends EventEmitter { _runStep (step, nextAction, cb) { const next = (err) => { if (err) { - this._progress({ type: 'step-end', status: 'error' }) this.hasErrored = true + this._progress({ type: 'step-end', status: 'error' }) this.runRemainingCleanups(function (err2) { if (err2) console.error(err2) cb(err) @@ -130,7 +149,9 @@ class Pipeline extends EventEmitter { } abort (err) { - this.waitFor(Promise.reject(err)) + if (!this.hasErrored) { + this.waitFor(Promise.reject(err)) + } } get asPromise () { diff --git a/test/api.js b/test/api.js index fcd0a05..b82a070 100644 --- a/test/api.js +++ b/test/api.js @@ -331,4 +331,35 @@ describe('api', function () { } }) } + + it('doesn\'t go into an infinite loop when trying to waitFor() on a cleanup step', function (done) { + this.timeout(60000) // 1 minute + + const opts = { + target: targetPath, + source: path.join(__dirname, 'assets', 'appdmg.json') + } + + const ee = appdmg(opts) + const err = new Error('test error') + err[require('util').inspect.custom] = () => '' // Hide from console.error + + ee.on('error', _err => { + if (_err === err) { + done() + } else { + done(new Error(`appdmg errored with wrong error: ${_err}`)) + } + }) + + ee.on('finish', () => { + done(new Error('appdmg should have errored, but didn\'t')) + }) + + ee.on('progress', info => { + if (info.type === 'step-begin' && info.title === 'Unmounting temporary image') { + ee.waitFor(Promise.reject(err)) + } + }) + }) })