From 0b123924b27e59d099fc96073e9d56ff6d0079b0 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 17 Jun 2022 10:39:48 -0700 Subject: [PATCH] Remove stdout/stderr replay (#310) I have come to feel that the stdout/stderr replay is an anti-feature that adds a lot of noise and very little value. I think there are some use cases where you might want it, e.g. where the whole purpose of running the script is to see some output on your terminal. But those are fairly rare. Perhaps we could add a per-script setting for those cases. I think the right default is to be quieter, though, so I'm inclined to just remove the feature for now, and think some more about adding it back in a more thoughtful way in the future. --- .prettierignore | 2 + CHANGELOG.md | 7 +- README.md | 6 +- package.json | 10 -- src/execution/one-shot.ts | 200 ++++-------------------- src/test/cache-common.ts | 114 -------------- src/test/stdio-replay.test.ts | 192 ----------------------- website/content/05-incremental-build.md | 3 +- website/content/06-caching.md | 3 +- 9 files changed, 42 insertions(+), 495 deletions(-) delete mode 100644 src/test/stdio-replay.test.ts diff --git a/.prettierignore b/.prettierignore index 0a593ac38..68f6105aa 100644 --- a/.prettierignore +++ b/.prettierignore @@ -7,3 +7,5 @@ /vscode-extension/.wireit /vscode-extension/lib /vscode-extension/built +/website/.wireit +/website/_site diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d4673449..bae4bd855 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - +## [Unreleased] + +### Removed + +- [**Breaking**] stdout/stderr are no longer replayed. Only if a script is + actually running will it now produce output to those streams. ## [0.6.1] - 2022-06-15 diff --git a/README.md b/README.md index 9623f2cd9..53e9cf29b 100644 --- a/README.md +++ b/README.md @@ -263,8 +263,7 @@ Setting these properties allow you to use more features of Wireit: Wireit can automatically skip execution of a script if nothing has changed that would cause it to produce different output since the last time it ran. This is -called _incremental build_. When a script is skipped, any `stdout` or `stderr` -that it produced in the previous run is replayed. +called _incremental build_. To enable incremental build, configure the input and output files for each script by specifying [glob patterns](#glob-patterns) in the @@ -280,8 +279,7 @@ script by specifying [glob patterns](#glob-patterns) in the If a script has previously succeeded with the same configuration and input files, then Wireit can copy the output from a cache, instead of running the -command. This can significantly improve build and test time. When a script is -restored from cache, any `stdout` or `stderr` is replayed. +command. This can significantly improve build and test time. To enable caching for a script, ensure you have defined both the [`files` and `output`](#input-and-output-files) arrays. diff --git a/package.json b/package.json index 592c5e179..dca9db980 100644 --- a/package.json +++ b/package.json @@ -47,7 +47,6 @@ "test:json-schema": "wireit", "test:optimize-mkdirs": "wireit", "test:parallelism": "wireit", - "test:stdio-replay": "wireit", "test:watch": "wireit" }, "wireit": { @@ -89,7 +88,6 @@ "test:json-schema", "test:optimize-mkdirs", "test:parallelism", - "test:stdio-replay", "test:watch" ] }, @@ -253,14 +251,6 @@ "files": [], "output": [] }, - "test:stdio-replay": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"stdio-replay\\.test\\.js$\"", - "dependencies": [ - "build" - ], - "files": [], - "output": [] - }, "test:watch": { "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"watch\\.test\\.js$\"", "dependencies": [ diff --git a/src/execution/one-shot.ts b/src/execution/one-shot.ts index 9f58ca742..18640b8c4 100644 --- a/src/execution/one-shot.ts +++ b/src/execution/one-shot.ts @@ -6,13 +6,11 @@ import * as pathlib from 'path'; import * as fs from 'fs/promises'; -import {createReadStream, createWriteStream} from 'fs'; import {WorkerPool} from '../util/worker-pool.js'; import {getScriptDataDir} from '../util/script-data-dir.js'; import {unreachable} from '../util/unreachable.js'; import {glob, GlobOutsideCwdError} from '../util/glob.js'; import {deleteEntries} from '../util/delete.js'; -import {posixifyPathIfOnWindows} from '../util/windows.js'; import lockfile from 'proper-lockfile'; import {ScriptChildProcess} from '../script-child-process.js'; import {BaseExecution} from './base.js'; @@ -25,7 +23,6 @@ import type {Executor} from '../executor.js'; import type {OneShotScriptConfig} from '../script.js'; import type {FingerprintString} from '../fingerprint.js'; import type {Logger} from '../logging/logger.js'; -import type {WriteStream} from 'fs'; import type {Cache, CacheHit} from '../caching/cache.js'; import type {StartCancelled} from '../event.js'; import type {AbsoluteEntry} from '../util/glob.js'; @@ -237,13 +234,7 @@ export class OneShotExecution extends BaseExecution { /** * Handle the outcome where the script is already fresh. */ - async #handleFresh(fingerprint: Fingerprint): Promise { - // TODO(aomarks) Does not preserve original order of stdout vs stderr - // chunks. See https://github.com/google/wireit/issues/74. - await Promise.all([ - this.#replayStdoutIfPresent(), - this.#replayStderrIfPresent(), - ]); + #handleFresh(fingerprint: Fingerprint): ExecutionResult { this.logger.log({ script: this.script, type: 'success', @@ -259,9 +250,9 @@ export class OneShotExecution extends BaseExecution { cacheHit: CacheHit, fingerprint: Fingerprint ): Promise { - // Delete the fingerprint file and stdio replay files. It's important we do - // this before restoring from cache, because we don't want to think that the - // previous fingerprint is still valid when it no longer is. + // Delete the fingerprint and other files. It's important we do this before + // restoring from cache, because we don't want to think that the previous + // fingerprint is still valid when it no longer is. await this.#prepareDataDir(); // If we are restoring from cache, we should always delete existing output. @@ -277,15 +268,6 @@ export class OneShotExecution extends BaseExecution { await cacheHit.apply(); this.#state = 'after-running'; - // We include stdout and stderr replay files when we save to the cache, so - // if there were any, they will now be in place. - // TODO(aomarks) Does not preserve original order of stdout vs stderr - // chunks. See https://github.com/google/wireit/issues/74. - await Promise.all([ - this.#replayStdoutIfPresent(), - this.#replayStderrIfPresent(), - ]); - const writeFingerprintPromise = this.#writeFingerprintFile(fingerprint); const outputFilesAfterRunning = await this.#globOutputFilesAfterRunning(); if (!outputFilesAfterRunning.ok) { @@ -316,9 +298,9 @@ export class OneShotExecution extends BaseExecution { // this. const shouldClean = await this.#shouldClean(fingerprint); - // Delete the fingerprint file and stdio replay files. It's important we do - // this before starting the command, because we don't want to think that the - // previous fingerprint is still valid when it no longer is. + // Delete the fingerprint and other files. It's important we do this before + // starting the command, because we don't want to think that the previous + // fingerprint is still valid when it no longer is. await this.#prepareDataDir(); if (shouldClean) { @@ -352,11 +334,6 @@ export class OneShotExecution extends BaseExecution { child.kill(); }); - // Only create the stdout/stderr replay files if we encounter anything on - // this streams. - let stdoutReplay: WriteStream | undefined; - let stderrReplay: WriteStream | undefined; - child.stdout.on('data', (data: string | Buffer) => { this.logger.log({ script: this.script, @@ -364,10 +341,6 @@ export class OneShotExecution extends BaseExecution { stream: 'stdout', data, }); - if (stdoutReplay === undefined) { - stdoutReplay = createWriteStream(this.#stdoutReplayPath); - } - stdoutReplay.write(data); }); child.stderr.on('data', (data: string | Buffer) => { @@ -377,43 +350,30 @@ export class OneShotExecution extends BaseExecution { stream: 'stderr', data, }); - if (stderrReplay === undefined) { - stderrReplay = createWriteStream(this.#stderrReplayPath); - } - stderrReplay.write(data); }); - try { - const result = await child.completed; - if (result.ok) { - this.logger.log({ - script: this.script, - type: 'success', - reason: 'exit-zero', - }); - } else { - // This failure will propagate to the Executor eventually anyway, but - // asynchronously. - // - // The problem with that is that when parallelism is constrained, the - // next script waiting on this WorkerPool might start running before - // the failure information propagates, because returning from this - // function immediately unblocks the next worker. - // - // By directly notifying the Executor about the failure while we are - // still inside the WorkerPool callback, we prevent this race - // condition. - this.executor.notifyFailure(); - } - return result; - } finally { - if (stdoutReplay !== undefined) { - await closeWriteStream(stdoutReplay); - } - if (stderrReplay !== undefined) { - await closeWriteStream(stderrReplay); - } + const result = await child.completed; + if (result.ok) { + this.logger.log({ + script: this.script, + type: 'success', + reason: 'exit-zero', + }); + } else { + // This failure will propagate to the Executor eventually anyway, but + // asynchronously. + // + // The problem with that is that when parallelism is constrained, the + // next script waiting on this WorkerPool might start running before + // the failure information propagates, because returning from this + // function immediately unblocks the next worker. + // + // By directly notifying the Executor about the failure while we are + // still inside the WorkerPool callback, we prevent this race + // condition. + this.executor.notifyFailure(); } + return result; }); this.#state = 'after-running'; @@ -512,29 +472,6 @@ export class OneShotExecution extends BaseExecution { if (paths.value === undefined) { return {ok: true, value: undefined}; } - // Also include the "stdout" and "stderr" replay files at their standard - // location within the ".wireit" directory for this script so that we can - // replay them after restoring. - paths.value.push( - // We're passing this to glob because we only want to list them if they - // exist, and because we need a dirent. - ...(await glob( - [ - // Convert Windows-style paths to POSIX-style paths if we are on Windows, - // because fast-glob only understands POSIX-style paths. - posixifyPathIfOnWindows(this.#stdoutReplayPath), - posixifyPathIfOnWindows(this.#stderrReplayPath), - ], - { - // The replay paths are absolute. - cwd: '/', - expandDirectories: false, - followSymlinks: false, - includeDirectories: false, - throwIfOutsideCwd: false, - } - )) - ); await this.#cache.set(this.script, fingerprint, paths.value); return {ok: true, value: undefined}; } @@ -625,20 +562,6 @@ export class OneShotExecution extends BaseExecution { return pathlib.join(this.#dataDir, 'fingerprint'); } - /** - * Get the path where the stdout replay is saved for this script. - */ - get #stdoutReplayPath(): string { - return pathlib.join(this.#dataDir, 'stdout'); - } - - /** - * Get the path where the stderr replay is saved for this script. - */ - get #stderrReplayPath(): string { - return pathlib.join(this.#dataDir, 'stderr'); - } - /** * Read this script's previous fingerprint from `fingerprint` file in the * `.wireit` directory. Cached after first call. @@ -674,14 +597,12 @@ export class OneShotExecution extends BaseExecution { } /** - * Delete the fingerprint file and any stdio replays for this script from the - * previous run, and ensure the data directory exists. + * Delete the fingerprint and other files for this script from the previous + * run, and ensure the data directory exists. */ async #prepareDataDir(): Promise { await Promise.all([ fs.rm(this.#fingerprintFilePath, {force: true}), - fs.rm(this.#stdoutReplayPath, {force: true}), - fs.rm(this.#stderrReplayPath, {force: true}), fs.mkdir(this.#dataDir, {recursive: true}), ]); } @@ -701,51 +622,6 @@ export class OneShotExecution extends BaseExecution { return {ok: true, value: undefined}; } - /** - * Write this script's stdout replay to stdout if it exists, otherwise do - * nothing. - */ - async #replayStdoutIfPresent(): Promise { - try { - for await (const chunk of createReadStream(this.#stdoutReplayPath)) { - this.logger.log({ - script: this.script, - type: 'output', - stream: 'stdout', - data: chunk as Buffer, - }); - } - } catch (error) { - if ((error as {code?: string}).code === 'ENOENT') { - // There is no saved replay. - return; - } - } - } - - /** - * Write this script's stderr replay to stderr if it exists, otherwise do - * nothing. - */ - async #replayStderrIfPresent(): Promise { - try { - for await (const chunk of createReadStream(this.#stderrReplayPath)) { - this.logger.log({ - script: this.script, - type: 'output', - stream: 'stderr', - data: chunk as Buffer, - }); - } - } catch (error) { - if ((error as {code?: string}).code === 'ENOENT') { - // There is no saved replay. - return; - } - throw error; - } - } - /** * Compute the output manifest for this script, which is the sorted list of * all output filenames, along with filesystem metadata that we assume is good @@ -831,19 +707,3 @@ export class OneShotExecution extends BaseExecution { return pathlib.join(this.#dataDir, 'manifest'); } } - -/** - * Close the given write stream and resolve or reject the returned promise when - * completed or failed. - */ -const closeWriteStream = (stream: WriteStream): Promise => { - return new Promise((resolve, reject) => { - stream.close((error) => { - if (error) { - reject(error); - } else { - resolve(); - } - }); - }); -}; diff --git a/src/test/cache-common.ts b/src/test/cache-common.ts index ac2a7dd32..30483a3a3 100644 --- a/src/test/cache-common.ts +++ b/src/test/cache-common.ts @@ -616,120 +616,6 @@ export const registerCommonCacheTests = ( }) ); - test( - 'replays stdout when restored from cache', - timeout(async ({rig}) => { - const cmdA = await rig.newCommand(); - await rig.write({ - 'package.json': { - scripts: { - a: 'wireit', - }, - wireit: { - a: { - command: cmdA.command, - files: ['input'], - output: ['output'], - }, - }, - }, - }); - - // Initial run with input v0. Writes some stdout. - { - await rig.write({input: 'v0'}); - const exec = rig.exec('npm run a'); - const invA = await cmdA.nextInvocation(); - invA.stdout('stdout v0'); - invA.exit(0); - const res = await exec.exit; - assert.match(res.stdout, 'stdout v0'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 1); - } - - // Input changed to v1. Run again. Writes different stdout. - { - await rig.write({input: 'v1'}); - const exec = rig.exec('npm run a'); - const invA = await cmdA.nextInvocation(); - invA.stdout('stdout v1'); - invA.exit(0); - const res = await exec.exit; - assert.match(res.stdout, 'stdout v1'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 2); - } - - // Input reverts to v0. Stdout should be replayed from cache. - { - await rig.write({input: 'v0'}); - const exec = rig.exec('npm run a'); - const res = await exec.exit; - assert.match(res.stdout, 'stdout v0'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 2); - } - }) - ); - - test( - 'replays stderr when restored from cache', - timeout(async ({rig}) => { - const cmdA = await rig.newCommand(); - await rig.write({ - 'package.json': { - scripts: { - a: 'wireit', - }, - wireit: { - a: { - command: cmdA.command, - files: ['input'], - output: ['output'], - }, - }, - }, - }); - - // Initial run with input v0. Writes some stderr. - { - await rig.write({input: 'v0'}); - const exec = rig.exec('npm run a'); - const invA = await cmdA.nextInvocation(); - invA.stderr('stderr v0'); - invA.exit(0); - const res = await exec.exit; - assert.match(res.stderr, 'stderr v0'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 1); - } - - // Input changed to v1. Run again. Writes different stderr. - { - await rig.write({input: 'v1'}); - const exec = rig.exec('npm run a'); - const invA = await cmdA.nextInvocation(); - invA.stderr('stderr v1'); - invA.exit(0); - const res = await exec.exit; - assert.match(res.stderr, 'stderr v1'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 2); - } - - // Input reverts to v0. Stdout should be replayed from cache. - { - await rig.write({input: 'v0'}); - const exec = rig.exec('npm run a'); - const res = await exec.exit; - assert.match(res.stderr, 'stderr v0'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 2); - } - }) - ); - test( 'does not cache when WIREIT_CACHE=none', timeout(async ({rig}) => { diff --git a/src/test/stdio-replay.test.ts b/src/test/stdio-replay.test.ts deleted file mode 100644 index d5f351cbc..000000000 --- a/src/test/stdio-replay.test.ts +++ /dev/null @@ -1,192 +0,0 @@ -/** - * @license - * Copyright 2022 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import {suite} from 'uvu'; -import * as assert from 'uvu/assert'; -import {timeout} from './util/uvu-timeout.js'; -import {WireitTestRig} from './util/test-rig.js'; - -const test = suite<{rig: WireitTestRig}>(); - -test.before.each(async (ctx) => { - try { - ctx.rig = new WireitTestRig(); - await ctx.rig.setup(); - } catch (error) { - // Uvu has a bug where it silently ignores failures in before and after, - // see https://github.com/lukeed/uvu/issues/191. - console.error('uvu before error', error); - process.exit(1); - } -}); - -test.after.each(async (ctx) => { - try { - await ctx.rig.cleanup(); - } catch (error) { - // Uvu has a bug where it silently ignores failures in before and after, - // see https://github.com/lukeed/uvu/issues/191. - console.error('uvu after error', error); - process.exit(1); - } -}); - -test( - 'replays stdout when script is fresh', - timeout(async ({rig}) => { - const cmdA = await rig.newCommand(); - await rig.write({ - 'package.json': { - scripts: { - a: 'wireit', - }, - wireit: { - a: { - command: cmdA.command, - files: [], - output: [], - }, - }, - }, - }); - - // Initial run. Script writes some stdout. - { - const exec = rig.exec('npm run a'); - const invA = await cmdA.nextInvocation(); - invA.stdout('this is my stdout'); - invA.exit(0); - const res = await exec.exit; - assert.match(res.stdout, 'this is my stdout'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 1); - } - - // No input changed, so script is fresh, and we replay saved stdout. - { - const exec = rig.exec('npm run a'); - const res = await exec.exit; - assert.match(res.stdout, 'this is my stdout'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 1); - } - }) -); - -test( - 'replays stderr when script is fresh', - timeout(async ({rig}) => { - const cmdA = await rig.newCommand(); - await rig.write({ - 'package.json': { - scripts: { - a: 'wireit', - }, - wireit: { - a: { - command: cmdA.command, - files: [], - output: [], - }, - }, - }, - }); - - // Initial run. Script writes some stderr. - { - const exec = rig.exec('npm run a'); - const invA = await cmdA.nextInvocation(); - invA.stderr('this is my stderr'); - invA.exit(0); - const res = await exec.exit; - assert.match(res.stderr, 'this is my stderr'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 1); - } - - // No input changed, so script is fresh, and we replay saved stderr. - { - const exec = rig.exec('npm run a'); - const res = await exec.exit; - assert.match(res.stderr, 'this is my stderr'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 1); - } - }) -); - -test( - 'deletes stdout and stderr across runs', - timeout(async ({rig}) => { - const cmdA = await rig.newCommand(); - await rig.write({ - 'package.json': { - scripts: { - a: 'wireit', - }, - wireit: { - a: { - command: cmdA.command, - files: ['input'], - output: [], - }, - }, - }, - input: 'v0', - }); - - // Initial run. Script writes some stdout and stderr which gets saved to - // replay files. - { - const exec = rig.exec('npm run a'); - const invA = await cmdA.nextInvocation(); - invA.stdout('v0 stdout'); - invA.stderr('v0 stderr'); - invA.exit(0); - const res = await exec.exit; - assert.match(res.stdout, 'v0 stdout'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 1); - } - - // Input did not change, so script should be fresh and we should replay the - // saved stdout and stderr. - { - const exec = rig.exec('npm run a'); - const res = await exec.exit; - assert.match(res.stdout, 'v0 stdout'); - assert.match(res.stderr, 'v0 stderr'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 1); - } - - // Change input, so script runs again. This time, the script doesn't write - // any stdout or stderr, but we still delete the previous stdout and stderr - // replays. - { - await rig.write({input: 'v1'}); - const exec = rig.exec('npm run a'); - const invA = await cmdA.nextInvocation(); - invA.exit(0); - const res = await exec.exit; - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 2); - } - - // Input did not change, so script should be fresh. There should be no - // stdout or stderr. - { - const exec = rig.exec('npm run a'); - const res = await exec.exit; - assert.not.match(res.stdout, 'v0 stdout'); - assert.not.match(res.stderr, 'v0 stderr'); - assert.equal(res.code, 0); - assert.equal(cmdA.numInvocations, 2); - } - }) -); - -test.run(); diff --git a/website/content/05-incremental-build.md b/website/content/05-incremental-build.md index 34b2d6829..0ccd434e6 100644 --- a/website/content/05-incremental-build.md +++ b/website/content/05-incremental-build.md @@ -11,8 +11,7 @@ eleventyNavigation: Wireit can automatically skip execution of a script if nothing has changed that would cause it to produce different output since the last time it ran. This is -called _incremental build_. When a script is skipped, any `stdout` or `stderr` -that it produced in the previous run is replayed. +called _incremental build_. To enable incremental build, configure the input files for each script by specifying [glob patterns](#glob-patterns) in the `wireit.