Skip to content

Commit

Permalink
Remove stdout/stderr replay (#310)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@aomarks
aomarks committed Jun 17, 2022
1 parent a921134 commit 0b12392
Show file tree
Hide file tree
Showing 9 changed files with 42 additions and 495 deletions.
2 changes: 2 additions & 0 deletions .prettierignore
View file
Expand Up @@ -7,3 +7,5 @@
/vscode-extension/.wireit
/vscode-extension/lib
/vscode-extension/built
/website/.wireit
/website/_site
7 changes: 6 additions & 1 deletion CHANGELOG.md
View file
Expand Up @@ -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] -->
## [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

Expand Down
6 changes: 2 additions & 4 deletions README.md
View file
Expand Up @@ -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
Expand All @@ -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.
Expand Down
10 changes: 0 additions & 10 deletions package.json
View file
Expand Up @@ -47,7 +47,6 @@
"test:json-schema": "wireit",
"test:optimize-mkdirs": "wireit",
"test:parallelism": "wireit",
"test:stdio-replay": "wireit",
"test:watch": "wireit"
},
"wireit": {
Expand Down Expand Up @@ -89,7 +88,6 @@
"test:json-schema",
"test:optimize-mkdirs",
"test:parallelism",
"test:stdio-replay",
"test:watch"
]
},
Expand Down Expand Up @@ -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": [
Expand Down
200 changes: 30 additions & 170 deletions src/execution/one-shot.ts
View file
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -237,13 +234,7 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
/**
* Handle the outcome where the script is already fresh.
*/
async #handleFresh(fingerprint: Fingerprint): Promise<ExecutionResult> {
// 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',
Expand All @@ -259,9 +250,9 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
cacheHit: CacheHit,
fingerprint: Fingerprint
): Promise<ExecutionResult> {
// 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.
Expand All @@ -277,15 +268,6 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
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) {
Expand Down Expand Up @@ -316,9 +298,9 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
// 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) {
Expand Down Expand Up @@ -352,22 +334,13 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
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,
type: 'output',
stream: 'stdout',
data,
});
if (stdoutReplay === undefined) {
stdoutReplay = createWriteStream(this.#stdoutReplayPath);
}
stdoutReplay.write(data);
});

child.stderr.on('data', (data: string | Buffer) => {
Expand All @@ -377,43 +350,30 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
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';
Expand Down Expand Up @@ -512,29 +472,6 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
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};
}
Expand Down Expand Up @@ -625,20 +562,6 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
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.
Expand Down Expand Up @@ -674,14 +597,12 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
}

/**
* 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<void> {
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}),
]);
}
Expand All @@ -701,51 +622,6 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
return {ok: true, value: undefined};
}

/**
* Write this script's stdout replay to stdout if it exists, otherwise do
* nothing.
*/
async #replayStdoutIfPresent(): Promise<void> {
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<void> {
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
Expand Down Expand Up @@ -831,19 +707,3 @@ export class OneShotExecution extends BaseExecution<OneShotScriptConfig> {
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<void> => {
return new Promise((resolve, reject) => {
stream.close((error) => {
if (error) {
reject(error);
} else {
resolve();
}
});
});
};

0 comments on commit 0b12392

Please sign in to comment.