From 2ce5992f985e638091a394d731a9b31de7728114 Mon Sep 17 00:00:00 2001 From: Saul Gutierrez Date: Mon, 25 Nov 2024 12:43:41 -0800 Subject: [PATCH] progress: backout D65769371 and D65138587 Summary: Some Windows users are having issues recently with ISL, seeing a bunch of black screens when opening ISL Original Phabricator Diff: D65769371 Original Phabricator Diff: D65138587 Reviewed By: evangrayk Differential Revision: D66465114 fbshipit-source-id: 29f5db406f18c0ff225e8dafa355f0454e4a36da --- addons/isl-server/src/Repository.ts | 78 +++------------ addons/isl-server/src/ServerToClientAPI.ts | 4 +- .../src/__tests__/Repository.test.ts | 98 ++++++++++--------- .../src/__tests__/analytics.test.ts | 22 +++-- addons/isl-server/src/commands.ts | 25 ++--- addons/isl-server/src/github/queryGraphQL.ts | 10 +- addons/isl-server/src/utils.ts | 9 +- addons/isl/integrationTests/setup.tsx | 4 +- addons/isl/src/types.ts | 1 - addons/screenshot-tool/src/testRepo.ts | 4 +- 10 files changed, 105 insertions(+), 150 deletions(-) diff --git a/addons/isl-server/src/Repository.ts b/addons/isl-server/src/Repository.ts index 61a72a971095f..5eee869ac02bd 100644 --- a/addons/isl-server/src/Repository.ts +++ b/addons/isl-server/src/Repository.ts @@ -10,6 +10,7 @@ import type {KindOfChange, PollKind} from './WatchForChanges'; import type {TrackEventName} from './analytics/eventNames'; import type {ConfigLevel, ResolveCommandConflictOutput} from './commands'; import type {RepositoryContext} from './serverTypes'; +import type {ExecaError} from 'execa'; import type { CommitInfo, Disposable, @@ -40,7 +41,6 @@ import type { CwdInfo, } from 'isl/src/types'; import type {Comparison} from 'shared/Comparison'; -import type {EjecaChildProcess, EjecaOptions, EjecaError} from 'shared/ejeca'; import {Internal} from './Internal'; import {OperationQueue} from './OperationQueue'; @@ -76,9 +76,10 @@ import { import { findPublicAncestor, handleAbortSignalOnProcess, - isEjecaError, + isExecaError, serializeAsyncCall, } from './utils'; +import execa from 'execa'; import { settableConfigNames, allConfigNames, @@ -91,7 +92,6 @@ import {revsetArgsForComparison} from 'shared/Comparison'; import {LRU} from 'shared/LRU'; import {RateLimiter} from 'shared/RateLimiter'; import {TypedEventEmitter} from 'shared/TypedEventEmitter'; -import {ejeca} from 'shared/ejeca'; import {exists} from 'shared/fs'; import {removeLeadingPathSep} from 'shared/pathUtils'; import {notEmpty, randomId, nullthrows} from 'shared/utils'; @@ -654,52 +654,6 @@ export class Repository { return {args, stdin}; } - private async operationIPC( - ctx: RepositoryContext, - onProgress: OperationCommandProgressReporter, - child: EjecaChildProcess, - options: EjecaOptions, - ): Promise { - if (!options.ipc) { - return; - } - - interface IpcProgressBar { - id: number; - topic: string; - unit: string; - total: number; - position: number; - parent_id?: number; - } - - while (true) { - try { - // eslint-disable-next-line no-await-in-loop - const message = await child.getOneMessage(); - if ( - message === null || - typeof message !== 'object' || - !('progress_bar_update' in message) - ) { - break; - } - const bars = message.progress_bar_update as IpcProgressBar[]; - const blen = bars.length; - if (blen > 0) { - const msg = bars[blen - 1]; - onProgress('progress', { - message: msg.topic, - progress: msg.position, - progressTotal: msg.total, - }); - } - } catch (err) { - break; - } - } - } - /** * Called by this.operationQueue in response to runOrQueueOperation when an operation is ready to actually run. */ @@ -717,12 +671,11 @@ export class Repository { this.getMergeToolEnvVars(ctx), ]); - const ipc = (ctx.knownConfigs?.get('isl.sl-progress-enabled') ?? 'false') === 'true'; const {command, args, options} = getExecParams( this.info.command, cwdRelativeArgs, cwd, - stdin ? {input: stdin, ipc} : {ipc}, + stdin ? {input: stdin} : undefined, { ...env[0], ...env[1], @@ -736,7 +689,7 @@ export class Repository { throw new Error(`command "${args.join(' ')}" is not allowed`); } - const execution = ejeca(command, args, options); + const execution = execa(command, args, options); // It would be more appropriate to call this in reponse to execution.on('spawn'), but // this seems to be inconsistent about firing in all versions of node. // Just send spawn immediately. Errors during spawn like ENOENT will still be reported by `exit`. @@ -752,11 +705,10 @@ export class Repository { }); handleAbortSignalOnProcess(execution, signal); try { - this.operationIPC(ctx, onProgress, execution, options); const result = await execution; onProgress('exit', result.exitCode || 0); } catch (err) { - onProgress('exit', isEjecaError(err) ? err.exitCode : -1); + onProgress('exit', isExecaError(err) ? err.exitCode : -1); throw err; } } @@ -837,7 +789,7 @@ export class Repository { this.uncommittedChangesEmitter.emit('change', this.uncommittedChanges); } catch (err) { let error = err; - if (isEjecaError(error)) { + if (isExecaError(error)) { if (error.stderr.includes('checkout is currently in progress')) { this.initialConnectionContext.logger.info( 'Ignoring `sl status` error caused by in-progress checkout', @@ -847,8 +799,8 @@ export class Repository { } this.initialConnectionContext.logger.error('Error fetching files: ', error); - if (isEjecaError(error)) { - error = simplifyEjecaError(error); + if (isExecaError(error)) { + error = simplifyExecaError(error); } // emit an error, but don't save it to this.uncommittedChanges @@ -943,13 +895,13 @@ export class Repository { if (internalError) { error = internalError; } - if (isEjecaError(error) && error.stderr.includes('Please check your internet connection')) { + if (isExecaError(error) && error.stderr.includes('Please check your internet connection')) { error = Error('Network request failed. Please check your internet connection.'); } this.initialConnectionContext.logger.error('Error fetching commits: ', error); - if (isEjecaError(error)) { - error = simplifyEjecaError(error); + if (isExecaError(error)) { + error = simplifyExecaError(error); } this.smartlogCommitsChangesEmitter.emit('change', { @@ -1421,7 +1373,7 @@ export class Repository { /** Which event name to track for this command. If undefined, generic 'RunCommand' is used. */ eventName: TrackEventName | undefined, ctx: RepositoryContext, - options?: EjecaOptions, + options?: execa.Options, timeout?: number, ) { const id = randomId(); @@ -1556,8 +1508,8 @@ function isUnhealthyEdenFs(cwd: string): Promise { } /** - * Extract the actually useful stderr part of the Ejeca Error, to avoid the long command args being printed first. + * Extract the actually useful stderr part of the Execa Error, to avoid the long command args being printed first. * */ -function simplifyEjecaError(error: EjecaError): Error { +function simplifyExecaError(error: ExecaError): Error { return new Error(error.stderr.trim() || error.message); } diff --git a/addons/isl-server/src/ServerToClientAPI.ts b/addons/isl-server/src/ServerToClientAPI.ts index 3ef90f056c7c1..0421c769140fe 100644 --- a/addons/isl-server/src/ServerToClientAPI.ts +++ b/addons/isl-server/src/ServerToClientAPI.ts @@ -11,6 +11,7 @@ import type {ServerSideTracker} from './analytics/serverSideTracker'; import type {Logger} from './logger'; import type {ServerPlatform} from './serverPlatform'; import type {RepositoryContext} from './serverTypes'; +import type {ExecaError} from 'execa'; import type {TypeaheadResult} from 'isl-components/Types'; import type {Serializable} from 'isl/src/serialize'; import type { @@ -28,7 +29,6 @@ import type { CodeReviewProviderSpecificClientToServerMessages, StableLocationData, } from 'isl/src/types'; -import type {EjecaError} from 'shared/ejeca'; import type {ExportStack, ImportedStack} from 'shared/types/stack'; import {generatedFilesDetector} from './GeneratedFiles'; @@ -963,7 +963,7 @@ export default class ServerToClientAPI { url: {value: result.stdout}, }); }) - .catch((err: EjecaError) => { + .catch((err: ExecaError) => { this.logger.error('Failed to get repo url at hash:', err); this.postMessage({ type: 'gotRepoUrlAtHash', diff --git a/addons/isl-server/src/__tests__/Repository.test.ts b/addons/isl-server/src/__tests__/Repository.test.ts index 9b801e70df027..962754791ee16 100644 --- a/addons/isl-server/src/__tests__/Repository.test.ts +++ b/addons/isl-server/src/__tests__/Repository.test.ts @@ -13,16 +13,20 @@ import type {RunnableOperation} from 'isl/src/types'; import {absolutePathForFileInRepo, Repository} from '../Repository'; import {makeServerSideTracker} from '../analytics/serverSideTracker'; import {extractRepoInfoFromUrl, setConfigOverrideForTests} from '../commands'; +import * as execa from 'execa'; import {CommandRunner, type MergeConflicts, type ValidatedRepoInfo} from 'isl/src/types'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import * as ejeca from 'shared/ejeca'; import * as fsUtils from 'shared/fs'; import {clone, mockLogger, nextTick} from 'shared/testUtils'; /* eslint-disable require-await */ +jest.mock('execa', () => { + return jest.fn(); +}); + jest.mock('../WatchForChanges', () => { class MockWatchForChanges { dispose = jest.fn(); @@ -38,12 +42,12 @@ const mockTracker = makeServerSideTracker( jest.fn(), ); -function mockEjeca( +function mockExeca( cmds: Array<[RegExp, (() => {stdout: string} | Error) | {stdout: string} | Error]>, ) { - return jest.spyOn(ejeca, 'ejeca').mockImplementation(((cmd: string, args: Array) => { + return jest.spyOn(execa, 'default').mockImplementation(((cmd: string, args: Array) => { const argStr = cmd + ' ' + args?.join(' '); - const ejecaOther = { + const execaOther = { kill: jest.fn(), on: jest.fn((event, cb) => { // immediately call exit cb to teardown timeout @@ -61,15 +65,15 @@ function mockEjeca( if (value instanceof Error) { throw value; } - return {...ejecaOther, ...value}; + return {...execaOther, ...value}; } } - return {...ejecaOther, stdout: ''}; - }) as unknown as typeof ejeca.ejeca); + return {...execaOther, stdout: ''}; + }) as unknown as typeof execa.default); } -function processExitError(code: number, message: string): ejeca.EjecaError { - const err = new Error(message) as ejeca.EjecaError; +function processExitError(code: number, message: string): execa.ExecaError { + const err = new Error(message) as execa.ExecaError; err.exitCode = code; return err; } @@ -90,9 +94,9 @@ describe('Repository', () => { }); it('setting command name', async () => { - const ejecaSpy = mockEjeca([]); + const execaSpy = mockExeca([]); await Repository.getRepoInfo({...ctx, cmd: 'slb'}); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'slb', expect.arrayContaining(['root']), expect.anything(), @@ -102,7 +106,7 @@ describe('Repository', () => { describe('extracting github repo info', () => { beforeEach(() => { setConfigOverrideForTests([['github.pull_request_domain', 'github.com']]); - mockEjeca([ + mockExeca([ [/^sl root --dotdir/, {stdout: '/path/to/myRepo/.sl'}], [/^sl root/, {stdout: '/path/to/myRepo'}], [ @@ -180,7 +184,7 @@ describe('Repository', () => { it('extracting repo info', async () => { setConfigOverrideForTests([]); setPathsDefault('mononoke://0.0.0.0/fbsource'); - mockEjeca([ + mockExeca([ [/^sl root --dotdir/, {stdout: '/path/to/myRepo/.sl'}], [/^sl root/, {stdout: '/path/to/myRepo'}], ]); @@ -199,7 +203,7 @@ describe('Repository', () => { it('handles cwd not exists', async () => { const err = new Error('cwd does not exist') as Error & {code: string}; err.code = 'ENOENT'; - mockEjeca([[/^sl root/, err]]); + mockExeca([[/^sl root/, err]]); const info = (await Repository.getRepoInfo(ctx)) as ValidatedRepoInfo; expect(info).toEqual({ type: 'cwdDoesNotExist', @@ -209,7 +213,7 @@ describe('Repository', () => { it('handles missing executables on windows', async () => { const osSpy = jest.spyOn(os, 'platform').mockImplementation(() => 'win32'); - mockEjeca([ + mockExeca([ [ /^sl root/, processExitError( @@ -231,7 +235,7 @@ describe('Repository', () => { it('prevents setting configs not in the allowlist', async () => { setConfigOverrideForTests([]); setPathsDefault('mononoke://0.0.0.0/fbsource'); - mockEjeca([ + mockExeca([ [/^sl root --dotdir/, {stdout: '/path/to/myRepo/.sl'}], [/^sl root/, {stdout: '/path/to/myRepo'}], ]); @@ -253,9 +257,9 @@ describe('Repository', () => { pullRequestDomain: undefined, }; - let ejecaSpy: ReturnType; + let execaSpy: ReturnType; beforeEach(() => { - ejecaSpy = mockEjeca([]); + execaSpy = mockExeca([]); }); async function runOperation(op: Partial) { @@ -280,7 +284,7 @@ describe('Repository', () => { args: ['commit', '--message', 'hi'], }); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'sl', ['commit', '--message', 'hi', '--noninteractive'], expect.anything(), @@ -292,7 +296,7 @@ describe('Repository', () => { args: ['rebase', '--rev', {type: 'succeedable-revset', revset: 'aaa'}], }); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'sl', ['rebase', '--rev', 'max(successors(aaa))', '--noninteractive'], expect.anything(), @@ -304,7 +308,7 @@ describe('Repository', () => { args: ['rebase', '--rev', {type: 'exact-revset', revset: 'aaa'}], }); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'sl', ['rebase', '--rev', 'aaa', '--noninteractive'], expect.anything(), @@ -316,7 +320,7 @@ describe('Repository', () => { args: ['add', {type: 'repo-relative-file', path: 'path/to/file.txt'}], }); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'sl', ['add', '../repo/path/to/file.txt', '--noninteractive'], expect.anything(), @@ -328,7 +332,7 @@ describe('Repository', () => { args: ['commit', {type: 'config', key: 'ui.allowemptycommit', value: 'True'}], }); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'sl', ['commit', '--config', 'ui.allowemptycommit=True', '--noninteractive'], expect.anything(), @@ -340,7 +344,7 @@ describe('Repository', () => { args: ['debugsh'], }); - expect(ejecaSpy).not.toHaveBeenCalledWith( + expect(execaSpy).not.toHaveBeenCalledWith( 'sl', ['debugsh', '--noninteractive'], expect.anything(), @@ -352,7 +356,7 @@ describe('Repository', () => { args: ['commit', {type: 'config', key: 'foo.bar', value: '1'}], }); - expect(ejecaSpy).not.toHaveBeenCalledWith( + expect(execaSpy).not.toHaveBeenCalledWith( 'sl', expect.arrayContaining(['commit', '--config', 'foo.bar=1']), expect.anything(), @@ -364,7 +368,7 @@ describe('Repository', () => { args: ['commit', '--config', 'foo.bar=1'], }); - expect(ejecaSpy).not.toHaveBeenCalledWith( + expect(execaSpy).not.toHaveBeenCalledWith( 'sl', expect.arrayContaining(['commit', '--config', 'foo.bar=1']), expect.anything(), @@ -390,10 +394,10 @@ www/flib/intern/entity/diff/EntPhabricatorDiffSchema.php it('parses sloc', async () => { const repo = new Repository(repoInfo, ctx); - const ejecaSpy = mockEjeca([[/^sl diff/, () => ({stdout: EXAMPLE_DIFFSTAT})]]); + const execaSpy = mockExeca([[/^sl diff/, () => ({stdout: EXAMPLE_DIFFSTAT})]]); const results = repo.fetchSignificantLinesOfCode(ctx, 'abcdef', ['generated.file']); await expect(results).resolves.toEqual({sloc: 45, strictSloc: 45}); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'sl', expect.arrayContaining([ 'diff', @@ -411,9 +415,9 @@ www/flib/intern/entity/diff/EntPhabricatorDiffSchema.php it('handles empty generated list', async () => { const repo = new Repository(repoInfo, ctx); - const ejecaSpy = mockEjeca([[/^sl diff/, () => ({stdout: EXAMPLE_DIFFSTAT})]]); + const execaSpy = mockExeca([[/^sl diff/, () => ({stdout: EXAMPLE_DIFFSTAT})]]); repo.fetchSignificantLinesOfCode(ctx, 'abcdef', []); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'sl', expect.arrayContaining(['diff', '-B', '-X', '**__generated__**', '-c', 'abcdef']), expect.anything(), @@ -422,11 +426,11 @@ www/flib/intern/entity/diff/EntPhabricatorDiffSchema.php it('handles multiple generated files', async () => { const repo = new Repository(repoInfo, ctx); - const ejecaSpy = mockEjeca([[/^sl diff/, () => ({stdout: EXAMPLE_DIFFSTAT})]]); + const execaSpy = mockExeca([[/^sl diff/, () => ({stdout: EXAMPLE_DIFFSTAT})]]); const generatedFiles = ['generated1.file', 'generated2.file']; repo.fetchSignificantLinesOfCode(ctx, 'abcdef', generatedFiles); await nextTick(); - expect(ejecaSpy).toHaveBeenCalledWith( + expect(execaSpy).toHaveBeenCalledWith( 'sl', expect.arrayContaining([ 'diff', @@ -466,33 +470,33 @@ www/flib/intern/entity/diff/EntPhabricatorDiffSchema.php it('uses correct revset in normal case', async () => { const repo = new Repository(repoInfo, ctx); - const ejecaSpy = mockEjeca([]); + const execaSpy = mockExeca([]); await repo.fetchSmartlogCommits(); expectCalledWithRevset( - ejecaSpy, + execaSpy, 'smartlog(((interestingbookmarks() + heads(draft())) & date(-14)) + .)', ); }); it('updates revset when changing date range', async () => { - const ejecaSpy = mockEjeca([]); + const execaSpy = mockExeca([]); const repo = new Repository(repoInfo, ctx); repo.nextVisibleCommitRangeInDays(); await repo.fetchSmartlogCommits(); expectCalledWithRevset( - ejecaSpy, + execaSpy, 'smartlog(((interestingbookmarks() + heads(draft())) & date(-60)) + .)', ); repo.nextVisibleCommitRangeInDays(); await repo.fetchSmartlogCommits(); - expectCalledWithRevset(ejecaSpy, 'smartlog((interestingbookmarks() + heads(draft())) + .)'); + expectCalledWithRevset(execaSpy, 'smartlog((interestingbookmarks() + heads(draft())) + .)'); }); it('fetches additional revsets', async () => { - const ejecaSpy = mockEjeca([]); + const execaSpy = mockExeca([]); const repo = new Repository(repoInfo, ctx); repo.stableLocations = [ @@ -500,7 +504,7 @@ www/flib/intern/entity/diff/EntPhabricatorDiffSchema.php ]; await repo.fetchSmartlogCommits(); expectCalledWithRevset( - ejecaSpy, + execaSpy, 'smartlog(((interestingbookmarks() + heads(draft())) & date(-14)) + . + present(aaa))', ); @@ -510,7 +514,7 @@ www/flib/intern/entity/diff/EntPhabricatorDiffSchema.php ]; await repo.fetchSmartlogCommits(); expectCalledWithRevset( - ejecaSpy, + execaSpy, 'smartlog(((interestingbookmarks() + heads(draft())) & date(-14)) + . + present(aaa) + present(bbb))', ); @@ -518,7 +522,7 @@ www/flib/intern/entity/diff/EntPhabricatorDiffSchema.php repo.nextVisibleCommitRangeInDays(); await repo.fetchSmartlogCommits(); expectCalledWithRevset( - ejecaSpy, + execaSpy, 'smartlog((interestingbookmarks() + heads(draft())) + . + present(aaa) + present(bbb))', ); }); @@ -606,7 +610,7 @@ ${MARK_OUT} const MOCK_CONFLICT_WITH_FILE1_RESOLVED: ResolveCommandConflictOutput = clone(MOCK_CONFLICT); MOCK_CONFLICT_WITH_FILE1_RESOLVED[0].conflicts.splice(0, 1); - // these mock values are returned by ejeca / fs mocks + // these mock values are returned by execa / fs mocks // default: start in a not-in-conflict state let slMergeDirExists = false; let conflictData: ResolveCommandConflictOutput = NOT_IN_CONFLICT; @@ -625,7 +629,7 @@ ${MARK_OUT} jest.spyOn(fsUtils, 'exists').mockImplementation(() => Promise.resolve(slMergeDirExists)); - mockEjeca([ + mockExeca([ [ /^sl resolve --tool internal:dumpjson --all/, () => ({stdout: JSON.stringify(conflictData)}), @@ -806,7 +810,7 @@ ${MARK_OUT} }); it('handles errors from `sl resolve`', async () => { - mockEjeca([ + mockExeca([ [/^sl resolve --tool internal:dumpjson --all/, new Error('failed to do the thing')], ]); @@ -991,7 +995,7 @@ describe('absolutePathForFileInRepo', () => { describe('getCwdInfo', () => { it('computes cwd path and labels', async () => { - mockEjeca([[/^sl root/, {stdout: '/path/to/myRepo'}]]); + mockExeca([[/^sl root/, {stdout: '/path/to/myRepo'}]]); jest.spyOn(fs.promises, 'realpath').mockImplementation(async (path, _opts) => { return path as string; }); @@ -1010,7 +1014,7 @@ describe('getCwdInfo', () => { }); it('uses realpath', async () => { - mockEjeca([[/^sl root/, {stdout: '/data/users/name/myRepo'}]]); + mockExeca([[/^sl root/, {stdout: '/data/users/name/myRepo'}]]); jest.spyOn(fs.promises, 'realpath').mockImplementation(async (path, _opts) => { return (path as string).replace(/^\/home\/name\//, '/data/users/name/'); }); @@ -1029,7 +1033,7 @@ describe('getCwdInfo', () => { }); it('returns null for non-repos', async () => { - mockEjeca([[/^sl root/, new Error('not a repository')]]); + mockExeca([[/^sl root/, new Error('not a repository')]]); await expect( Repository.getCwdInfo({ cmd: 'sl', diff --git a/addons/isl-server/src/__tests__/analytics.test.ts b/addons/isl-server/src/__tests__/analytics.test.ts index 9b93d187c614f..036bc6585f3f5 100644 --- a/addons/isl-server/src/__tests__/analytics.test.ts +++ b/addons/isl-server/src/__tests__/analytics.test.ts @@ -13,7 +13,7 @@ import type {RepositoryContext} from '../serverTypes'; import {Repository} from '../Repository'; import {makeServerSideTracker} from '../analytics/serverSideTracker'; import {setConfigOverrideForTests} from '../commands'; -import * as ejeca from 'shared/ejeca'; +import * as execa from 'execa'; import {mockLogger} from 'shared/testUtils'; import {defer} from 'shared/utils'; @@ -41,12 +41,16 @@ jest.mock('../WatchForChanges', () => { return {WatchForChanges: MockWatchForChanges}; }); -function mockEjeca( +jest.mock('execa', () => { + return jest.fn(); +}); + +function mockExeca( cmds: Array<[RegExp, (() => {stdout: string} | Error) | {stdout: string} | Error]>, ) { - return jest.spyOn(ejeca, 'ejeca').mockImplementation(((cmd: string, args: Array) => { + return jest.spyOn(execa, 'default').mockImplementation(((cmd: string, args: Array) => { const argStr = cmd + ' ' + args?.join(' '); - const ejecaOther = { + const execaOther = { kill: jest.fn(), on: jest.fn((event, cb) => { // immediately call exit cb to teardown timeout @@ -64,11 +68,11 @@ function mockEjeca( if (value instanceof Error) { throw value; } - return {...ejecaOther, ...value}; + return {...execaOther, ...value}; } } - return {...ejecaOther, stdout: ''}; - }) as unknown as typeof ejeca.ejeca); + return {...execaOther, stdout: ''}; + }) as unknown as typeof execa.default); } describe('track', () => { @@ -120,7 +124,7 @@ describe('track', () => { ['path.default', 'https://github.com/facebook/sapling.git'], ['github.pull_request_domain', 'github.com'], ]); - const ejecaSpy = mockEjeca([ + const execaSpy = mockExeca([ [/^sl root --dotdir/, {stdout: '/path/to/myRepo/.sl'}], [/^sl root/, {stdout: '/path/to/myRepo'}], [ @@ -156,7 +160,7 @@ describe('track', () => { mockLogger, ); repo.dispose(); - ejecaSpy.mockClear(); + execaSpy.mockClear(); }); it('uses consistent session id, but different track ids', () => { diff --git a/addons/isl-server/src/commands.ts b/addons/isl-server/src/commands.ts index ad2044f552e68..b6cce19b4fccd 100644 --- a/addons/isl-server/src/commands.ts +++ b/addons/isl-server/src/commands.ts @@ -6,12 +6,11 @@ */ import type {RepositoryContext} from './serverTypes'; -import type {EjecaOptions, EjecaReturn} from 'shared/ejeca'; -import {isEjecaError} from './utils'; +import {isExecaError} from './utils'; +import execa from 'execa'; import {ConflictType, type AbsolutePath, type MergeConflicts} from 'isl/src/types'; import os from 'node:os'; -import {ejeca} from 'shared/ejeca'; export const MAX_FETCHED_FILES_PER_COMMIT = 25; export const MAX_SIMULTANEOUS_CAT_CALLS = 4; @@ -53,12 +52,12 @@ export type ResolveCommandConflictOutput = [ export async function runCommand( ctx: RepositoryContext, args_: Array, - options_?: EjecaOptions, + options_?: execa.Options, timeout: number = READ_COMMAND_TIMEOUT_MS, -): Promise { +): Promise> { const {command, args, options} = getExecParams(ctx.cmd, args_, ctx.cwd, options_); ctx.logger.log('run command: ', ctx.cwd, command, args[0]); - const result = ejeca(command, args, options); + const result = execa(command, args, options); let timedOut = false; let timeoutId: NodeJS.Timeout | undefined; @@ -77,7 +76,7 @@ export async function runCommand( const val = await result; return val; } catch (err: unknown) { - if (isEjecaError(err)) { + if (isExecaError(err)) { if (err.killed) { if (timedOut) { throw new Error('Timed out'); @@ -175,12 +174,12 @@ export function getExecParams( command: string, args_: Array, cwd: string, - options_?: EjecaOptions, + options_?: execa.Options, env?: NodeJS.ProcessEnv | Record, ): { command: string; args: Array; - options: EjecaOptions; + options: execa.Options; } { let args = [...args_, '--noninteractive']; // expandHomeDir is not supported on windows @@ -204,7 +203,7 @@ export function getExecParams( // allow looking up diff numbers even in plain mode. // allow constructing the `.git/sl` repo regardless of the identity. // allow automatically setting ui.username. - SL_AUTOMATION_EXCEPT: 'ghrevset,phrevset,progress,sniff,username', + SL_AUTOMATION_EXCEPT: 'ghrevset,phrevset,sniff,username', EDITOR: undefined, VISUAL: undefined, HGUSER: undefined, @@ -215,7 +214,7 @@ export function getExecParams( langEnv = 'C.UTF-8'; } newEnv.LANG = langEnv; - const options: EjecaOptions = { + const options: execa.Options = { ...options_, env: newEnv, cwd, @@ -226,10 +225,6 @@ export function getExecParams( args.push('--config', 'fsmonitor.watchman-query-lock=True'); } - if (options.ipc) { - args.push('--config', 'progress.renderer=nodeipc'); - } - // TODO: we could run with systemd for better OOM protection when on linux return {command, args, options}; } diff --git a/addons/isl-server/src/github/queryGraphQL.ts b/addons/isl-server/src/github/queryGraphQL.ts index 7d658fb4314a2..e9265b5069f96 100644 --- a/addons/isl-server/src/github/queryGraphQL.ts +++ b/addons/isl-server/src/github/queryGraphQL.ts @@ -6,8 +6,8 @@ */ import {Internal} from '../Internal'; -import {isEjecaError} from '../utils'; -import {ejeca} from 'shared/ejeca'; +import {isExecaError} from '../utils'; +import execa from 'execa'; export default async function queryGraphQL( query: string, @@ -39,7 +39,7 @@ export default async function queryGraphQL( args.push('-f', `query=${query}`); try { - const {stdout} = await ejeca('gh', args, { + const {stdout} = await execa('gh', args, { env: { ...((await Internal.additionalGhEnvVars?.()) ?? {}), }, @@ -52,7 +52,7 @@ export default async function queryGraphQL( return json.data; } catch (error: unknown) { - if (isEjecaError(error)) { + if (isExecaError(error)) { if (error.code === 'ENOENT' || error.code === 'EACCES') { // `gh` not installed on path throw new Error(`GhNotInstalledError: ${(error as Error).stack}`); @@ -76,7 +76,7 @@ export async function isGithubEnterprise(hostname: string): Promise { args.push('--hostname', hostname); try { - await ejeca('gh', args, { + await execa('gh', args, { env: { ...((await Internal.additionalGhEnvVars?.()) ?? {}), }, diff --git a/addons/isl-server/src/utils.ts b/addons/isl-server/src/utils.ts index 8a29fe644040c..7229ec3b15e9b 100644 --- a/addons/isl-server/src/utils.ts +++ b/addons/isl-server/src/utils.ts @@ -5,8 +5,9 @@ * LICENSE file in the root directory of this source tree. */ +import type execa from 'execa'; +import type {ExecaChildProcess, ExecaError} from 'execa'; import type {CommitInfo, SmartlogCommits} from 'isl/src/types'; -import type {EjecaError, EjecaReturn, EjecaChildProcess} from 'shared/ejeca'; import os from 'node:os'; import {truncate} from 'shared/utils'; @@ -78,7 +79,7 @@ export function serializeAsyncCall(asyncFun: () => Promise): () => Promise * This is slightly more robust than execa 6.0 and nodejs' `signal` support: * if a process was stopped (by `SIGTSTP` or `SIGSTOP`), it can still be killed. */ -export function handleAbortSignalOnProcess(child: EjecaChildProcess, signal: AbortSignal) { +export function handleAbortSignalOnProcess(child: ExecaChildProcess, signal: AbortSignal) { signal.addEventListener('abort', () => { if (os.platform() == 'win32') { // Signals are ignored on Windows. @@ -129,7 +130,7 @@ export function findPublicAncestor( * Return a JSON object. On error, the JSON object has property "error". */ export function parseExecJson( - exec: Promise, + exec: Promise>, reply: (parsed?: T, error?: string) => void, ) { exec @@ -164,7 +165,7 @@ export function parseExecJson( }); } -export function isEjecaError(s: unknown): s is EjecaError & {code?: string} { +export function isExecaError(s: unknown): s is ExecaError & {code?: string} { return s != null && typeof s === 'object' && 'exitCode' in s; } diff --git a/addons/isl/integrationTests/setup.tsx b/addons/isl/integrationTests/setup.tsx index 46136cd27d591..1f015d9d08826 100644 --- a/addons/isl/integrationTests/setup.tsx +++ b/addons/isl/integrationTests/setup.tsx @@ -7,7 +7,7 @@ import type {MessageBusStatus} from '../src/MessageBus'; import type {Disposable, RepoRelativePath} from '../src/types'; -import type {EjecaOptions} from 'shared/ejeca'; +import type {Options as ExecaOptions} from 'execa'; import type {Logger} from 'isl-server/src/logger'; import type {ServerPlatform} from 'isl-server/src/serverPlatform'; import type {RepositoryContext} from 'isl-server/src/serverTypes'; @@ -151,7 +151,7 @@ export async function initRepo() { tracker: mockTracker, }; - async function sl(args: Array, options?: EjecaOptions) { + async function sl(args: Array, options?: ExecaOptions) { testLogger.log(ctx.cmd, ...args); const result = await runCommand(ctx, args, { ...options, diff --git a/addons/isl/src/types.ts b/addons/isl/src/types.ts index 81fe5f74438a0..bb08e5752a999 100644 --- a/addons/isl/src/types.ts +++ b/addons/isl/src/types.ts @@ -717,7 +717,6 @@ export const allConfigNames = [ 'isl.download-commit-rebase-type', 'isl.experimental-features', 'isl.hold-off-refresh-ms', - 'isl.sl-progress-enabled', 'isl.use-sl-graphql', 'github.preferred_submit_command', 'isl.open-file-cmd', diff --git a/addons/screenshot-tool/src/testRepo.ts b/addons/screenshot-tool/src/testRepo.ts index 158d5aff6854e..5625f7752278f 100644 --- a/addons/screenshot-tool/src/testRepo.ts +++ b/addons/screenshot-tool/src/testRepo.ts @@ -6,7 +6,7 @@ */ import {getCacheDir, sha1} from './utils'; -import {ejeca} from 'shared/ejeca'; +import {execa} from 'execa'; import * as fs from 'node:fs/promises'; import {join} from 'node:path'; import {dirSync} from 'tmp'; @@ -72,7 +72,7 @@ export class TestRepo { async run(args: Array, input = ''): Promise { const env = {...process.env, SL_AUTOMATION: '1', HGPLAIN: '1'}; logger.info('Running', this.command, args.join(' ')); - const child = await ejeca(this.command, args, { + const child = await execa(this.command, args, { cwd: this.repoPath, input: Buffer.from(input), env,