Skip to content

Run preview and render as tasks with ShellExecution, so VS Code quotes arguments for the shell #1203

Open

Description

@juliasilge

Summary

The extension runs Quarto for preview and render with terminal.sendText(). It builds the command line itself, so it must quote each argument for the shell that the terminal starts. In #1197, we are correcting some reported problems (#1194, #1102, posit-dev/positron#15907) by getting the shell from vscode.env.shell and quoting for bash, zsh, sh, PowerShell, and cmd. However, this approach still guesses which shell reads the command line.

I'd like to propose to run these commands as tasks with ShellExecution, and to give the arguments as ShellQuotedString values. Then the task system finds the shell and quotes each argument for that shell. Positron uses this approach, and it works well there.

Problems that remain with the current approach

  • The extension knows only bash, zsh, sh, PowerShell, and cmd. For all other shells (for example fish, nu, and WSL), it quotes only on whitespace. As a result, paths with parentheses, ;, or $ still break in these shells. For example, fish runs (md) as a command substitution.
  • vscode.env.shell gives the default profile at the time that the extension sends the command. This shell is not always the shell that the terminal runs.
  • With useCmd (Quarto 1.1.162 and earlier on Windows), the extension quotes the path for cmd.exe and puts the line in cmd /C"...". But the extension types this line into the default shell of the user, which is often PowerShell.
  • The extension quotes only the document path. It does not quote the other arguments. For example, R package preview adds --output-dir <temp dir> with no quotes, so a Windows user name with a space breaks the preview.

Scope of the change

Render (apps/vscode/src/providers/render.ts) would be the best place to start. It makes a terminal, sends the command, and removes the terminal by name before the next render. No other code tracks this terminal.

Preview (apps/vscode/src/providers/preview/preview.ts) needs more work, because its design uses a Terminal object:

  • It keeps this.terminal_, reads exitStatus, calls show(), and uses onDidCloseTerminal to send the terminate request.
  • The Positron Viewer stop button (Show Positron's Viewer stop button for Quarto previews #1193) uses onDidEndTerminalShellExecution for that terminal. After the user stops the preview, the terminal stays open, so the user can read the output.
  • killTerminal() finds the terminal by its name.

A task gives a TaskExecution, not a Terminal. Thus this lifecycle must move to tasks.onDidStartTaskProcess, tasks.onDidEndTaskProcess, and TaskExecution.terminate().

Open questions

  • Python environment activation: before it types the command, sendTerminalCommand() waits up to 5 seconds for venv or conda activation. How does activation work in task terminals, in VS Code and in Positron?
  • After the user stops the preview, can a task terminal keep its output visible, as the stop button does now?
  • Can we remove the useCmd path? It applies only to Quarto 1.1.162 and earlier.

Proposed plan

  1. Move render to ShellExecution. Do a test with conda and venv environments.
  2. Move preview, with the stop button and the restart behavior, in VS Code and in Positron.
  3. When no code uses them, remove the shell detection and quoting from terminalCommand() (shQuote(), shQuoteUnknownShell(), terminalShellForPath()).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions