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.

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

Terminal: add a way of getting the exit code #62103

Closed
fabiospampinato opened this issue Oct 30, 2018 · 17 comments · Fixed by #84000
Closed

Terminal: add a way of getting the exit code #62103

fabiospampinato opened this issue Oct 30, 2018 · 17 comments · Fixed by #84000
Assignees
Labels
api api-finalization feature-request Request for new features or functionality on-testplan terminal Integrated terminal issues
Milestone

Comments

@fabiospampinato
Copy link
Contributor

It would be great if each terminal object had an exitCode property that will resolve to the actual exit code of the process. Something like this:

Terminal {
  exitCode: Thenable<number> 
}

It would be especially useful when running commands in the terminal dynamically from extensions, this way we could easily detect when the terminal exited and if it the command exited without errors.

@vscodebot
Copy link

vscodebot bot commented Oct 30, 2018

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@vscodebot vscodebot bot added the terminal Integrated terminal issues label Oct 30, 2018
@Tyriar Tyriar added feature-request Request for new features or functionality api labels Oct 31, 2018
@Tyriar Tyriar added this to the Backlog milestone Oct 31, 2018
@mrmeku
Copy link

mrmeku commented Dec 30, 2018

The upcoming Angular Console VS Code extension (https://angularconsole.com/) would greatly benefit from the addition of this API. Without it, we are forced to run commands and echo a message after wards to indicate the exit code and screen scrap the terminal to get it.

@Tyriar
Copy link
Member

Tyriar commented Jan 1, 2019

I'm not sure we would want a Thenable on the API for this as that's what onDidCloseTerminal is for. Maybe this?

class Terminal {
  /**
   * The exit code of the terminal process, if the process has not yet
   * exited the value will be -1.
   */
  exitCode: number
}

I'll bring it up in an upcoming API meeting so I can open this up to PRs.

@fabiospampinato
Copy link
Contributor Author

@Tyriar To give some more context the use-case I've encountered the most is: being able to execute a command in a newly created terminal, closing it immediately after that and knowing if the command succeeded and what the output was.

I think currently most of this can be achieved reasonably easily via APIs, but retrieving the exit code is still a pain point.

Also if the proposed API were to be implemented, how would that work, should I execute my commands like so to retrieve the exit code?:

( my-command && exit $? ) || exit $? 

@Tyriar
Copy link
Member

Tyriar commented Jan 2, 2019

@fabiospampinato you would run the command inside a shell: bash -c my-command

@fabiospampinato
Copy link
Contributor Author

@Tyriar I'm not sure I'm following, aren't all terminal commands run inside a shell? Don't we have to explicitly exit it to get an exit code via Terminal#exitCode?

@Tyriar
Copy link
Member

Tyriar commented Jan 2, 2019

@fabiospampinato you'll need to do bash -c if you want to change the shell/shellArgs, you could also run exit as you suggest if you're using the configured shell via sendText. The latter comes with some significant downsides in that syntax for doing this differs between shells so it might not work on the user's config.

@fabiospampinato
Copy link
Contributor Author

@Tyriar Thanks for the explanation. Is this ergonomic enough though? I imagine things will get unpleasant quickly if each extension author has to think about what shell is available where and what's each shell's syntax for executing a command 🤔

I would be great if there was an official API that abstracts all this complexity away, at least for the most popular shells, so that we could just write something like:

const {output, exitCode} = await Terminal.execAndExit ( command, TerminalOptions );

@Tyriar
Copy link
Member

Tyriar commented Jan 2, 2019

@fabiospampinato well that's basically what the tasks API is meant to do. I'm beginning to think maybe we shouldn't have this API for this reason.

@mrmeku
Copy link

mrmeku commented Jan 17, 2019

The tasks API doesn't expose Stdout though. The use case in which you want to have access to stdout and an exit code is not supported by either api right now

@Tyriar
Copy link
Member

Tyriar commented Oct 8, 2019

Proposal:

export interface Terminal {
  /**
   * The exit code of the terminal process, this will be undefined when the process was killed by
   * the user or no exit code was specified (for example when using CustomExecution).
   */
  readonly exitCode: Thenable<number | undefined>;
}

@Tyriar
Copy link
Member

Tyriar commented Oct 8, 2019

Adding to October to discuss, it won't happen in October though.

@Tyriar Tyriar modified the milestones: Backlog, October 2019 Oct 8, 2019
@Tyriar
Copy link
Member

Tyriar commented Oct 14, 2019

Discussed at the API meeting today and we ended up landing on this:

export interface TerminalExitStatus {
	/**
	 * The exit code that a terminal exited with, it can have the following values:
	 * - Zero: the terminal process or custom execution succeeded.
	 * - Non-zero: the terminal process or custom execution failed.
	 * - `undefined`: the user forcefully closed the terminal or a custom execution exited without
	 *   providing an exit code.
	 */
	code: number | undefined;
}

export interface Terminal {
  /**
   * The exit status of the terminal, this will be undefined while the terminal is active.
   *
   * TODO: Add an example here using onDidCloseTerminal
   */
  exitStatus: TerminalExitStatus | undefined;
}

That way there's a single async listener for exit in onDidCloseTerminal and exitStatus can be fetched during that event:

window.onDidCloseTerminal(t => {
	console.log('exit status', t.exitStatus!.code);
});

@Tyriar Tyriar modified the milestones: October 2019, November 2019 Oct 14, 2019
@Tyriar
Copy link
Member

Tyriar commented Nov 4, 2019

We should use readonly for exitStatus/code

@Tyriar
Copy link
Member

Tyriar commented Nov 5, 2019

This will be available for testing in the first 1.41 insiders (probably 1-2 days away), let me know how it goes. FYI @eamodio

@Tyriar Tyriar reopened this Nov 29, 2019
@Tyriar Tyriar modified the milestones: November 2019, On Deck Nov 29, 2019
@Tyriar
Copy link
Member

Tyriar commented Nov 29, 2019

The plan is to promote this to stable for December, any feedback/validation in the meantime would be appreciated.

@Tyriar Tyriar modified the milestones: On Deck, December 2019 Nov 29, 2019
@Tyriar
Copy link
Member

Tyriar commented Dec 30, 2019

Has anyone validated that this is working as expected? I'll hold off finalizing until I get some confirmation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization feature-request Request for new features or functionality on-testplan terminal Integrated terminal issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants