From 27c23058dc0057569c67a3beee33967f98d9d4b9 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 25 Sep 2018 11:20:58 -0600 Subject: [PATCH] Alter build process to rely on canaries only With the react-sdk and js-sdk having their `npm start`s split out (as per https://github.com/matrix-org/matrix-react-sdk/pull/2175 and https://github.com/matrix-org/matrix-js-sdk/pull/742) we can trigger an initial build ourselves and start the watcher afterwards. This canary approach has a very slight speed increase over serially running all the commands as the watcher can be started as early as possible. This all can be improved and potentially eliminated with a bit more planning, however: https://github.com/vector-im/riot-web/issues/7386 --- package-lock.json | 32 ++++++++++------- package.json | 1 - scripts/block-on-sdk-build.js | 57 +++++------------------------ scripts/build-watch-sdk.js | 67 ++++++++++++++++++++++++----------- 4 files changed, 73 insertions(+), 84 deletions(-) diff --git a/package-lock.json b/package-lock.json index e7da86c95e..ab3b6f0dde 100644 --- a/package-lock.json +++ b/package-lock.json @@ -384,12 +384,6 @@ "integrity": "sha1-GdOGodntxufByF04iu28xW0zYC0=", "dev": true }, - "async-lock": { - "version": "1.1.3", - "resolved": "https://registry.npmjs.org/async-lock/-/async-lock-1.1.3.tgz", - "integrity": "sha512-nxlfFLGfCJ1r7p9zhR5OuL6jYkDd9P7FqSitfLji+C1NdyhCz4+rWW3kiPiyPASHhN7VlsKEvRWWbnME9lYngw==", - "dev": true - }, "asynckit": { "version": "0.4.0", "resolved": "https://registry.npmjs.org/asynckit/-/asynckit-0.4.0.tgz", @@ -4180,12 +4174,14 @@ "balanced-match": { "version": "1.0.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, "dev": true, + "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -4200,17 +4196,20 @@ "code-point-at": { "version": "1.1.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "concat-map": { "version": "0.0.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "console-control-strings": { "version": "1.1.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "core-util-is": { "version": "1.0.2", @@ -4327,7 +4326,8 @@ "inherits": { "version": "2.0.3", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "ini": { "version": "1.3.5", @@ -4339,6 +4339,7 @@ "version": "1.0.0", "bundled": true, "dev": true, + "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -4353,6 +4354,7 @@ "version": "3.0.4", "bundled": true, "dev": true, + "optional": true, "requires": { "brace-expansion": "^1.1.7" } @@ -4360,7 +4362,8 @@ "minimist": { "version": "0.0.8", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "minipass": { "version": "2.2.4", @@ -4384,6 +4387,7 @@ "version": "0.5.1", "bundled": true, "dev": true, + "optional": true, "requires": { "minimist": "0.0.8" } @@ -4464,7 +4468,8 @@ "number-is-nan": { "version": "1.0.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "object-assign": { "version": "4.1.1", @@ -4597,6 +4602,7 @@ "version": "1.0.2", "bundled": true, "dev": true, + "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", diff --git a/package.json b/package.json index 333966ce30..a3f6985ca9 100644 --- a/package.json +++ b/package.json @@ -81,7 +81,6 @@ "url": "^0.11.0" }, "devDependencies": { - "async-lock": "^1.1.3", "autoprefixer": "^6.6.0", "babel-cli": "^6.5.2", "babel-core": "^6.14.0", diff --git a/scripts/block-on-sdk-build.js b/scripts/block-on-sdk-build.js index 022543c615..eb3597292d 100644 --- a/scripts/block-on-sdk-build.js +++ b/scripts/block-on-sdk-build.js @@ -1,19 +1,12 @@ const path = require('path'); const chokidar = require('chokidar'); -const AsyncLock = require('async-lock'); -// This script sits and waits for a build of an underlying SDK (js or react) -// to complete before exiting. This is done by cooperating with build-watch-sdk.js -// by waiting for it's signal to start watching for file changes, then watching -// the SDK's build output for a storm of file changes to stop. Both the js-sdk -// and react-sdk compile each file one by one, so by waiting for file changes to -// stop we know it is safe to continue and therefore exit this script. We give -// some leeway to the SDK's build process to handle larger/more complex files -// through use of a reset-on-touch countdown timer. When a file change occurs, -// we reset the countdown to WAIT_TIME and let it count down. If the count down -// completes, we consider ourselves having left the file system update storm and -// therefore can consider a build of the SDK to be fully completed. +// This script waits for a signal that an underlying SDK (js or react) has finished +// enough of the build process to be safe to rely on. In riot-web's case, this means +// that the underlying SDK has finished an initial build and is getting ready to watch +// for changes. This is done through use of a canary file that is deleted when it is +// safe to continue (see build-watch-sdk.js for why we listen for a delete event). // Why do we block in the first place? Because if riot-web starts it's initial // build (via webpack-dev-server) and the react-sdk or js-sdk are halfway through @@ -26,21 +19,12 @@ const AsyncLock = require('async-lock'); // build which riot-web is waiting for. When complete, riot-web starts building as // per normal. -// Why the canary to begin watching? Because we can't reliably determine that the -// build triggered by `npm install` in each SDK is actually the process we need to -// be watching for. To work around this, build-watch-sdk.js does the `npm install` -// and follows through with a canary to signal to this script that it should start -// watching for changes produced by that SDK's `npm start` (run immediately after -// the canary is sent). - - -const WAIT_TIME = 5000; // ms function waitForCanary(canaryName) { return new Promise((resolve, reject) => { - const filename = path.resolve(path.join(".tmp", canaryName)); + const filename = path.resolve(path.join(".tmp", canaryName + ".canary")); - // See triggerCanarySignal in build-watch-sdk.js for why we watch for `unlink` + // See build-watch-sdk.js for why we listen for 'unlink' specifically. const watcher = chokidar.watch(filename).on('unlink', (path) => { console.log("[block-on-build] Received signal to start watching for builds"); watcher.close(); @@ -49,31 +33,6 @@ function waitForCanary(canaryName) { }); } -function waitOnSdkBuild(sdkName) { - // First we wait for a local canary file to be changed - return waitForCanary(sdkName).then(() => new Promise((resolve, reject) => { - const buildDirectory = path.dirname(require.resolve(`matrix-${sdkName}-sdk`)); - const lock = new AsyncLock(); - let timerId = null; - - const watcher = chokidar.watch(buildDirectory).on('all', (event, path) => { - lock.acquire("timer", (done) => { - if (timerId !== null) { - //console.log("Resetting countdown"); - clearTimeout(timerId); - } - //console.log(`Waiting ${WAIT_TIME}ms for another file update...`); - timerId = setTimeout(() => { - console.log("[block-on-build] No updates - unblocking"); - watcher.close(); - resolve(); - }, WAIT_TIME); - done(); - }, null, null); - }); - })); -} - const sdkName = process.argv[2]; if (!sdkName) { console.error("[block-on-build] No SDK name provided"); @@ -81,7 +40,7 @@ if (!sdkName) { } console.log("[block-on-build] Waiting for SDK: " + sdkName); -waitOnSdkBuild(sdkName).then(() => { +waitForCanary(sdkName).then(() => { console.log("[block-on-build] Unblocked"); process.exit(0); }); \ No newline at end of file diff --git a/scripts/build-watch-sdk.js b/scripts/build-watch-sdk.js index e240e3a8c6..44607f6b7b 100644 --- a/scripts/build-watch-sdk.js +++ b/scripts/build-watch-sdk.js @@ -16,7 +16,15 @@ if (!sdkName) { } const sdkPath = path.dirname(require.resolve(`matrix-${sdkName}-sdk/package.json`)); -console.log(sdkPath); + +// Note: we intentionally create then delete the canary file to work +// around a file watching problem where if the file exists on startup it +// may fire a "created" event for the file. By having the behaviour be "do +// something on delete" we avoid accidentally firing the signal too early. +// We also need to ensure the create and delete events are not too close +// together, otherwise the filesystem may not fire the watcher. Therefore +// we create the canary as early as possible and delete it as late as possible. +prepareCanarySignal(sdkName); // We only want to build the SDK if it looks like it was `npm link`ed if (fs.existsSync(path.join(sdkPath, '.git'))) { @@ -37,25 +45,37 @@ if (fs.existsSync(path.join(sdkPath, '.git'))) { }); } - // Send a signal so that the various blocks can unblock. See the top of - // block-on-sdk-build.js for more information on how this is used. - console.log("Sending signal that other processes may unblock"); - triggerCanarySignal(sdkName); - - // Actually start the watcher process for the sdk. This is what block-on-sdk-build.js - // is going to monitor. - console.log("Performing task: " + task); - child_process.execSync(`npm ${task === "build" ? "run build" : "start"}`, { + // Prepare an initial build of the SDK + child_process.execSync("npm run start:init", { env: process.env, cwd: sdkPath, }); -} + + // Send a signal to unblock the build for other processes. Used by block-on-sdk-build.js + console.log("Sending signal that other processes may unblock"); + triggerCanarySignal(sdkName); + + // Actually start the watcher process for the SDK (without an initial build) + console.log("Performing task: " + task); + const watchTask = sdkName === 'js' ? "start:watch" : "start:all"; + const buildTask = "build"; + child_process.execSync(`npm run ${task === "build" ? buildTask : watchTask}`, { + env: process.env, + cwd: sdkPath, + }); +} else triggerCanarySignal(sdkName); function triggerCanarySignal(sdkName) { - const tmpPath = path.resolve(".tmp"); + fs.unlinkSync(getCanaryPath(sdkName)); +} + +function prepareCanarySignal(sdkName) { + const canaryPath = getCanaryPath(sdkName); + const canaryDir = path.dirname(canaryPath); try { - fs.mkdirSync(tmpPath); + console.log("Creating canary temp path..."); + fs.mkdirSync(canaryDir); } catch (e) { if (e.code !== 'EEXIST') { console.error(e); @@ -63,12 +83,17 @@ function triggerCanarySignal(sdkName) { } } - // Note: we intentionally create then delete the file to work around - // a file watching problem where if the file exists on startup it may - // fire a "created" event for the file. By having the behaviour be "do - // something on delete" we avoid accidentally firing the signal too - // early. - const canaryPath = path.join(tmpPath, sdkName); - fs.closeSync(fs.openSync(canaryPath, 'w')); - fs.unlinkSync(canaryPath); + try { + console.log("Creating canary file: " + canaryPath); + fs.closeSync(fs.openSync(canaryPath, 'w')); + } catch (e) { + if (e.code !== 'EEXIST') { + console.error(e); + throw "Failed to create canary file"; + } + } +} + +function getCanaryPath(sdkName) { + return path.join(path.resolve(".tmp"), sdkName + ".canary"); } \ No newline at end of file