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

Improve windows shell mode; fix edit() function for windows #7108

Closed
wants to merge 2 commits into from
Closed

Improve windows shell mode; fix edit() function for windows #7108

wants to merge 2 commits into from

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jun 3, 2014

Prepend 'cmd /c' to edit function start command to allow files to be opened properly on windows. Also prepend it to repl_cmd commands to better emulate shell mode for windows. Fixes #7107.

…e opened properly on windows. Also prepend it to repl_cmd commands to better emulate shell mode for windows. Fixes #7107.
@Keno
Copy link
Member

Keno commented Jun 3, 2014

Do we need some kind of quoting here. What happens when you do e.g. cmd /C ls /?

@Keno
Copy link
Member

Keno commented Jun 3, 2014

Seems to work. LGTM then.

…sures windows commands are handled by spawn(`cmd /c $editor $file`).
@vtjnash
Copy link
Member

vtjnash commented Jun 4, 2014

start can't handle paths with spaces in the name, such as "C:\Program Files" (it handling of escape sequences is very bady flawed), we should probably just replace this will a call to the windows API function which does the same thing

cmd also has awful handling of spaces and quotes (it may expand escape sequences multiple times), so I would rather not invoke it

If /C or /K is specified, then the remainder of the command line after
the switch is processed as a command line, where the following logic is
used to process quote (") characters:

    1.  If all of the following conditions are met, then quote characters
        on the command line are preserved:

        - no /S switch
        - exactly two quote characters
        - no special characters between the two quote characters,
          where special is one of: &<>()@^|
        - there are one or more whitespace characters between the
          two quote characters
        - the string between the two quote characters is the name
          of an executable file.

    2.  Otherwise, old behavior is to see if the first character is
        a quote character and if so, strip the leading character and
        remove the last quote character on the command line, preserving
        any text after the last quote character.
Note that multiple commands separated by the command separator '&&'
are accepted for string if surrounded by quotes.

@quinnj
Copy link
Member Author

quinnj commented Jun 4, 2014

Thanks for taking a look @vtjnash. Should we change the default JULIA_EDITOR for windows then? Since it's set to start? I think the edit function should be ok using cmd /c, since it's well-structured, but for shelling out, do you have other suggestions? It seems to work fairly well for most simple commands/opening files and such.

@vtjnash
Copy link
Member

vtjnash commented Jun 4, 2014

for very limiting shelling out, it might be OK to call cmd, but be aware that anything complicated (like paths with spaces in them) will probably be parsed wrong sometimes. the rules cmd uses for text transformations aren't particularly sane (and are quite different from the rules used by the windows function for parsing arguments -- which are also the rules used by libuv to quote the args). to make this change, you would first need to write a function to correctly quote args for cmd, then set the flag VERBATIM flag when calling libuv's uv_spawn function.

@quinnj
Copy link
Member Author

quinnj commented Jun 4, 2014

Hmmm....I think we'll be best off to use cmd for the shell mode, at least for now. Take the case with spaces in folder names, I would try to open a file:

shell> C:/Users/karbarcca/Google Drive/Dropbox/Dropbox/keys.docx
'C:/Users/karbarcca/Google' is not recognized as an internal or external command
,
operable program or batch file.

Then I'd realize there are spaces, so I can just run:

shell> 'C:/Users/karbarcca/Google Drive/Dropbox/Dropbox/keys.docx'

Which works, and which is what I have run in the terminal anyway to open files, so I'm familiar with it.

@vtjnash
Copy link
Member

vtjnash commented Jun 5, 2014

No, using cmd will start to break down quickly, since libuv does quote the args so that programs that use the windows API for parsing command lines get what they expect. Cmd uses different rules, and the combination of the two is just a mess

@quinnj
Copy link
Member Author

quinnj commented Jun 5, 2014

What would you suggest then?

@vtjnash
Copy link
Member

vtjnash commented Jun 5, 2014

As I mentioned in my post yesterday, you need to implement rules for quoting calls to cmd and set the flag to disable quoting by libuv

@quinnj
Copy link
Member Author

quinnj commented Jun 6, 2014

Ok, I'll play around with it some more today.

@ncnc
Copy link
Contributor

ncnc commented Aug 26, 2014

I played around a bit with this. It seems like CMD /S /C "..." leaves any quoting within the quotes (the ... part) intact, which would make it easy e.g. to use a command with spaces in combination with an argument with spaces. Any other requirements for a sane handling of shell commands?

Also, how would one go about passing the VERBATIM flag to libuv? Is it necessary to add another input argument to jl_spawn?

@vtjnash
Copy link
Member

vtjnash commented Aug 26, 2014

yes, you would need to add another argument to spawn, and the corresponding flags in the Cmd object.

Note that rather than using cmd to invoke the shell (start, aka explorer.exe), you can also just invoke the shell API directly:
http://msdn.microsoft.com/en-us/library/windows/desktop/bb762154(v=vs.85).aspx

@jiahao jiahao force-pushed the master branch 3 times, most recently from 6c7c7e3 to 1a4c02f Compare October 11, 2014 22:06
@tkelman
Copy link
Contributor

tkelman commented Oct 21, 2014

Mostly superseded by #8725, yeah?

@dhoegh
Copy link
Contributor

dhoegh commented Jan 31, 2015

I think this one could be closed due to #8725 and #8838. #8838 fixes the issue with shell>C:/Users/karbarcca/Google Drive/Dropbox/Dropbox/keys.docx which today will look like C:\\Users\\karbarcca\\Google\ Drive\\Dropbox\\Dropbox\\keys.docx which will call the correct file.

@ivarne ivarne closed this Feb 2, 2015
ncnc added a commit to ncnc/julia that referenced this pull request May 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

start is a cmd builtin, doesn't work for edit
7 participants