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

REPL shell mode for Windows #31490

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

REPL shell mode for Windows #31490

wants to merge 3 commits into from

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Mar 26, 2019

Updated and simplified version of #23703 and #21294.

On Windows, the JULIA_SHELL environment variable can be cmd, powershell, busybux, or "", defaulting to cmd, and REPL shell-mode commands are now passed through this shell.

(Compared to the previous PR, I removed the changes to line parsing and the cd command.)

I haven't tested this myself yet — just wanted to bring it back to a testable state. One thing I'm not sure of is the correct quoting of commands to pass to the shell — Windows escaping is a huge annoyance because of the lack of a proper fork.

cc @GregPlowman

REPL shell mode for Windows

Documentation JULIA_SHELL environment variable

Interacting with Julia docs - xref JULIA_SHELL

Fix whitespace JULIA_SHELL doc

Fix JULIA_SHELL cross-reference

Address code review comments

rebase fix
@stevengj stevengj added system:windows Affects only Windows REPL Julia's REPL (Read Eval Print Loop) labels Mar 26, 2019
@musm
Copy link
Contributor

musm commented Mar 26, 2019

Windows escaping is a huge annoyance because of the lack of a proper fork.

https://github.com/JuliaLang/julia/pull/30614/files#diff-06a75b3b926c52ae4594356ce1eb5405R343

@stevengj
Copy link
Member Author

stevengj commented Mar 26, 2019

@musm, yes, but that depends on the program to interpret it correctly. Does busybox use this convention, for example? PowerShell? cmd.exe? You have to check on a case-by-case basis. I don't know the answers offhand in this case, but if someone could check that would be great.

Also, I thought that libuv already executes this escaping algorithm (unless you set windows_verbatim=true… why do we need a Julia version)?

@stevengj
Copy link
Member Author

Ah, it looks like the escaping you linked was specifically for cmd.exe, so we should use it there. Not sure what to do about busybox and powershell.

@GregPlowman
Copy link
Contributor

Thanks for taking this over. (I'm somewhat out of my depth here).

To clarify a few things from the previous PR, if I remember correctly:

The (probably flawed) approach was to pass the raw input line as a String to repl_cmd rather than as a Cmd. The idea was that this raw string could be sent to cmd.exe via windows_verbatim=true without any processing. i.e. without converting to a Cmd and then re-assembling back into a String, avoiding all the escaping and unescaping pitfalls.

The reason for the eval in the previous PR (cmd = cmd_gen(eval(Main, shell_parse(line)[1])) is to convert the raw line into a Cmd. (Currently this conversion happens earlier in REPL.jl and the Cmd is passed to repl_cmd, rather than raw input line)

With this PR manipulating a Cmd rather than the raw line, I think for cmd.exe we should have something like:

command = `$shell /s /c $(shell_escape_dosly(cmd))`

See comments here regarding shell_escape_dosly and shell_escape_win32ly being split and moved alongside shell_escape_posixly.

If you remove the special code for cd, will it be persistent, since it will be executed in a separate shell?

(Also, in the previous PR, there was a change for when there are 2 or more arguments to cd. In this case, the command line is processed as normal, rather than throwing an error. This allows commands such as cd $dir && $command to be executed.)

@stevengj
Copy link
Member Author

The special handling of cd is unchanged in this PR. Any changes for more than 2 arguments can be a separate PR if desired.

Calling eval in this function is undesirable, and it seems more conservative in any case to keep the Cmd parsing as-is, the same on Windows and Unix.

@musm
Copy link
Contributor

musm commented May 8, 2019

We should also add "pwsh" for the shell_name to accommodate PowerShell 6 . The exe name is pwsh

@stevengj
Copy link
Member Author

Would be good if someone who actually uses Windows could work on this…

@musm
Copy link
Contributor

musm commented Sep 17, 2019

At some point I did try to revive this PR, but I wasn't able to determine the exact shell-escaping rules et al. for powershell and cmd and which shell-escaping rules julia implements which halted progress.

@stevengj
Copy link
Member Author

If you use windows_verbatim=true in the Cmd, then Julia does no escaping at all, so you have full manual control. By default libuv uses the escaping rules from CommandLineToArgW, which is the right thing for most Windows software. See also #13776

@musm
Copy link
Contributor

musm commented Sep 23, 2019

For CMD why can't we then just do :

                command = Cmd(`$shell /s /c $(string('"', join(cmd.exec, " "), '"'))`, windows_verbatim=true)

i.e. just join the cmd passed to repl_cmd

It looks like it works locally. The only issue is interpolation clashing with environment variables.
E.g. shell> echo $HOME for powershell

@musm
Copy link
Contributor

musm commented Sep 23, 2019

So general question:

How do you handle string interpolation and the fact that powershell and cmd allow $ ?

@stevengj
Copy link
Member Author

How do you handle string interpolation and the fact that powershell and cmd allow $?

The user needs to escape such characters:

shell> echo \$
$

@PallHaraldsson
Copy link
Contributor

Bump, is this done and/or forgotten? It's seemingly good simple code ready to merge. Or outdated then just close? I'm not on Windows to test...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop) system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants