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

@edit on Windows using default editor #20364

Closed
GregPlowman opened this issue Feb 1, 2017 · 13 comments
Closed

@edit on Windows using default editor #20364

GregPlowman opened this issue Feb 1, 2017 · 13 comments
Labels
system:windows Affects only Windows

Comments

@GregPlowman
Copy link
Contributor

Issue discussed on Discourse https://discourse.julialang.org/t/edit-on-windows/1695

As an example @edit cmp(1,2) uses

cmd = `cmd /c start /b $path`

which runs the command:

cmd /c start /b 'C:\Program Files\Julia-0.5.0\bin\../share\julia\base\operators.jl'

I think there are two issues here for the default on Windows:

  1. The space in file path: start tries run C:\Program. I tried many variations of extra quoting with single and double quotes but couldn't get it to work.
  2. The forward slash in path (../share)

A workaround is:

cmd = `cmd /c start /b /D $(realpath(dirname(path))) $(basename(path))`

which runs the command:

cmd /c start /b /D 'C:\Program Files\Julia-0.5.0\share\julia\base' operators.jl

For some reason the space is OK in directory /D, realpath removes the forward slash.

Regarding the forward slash, comment from @nalimilan:

This path indeed comes from base/build_h.jl and is computed at compile time by the contrib/relative_path.sh script. I'm not sure what's the best solution to fix this: either improve the script to detect Windows or fix the path later from Julia. Could you file an issue on GitHub to discuss the best approach?

@ararslan ararslan added the system:windows Affects only Windows label Feb 1, 2017
@ararslan ararslan changed the title `@edit on Windows using default editor @edit on Windows using default editor Feb 1, 2017
@stevengj
Copy link
Member

stevengj commented Feb 3, 2017

The spaces issue is an annoyance of Windows: the Win32 API only allows you to pass a single string for the command-line arguments (rather than passing one string per argument as in Unixy operating systems), and you have to guess at how the program breaks this strings into tokens.

See this blog entry, for example.

In particular, if the start program is parsing its command-line in a nonstandard way, we could maybe use the windows_verbatim option to Cmd (#13780) so that we can adjust the string "manually" according to whatever start needs.

@stevengj
Copy link
Member

stevengj commented Feb 3, 2017

This stackoverflow thread says it is a known bug in the start command and gives some workarounds.

@stevengj
Copy link
Member

stevengj commented Feb 3, 2017

According to stackoverflow, cmd = `cmd /c start /b "" $path` should work?

@GregPlowman
Copy link
Contributor Author

According to stackoverflow, cmd = cmd /c start /b "" $path should work?

Yes this works for me (Windows 10, I can test on Window 7 later).
It works when path contains spaces and forward slashes.

@stevengj
Copy link
Member

stevengj commented Feb 4, 2017

Great! Could you submit a PR?

@stevengj
Copy link
Member

stevengj commented Feb 4, 2017

Although according to the blog article linked above, we are supposed to put ^ before " and other special characters when passing quoted strings to cmd.exe.

@vtjnash
Copy link
Member

vtjnash commented Feb 4, 2017

dup #7107.

at some level, it's probably easier just to write a non-buggy start.exe program than deal with cmd's incomplete quoting rules; it's just a thin wrapper for calling ShellExecuteEx https://msdn.microsoft.com/en-us/library/windows/desktop/bb762154(v=vs.85).aspx from the command line.

@GregPlowman
Copy link
Contributor Author

According to stackoverflow, cmd = cmd /c start /b "" $path should work?

It also works on Windows 7.

The following also works, but note that realpath() is required to convert forward slashes to back slashes.

cmd = `explorer $(realpath(path))`

Great! Could you submit a PR?

Sure, I can create a PR with cmd = `cmd /c start /b "" $path`
(since it doesn't have the issue with forward slashes, and we don't yet have a "non-buggy start.exe")

  1. If so, there is a related PR for adding Notepad++ support to @edit Add Notepad++ support for @edit #20416
    Should I add it to that PR or create a new one?

  2. Do we need a separate issue regarding backslashes? Could this be a problem elsewhere?

This path indeed comes from base/build_h.jl and is computed at compile time by the contrib/relative_path.sh script. I'm not sure what's the best solution to fix this: either improve the script to detect Windows or fix the path later from Julia. Could you file an issue on GitHub to discuss the best approach?

@StefanKarpinski
Copy link
Member

This is why we avoid going through a shell at all on UNIX systems – it's nothing but pain and suffering for no gain whatsoever. I think what @vtjnash suggested is a good path forward. @vtjnash, any chance you're willing to do that?

@vtjnash
Copy link
Member

vtjnash commented Feb 6, 2017

I only have a finite amount of time to deal with adding enough hacks to get Windows to be almost feature-complete with Unix. However, since we already have a getopt implementation at https://github.com/JuliaLang/julia/blob/30bf89f3d8e564b588b8e48993e92a551b384f2c/src/getopt.h, any sufficiently motivated developer should be able to make a new repo and start populating it with better implementations of the basic command line tools (starting with a start.exe replacement).

@StefanKarpinski
Copy link
Member

I only have a finite amount of time to deal with adding enough hacks to get Windows to be almost feature-complete with Unix.

LOL

@stevengj
Copy link
Member

stevengj commented Feb 6, 2017

I just tried:

ShellExecute(op, file) =
    ccall((:ShellExecuteW,"shell32"), Int, (Ptr{Void}, Cwstring, Cwstring, Ptr{Void}, Ptr{Void}, Cint),
          C_NULL, op, file, C_NULL, C_NULL, 10)

and ShellExecute("open", filename) seems to work.

@vtjnash
Copy link
Member

vtjnash commented Feb 6, 2017

cmd = explorer $(realpath(path))

I just saw this. Let's do this instead of cmd start. I forgot that explorer is just a thin wrapper around ShellExecuteEx, but it seems like this should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system:windows Affects only Windows
Projects
None yet
Development

No branches or pull requests

5 participants