-
Notifications
You must be signed in to change notification settings - Fork 29
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.
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
Send all stderr and stdout to the output channel when a DVC process fails #3857
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import { Event, EventEmitter } from 'vscode' | ||
import { getCommandString } from './command' | ||
import { CliError, MaybeConsoleError } from './error' | ||
import { notifyCompleted, notifyStarted } from './util' | ||
import { notifyCompleted, notifyStarted, transformChunkToString } from './util' | ||
import { createProcess, Process, ProcessOptions } from '../process/execution' | ||
import { StopWatch } from '../util/time' | ||
import { Disposable } from '../class/dispose' | ||
|
@@ -13,7 +13,7 @@ export type CliEvent = { | |
} | ||
|
||
export type CliResult = CliEvent & { | ||
stderr?: string | ||
errorOutput?: string | ||
duration: number | ||
exitCode: number | null | ||
} | ||
|
@@ -72,13 +72,19 @@ export class Cli extends Disposable implements ICli { | |
|
||
protected async executeProcess(options: ProcessOptions): Promise<string> { | ||
const { baseEvent, stopWatch } = this.getProcessDetails(options) | ||
let all = '' | ||
try { | ||
const process = this.dispose.track(this.createProcess(baseEvent, options)) | ||
|
||
void process.on('close', () => { | ||
void this.dispose.untrack(process) | ||
}) | ||
|
||
process.all?.on( | ||
'data', | ||
chunk => (all += transformChunkToString(chunk as Buffer)) | ||
) | ||
|
||
const { stdout, exitCode } = await process | ||
|
||
notifyCompleted( | ||
|
@@ -96,18 +102,26 @@ export class Cli extends Disposable implements ICli { | |
error as MaybeConsoleError, | ||
options, | ||
baseEvent, | ||
stopWatch.getElapsedTime() | ||
stopWatch.getElapsedTime(), | ||
all | ||
) | ||
} | ||
} | ||
|
||
protected async createBackgroundProcess(options: ProcessOptions) { | ||
const { baseEvent, stopWatch } = this.getProcessDetails(options) | ||
let all = '' | ||
try { | ||
const backgroundProcess = this.createProcess(baseEvent, { | ||
detached: true, | ||
...options | ||
}) | ||
|
||
backgroundProcess.all?.on( | ||
'data', | ||
chunk => (all += transformChunkToString(chunk as Buffer)) | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be moved to a private function, or even a util (just because it's being duplicated from line 83)? That would also make sure this line is covered since line 83 is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did have this in a separate helper function but the string was not captured as expected and passed to the catch block. |
||
|
||
return await this.getOutputAndDisconnect( | ||
baseEvent, | ||
backgroundProcess, | ||
|
@@ -118,7 +132,8 @@ export class Cli extends Disposable implements ICli { | |
error as MaybeConsoleError, | ||
options, | ||
baseEvent, | ||
stopWatch.getElapsedTime() | ||
stopWatch.getElapsedTime(), | ||
all | ||
) | ||
} | ||
} | ||
|
@@ -170,7 +185,8 @@ export class Cli extends Disposable implements ICli { | |
error: MaybeConsoleError, | ||
options: ProcessOptions, | ||
baseEvent: CliEvent, | ||
duration: number | ||
duration: number, | ||
all: string | ||
) { | ||
const cliError = new CliError({ | ||
baseError: error, | ||
|
@@ -180,8 +196,8 @@ export class Cli extends Disposable implements ICli { | |
{ | ||
...baseEvent, | ||
duration, | ||
exitCode: cliError.exitCode, | ||
stderr: cliError.stderr | ||
errorOutput: all || cliError.stderr, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [F] If we have captured a valid all string (length > 0) then we will use that, otherwise use the stderr from the generated error. I feel like this way there will always be something provided to the output channel |
||
exitCode: cliError.exitCode | ||
}, | ||
this.processCompleted | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] The runner and viewer show processes in the foreground so there is no change for these