Fix pwsh compatibility bug

[shishini]

The command line needs a different formatting for pwsh compared to powershell

More details can be found in this bug reports #3588
In summary, initializing a repl works when installing clojure-cli to Powershell 5.1 (Windows Powershell) but was failing when clojure-cli was installed in pwsh (when (setq cider-clojure-cli-command "pwsh") )

[vemv]

Looks good to me - great work!

If you have it easy, please make the failing CI pass. Othewise we can cherry-pick + amend your commit.

@ikappaki review also welcome.

Thanks - V

[shishini]

Not really sure how to
Do I add a new commit to my branch, will it show up automatically, ?

[vemv]

Yes, commit and push

[bbatsov]

Might be good to add some comment explaining the need for the different escaping rules.

[shishini]

Its important to clarify that this for me, is workaround for a bug that is either in the clojure cli tool or in powershell
clojure cli should behave the same on both shells, since as per powerhsell docs the quoting rules are the same

I created a question on ask clojure , suggesting that this is a bug in the clojure command line tool, since i cannot replicate this behavior with any other command

Are you ok with documenting this as a workaround, or do your prefer another solution, i think the calva vs code plugin in windows skips the cli tool and use the java command directly , so they dont seem to have this issue

they seem to run this command

; Starting Jack-in Terminal: pushd c:\dev\lang\clojure\project\ukg & java -jar ".calva\deps.clj.jar" -Sdeps "{:deps {nrepl/nrepl {:mvn/version,""1.0.0""},cider/cider-nrepl {:mvn/version,""0.28.5""}}}" -M -m nrepl.cmdline --middleware "[cider.nrepl/cider-middleware]" & popd

for me the calva solution, seem a bit heavy handed, as the plugin on vs code, download his own cli jar files and i dont know what else, i like that cider doesnt install java jars and use the system cli tools

[bbatsov]

Documenting this as a workaround is fine - e.g. I just learned what this is about and I imagine others reading the codebase will be in the same boat as me. :-)

[bbatsov]

(I thought powershell and pwsh were exactly the same thing)

[bbatsov]

Don't forget to add a changelog entry about this bugfix.

[ikappaki]

Hi all

I wanted to add some analysis on why we're seeing a different behavior with pwsh. It appears the double quoting rules have changed with pwsh 7.3 as per this stackoverflow post, using double double quotes is not required with that version or newer.

Here I'm using the GNU coreutils echo command to demonstrate the change in behaviour:

powershell.exe treats "" as " while pwsh.exe treats "" as "" and thus the error in pwsh case.

# powershell
> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.22621.2428

PS> C:\local\msys64\usr\bin\echo.exe -Sdeps '{:deps {nrepl/nrepl {:mvn/version ""1.0.0""}}}'
-Sdeps {:deps {nrepl/nrepl {:mvn/version "1.0.0"}}}

# pwsh
> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      7.4.0

PS > C:\local\msys64\usr\bin\echo.exe -Sdeps '{:deps {nrepl/nrepl {:mvn/version ""1.0.0""}}}'
-Sdeps {:deps {nrepl/nrepl {:mvn/version ""1.0.0""}}}

@shishini's hack is good.

There is also a one liner hack that forces pwsh.exe to the old behavior with $PSNativeCommandArgumentPassing = 'Legacy' as mentioned in the stackoverflow post. So it is possible to get away with the following in cider.el, i.e. prepending that at the front of the clojure command execution at

cider/cider.el

Line 813 in 0134a0b

(command (format "clojure %s" quoted-params))

(defun cider--powershell-encode-command (cmd-params)
  "Base64 encode the powershell command and jack-in CMD-PARAMS for clojure-cli."
  (let* ((quoted-params cmd-params)
         (command (format "$PSNativeCommandArgumentPassing = 'Legacy'; clojure %s" quoted-params))
         (utf-16le-command (encode-coding-string command 'utf-16le)))
    (format "-encodedCommand %s" (base64-encode-string utf-16le-command t))))

I've tested this to work with both powershell.exe and pwsh.exe. Perhaps this is a slightly better solution because we won't have to maintain two branches of logic for each powershell executable?

Irrespective of the solution, i've stumbled upon a problem with cider-nrepl and specifically one of its dependencies, orchard, when I try to jack in with only pwsh in the PATH, it complains that it can't find powershell

;;  Startup: "d:/removeme/pwsh/pwsh.exe" -encodedCommand JABQAFMATgBhAHQAaQB2AGUAQwBvAG0AbQBhAG4AZABBAHIAZwB1AG0AZQBuAHQAUABhAHMAcwBpAG4AZwAgAD0AIAAnAEwAZQBnAGEAYwB5ACcAOwAgAGMAbABvAGoAdQByAGUAIAAtAFMAZABlAHAAcwAgACcAewA6AGQAZQBwAHMAIAB7AG4AcgBlAHAAbAAvAG4AcgBlAHAAbAAgAHsAOgBtAHYAbgAvAHYAZQByAHMAaQBvAG4AIAAiACIAMQAuADAALgAwACIAIgB9ACAAYwBpAGQAZQByAC8AYwBpAGQAZQByAC0AbgByAGUAcABsACAAewA6AG0AdgBuAC8AdgBlAHIAcwBpAG8AbgAgACIAIgAwAC4ANAAwAC4AMAAiACIAfQB9ACAAOgBhAGwAaQBhAHMAZQBzACAAewA6AGMAaQBkAGUAcgAvAG4AcgBlAHAAbAAgAHsAOgBtAGEAaQBuAC0AbwBwAHQAcwAgAFsAIgAiAC0AbQAiACIAIAAiACIAbgByAGUAcABsAC4AYwBtAGQAbABpAG4AZQAiACIAIAAiACIALQAtAG0AaQBkAGQAbABlAHcAYQByAGUAIgAiACAAIgAiAFsAYwBpAGQAZQByAC4AbgByAGUAcABsAC8AYwBpAGQAZQByAC0AbQBpAGQAZABsAGUAdwBhAHIAZQBdACIAIgBdAH0AfQB9ACcAIAAtAE0AOgBjAGkAZABlAHIALwBuAHIAZQBwAGwA
ERROR: Unhandled REPL handler exception processing message {:op init-debugger, :nrepl.middleware.print/stream? 1, :nrepl.middleware.print/print cider.nrepl.pprint/pprint, :nrepl.middleware.print/quota 1048576, :nrepl.middleware.print/buffer-size 4096, :nrepl.middleware.print/options {:right-margin 70}, :session a731b0b0-85e4-4c4f-9710-465d0e9671dc, :id 6}
Syntax error macroexpanding at (clojuredocs.clj:19:35).
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3719)
...
	at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.io.IOException: Cannot run program "powershell.exe": CreateProcess error=2, The system cannot find the file specified
	at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143)
	at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073)
	at orchard.util.os$run_commands.invokeStatic(os.clj:28)
	at orchard.util.os$run_commands.invoke(os.clj:24)
	at orchard.util.os$get_win_dirs.invokeStatic(os.clj:56)
	at orchard.util.os$get_win_dirs.invoke(os.clj:35)
	at orchard.util.os$cache_dir.invokeStatic(os.clj:73)
	at orchard.util.os$cache_dir.invoke(os.clj:58)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3714)
	... 158 more

...

This is due to orchard having hardcoded powershell as the executable of choice for invoking a command (I haven't look in detail why it does that)

https://github.com/clojure-emacs/orchard/blob/8169deff8abfdf68bccf3ceb7b1e953cc0a8918b/src/orchard/util/os.clj#L56

Thus, if we only have a pwsh executable in the PATH and no powershell executable, cider is going to fail at the start of cider-jack-in due to orchard. @shishini are you able to jack in with pwsh without getting the above issue with your patch?

Thanks

[vemv]

Thanks for another amazing analysis, @ikappaki 👏

Sounds like we should improve Orchard.

This code looks overly complicated to me...

Perhaps there's some pure-JVM approach we could use instead?

Else we could try/catch and fallback to some other choice.

But I wouldn't be keen on dynamically choosing one of pwsh.exe, powershell.exe, which might mean maintaining two scripts?

[shishini]

Hi @ikappaki

Well I could only temporarily hide powershell.exe from my path by modifying the $env:path variable
I tried renaming powerhsell.exe but windows wont let me ( i didnt try to delete it, but i think i probably cant)

Anyway, when I did i got this error, in my message buffer

[nREPL] Starting server via "c:/Program Files/PowerShell/7/pwsh.exe" -encodedCommand YwBsAG8AagB1AHIAZQAgAC0AUwBkAGUAcABzACAAJwB7ADoAZABlAHAAcwAgAHsAbgByAGUAcABsAC8AbgByAGUAcABsACAAewA6AG0AdgBuAC8AdgBlAHIAcwBpAG8AbgAgACIAMQAuADAALgAwACIAfQAgAGMAaQBkAGUAcgAvAGMAaQBkAGUAcgAtAG4AcgBlAHAAbAAgAHsAOgBtAHYAbgAvAHYAZQByAHMAaQBvAG4AIAAiADAALgA0ADQALgAwACIAfQB9ACAAOgBhAGwAaQBhAHMAZQBzACAAewA6AGMAaQBkAGUAcgAvAG4AcgBlAHAAbAAgAHsAOgBtAGEAaQBuAC0AbwBwAHQAcwAgAFsAIgAtAG0AIgAgACIAbgByAGUAcABsAC4AYwBtAGQAbABpAG4AZQAiACAAIgAtAC0AbQBpAGQAZABsAGUAdwBhAHIAZQAiACAAIgBbAGMAaQBkAGUAcgAuAG4AcgBlAHAAbAAvAGMAaQBkAGUAcgAtAG0AaQBkAGQAbABlAHcAYQByAGUAXQAiAF0AfQB9AH0AJwAgAC0ATQA6AGMAaQBkAGUAcgAvAG4AcgBlAHAAbAA=
error in process sentinel: nrepl-server-sentinel: Could not start nREPL server: Execution error (IOException) at java.lang.ProcessImpl/create (ProcessImpl.java:-2).

CreateProcess error=2, The system cannot find the file specified



Full report at:

C:\Users\XXXXXX~1\AppData\Local\Temp\clojure-7741569257158880857.edn

 ("finished")
error in process sentinel: Could not start nREPL server: Execution error (IOException) at java.lang.ProcessImpl/create (ProcessImpl.java:-2).

CreateProcess error=2, The system cannot find the file specified



Full report at:

C:\Users\XXXXXX~1\AppData\Local\Temp\clojure-7741569257158880857.edn

 ("finished")
 

And this line was one of the line in clojure-7741569257158880857.edn

     :message
    "Cannot run program \"powershell.exe\": CreateProcess error=2, The system cannot find the file specified"
 

So in short my patch doesnt fix this other issue, i think the orchard module should use cider-clojure-cli-command

[ikappaki]

Hi @ikappaki

Well I could only temporarily hide powershell.exe from my path by modifying the $env:path variable I tried renaming powerhsell.exe but windows wont let me ( i didnt try to delete it, but i think i probably cant)

Anyway, when I did i got this error, in my message buffer
...

    "Cannot run program \"powershell.exe\": CreateProcess error=2, The system cannot find the file specified"
 

So in short my patch doesnt fix this other issue, i think the orchard module should use cider-clojure-cli-command

It seems as if powershell.exe is included in every recent version of Windows as an integral part of the system? Therefore, it is highly improbable that Orchard's code will cause any issues, unless in deliberately engineered setups where the user has removed the powershell.exe from the PATH.

Out of curiosity, why are you opting for pwsh.exe when it looks like powershell.exe is already accessible in the PATH?

Thanks

[shishini]

@ikappaki in retrospect, i probably should just have used powershell

but my luck kinda, i installed clojure cli in pwsh, (in a folder only accessed by pwsh , only in pwsh module path)
because its the new shell and its what i use, and from that point i came across those issues

again in retrospect, i should have thought of this as bash vs sh situation, where they can coexist and some scripts might only be compatible wish sh and not bash

but i just kinda thought pwsh should work its what i use

[ikappaki]

@ikappaki in retrospect, i probably should just have used powershell

but my luck kinda, i installed clojure cli in pwsh, (in a folder only accessed by pwsh , only in pwsh module path) because its the new shell and its what i use, and from that point i came across those issues

again in retrospect, i should have thought of this as bash vs sh situation, where they can coexist and some scripts might only be compatible wish sh and not bash

but i just kinda thought pwsh should work its what i use

This is a very good reason why we should support pwsh.

It would also be nice if we can update the documentation at the below link with the above info?

cider/doc/modules/ROOT/pages/basics/up_and_running.adoc

Lines 211 to 215 in 0134a0b

	To use `cider-jack-in` with `tools.deps` on Windows set the
	`cider-clojure-cli-command` to `"powershell"`. This happens by default
	if you are on Windows and no `clojure` executable is found. Using
	`"powershell"` will Base64 encode the clojure launch command before
	passing it to PowerShell and avoids shell-escaping issues.

To summarise

1. There are real use cases to support the pwsh executable in CIDER.
2. This patch fixes it. There is also a one-liner alternative as mentioned above using $PSNativeCommandArgumentPassing = 'Legacy'
3. It will also be nice if we could update the documentation with regards to pwsh.
4. There is an issue with orchard if we only have pwsh.exe in the PATH but not powershell.exe. We think this is a non issue, since powershell.exe is always in the path and can't think of a reason why someone might manually remove it.

(I think we can also add an integration test with pwsh in CI... I can look into this once this is checked in)

thanks

[vemv]

This patch fixes it. There is also a one-liner alternative as mentioned above using $PSNativeCommandArgumentPassing = 'Legacy'

I think I would prefer this approach - the code would have no branches.

Do you agree @ikappaki ?

[ikappaki]

This patch fixes it. There is also a one-liner alternative as mentioned above using $PSNativeCommandArgumentPassing = 'Legacy'

I think I would prefer this approach - the code would have no branches.

Do you agree @ikappaki ?

Hi @vemv, I'd prefer the one line edit, but I don't want to deter @shishini in their first check in :)

[vemv]

I see! I'd suggest @ikappaki , please create a PR with the fix, making sure it works as intended (if you have it at hand).

Feel free to add shishini@users.noreply.github.com as a commit co-author.