Test commands do not need quote escapes on Windows

[fleimgruber]

Another one for Windows compatibility. In essence, on Windows we do not need to escape quotes in the test run command. Test commands started from elpy will fail.

My approach was to let bind the correct function depending on OS and leave the mapping in. For Windows the identity function is mapped.

The Travis CI results show that this does not work on Emacs version 24.3. Any ideas why it is just failing for that version?

Also, I was trying to add a test for this, but found no test-quote-escape-in-test-run or similar in the test suite. Also, I do not quite know how I would go about to test this function. Should we refactor the quote escape mapping to a separate function that just returns the test command string (so we can test it separately)?

[jorgenschaefer]

Thank you for the contribution!

You do not need cl-letf there. (let ((func #'1+)) (mapcar func '(1 2 3))) => (2 3 4) works fine.

I am confused why shell-quote-argument is not required, or does not do the right thing, on Windows. That seems to be a bug in Emacs? Can you give me an example of where it does the wrong thing?

The Travis CI results show that this does not work on Emacs version 24.3. Any ideas why it is just failing for that version?

Apparently, symbol-function on a symbol with no function in the function slot throws an error in Emacs 24.3. I do not know why.

Also, I was trying to add a test for this, but found no test-quote-escape-in-test-run or similar in the test suite. Also, I do not quite know how I would go about to test this function. Should we refactor the quote escape mapping to a separate function that just returns the test command string (so we can test it separately)?

There's elpy-test-run-should-escape-arguments. The requirements here are that "elpy-test-run should escape arguments on Unix" and "elpy-test-run should not escape arguments on Windows". That is, two test cases similar to the old one that set system-type appropriately.

[coveralls]

Coverage remained the same when pulling b213bb6 on fleimgruber:windows_test_quote_escape into 7c2fe62 on jorgenschaefer:master.

[coveralls]

Coverage remained the same when pulling 3a21848 on fleimgruber:windows_test_quote_escape into 7c2fe62 on jorgenschaefer:master.

[fleimgruber]

I am confused why shell-quote-argument is not required, or does not do the right thing, on Windows. That seems to be a bug in Emacs? Can you give me an example of where it does the wrong thing?

The error is

Compilation started at Fri Nov 21 19:03:08

"py.test" "d:/my_project/tests/test_query.py"
'py.test" "d:' is not recognized as an internal or external command, operable program or batch file.

with the patch from this PR it works. But I do not fully understand. I guess you will need to have access to a Windows machine to reproduce.

There's elpy-test-run-should-escape-arguments. The requirements here are that "elpy-test-run should escape arguments on Unix" and "elpy-test-run should not escape arguments on Windows". That is, two test cases similar to the old one that set system-type appropriately.

I added the appropriate test cases elpy-test-run-should-escape-arguments-on-unix elpy-test-run-should-not-escape-arguments-on-windows. Any ideas on how to test / handle all the *nixes that could come back from system-type?

You do not need cl-letf there. (let ((func #'1+)) (mapcar func '(1 2 3))) => (2 3 4) works fine.

I tried that with

  (let ((default-directory working-directory)
        (quote-fun
         (cond ((eq system-type 'windows-nt) #'identity)
               (t  #'shell-quote-argument))))

and with this all 4 tests in elpy-test-run-test.el fail

(void-function quote-fun)

[jorgenschaefer]

The error is

What does M-: (shell-quote-argument "d:/my_project/tests/test_query.py") return for you on Windows?

Any ideas on how to test / handle all the *nixes that could come back from system-type?

No need to, just test the case where it is "not windows-nt" to cover that branch. Your solution is fine. (If one wanted to, one could use dolist to go through all options and call the same code for all of them, but I do not think that's necessary here.)

I tried that with

Do not quote the argument to mapconcat. This works:

(let ((quote-fun (if (eq system-type 'windows-nt)
                     #'identity
                   #'shell-quote-argument)))
  (mapconcat quote-fun
             '("foo" "bar" "baz")
             " "))

[coveralls]

Coverage remained the same when pulling bd9445e on fleimgruber:windows_test_quote_escape into 7c2fe62 on jorgenschaefer:master.

[coveralls]

Coverage remained the same when pulling 33ff82d on fleimgruber:windows_test_quote_escape into 7c2fe62 on jorgenschaefer:master.

[coveralls]

Coverage remained the same when pulling 75d16a4 on fleimgruber:windows_test_quote_escape into 7c2fe62 on jorgenschaefer:master.

[coveralls]

Coverage remained the same when pulling b5c6d88 on fleimgruber:windows_test_quote_escape into 7c2fe62 on jorgenschaefer:master.

[fleimgruber]

No need to, just test the case where it is "not windows-nt" to cover that branch. Your solution is fine. (If one wanted to, one could use dolist to go through all options and call the same code for all of them, but I do not think that's necessary here.)

Ok, "just" covering both branches makes more sense than testing for all values.

Do not quote the argument to mapconcat. This works:

True, my bad.

What does M-: (shell-quote-argument "d:/my_project/tests/test_query.py") return for you on Windows?

This gives

"\"d:/my_project/tests/test_query.py\""

In a Command Window,

D:\my_project\tests> py.test "d:/my_project/tests/test_query.py"

works (and so does py.test d:/my_project/tests/test_query.py) , but

D:\my_project\tests> "py.test" "d:/my_project/tests/test_query.py"

fails (and so does "py.test" d:/my_project/tests/test_query.py) with the error

'"D:\my_project\tests\py.test\..\..\python.exe"' is not recognized as an internal or external command,
operable program or batch file.

similar to the one I initially got from Emacs *compilation* buffer when doing the elpy-test. So it seems that the error is cause by a quoted py.test command. I digged around more and py.test calls a py.test.bat batch script from D:/miniconda3/envs/py33/Scripts/ with the contents

@echo off
set PYFILE=%~f0
set PYFILE=%PYFILE:~0,-4%-script.py
"%~f0\..\..\python.exe" "%PYFILE%" %*

which looks suspicious judging from the above error message. I tested with the py.test.exe from the system Python installation (i.e. not the one from Continuums Anaconda distribution), and the quoted call works with the .exe.

To sum up, the py.test.bat script fails when quoted, but AFAICS this has nothing to do with elpy or Emacs. I should have looked into this before creating this PR... the correct thing would be to file a bug with the Anaconda distribution - or not to use Windows :-)

[jorgenschaefer]

Thank you for looking into this! Could you report the bug with Anaconda?

Shell escape mechanisms with CMD.EXE seem to be even more tricky than with /bin/sh :-|

[coveralls]

Coverage remained the same when pulling d140d8c on fleimgruber:windows_test_quote_escape into 7c2fe62 on jorgenschaefer:master.

[fleimgruber]

I updated to latest conda and py.test. They use an .exe proper now. This means that we are again stuck with the question why

(compile "py.test d:/ecogrid_monitor/tests/test_query.py")

works and

(compile "\"py.test\" \"d:/ecogrid_monitor/tests/test_query.py\"")

fails, but on the other hand (on CMD.EXE)

C:\Users\user>"py.test" "d:/ecogrid_monitor/tests/test_query.py"

works now. This behaviour for Windows is mentioned in the official Emacs documentation[1]. If I read it correctly, I would assume that #'shell-quote-argument is used to escape shell characters with special meaning such as >, *, $ etc.?

How about only escaping the args, not the command? One could argue that this is a valid approach even on *nix systems - correct me if I am wrong. A very crude solution just for demonstration is in d140d8c. I would like to have your opinion before making the implementation better.

The cause of this is still foggy and I think lies somewhere in the way this py.test.exe and Emacs subprocess calls interact with each other. I do not really know who is to blame here. Emacs (compile) with escapes works fine with other commands, py.test works with quotes from CMD.EXE...

[1] http://www.gnu.org/software/emacs/manual/html_node/elisp/Shell-Arguments.html

[jorgenschaefer]

If the command includes a space (e.g. a full path with directories that contain spaces), it needs to be escaped under UNIX.

(The correct solution here would be for compile to not automatically pass arguments through the shell, but I guess we won't get that … :-/)

[fleimgruber]

With d140d8c one gets the space-escaping for a path with spaces, the old "unix" test passes. Am I missing something?

[jorgenschaefer]

With d140d8c one gets the space-escaping for a path with spaces

How?

[fleimgruber]

With d140d8c one gets the space-escaping for a path with spaces

How?

The change is that in elpy-run-test just the argument args is shell-quote-argument'ed, not the command argument. AFAICS, this command will be one of the test runners which are all hard-coded in the elpy-test-*-runner functions. My reasoning was that since the command won't contain spaces it would be possible to just quote the args argument.

Please correct me if I am wrong - and thanks for your patience with an elisp rookie.

[jorgenschaefer]

AFAICS, this command will be one of the test runners which are all hard-coded in the elpy-test-*-runner functions.

Ah. That would require documenting, then, for future test commands (and poses the question why the test commands have to shell-quote the commands and not the arguments).

You are right, though, this is indeed a possible work-around for this problem.

Maybe we can fix the original problem, though. Deep down at the bottom, compile calls start-file-process-shell-command, which runs the arguments using shell-file-name (usually "/bin/sh") and shell-command-switch (usually"-c"), so it ends up as"/bin/sh" "-c" ""`. Which is likely not what is being done on Windows. What are the values of those variables for you?

[fleimgruber]

Maybe we can fix the original problem, though. Deep down at the bottom, compile calls start-file-process-shell-command, which runs the arguments using shell-file-name (usually "/bin/sh") and shell-command-switch (usually"-c"), so it ends up as"/bin/sh" "-c" ""`. Which is likely not what is being done on Windows. What are the values of those variables for you?

shell-file-name is "C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe" and shell-command-switch is "-c". I tested it via

C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe -c "\"py.test\" \"d:/my_project/tests/test_query.py\""

and it fails exactly as from (compile ...) with

'py.test" "d:' is not recognized as an internal or external command,
operable program or batch file.

OTOH,

C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe -c "\"ls\" \"d:/\""

works (where ls is from cygwin).

[jorgenschaefer]

Aha! That sounds like a bug in cmdproxy.exe. Would you report this, or should I?

[fleimgruber]

A last thing I wanted to test was whether py.test is on the PATH of the shell that is spawned by cmdproxy, but it is:

C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe -c "where py.test"

d:\Miniconda3\envs\py33\Scripts\py.test.exe

C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe -c "echo %PATH%"

...;d:\Miniconda3\envs\py33\Scripts;...

If you have an idea on how to formulate the bug report and reproduce, please go ahead :)

[jorgenschaefer]

I'd simply have reported that C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe -c "\"py.test\" \"d:/my_project/tests/test_query.py\"" gives the error you reported, which seems wrong – there is some bogus parsing going on:

'py.test" "d:' is not recognized

This means cmdproxy.exe split the string up at the wrong point. I do not know why. But that's the bug.

(Not having windows makes it tricky for me to report this.)

[fleimgruber]

I am still wondering why

C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe -c "\"ls.exe\" \"d:/my_project/tests/test_query.py\""

works regarding parsing...

I really suspect py.test here, but I will go ahead and report it.

[jorgenschaefer]

The error you provided does not look like py.test is executed at all. Maybe the dot confuses cmdproxy.exe? If you rename ls.exe to some.ls.exe, does that work? :-)

[fleimgruber]

C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe -c "\"some.ls.exe\" \"d:/my_project/tests/test_query\""

'some.ls.exe" "d:' is not recognized as an internal or external command,
operable program or batch file.

:-) Nice one. Ok will go ahead and report.

Update:
The obvious next experiment:

C:/opt/Emacs/libexec/emacs/24.3.92/x86_64-w64-mingw32/cmdproxy.exe -c "\"py.test.exe\" \"d:/my_project/tests/test_query.py\""

works. I think that interactive CMD.EXE appends an .exe to a command it does not know about. I will use a custom elpy-test-pytest-windows-runner in the meantime :)

[jorgenschaefer]

I just noticed that github does not show PRs in the issue list when I check for the latest release. Great. Sorry for neglecting this for so long.

I suspect this does not require merging, right? It's a bug in Emacs?

Do you have a link to the upstream bug report?

[fleimgruber]

I neglected it for this long as well :)
FYI, I put up a bug report today, see http://lists.gnu.org/archive/html/bug-gnu-emacs/2015-02/msg00227.html.
I still need to provide tests for this, i.e. to show that compile fails in this specific case.

[jorgenschaefer]

… ah, the supportive and helpful tone of emacs core development, it catches me off-guard every time :-D

In case it helps:

(defun compile-quoted (command &rest args)
  "Run `compile' with COMMAND and ARGS quoted.                                  

Useful if you can not be sure if there are special characters in                
the command or arguments, like `start-process' allows."
  (mapconcat #'shell-quote-argument
             (cons command args)
             " "))

(compile-quoted "write" "d:/asdf.txt")
(compile-quoted "write.test" "d:/asdf.txt")

Thanks for passing this on upstream.

[fleimgruber]

Thanks for the use case sketch, I adapted it and put it up on the bug report.

[fleimgruber]

FYI, this has been resolved upstream :)
http://lists.gnu.org/archive/html/bug-gnu-emacs/2015-02/msg00420.html

[jorgenschaefer]

Thank you!