Skip to content

elixir.bat can't use quoted mix path as argument #6455

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.

Sign up for GitHub

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

Jump to bottom
Closed
KronicDeth opened this issue Aug 12, 2017 · 20 comments
Closed

elixir.bat can't use quoted mix path as argument #6455

KronicDeth opened this issue Aug 12, 2017 · 20 comments
Labels
App:Elixir (stdlib) Level:Advanced Note:Windows

Comments

@KronicDeth
Copy link
Contributor

KronicDeth commented Aug 12, 2017 •
edited
Loading

Environment

  • Elixir & Erlang versions (elixir --version):
    • Elixir 1.5.1
    • Erlang 20
  • Operating system: Windows 10

Current behavior

IntelliJ Elixir needs to require files from a directory it setups for a formatter that outputs in the TeamCity format used by JetBrains IDEs, this means when using mix test it needs to invoke mix the long way, as an argument to elixir. JetBrains API has support for automatic, OS-dependent quoting, so it ends up with a commandline with the elixir.bat and mix paths quoted because they have spaces from being under "C:\Program Files(x86)\", but elixir.bat (or something in it) misreads the spaces as unquoted:

"C:\Program Files (x86)\Elixir\bin\elixir.bat"
-r C:\Users\...\AppData\Local\Temp\intellij_elixir_modules2\team_city_ex_unit_formatter.ex
-r C:\Users\...\AppData\Local\Temp\intellij_elixir_modules2\test_with_formatter.ex
"C:\Program Files (x86)\Elixir\bin\mix" test_with_formatter
--formatter TeamCityExUnitFormatter
Testing started at 4:05 AM ...
\Elixir\bin\mix was unexpected at this time.

-- KronicDeth/intellij-elixir#603

Expected behavior

The mix script passed to elixir would be run and mix test output would show up.

Work-arounds

In cmd.exe

If I do the quoting manually in cmd.exe, I can get the mix test (or mix test_with_formatter for Elixir < 1.4) to run by this weird quoting:

"C:\Program Files (x86)\Elixir\bin\elixir.bat"
-r C:\Users\...\AppData\Local\Temp\intellij_elixir_modules2\team_city_ex_unit_formatter.ex
-r C:\Users\...\AppData\Local\Temp\intellij_elixir_modules2\test_with_formatter.ex C:\Program" Files (x86)"\Elixir\bin\mix test_with_formatter --formatter TeamCityExUnitFormatter
Works Mix Path Quoting Makes Sense
No "C:\Program Files (x86)\Elixir\bin\mix" Yes
Yes C:\Program" Files (x86)"\Elixir\bin\mix No

I don't really understand Windows quoting rules and how shift and %* works in .bat files, so maybe this is expected behavior for .bats.

In IntelliJ Elixir

I can't do the weird C:\Program" Files (x86)"\Elixir\bin\mix quoting using the JetBrains APIs, because it always wants to escape quotes and add outer quotes, so my current work-around is to copy what elixir.bat is doing and invoke erl.exe directly (when on Windows):

screen shot 2017-08-12 at 6 02 51 pm

erl.exe -pa "C:\Program Files (x86)\Elixir\lib\eex\ebin"
-pa "C:\Program Files (x86)\Elixir\lib\elixir\ebin"
-pa "C:\Program Files (x86)\Elixir\lib\ex_unit\ebin"
-pa "C:\Program Files (x86)\Elixir\lib\iex\ebin"
-pa "C:\Program Files (x86)\Elixir\lib\logger\ebin"
-pa "C:\Program Files (x86)\Elixir\lib\mix\ebin"
-noshell 
-s elixir start_cli 
-extra 
-r C:\Users\User\AppData\Local\Temp\intellij_elixir_modules\team_city_ex_unit_formatting.ex 
-r C:\Users\User\AppData\Local\Temp\intellij_elixir_modules\1.4.0\team_city_ex_unit_formatter.ex
"C:\Program Files (x86)\Elixir\bin\mix" test
--formatter TeamCityExUnitFormatter
C:/Users/User/Documents/GitHub/alembic/test

It is partly from the outer quotes working in this work-around, that I think the argument processing in elixir.bat itself has a bug.
@KronicDeth KronicDeth mentioned this issue Aug 12, 2017
Closed
KronicDeth added a commit to KronicDeth/intellij-elixir that referenced this issue Aug 12, 2017
@KronicDeth
Expand elixir.bat to erl.exe with arguments to work-around mix quotes
b58ca3e 
Fixes #603
See also elixir-lang/elixir#6455.

`elixir.bat` in at least Elixir 1.5.1, will not properly parse a mix
path with spaces in it on Windows even when the path has outer quotes.
It is not possible to use inner quotes, as you can do in cmd.exe, using
the JetBrains and Java libraries hat auto-matically quote, so instead
bypass the bad quoting in `elixir.bat` by doing what `elixir.bat` does:
call `erl.exe` with all the SDK ebin paths added with `-pa`, then
`-noshell -s elixir start_cli` to run Elixir and -extra to run requires
(`-r`), mix and it's task.
@KronicDeth KronicDeth mentioned this issue Aug 12, 2017
Merged
@josevalim
Copy link
Member

If someone with more Windows knowledge could take a look at this, it would be really appreciated! Thank you! /cc @OnorioCatenacci @OvermindDL1 @robi-wan

@OvermindDL1
Copy link
Contributor

I don't really understand Windows quoting rules and how shift and %* works in .bat files, so maybe this is expected behavior for .bats.

This is, properly quoting in windows 'bat' files is horribly painful (a lot of 'simple' stuff in bat files is horribly painful).

Perhaps look at https://stackoverflow.com/questions/12551842/cmd-nested-double-quotes-in-argument as I'm not at work at the moment. If that does not help then ping me again during the week and I'll give some things a try. :-)

@robi-wan
Copy link
Contributor

I can't look at it now (currently on vacation) - maybe next weekend.
In addition to @OvermindDL1's Link:
http://www.robvanderwoude.com/battech_inputvalidation_commandline.php#CommandLine
(Parsing arguments is hard)

Maybe the braces ('(', ')') are also involved: http://www.robvanderwoude.com/escapechars.php

@OnorioCatenacci
Copy link

Hi all,

I don't know if this is a potential workaround for you Luke but if you put the quotes right around the forward slashes e. g. C:"Program Files (x86)"\ . . . that should also work.

Another option is to generate the 8.3 equivalent of the directory path and use that. Look at my pragmatic package (https://hex.pm/packages/pragmatic) to see how I did it. I just recently learned that the 8.3 translation isn't always available though so I'm not sure if that's a great work around either.

I know it's unlikely to fly but we could solve a lot of issues by installing everything into /usr/bin. That's what I do on my machine, MS standards not withstanding. I mean my erlang is in c:\usr\bin\erl9.0 and my elixir is in c:\usr\bin\elixir. As I say I know what the MS standards are in regards to installation but whomever created them had no notion of shell scripting. We can recommend installing Elixir into \usr\bin if we want and we could even make that the default install path--Microsoft might complain that it's non-standard but it does work.

I've fought with the quoting in Windows for paths a few times trying to help Paul get exrm/distillery working right on Windows. It's a huge pain in the backside.

@KronicDeth
Copy link
Contributor Author

I don't know if this is a potential workaround for you Luke but if you put the quotes right around the forward slashes e. g. C:"Program Files (x86)"\ . . . that should also work.

Maybe? I can subclass com.intellij.execution.configuration.GeneralCommandLine and override #prepareCommandLine, but the code I'd be going around is fairly complex to replicate. It's less error prone to call erl.exe directly as it consumes outer quotes fine.

Another option is to generate the 8.3 equivalent of the directory path and use that. Look at my pragmatic package (https://hex.pm/packages/pragmatic) to see how I did it. I just recently learned that the 8.3 translation isn't always available though so I'm not sure if that's a great work around either.

I knew 8.3s could be turned off, which is why I gave it as a work-around to my users, but wasn't trying to use it as a fix in IntelliJ Elixir's code itself.

installing everything into /usr/bin

This only really works if people don't go putting it on a path with spaces and the installer allows directory selection.

  1. Could we fix just this spacing bug and put the arguments that get broken up back as one argument before calling erl.exe?
  2. erl.exe seems to consume outer quote arguments fine, could we move to a compiled executable instead of a batch file, so we don't need to depend on batch file argument consumption?

@OnorioCatenacci
Copy link

OnorioCatenacci commented Aug 14, 2017 via email

@OvermindDL1
Copy link
Contributor

Do remember, powershell scripts execution is disabled by default on many Windows installs for security reasons.

@robi-wan
Copy link
Contributor

Quick update:

The error seems to be caused by this line (note the expansion of the first script parameter %~1:

if """"=="%par:--erl=%"                 (set beforeExtra=%beforeExtra% %~1 && shift)

The %~1 means:

Expand %1 removing any surrounding quotes (")

The cmd.exe evaluates this line to something like this:

if """" == ""-r"" (set beforeExtra= d:\home\docs\dev\git_repos\elixir (x86)\bin\mix   && shift)

(My minimal call to reproduce this is "d:\home\docs\dev\git_repos\elixir (x86)\bin\elixir.bat" -r issue_6455\exunit\team_city_ex_unit_formatting.ex "d:\home\docs\dev\git_repos\elixir (x86)\bin\mix".)

This outputs:

"\bin\mix" kann syntaktisch an dieser Stelle nicht verarbeitet werden.

That should mean the same as

\Elixir\bin\mix was unexpected at this time.

The expansion for the batch file parameter for the option --erl was there since it was introduced in 8ece000.

Changing %~1 to %1 helps in this case but I suspect it will break some other use cases.

This needs more investigation.

@OnorioCatenacci
Copy link

OnorioCatenacci commented Aug 21, 2017 via email

@OvermindDL1
Copy link
Contributor

.NET is not ubiquitous though. If anything you could just make an escript or a C/C++ shim. But definitely stay away from .NET (it's like asking for java, sure 'most' may have it, but not everyone, and that is a crazy-heavy dependency to include just for a stupid little script).

@OnorioCatenacci
Copy link

OnorioCatenacci commented Aug 21, 2017 via email

@OvermindDL1
Copy link
Contributor

I could hack together something in Rust then. If I were to build something
in C/C++ I would just as soon build it in Rust. Safer. But I thought the
CLR would be pretty much everywhere on Windows beyond a certain version. I
say that because I thought the CLR was pretty much standard in IE.

Rust would be fine, can just include the precompiled native file (with the source elsewhere). I'd personally opt for an escript if possible and lightweight enough first (it might not be), but C/C++ compilers is generally (cough) what someone building Erlang already has, hence why it is useful to have as the base file since the main dev's here may not have Rust installed. ^.^

@robi-wan
Copy link
Contributor

As I mentioned earlier:
the issue is caused by the removal of surrounding quotes of the argument (%1).

Solution 1
Do not remove the surrounding quotes when parsing --erl arguments to the
elixir.bat. This will be achieved by removing the ~.

diff --git a/bin/elixir.bat b/bin/elixir.bat
index 89c7d2c47..c67abe94c 100644
--- a/bin/elixir.bat
+++ b/bin/elixir.bat
@@ -88,7 +88,7 @@ if """"=="%par:--sname=%"               (set parsErlang=%parsErlang% -sname %1 &
 if """"=="%par:--name=%"                (set parsErlang=%parsErlang% -name %1 && shift)
 if """"=="%par:--logger-otp-reports=%"  (set parsErlang=%parsErlang% -logger handle_otp_reports %1 && shift)
 if """"=="%par:--logger-sasl-reports=%" (set parsErlang=%parsErlang% -logger handle_sasl_reports %1 && shift)
-if """"=="%par:--erl=%"                 (set beforeExtra=%beforeExtra% %~1 && shift)
+if """"=="%par:--erl=%"                 (set beforeExtra=%beforeExtra% %1 && shift)
 goto:startloop

 rem ******* assume all pre-params are parsed ********************

As this feature is there since it was introduced I cannot predict the
consequences of this change.

Solution 2
Add surrounding quotes to the set call. This compensates the removal of the
surrounding quotes of the argument %1 and let the cmd.exe handle paths with
spaces.

diff --git a/bin/elixir.bat b/bin/elixir.bat
index 89c7d2c47..6d7e00e47 100644
--- a/bin/elixir.bat
+++ b/bin/elixir.bat
@@ -88,7 +88,7 @@ if """"=="%par:--sname=%"               (set parsErlang=%parsErlang% -sname %1 &
 if """"=="%par:--name=%"                (set parsErlang=%parsErlang% -name %1 && shift)
 if """"=="%par:--logger-otp-reports=%"  (set parsErlang=%parsErlang% -logger handle_otp_reports %1 && shift)
 if """"=="%par:--logger-sasl-reports=%" (set parsErlang=%parsErlang% -logger handle_sasl_reports %1 && shift)
-if """"=="%par:--erl=%"                 (set beforeExtra=%beforeExtra% %~1 && shift)
+if """"=="%par:--erl=%"                 (set "beforeExtra=%beforeExtra% %~1" && shift)
 goto:startloop

 rem ******* assume all pre-params are parsed ********************

I tested these changes only with @KronicDeth's original use case. Are there
any other known CLI calls which I can test?

Any thoughts or recommendations?

@josevalim
Copy link
Member

Thanks for looking into this @robi-wan!

I blamed the code and apparently it has always been written like that so I think it should be fine to test solution number 1.

Some good tests to check the options are still getting downstream would be this:

$ elixir --erl "-foo bar" -e "IO.inspect :init.get_argument(:foo)"
{:ok, [['bar']]}

$ elixir --erl "-foo bar" --sname baz -e "IO.inspect {:init.get_argument(:foo), :init.get_argument(:sname)}"
{{:ok, [['bar']]}, {:ok, [['baz']]}}

$ elixir --sname baz --erl "-foo bar" -e "IO.inspect {:init.get_argument(:foo), :init.get_argument(:sname)}"
{{:ok, [['bar']]}, {:ok, [['baz']]}}

@OnorioCatenacci
Copy link

OnorioCatenacci commented Sep 19, 2017 via email

@josevalim
Copy link
Member

@OnorioCatenacci this is also extremely frustrating on Unix systems too. But at the end of the day we have a bug which needs to be addressed in the short term, even if it would be better to completely revisit this long term.

@OnorioCatenacci
Copy link

OnorioCatenacci commented Sep 19, 2017 via email

@robi-wan
Copy link
Contributor

Output with solution 1:

D:\home\docs\dev\git_repos\elixir (x86)
$ "d:\home\docs\dev\git_repos\elixir (x86)\bin\elixir.bat" --erl "-foo bar" -e "IO.inspect :init.get_argument(:foo)"
:error

D:\home\docs\dev\git_repos\elixir (x86)
$ "d:\home\docs\dev\git_repos\elixir (x86)\bin\elixir.bat" --erl "-foo bar" --sname baz -e "IO.inspect {:init.get_argument(:foo), :init.get_argument(:sname)}"
{:error, {:ok, [['baz']]}}

D:\home\docs\dev\git_repos\elixir (x86)
$ "d:\home\docs\dev\git_repos\elixir (x86)\bin\elixir.bat" --sname baz --erl "-foo bar" -e "IO.inspect {:init.get_argument(:foo), :init.get_argument(:sname)}"
{:error, {:ok, [['baz']]}}

For the call "d:\home\docs\dev\git_repos\elixir (x86)\bin\elixir.bat" --erl "-foo bar" -e "IO.inspect :init.get_argument(:foo)" elixir.bat computes and executes a call which looks like this (formatted for better readability):

erl.exe  -pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\eex\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\elixir\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\ex_unit\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\iex\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\logger\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\mix\ebin"   
-noshell -s elixir start_cli  "-foo bar"  -extra --erl "-foo bar" -e "IO.inspect :init.get_argument(:foo)"

The interesting part are the arguments after start_cli: these arguments are (after the change) surrounded with quotes - I think this is not valid/expected syntax for an erlang user flag.

With solution 2 the executed call looks like this (again: formatted for better readability):

erl.exe  -pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\eex\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\elixir\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\ex_unit\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\iex\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\logger\ebin" 
-pa "d:\home\docs\dev\git_repos\elixir (x86)\lib\mix\ebin"   
-noshell -s elixir start_cli  -foo bar -extra --erl "-foo bar" -e "IO.inspect :init.get_argument(:foo)"

This time (with solution 2) the arguments after start_cli are not surrounded with quotes - the call completes successfully.

D:\home\docs\dev\git_repos\elixir (x86)
$ "d:\home\docs\dev\git_repos\elixir (x86)\bin\elixir.bat" --erl "-foo bar" -e "IO.inspect :init.get_argument(:foo)"
{:ok, [['bar']]}

D:\home\docs\dev\git_repos\elixir (x86)
$ "d:\home\docs\dev\git_repos\elixir (x86)\bin\elixir.bat" --erl "-foo bar" --sname baz -e "IO.inspect {:init.get_argument(:foo), :init.get_argument(:sname)}"
{{:ok, [['bar']]}, {:ok, [['baz']]}}

D:\home\docs\dev\git_repos\elixir (x86)
$ "d:\home\docs\dev\git_repos\elixir (x86)\bin\elixir.bat" --sname baz --erl "-foo bar" -e "IO.inspect {:init.get_argument(:foo), :init.get_argument(:sname)}"
{{:ok, [['bar']]}, {:ok, [['baz']]}}

BTW: The correspondig part of bin/elixir is this:

    --erl)
        I=$(expr $I + 1)
        eval "VAL=\${$I}"
        ERL="$ERL "$VAL""
        ;;

@josevalim
Copy link
Member

Beautiful, thank you @robi-wan! Can you please send a PR with 2 then? ❤️

@robi-wan
Copy link
Contributor

I'll send a PR (no later than the end of the week).

robi-wan added a commit to robi-wan/elixir that referenced this issue Sep 23, 2017
@robi-wan
Support path with spaces as argument to elixir.bat
e6db85d 
Erlang parameters (CLI switch '--erl') are used with possible quotes around the argument removed ('%~1').
This causes an error if one arbitrary argument to elixir.bat represents a quoted path with spaces in it:
"[part of path after space] was unexpected at this time."

Enclose the complete expression with quotes to work around this (the quotes are not added to the content).

This behavior is not documented in the [Windows XP Command Line Reference for SET](https://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-us/set.mspx)
but there are other places which document the [SET command](https://ss64.com/nt/set.html).

Fix for elixir-lang#6455
@robi-wan robi-wan mentioned this issue Sep 23, 2017
Merged
josevalim pushed a commit that referenced this issue Sep 24, 2017
@robi-wan @josevalim
Support path with spaces as argument to elixir.bat (#6596)
9386542 
Erlang parameters (CLI switch '--erl') are used with possible quotes around the argument removed ('%~1').
This causes an error if one arbitrary argument to elixir.bat represents a quoted path with spaces in it:
"[part of path after space] was unexpected at this time."

Enclose the complete expression with quotes to work around this (the quotes are not added to the content).

This behavior is not documented in the [Windows XP Command Line Reference for SET](https://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-us/set.mspx)
but there are other places which document the [SET command](https://ss64.com/nt/set.html).

Fix for #6455
@E14 E14 mentioned this issue May 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
App:Elixir (stdlib) Level:Advanced Note:Windows
Milestone
No milestone
Development

No branches or pull requests
5 participants
@josevalim @robi-wan @OnorioCatenacci @KronicDeth @OvermindDL1