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

Allow built-in commands in shell mode for Windows #11478

Closed
wants to merge 2 commits into from

Conversation

ncnc
Copy link
Contributor

@ncnc ncnc commented May 28, 2015

This was originally part of #7108 by @quinnj but appears to have fallen between the cracks. It allows using built-in dos commands from the Julia shell-mode. edit() already works the same way.

@tkelman tkelman added system:windows Affects only Windows REPL Julia's REPL (Read Eval Print Loop) labels May 29, 2015
@ncnc
Copy link
Contributor Author

ncnc commented May 29, 2015

Thinking about this some more, there was some discussion about proper quoting in the original pull request. I think I need to explore fixing that part better. I originally couldn't think of good examples of the code breaking down, but I just realized that refering to filenames with special characters and no whitespace won't work.

E.g.

shell> type "test &.txt"
[works]

shelll> type "test&.txt"
[won't work because cmd.exe eats the quoting]

I might as well see if I can fix that before this gets committed.

@ncnc
Copy link
Contributor Author

ncnc commented May 29, 2015

Here's an updated version, largely following the tips from @vtjnash in the prior thread.

I added another argument to jl_spawn(), which stops libuv from further escaping the command line. I then build a command string where I quote (") all arguments containing characters that are special in cmd.exe and pass that string on as-is.

Any feedback and examples of general brokenness are welcome.

cmdstr, sep = "", ""
for s in cmd.exec
dq = ismatch(special, s) ? "\"" : ""
cmdstr = cmdstr * sep * dq * s * dq
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better to do this through an IOBuffer rather than repeated string concatenation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@tkelman
Copy link
Contributor

tkelman commented May 30, 2015

This seems awfully complicated for not a whole lot of benefit. cmd is a really really dumb shell, there's nothing there worth using.

@ncnc
Copy link
Contributor Author

ncnc commented May 30, 2015

Yes, the key use case for my own part is mostly about managing files and directories in a convenient way and as long as I can refer to arbitrary files I'm probably ok with some limitations. My first take had problems with special characters.

I think I found a simpler way of doing some of these things. Rather than quoting whole expressions, one can actually quote just the special characters using either " (whitespace) or ^ (other special characters). In combination with a generic cmdshell_escape I think that will simplify things somewhat.

I'll make another round through the code...

@ncnc
Copy link
Contributor Author

ncnc commented May 30, 2015

A new version coming up soon. Cmd-related documentation is not particularly good or consistent, but i decided to use this:
http://stackoverflow.com/questions/4094699/how-does-the-windows-command-interpreter-cmd-exe-parse-scripts?lq=1
as my main reference (answer by ´jeb`).

The main functional difference compared to the previous version is that I now escape single characters, not words. Special characters are divided into two types, delimiters \t,;= and others "\n^&|<>(). Delimiters are double-quoted, the rest are escaped with a caret ^. As a special case I decided not to escape pipe related characters |<> in single-character words. This allows piping in shell mode if the piping operator is separated by whitespace.

Escaping is now separated into its own generic function and the command string is built using an IOBuffer().

[Edit: Clarified by striking soon. The new version came within a few minutes of the comment.]

@windows_only begin
cmdbuf = IOBuffer()
print_cmdshell_escaped(cmdbuf, cmd.exec...)
cmd.exec = ["cmd"; "/s /c \"$(bytestring(cmdbuf.data))\""]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"cmd" should be joinpath(SHGetKnownFolderPath(FOLDERID_Windows), "cmd.exe") (ref #11331), but for now, joinpath(ENV["WINDIR"], "cmd.exe") should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure about this. Neither cmd.exe nor powershell.exe reside directly under WINDIR on my system:

c:\windows\system32\cmd.exe
c:\windows\System32\WindowsPowerShell\v1.0\powershell.exe

Relying on Windows path settings seems like a more robust choice.

@vtjnash
Copy link
Member

vtjnash commented Jun 4, 2015

actually, thinking about this further, i think we should be able to just pretend that the abomination known as cmd doesn't exist and get all of this running through powershell.exe. that should cover the use cases people would have cared about with somewhat more sanity. from wikipedia, it sounds like it should be present on every windows computer that Julia supports (http://en.wikipedia.org/wiki/Windows_PowerShell#cite_ref-42).

but this "magic feature" has me shuddering painfully: http://stackoverflow.com/a/13770568/1712368

end
end
special_dq = " \t,;=" # delimiters can't be quoted with "^"
special_caret = "\"\n^&|<>()"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it might be better to throw an error if the passed command line includes a \n, since afaict you can't create a command line that includes an embedded \n (or \r) symbol:

If it is a caret (^) the next character has no special meaning, the caret itself is removed, if the caret is the last character of the line, the next line is appended, the first charater of the next line is always handled as escaped charater.

@tkelman
Copy link
Contributor

tkelman commented Jun 4, 2015

I believe powershell is installed by default since Vista and as an optional download on XP (an XP machine I use has it), though with slightly different initial versions per Windows version. One downside of powershell is a noticeable few-seconds .net runtime startup delay the first time you call into it, which is why I've only started using it in a few places initially. It's a way way nicer and more sane shell than cmd though. It has unix-y aliases for a lot of simple things, though the flags and output formats are generally different.

@vtjnash
Copy link
Member

vtjnash commented Jun 4, 2015

that's a lot faster than my alternative solution of downloading wubi on-the-fly, although perhaps less exciting than just replacing the julia installer with wubi :)

but seriously, i don't think we can tradeoff correctness for avoiding a bit of initial startup delay. is there any way to avoid that delay? it sounds pretty absurd given how much we've reduced the julia startup time that a statically compiled binary can't load faster. is this delay once per boot?

@tkelman
Copy link
Contributor

tkelman commented Jun 4, 2015

I don't know enough about .net to know what would cause it to cool down. We could fire off a background no-op powershell task at (or shortly after?) startup, if that might help mitigate things.

There may also be corporate environments which impose execution policy restrictions on powershell tasks, which is why my initial experiments with file downloading in BinDeps have been somewhat experimental/provisional/hoping to hear back if anything goes wrong.

a statically compiled binary can't load faster

Huh? Powershell relies on the .net runtime, which is pretty big - this is comparable to starting any jvm application AIUI.

edit: this looks like it might be fun to play with https://msdn.microsoft.com/en-us/library/99sz37yh%28v=vs.110%29.aspx

@pao
Copy link
Member

pao commented Jun 4, 2015

edit: this looks like it might be fun to play with https://msdn.microsoft.com/en-us/library/99sz37yh%28v=vs.110%29.aspx

"This function has been deprecated in the .NET Framework 4."

@tkelman
Copy link
Contributor

tkelman commented Jun 4, 2015

Could use https://msdn.microsoft.com/en-us/library/dd537633(v=vs.110).aspx for newer .net runtimes, but I don't keep close tabs on which versions of .net are available on which windows versions. I was just looking at powershell.exe in dependency walker on Windows 7 and that's the only thing I saw calling into mscoree.dll.

(isn't wubi deprecated too?)

@ncnc
Copy link
Contributor Author

ncnc commented Jun 5, 2015

Powershell sounds like an interesting approach. I'll have a look at that over the weekend and see how the escaping mechanism works.

@tkelman
Copy link
Contributor

tkelman commented Jun 5, 2015

This is a bit worrying though:

julia> @time run(`powershell -Command ""`)
elapsed time: 0.69077043 seconds (371464 bytes allocated)
julia> @time run(`powershell -Command ""`)
elapsed time: 0.46099512 seconds (1848 bytes allocated)
julia> @time run(`powershell -Command ""`)
elapsed time: 0.481438796 seconds (1848 bytes allocated)
julia> @time run(`powershell -Command ""`)
elapsed time: 0.385494125 seconds (1848 bytes allocated)
julia> @time run(`powershell -Command ""`)
elapsed time: 0.653287281 seconds (1848 bytes allocated)
julia> @time run(`cmd /c ""`)
elapsed time: 0.150796221 seconds (1848 bytes allocated)
julia> @time run(`cmd /c ""`)
elapsed time: 0.007307177 seconds (1848 bytes allocated)
julia> @time run(`cmd /c ""`)
elapsed time: 0.007413397 seconds (1848 bytes allocated)
julia> @time run(`cmd /c ""`)
elapsed time: 0.00727331 seconds (1848 bytes allocated)

@vtjnash
Copy link
Member

vtjnash commented Jun 5, 2015

Powershell sounds like an interesting approach. I'll have a look at that over the weekend and see how the escaping mechanism works.

IIUC, it uses the standard escaping mechanism for launching windows programs, so it won't need special handling, just escaping for --% as the first argument of the command line of the program that is being handed to powershell to run, if powershell is v3 or newer:

PS> echo %-- "a b" c %--
%--
"a b" c %--

PS> echo "%--" "a b" c %--
%--
a b
c
%--

(see link from http://stackoverflow.com/a/13770568/1712368)

@ncnc
Copy link
Contributor Author

ncnc commented Jun 7, 2015

I did the simplifications to the CMD escape code discussed above as well as throwing an error for newline. Making the CMD.EXE path absolute based on WINDIR won't work as such, as cmd.exe and powershell.exe both reside in various subdirectories of WINDIR.

For testing purposes I also added preliminary support for powershell through JULIA_SHELL. It currently escapes the --% command and passes everything as-is to powershell -command, but I think some additional quoting of e.g. " may be needed.

@@ -1251,6 +1251,43 @@ print_shell_escaped(io::IO) = nothing

shell_escape(args::AbstractString...) = sprint(print_shell_escaped, args...)

# Functions for Windows CMD-shell escaping
function print_cmdshell_word(io::IO, word::AbstractString)
if in('\n', word)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also applies to \r

@ncnc
Copy link
Contributor Author

ncnc commented Jun 9, 2015

Here's an update with escaping both for CMD and powershell. The escaping for powershell is partially based on empirical tests, since I haven't found all that good documentation. I also left out any special handling of --% in powershell.

@@ -1251,6 +1251,79 @@ print_shell_escaped(io::IO) = nothing

shell_escape(args::AbstractString...) = sprint(print_shell_escaped, args...)

# Functions for Windows shell escaping (mode can be :cmd or :powershell)
function print_cmdshell_word(io::IO, word::AbstractString, mode=:cmd)
if in('\n', word) || in('\r', word)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this true even of powershell?

@vtjnash
Copy link
Member

vtjnash commented Jul 18, 2015

looks like someone wants to add upstream support for this (libuv/libuv#358)

@tkelman
Copy link
Contributor

tkelman commented Jul 18, 2015

that would do bat files, but not cmd builtins, right?

ncnc added 2 commits July 21, 2015 00:47
- Use either CMD or Powershell based on SHELL / JULIA_SHELL (CMD is the default)
- Build properly formatted Cmd objects for the respective shells
- Add functions cmdshell_escape and friends for escaping data to CMD.
- Add functions powershell_escape and friends for escaping data to Powershell.
- Add a "verbatimargument" input parameter to jl_spawn(), which passes the
  flag UV_PROCESS_WINDOW_VERBATIM_ARGUMENTS to libuv, in order not to
  change escaping to CMD.
@ncnc
Copy link
Contributor Author

ncnc commented Jul 20, 2015

I did a rebase, simplified bytestring(cmdbuf.data) and made the newline exception active only for cmd and not for powershell.

In addition I did a separate commit that combines the print_shell_word functions, makes the print_shell_escaped functions use the same body and also combines the different shell_escape functions.

@quinnj
Copy link
Member

quinnj commented May 9, 2016

I still think this would be good functionality. Obviously needs a rebase, but are there any objections to the functionality or implementation here? I'd be willing to rebase if @ncnc is no longer interested (obviously this kind of fell by the way-side....)

@tkelman
Copy link
Contributor

tkelman commented May 9, 2016

cmd is not worth this amount of complexity.

@StefanKarpinski
Copy link
Member

This would be better as an external REPL mode extension.

@musm
Copy link
Contributor

musm commented Dec 11, 2020

Closing:

verbatim passthrough to libuv is available already available. The cmd escaping rules have also been merged to master. The only remaining task is allowing CMD as a shell, which will require a seperate PR since this is too stale.

@musm musm closed this Dec 11, 2020
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.

7 participants