Skip to content

Commit

Permalink
Replace simple Watchman existence check with capability check
Browse files Browse the repository at this point in the history
Summary:
Replace the `canUseWatchman` existence check (`watchman --version` returns `0`) with a call to `list-capabilities`, which we'll use shortly to safely drop support for very old Watchman versions.

### We're already getting the version string, why not use that?
Unfortunately, Watchman's versioning is a bit all over the place.
 - Watchman used SemVer up until `4.9.0`, and then several releases after that were labelled with an incrementing suffix up to `4.9.0_5`
 - Watchman uses a kind of CalVer now, which doesn't indicate breaking changes - and on top of that the version string is formatted differently by different builds:
   - Internal laptop: `20220919`
   - Internal dev server: `2022-09-19T02:58:16Z`
   - Homebrew: `v2022.09.19.00`

### Why not use `fb-watchman`'s `capabilityCheck`?
 - `fb-watchman` doesn't allow us to pass an equivalent of `--no-spawn`, which is a useful performance boost in the case that we don't end up using Watchman (if we do, we spawn it immediately afterwards anyway), and avoids (IMO) the surprising side-effect of launching a server.
 - Using `execFile` directly allows us to give more actionable feedback (because `ENOENT` is a reliable, machine-readable error code) in the case that Watchman is not installed / not on `PATH`.
 - The full capability list is still very small.

Reviewed By: jacdebug

Differential Revision: D39795909

fbshipit-source-id: 344e15e9af90de67cea833ba5c6c838b4b21995b
  • Loading branch information
robhogan authored and facebook-github-bot committed Sep 27, 2022
1 parent 90a7ded commit d831400
Show file tree
Hide file tree
Showing 6 changed files with 166 additions and 69 deletions.
4 changes: 2 additions & 2 deletions packages/metro-file-map/src/__tests__/index-test.js
Expand Up @@ -17,9 +17,9 @@ function mockHashContents(contents) {
return crypto.createHash('sha1').update(contents).digest('hex');
}

jest.mock('../lib/canUseWatchman', () => ({
jest.mock('../lib/checkWatchmanCapabilities', () => ({
__esModule: true,
default: async () => true,
default: async () => {},
}));

jest.mock('jest-worker', () => ({
Expand Down
16 changes: 14 additions & 2 deletions packages/metro-file-map/src/index.js
Expand Up @@ -37,7 +37,7 @@ import {DiskCacheManager} from './cache/DiskCacheManager';
import H from './constants';
import getMockName from './getMockName';
import HasteFS from './HasteFS';
import canUseWatchman from './lib/canUseWatchman';
import checkWatchmanCapabilities from './lib/checkWatchmanCapabilities';
import deepCloneInternalData from './lib/deepCloneInternalData';
import * as fastPath from './lib/fast_path';
import getPlatformExtension from './lib/getPlatformExtension';
Expand Down Expand Up @@ -1158,7 +1158,19 @@ export default class HasteMap extends EventEmitter {
return false;
}
if (!this._canUseWatchmanPromise) {
this._canUseWatchmanPromise = canUseWatchman();
// TODO: Ensure minimum capabilities here
this._canUseWatchmanPromise = checkWatchmanCapabilities([])
.then(() => true)
.catch(e => {
// TODO: Advise people to either install Watchman or set
// `useWatchman: false` here?
this._options.perfLogger?.annotate({
string: {
watchmanFailedCapabilityCheck: e?.message ?? '[missing]',
},
});
return false;
});
}
return this._canUseWatchmanPromise;
}
Expand Down
44 changes: 0 additions & 44 deletions packages/metro-file-map/src/lib/__tests__/canUseWatchman-test.js

This file was deleted.

@@ -0,0 +1,83 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @format
* @flow strict
*/

import checkWatchmanCapabilities from '../checkWatchmanCapabilities';

const mockExecFile = jest.fn();
jest.mock('child_process', () => ({
execFile: (...args) => mockExecFile(...args),
}));

const mockSuccessResponse = JSON.stringify({
version: 'v123',
capabilities: ['c1', 'c2'],
});

function setMockExecFileResponse(err: mixed, stdout?: mixed) {
mockExecFile.mockImplementation((file, args, cb) => {
expect(file).toBe('watchman');
cb(err, err == null ? {stdout} : null);
});
}

describe('checkWatchmanCapabilities', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('executes watchman list-capabilities and resolves on success', async () => {
setMockExecFileResponse(null, mockSuccessResponse);
await expect(checkWatchmanCapabilities(['c1', 'c2'])).resolves.toEqual();
expect(mockExecFile).toHaveBeenCalledWith(
'watchman',
[
'list-capabilities',
'--output-encoding=json',
'--no-pretty',
'--no-spawn',
],
expect.any(Function),
);
});

it('rejects when execFile reports ENOENT', async () => {
setMockExecFileResponse({code: 'ENOENT'});
await expect(checkWatchmanCapabilities([])).rejects.toMatchInlineSnapshot(
`[Error: Watchman is not installed or not available on PATH]`,
);
expect(mockExecFile).toHaveBeenCalled();
});

it('rejects when execFile fails', async () => {
setMockExecFileResponse(new Error('execFile error'));
await expect(checkWatchmanCapabilities([])).rejects.toMatchInlineSnapshot(
`[Error: execFile error]`,
);
expect(mockExecFile).toHaveBeenCalled();
});

it('rejects when the response is not JSON', async () => {
setMockExecFileResponse(null, 'not json');
await expect(checkWatchmanCapabilities([])).rejects.toMatchInlineSnapshot(
`[Error: Failed to parse response from \`watchman list-capabilities\`]`,
);
expect(mockExecFile).toHaveBeenCalled();
});

it('rejects when we are missing a required capability', async () => {
setMockExecFileResponse(null, mockSuccessResponse);
await expect(
checkWatchmanCapabilities(['c1', 'other-cap']),
).rejects.toMatchInlineSnapshot(
`[Error: The installed version of Watchman (v123) is missing required capabilities: other-cap]`,
);
expect(mockExecFile).toHaveBeenCalled();
});
});
21 changes: 0 additions & 21 deletions packages/metro-file-map/src/lib/canUseWatchman.js

This file was deleted.

67 changes: 67 additions & 0 deletions packages/metro-file-map/src/lib/checkWatchmanCapabilities.js
@@ -0,0 +1,67 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @format
* @flow strict
*/

import {execFile} from 'child_process';
import {promisify} from 'util';

export default async function checkWatchmanCapabilities(
requiredCapabilities: $ReadOnlyArray<string>,
): Promise<void> {
const execFilePromise: (
cmd: string,
args: $ReadOnlyArray<string>,
) => Promise<{stdout: string}> = promisify(execFile);

let rawResponse;
try {
const result = await execFilePromise('watchman', [
'list-capabilities',
'--output-encoding=json',
'--no-pretty',
'--no-spawn', // The client can answer this, so don't spawn a server
]);
rawResponse = result.stdout;
} catch (e) {
if (e?.code === 'ENOENT') {
throw new Error('Watchman is not installed or not available on PATH');
}
throw e;
}

let parsedResponse;
try {
parsedResponse = (JSON.parse(rawResponse): mixed);
} catch {
throw new Error(
'Failed to parse response from `watchman list-capabilities`',
);
}

if (
parsedResponse == null ||
typeof parsedResponse !== 'object' ||
typeof parsedResponse.version !== 'string' ||
!Array.isArray(parsedResponse.capabilities)
) {
throw new Error('Unexpected response from `watchman list-capabilities`');
}
const version = parsedResponse.version;
const capabilities = new Set(parsedResponse.capabilities);
const missingCapabilities = requiredCapabilities.filter(
requiredCapability => !capabilities.has(requiredCapability),
);
if (missingCapabilities.length > 0) {
throw new Error(
`The installed version of Watchman (${version}) is missing required capabilities: ${missingCapabilities.join(
', ',
)}`,
);
}
}

0 comments on commit d831400

Please sign in to comment.