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.

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

Escape clojure cli command params on MS-win for native commands #3342

Merged
merged 3 commits into from
May 18, 2023
Merged

Escape clojure cli command params on MS-win for native commands #3342

merged 3 commits into from
May 18, 2023

Conversation

ikappaki
Copy link
Contributor

Hi,

could you please review patch to encode clojure cli native command args on MS-Windows. It addresses #3341.

The clojure cli command line args for clojure cli invocations other than powershell also need to be escaped.

I've relocated the escaping logic into cider-clojure-cli-jack-in-dependencies, assuming that powershell invocations are only relevant to clojure-cli, as is the new native escaping.

I've also added a windows-nt only clojure-cli deps.exe integration test to test the same. For this, I need to install the deps.clj tools in the workflow for windows-nt.

Thanks

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

@ikappaki
Always escape clojure cli command params on MS-win
6447683 
even for non powershell invocations
@ikappaki ikappaki force-pushed the issue/clojure-exec-win branch from 80deae2 to 30f260e Compare May 11, 2023 23:48
vemv
vemv approved these changes May 12, 2023
View reviewed changes
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - great work!

CHANGELOG.md Outdated Show resolved Hide resolved
@ikappaki
Copy link
Contributor Author

lgtm - great work!

Thanks @vemv, I've just released I need to rebase some tests that escaped my attention. I'm also thinking of some further improvements to the code to treat powershell quoting a special shell-quote-args case rather than a hack.

I'll post an update soon.

@ikappaki
Treat PowerShell quoting as a special case of`shell-quote-args'
f11a8a9 
- `cider-inject-jack-in-dependencies' requires an additional arg
- introduces `cider--shell-quote-argument' to support PowerShell arg quoting
- `cider--powershell-encode-command' now only encodes but does not
quotes args (this is the job of `cider--shell-quote-argument' now).

Tests rebased to

- Account for additional command arg in jack in commands
- Account for the fact that cider--powershell-encode-command only encodes but not quotes any more
- quoting command for dependencies is os/command specific.
- Run an example jack-in command test for powershell.
@ikappaki ikappaki force-pushed the issue/clojure-exec-win branch from a759a44 to f11a8a9 Compare May 15, 2023 20:46
@ikappaki
Copy link
Contributor Author

Hi,

I've updated the code to treat PowerShell quoting similarly to shell-quote-argument via the new cider--shell-quote-argument, rebased the tests that were failing due to the previous commit, and extended an existing test to cover a PowerShell jack in invocation.

all tests are passing, I think we are good to go for the final review.

Thanks

@ikappaki ikappaki changed the title Sscape clojure cli command params on MS-win for native commands Escape clojure cli command params on MS-win for native commands May 15, 2023
vemv
vemv reviewed May 16, 2023
View reviewed changes
cider.el Outdated Show resolved Hide resolved
bbatsov
bbatsov reviewed May 17, 2023
View reviewed changes
test/cider-tests.el Outdated
@@ -268,7 +268,7 @@
(shell-quote-argument "cider.nrepl/cider-middleware")
" repl -s wait")))
(it "can concat in a gradle project"
(expect (cider-inject-jack-in-dependencies "--no-daemon" ":clojureRepl" 'gradle)
(expect (cider-inject-jack-in-dependencies "--no-daemon" ":clojureRepl" 'gradle "grandle")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gradle :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the API and this argument is now optional, thus I removed the new argument altogether since it only effective in clojure-cli

bbatsov
bbatsov reviewed May 17, 2023
View reviewed changes
cider.el Outdated Show resolved Hide resolved
@ikappaki
Make new jack-in-dependencies COMMAND arg &optional
ffacf86 
so that it does not break API compatibility
@ikappaki
Copy link
Contributor Author

Hi, I've provisionally set the new jack-in-dependencies COMMAND arg to &optional pending further discussion whether this is satisfactory or go with the private fns solution. Let me know what you think.

I've also added a test for cider--shell-quote-argument which I've missed with my previous commit.

thanks

@bbatsov bbatsov merged commit 99c90fc into clojure-emacs:master May 18, 2023
@bbatsov
Copy link
Member

bbatsov commented May 18, 2023

I think we're good to go now. Thanks!

@vemv
Copy link
Member

vemv commented May 18, 2023

Unfortunately the build is red now:

image

@ikappaki
Copy link
Contributor Author

Unfortunately the build is red now:
image

Hi @vemv,

could this have been a GH transient issue GH accessing ... GH via PowerShell? Can you please rerun the action and see if it passes?

I've just updated my master CIDER fork with the latest updates and that particular bit worked fine:

Run iwr -Uri https://raw.githubusercontent.com/borkdude/deps.clj/master/install.ps1 -outfile install_clojure.ps1
iwr -Uri https://raw.githubusercontent.com/borkdude/deps.clj/master/install.ps1 -outfile install_clojure.ps1
.\install_clojure.ps1
get-command deps.exe | split-path -parent | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
env:
PATH: C:\Program Files\MongoDB\Server\5.0\bin;C:\aliyun-cli;C:\vcpkg;C:\Program Files (x86)\NSIS;C:\tools\zstd;C:\Program Files\Mercurial;C:\hostedtoolcache\windows\stack\2.9.3\x64;C:\cabal\bin; ...
Downloading...
Extracting...
Installing deps.exe to C:\Users\runneradmin\deps.clj...
Adding ~\deps.clj to your path.
Cleaning up...
Successfully installed deps.exe.
Restart cmd.exe for changes to the path to take effect.

Looking at the deps.clj install_clojure.clj script at https://raw.githubusercontent.com/borkdude/deps.clj/master/install.ps1, and specifically the lines 30 and 31 (it fails at L31)

L30: $releast_version_url = "https://raw.githubusercontent.com/borkdude/deps.clj/master/resources/DEPS_CLJ_RELEASED_VERSION"
L31: $latest_release = (Invoke-WebRequest -Uri $releast_version_url -UseBasicParsing)

it tries to download https://raw.githubusercontent.com/borkdude/deps.clj/master/resources/DEPS_CLJ_RELEASED_VERSION, that shouldn't fail, unless there was an transient networking issue perhaps?

Thanks

@vemv
Copy link
Member

vemv commented May 18, 2023

Sounds plausible. Let's see how the build goes over the following days/weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vemv vemv vemv approved these changes

@bbatsov bbatsov bbatsov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ikappaki @bbatsov @vemv