Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for filesystem polling #1176

Merged
merged 8 commits into from
Sep 3, 2024
Merged

Add support for filesystem polling #1176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5826027
Sort environment variables in README
aomarks Aug 31, 2024
01446b4
Document WIREIT_WATCH_STRATEGY and WIREIT_WATCH_POLL_MS
aomarks Aug 31, 2024
7d36084
Sort imports
aomarks Aug 31, 2024
08ccb49
Add tests for polling strategy
aomarks Aug 31, 2024
d890186
Test watch CLI options
aomarks Aug 31, 2024
48d07bc
Pass down polling options to chokidar
aomarks Aug 31, 2024
6879ab1
Update CHANGELOG
aomarks Aug 31, 2024
00cc231
Backtick number
aomarks Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ 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

### Added

- Add support for forcing the use of filesystem polling instead of OS events in watch mode. Set the environment variable `WIREIT_WATCH_STRATEGY=poll`, and optionally the `WIREIT_WATCH_POLL_MS` (default `500`).

## [0.14.8] - 2024-08-22

Expand Down
13 changes: 10 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,11 @@ The benefit of Wireit's watch mode over the built-in watch modes of Node and oth
simultaneously, such as build steps being triggered before all preceding steps
have finished.

By default, watch mode uses whichever filesystem change API is available on your
OS. This behavior can be changed with the `WIREIT_WATCH_STRATEGY` and
`WIREIT_WATCH_POLL_MS` environment variables (see
[below](#environment-variable-reference)).

## Environment variables

Use the `env` setting to either directly set environment variables, or to
Expand Down Expand Up @@ -874,12 +879,14 @@ The following environment variables affect the behavior of Wireit:

| Variable | Description |
| ----------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `WIREIT_CACHE` | [Caching mode](#caching).<br><br>Defaults to `local` unless `CI` is `true`, in which case defaults to `none`.<br><br>Automatically set to `github` by the [`google/wireit@setup-github-actions-caching/v2`](#github-actions-caching) action.<br><br>Options:<ul><li>[`local`](#local-caching): Cache to local disk.</li><li>[`github`](#github-actions-caching): Cache to GitHub Actions.</li><li>`none`: Disable caching.</li></ul> |
| `WIREIT_FAILURES` | [How to handle script failures](#failures-and-errors).<br><br>Options:<br><ul><li>[`no-new`](#failures-and-errors) (default): Allow running scripts to finish, but don't start new ones.</li><li>[`continue`](#continue): Allow running scripts to continue, and start new ones unless any of their dependencies failed.</li><li>[`kill`](#kill): Immediately kill running scripts, and don't start new ones.</li></ul> |
| `WIREIT_LOGGER` | How to present progress and results on the command line.<br><br>Options:<br><ul><li>`quiet` (default): writes a single dynamically updating line summarizing progress. Only passes along stdout and stderr from commands if there's a failure, or if the command is a service. The planned new default, please try it out.</li><li>`simple` (default): A verbose logger that presents clear information about the work that Wireit is doing.</li><li>`metrics`: Like `simple`, but also presents a summary table of results once a command is finished.</li><li>`quiet-ci` (default when env.CI or !stdout.isTTY): like `quiet` but optimized for non-interactive environments, like GitHub Actions runners.</li></ul> |
| `WIREIT_MAX_OPEN_FILES` | Limits the number of file descriptors Wireit will have open concurrently. Prevents resource exhaustion when checking large numbers of cached files. Set to a lower number if you hit file descriptor limits. |
| `WIREIT_PARALLEL` | [Maximum number of scripts to run at one time](#parallelism).<br><br>Defaults to 2×logical CPU cores.<br><br>Must be a positive integer or `infinity`. |
| `WIREIT_CACHE` | [Caching mode](#caching).<br><br>Defaults to `local` unless `CI` is `true`, in which case defaults to `none`.<br><br>Automatically set to `github` by the [`google/wireit@setup-github-actions-caching/v2`](#github-actions-caching) action.<br><br>Options:<ul><li>[`local`](#local-caching): Cache to local disk.</li><li>[`github`](#github-actions-caching): Cache to GitHub Actions.</li><li>`none`: Disable caching.</li></ul> |
| `WIREIT_WATCH_STRATEGY` | How Wireit determines when a file has changed which warrants a new watch iteration.<br><br>Options:<br><ul><li>`event` (default): Register OS file system watcher callbacks (using [chokidar](https://github.com/paulmillr/chokidar)).</li><li>`poll`: Poll the filesystem every `WIREIT_WATCH_POLL_MS` milliseconds. Less responsive and worse performance than `event`, but a good fallback for when `event` does not work well or at all (e.g. filesystems that don't support filesystem events, or performance and memory problems with large file trees).</li></ul> |
| `WIREIT_WATCH_POLL_MS` | When `WIREIT_WATCH_STRATEGY` is `poll`, how many milliseconds to wait between each filesystem poll. Defaults to `500`. |
| `CI` | Affects the default value of `WIREIT_CACHE`.<br><br>Automatically set to `true` by [GitHub Actions](https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables) and most other CI (continuous integration) services.<br><br>Must be exactly `true`. If unset or any other value, interpreted as `false`. |
| `WIREIT_MAX_OPEN_FILES` | Limits the number of file descriptors Wireit will have open concurrently. Prevents resource exhaustion when checking large numbers of cached files. Set to a lower number if you hit file descriptor limits. |
| `WIREIT_LOGGER` | How to present progress and results on the command line.<br><br>Options:<br><ul><li>`quiet` (default): writes a single dynamically updating line summarizing progress. Only passes along stdout and stderr from commands if there's a failure, or if the command is a service. The planned new default, please try it out.</li><li>`simple` (default): A verbose logger that presents clear information about the work that Wireit is doing.</li><li>`metrics`: Like `simple`, but also presents a summary table of results once a command is finished.</li><li>`quiet-ci` (default when env.CI or !stdout.isTTY): like `quiet` but optimized for non-interactive environments, like GitHub Actions runners.</li></ul> |

### Glob patterns

Expand Down
55 changes: 51 additions & 4 deletions src/cli-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export type Agent = 'npm' | 'nodeRun' | 'pnpm' | 'yarnClassic' | 'yarnBerry';

export interface Options {
script: ScriptReference;
watch: boolean;
watch: false | {strategy: 'event'} | {strategy: 'poll'; interval: number};
extraArgs: string[] | undefined;
numWorkers: number;
cache: 'local' | 'github' | 'none';
Expand Down Expand Up @@ -277,7 +277,10 @@ function getArgvOptions(
// npm 8.11.0
// - Like npm 6, except there is no "npm_config_argv" environment variable.
return {
watch: process.env['npm_config_watch'] !== undefined,
watch:
process.env['npm_config_watch'] !== undefined
? readWatchConfigFromEnv()
: false,
extraArgs: process.argv.slice(2),
};
}
Expand Down Expand Up @@ -421,7 +424,7 @@ function findRemainingArgsFromNpmConfigArgv(
function parseRemainingArgs(
args: string[],
): Pick<Options, 'watch' | 'extraArgs'> {
let watch = false;
let watch: Options['watch'] = false;
let extraArgs: string[] = [];
const unrecognized = [];
for (let i = 0; i < args.length; i++) {
Expand All @@ -430,7 +433,7 @@ function parseRemainingArgs(
extraArgs = args.slice(i + 1);
break;
} else if (arg === '--watch') {
watch = true;
watch = readWatchConfigFromEnv();
} else {
unrecognized.push(arg);
}
Expand All @@ -448,3 +451,47 @@ function parseRemainingArgs(
extraArgs,
};
}

const DEFAULT_WATCH_STRATEGY = {strategy: 'event'} as const;
const DEFAULT_WATCH_INTERVAL = 500;

/**
* Interpret the WIREIT_WATCH_* environment variables.
*/
function readWatchConfigFromEnv(): Options['watch'] {
switch (process.env['WIREIT_WATCH_STRATEGY']) {
case 'event':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: I definitely prefer avoid fall-through myself. Would this be easier as an if/else statement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole file (and other files) uses this style of switching on state. We're doing exhaustive checking for every state for every transition. I actually kinda like using fallthrough here, it's just the most concise way to write out all the permutations.

case '':
case undefined: {
return DEFAULT_WATCH_STRATEGY;
}
case 'poll': {
let interval = DEFAULT_WATCH_INTERVAL;
const intervalStr = process.env['WIREIT_WATCH_POLL_MS'];
if (intervalStr) {
const parsed = Number(intervalStr);
if (Number.isNaN(parsed) || parsed <= 0) {
console.error(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is continuing to operate with the fallback value, should this be console.warn() instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both go to stderr, but in some configurations warnings might get filtered out? I guess because it's definitely something wrong, I think it should be an error to maximize the chance of somebody seeing it.

`⚠️ Expected WIREIT_WATCH_POLL_MS to be a positive integer, ` +
`got ${JSON.stringify(intervalStr)}. Using default interval of ` +
`${DEFAULT_WATCH_INTERVAL}ms.`,
);
} else {
interval = parsed;
}
}
return {
strategy: 'poll',
interval,
};
}
default: {
console.error(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

`⚠️ Unrecognized WIREIT_WATCH_STRATEGY: ` +
`${JSON.stringify(process.env['WIREIT_WATCH_STRATEGY'])}. ` +
`Using default strategy of ${DEFAULT_WATCH_STRATEGY.strategy}.`,
);
return DEFAULT_WATCH_STRATEGY;
}
}
}
11 changes: 6 additions & 5 deletions src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
* SPDX-License-Identifier: Apache-2.0
*/

import {Result} from './error.js';
import {Analyzer} from './analyzer.js';
import {Executor} from './executor.js';
import {WorkerPool} from './util/worker-pool.js';
import {unreachable} from './util/unreachable.js';
import {getOptions, Options, packageDir} from './cli-options.js';
import {Result} from './error.js';
import {Failure} from './event.js';
import {packageDir, getOptions, Options} from './cli-options.js';
import {Executor} from './executor.js';
import {DefaultLogger} from './logging/default-logger.js';
import {Console} from './logging/logger.js';
import {unreachable} from './util/unreachable.js';
import {WorkerPool} from './util/worker-pool.js';

const run = async (options: Options): Promise<Result<void, Failure[]>> => {
using logger = options.logger;
Expand Down Expand Up @@ -66,6 +66,7 @@ const run = async (options: Options): Promise<Result<void, Failure[]>> => {
cache,
options.failureMode,
options.agent,
options.watch,
);
process.on('SIGINT', () => {
watcher.abort();
Expand Down
92 changes: 84 additions & 8 deletions src/test/cli-options.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) {
packageDir: rig.temp,
name: 'main',
},
watch: true,
watch: {strategy: 'event'},
});
}),
);
Expand All @@ -179,7 +179,7 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) {
name: 'main',
},
extraArgs: ['--extra'],
watch: true,
watch: {strategy: 'event'},
},
);
}),
Expand All @@ -199,7 +199,7 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) {
name: 'other',
},
extraArgs: [],
watch: true,
watch: {strategy: 'event'},
},
undefined,
{
Expand Down Expand Up @@ -269,7 +269,7 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) {
name: 'other',
},
extraArgs: ['--extra'],
watch: true,
watch: {strategy: 'event'},
},
undefined,
{
Expand Down Expand Up @@ -318,7 +318,7 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) {
packageDir: rig.temp,
name: 'test',
},
watch: true,
watch: {strategy: 'event'},
});
}),
);
Expand All @@ -334,7 +334,7 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) {
name: 'test',
},
extraArgs: ['--extra'],
watch: true,
watch: {strategy: 'event'},
});
}),
);
Expand Down Expand Up @@ -377,7 +377,7 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) {
packageDir: rig.temp,
name: 'start',
},
watch: true,
watch: {strategy: 'event'},
});
}),
);
Expand All @@ -392,11 +392,87 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) {
name: 'start',
},
extraArgs: ['--extra'],
watch: true,
watch: {strategy: 'event'},
});
}),
);
}

test(
`${agent} --watch WIREIT_WATCH_STRATEGY=poll`,
rigTest(async ({rig}) => {
await assertOptions(
rig,
`${runCmd} main ${extraDashes} --watch`,
{
agent,
script: {
packageDir: rig.temp,
name: 'main',
},
logger: 'QuietLogger',
watch: {
strategy: 'poll',
interval: 500,
},
},
{
WIREIT_WATCH_STRATEGY: 'poll',
},
);
}),
);

test(
`${agent} --watch WIREIT_WATCH_STRATEGY=poll WIREIT_WATCH_POLL_MS=74`,
rigTest(async ({rig}) => {
await assertOptions(
rig,
`${runCmd} main ${extraDashes} --watch`,
{
agent,
script: {
packageDir: rig.temp,
name: 'main',
},
logger: 'QuietLogger',
watch: {
strategy: 'poll',
interval: 74,
},
},
{
WIREIT_WATCH_STRATEGY: 'poll',
WIREIT_WATCH_POLL_MS: '74',
},
);
}),
);

test(
`${agent} WIREIT_WATCH_STRATEGY=poll WIREIT_WATCH_POLL_MS=74`,
rigTest(async ({rig}) => {
await assertOptions(
rig,
`${runCmd} main ${extraDashes}`,
{
agent,
script: {
packageDir: rig.temp,
name: 'main',
},
logger: 'QuietLogger',
// This is just testing that the WIREIT_WATCH environment variables
// don't actually turn on watch mode. Only the --watch flag does that.
watch: false,
},
{
WIREIT_WATCH_STRATEGY: 'poll',
WIREIT_WATCH_POLL_MS: '74',
},
);
}),
);
}

test.run();
7 changes: 4 additions & 3 deletions src/test/glob.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
*/

import * as pathlib from 'path';
import * as assert from 'uvu/assert';
import {suite} from 'uvu';
import * as assert from 'uvu/assert';
import {glob} from '../util/glob.js';
import {FilesystemTestRig} from './util/filesystem-test-rig.js';
import {makeWatcher} from '../watcher.js';
import {IS_WINDOWS} from '../util/windows.js';
import {makeWatcher} from '../watcher.js';
import {FilesystemTestRig} from './util/filesystem-test-rig.js';

interface Symlink {
/** Where the symlink file points to. */
Expand Down Expand Up @@ -116,6 +116,7 @@ test.before.each(async (ctx) => {
// events to find out what chokidar has found (we usually only care
// about changes, not initial files).
false,
{strategy: 'event'},
);
const watcher = fsWatcher.watcher;
watcher.on('add', (path) => {
Expand Down
Loading