Closed KronicDeth closed 7 years ago
If someone with more Windows knowledge could take a look at this, it would be really appreciated! Thank you! /cc @OnorioCatenacci @overminddl1 @robi-wan
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. :-)
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
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.
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.
erl.exe
?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?"This only really works if people don't go putting it on a path with spaces and the installer allows directory selection."
Good points both Luke. Most installers do allow directory selection and we could change the elixir installer to default to /usr/bin/elixir (rather than "program files (x86)" Most people won't bother to change the default. But then we'd probably get a ton of bug reports about installing in a non-standard directory on Windows.
"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?"
Moving to a compiled exe is an interesting thought Luke. I had been trying to use Powershell for the Distillery stuff (because PS is a lot more intelligent about a lot of things than CMD is); I mean to say that it seems as if PS might be another good option. A compiled exe would certainly be another good approach. Cmd batch files are quite primitive and error prone and combined with the need to be able to quote the command line correctly, they're a real pain in the backside.
On Mon, Aug 14, 2017 at 9:09 AM, Luke Imhoff notifications@github.com wrote:
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.
- Could we fix just this spacing bug and put the arguments that get broken up back as one argument before calling erl.exe?
- 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?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/elixir/issues/6455#issuecomment-322185382, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJXFXRlqulOWJCzBQIyE6IfKiYl0iIcks5sYEcSgaJpZM4O1iwP .
-- Onorio Catenacci
Do remember, powershell scripts execution is disabled by default on many Windows installs for security reasons.
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 https://github.com/elixir-lang/elixir/commit/8ece0004892697f236ff1021937dbd28e78145ea.
Changing %~1
to %1
helps in this case but I suspect it will break some other use cases.
This needs more investigation.
I guess it's just a bit of bias on my part but I've never cared for the cmd script solution. I've tried to find a way to do this with powershell (as @overminddl1 points out that's not a great solution since lots of people have powershell script execution disabled by default on their windows boxes). I've tried to figure out a way to do this with visual BASIC script; yes, that's still a thing that works on most Windows boxes.
I'd propose this (and let me know if you want me to post this on the Elixir Core group Jose); I'd propose we rewrite our shell scripts in C#. While I would much rather use F# for .Net coding, the "executables" we can get from C# should run on pretty much any Windows box around since most of them should have the .Net runtime installed already. F# would require adding some additional assemblies and since I don't want to add unneeded complication, I'd avoid this.
Rewriting in C# wouldn't necessarily address the issue of paths with spaces but it would (I hope) give us better tooling to deal with it.
Please let me know what you think.
On Mon, Aug 21, 2017 at 4:57 PM, robi-wan notifications@github.com wrote:
Quick update:
The error seems to be caused by this line https://github.com/elixir-lang/elixir/blob/master/bin/elixir.bat#L91 (note the expansion of the first script parameter %~1:
if """"=="%par:--erl=%" (set beforeExtra=%beforeExtra% %~1 && shift)
The %~1 means https://ss64.com/nt/syntax-args.html:
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 https://github.com/elixir-lang/elixir/commit/8ece0004892697f236ff1021937dbd28e78145ea .
Changing %~1 to %1 helps in this case but I suspect it will break some other use cases.
This needs more investigation.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/elixir/issues/6455#issuecomment-323850949, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJXFVTDtTGsqedDvsi3ZP9-g1URplO_ks5sae8igaJpZM4O1iwP .
-- Onorio Catenacci
.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).
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.
On Mon, Aug 21, 2017, 6:28 PM OvermindDL1 notifications@github.com wrote:
.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).
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/elixir/issues/6455#issuecomment-323869105, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJXFQhYKrk-kVDstATVUog8zT8EdApbks5sagSMgaJpZM4O1iwP .
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. ^.^
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?
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']]}}
I agree with you Robi-Wan but this seems like a band-aid on a deep laceration. The issue of spaces in the directory names not only impacts these bat files it also affected EXRM and it's affecting Distillery too. I wish I had a better answer for the issue but since moving our preferred install directory to a directory that doesn't contain spaces seems to be a non-starter, I can't think of anything better. I wish I could talk to whomever it was at MS that made the decision back in Win95 days to make the standard home directory "Program Files"; I'd love to hear their justification for such a myopic decision.
Yes it's as simple as making sure the strings are quoted correctly. Still strikes me as hacky as it can be though.
Sorry I'm just ranting because I've struggled with this spaces in the directory name several times and it's been the source of lots of headaches (and not just in Elixir either).
On Mon, Sep 18, 2017 at 5:39 PM, robi-wan notifications@github.com wrote:
As I mentioned earlier https://github.com/elixir-lang/elixir/issues/6455#issuecomment-323850949 : 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 https://github.com/kronicdeth's original use case. Are there any other known CLI calls which I can test?
Any thoughts or recommendations?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/elixir/issues/6455#issuecomment-330364153, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJXFZ5u0PjfVPMBxZruOyHvZk-Mm2fjks5sjuMbgaJpZM4O1iwP .
-- Onorio Catenacci
@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.
Yes, you're right, of course José. Sorry to indulge myself in a rant :)
On Tue, Sep 19, 2017 at 8:16 AM, José Valim notifications@github.com wrote:
@OnorioCatenacci https://github.com/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.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/elixir/issues/6455#issuecomment-330520405, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJXFY-RQNdSCiKvtZg8yrhRl0Gyetqeks5sj7CDgaJpZM4O1iwP .
-- Onorio Catenacci
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""
;;
Beautiful, thank you @robi-wan! Can you please send a PR with 2 then? :heart:
I'll send a PR (no later than the end of the week).
Environment
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 invokemix
the long way, as an argument toelixir
. JetBrains API has support for automatic, OS-dependent quoting, so it ends up with a commandline with theelixir.bat
andmix
paths quoted because they have spaces from being under"C:\Program Files(x86)\"
, butelixir.bat
(or something in it) misreads the spaces as unquoted:-- https://github.com/KronicDeth/intellij-elixir/issues/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 themix test
(ormix test_with_formatter
for Elixir < 1.4) to run by this weird quoting:I don't really understand Windows quoting rules and how
shift
and%*
works in.bat
files, so maybe this is expected behavior for.bat
s.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 whatelixir.bat
is doing and invokeerl.exe
directly (when on Windows):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.