Skip to content

Commit

Permalink
progress: backout D65769371 and D65138587
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@sggutier @facebook-github-bot
sggutier authored and facebook-github-bot committed Nov 25, 2024
1 parent efbd9d3 commit 2ce5992
Show file tree
Hide file tree
Showing 10 changed files with 105 additions and 150 deletions.
78 changes: 15 additions & 63 deletions addons/isl-server/src/Repository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -76,9 +76,10 @@ import {
import {
findPublicAncestor,
handleAbortSignalOnProcess,
isEjecaError,
isExecaError,
serializeAsyncCall,
} from './utils';
import execa from 'execa';
import {
settableConfigNames,
allConfigNames,
Expand All @@ -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';
Expand Down Expand Up @@ -654,52 +654,6 @@ export class Repository {
return {args, stdin};
}

private async operationIPC(
ctx: RepositoryContext,
onProgress: OperationCommandProgressReporter,
child: EjecaChildProcess,
options: EjecaOptions,
): Promise<void> {
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.
*/
Expand All @@ -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],
Expand All @@ -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`.
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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',
Expand All @@ -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
Expand Down Expand Up @@ -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', {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -1556,8 +1508,8 @@ function isUnhealthyEdenFs(cwd: string): Promise<boolean> {
}

/**
* 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);
}
4 changes: 2 additions & 2 deletions addons/isl-server/src/ServerToClientAPI.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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';
Expand Down Expand Up @@ -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',
Expand Down
Loading

0 comments on commit 2ce5992

Please sign in to comment.