cider icon indicating copy to clipboard operation
cider copied to clipboard

Remote `cider-jack-in` breaks with `cider-enrich-classpath`: `No such file or directory`

Open caadr opened this issue 1 year ago • 8 comments

This is an attempt to fix enrich-classpath erroring out, when default-directory is remote, by creating a copy of the lein.sh|clojure.sh script on the remote before jacking in.

The core of it is happening here.

Conversation from the original Issue below.


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

Expected behavior

With point in a project-file visited via ssh/tramp and cider-enrich-classpath = t, cider-jack-in-clj creates a remote repl and connects to it.

Actual behavior

nrepl-server-sentinel throws:

error("Could not start nREPL server: %s (%S)" "bash: /home/**/cider/clojure.sh: No such file or directory\n" "exited abnormally with code 127")

Reason

Cider is trying to use the local path (from cider--get-enrich-classpath-clojure-cli-script) on the remote, where it does not exist.

Possible Workaround

Copy the script to the remote before starting the server.

caadr avatar Nov 01 '23 18:11 caadr

Hi @adrech !

I'm happy to hear you're using Enrich. The proposed branch would LGTM after the following feedback:

  • Extract the files to cider-util.el instead
    • cider.el has grown pretty large
  • If it's at hand, prefer mktemp -d -t cider.XXXXXX to /tmp
  • Detect if the copying failed
    • If so, ~setq-local~ bind cider-enrich-classpath nil and proceed
    • Would appreciate if you could pretend (hardcode) that this failed while QAing this, to verify that the fallback works as intended.

Thanks - V

vemv avatar Nov 01 '23 19:11 vemv

Let me know if you'd like to target this during the week, matching our fortcoming major release.

Else of course, it's no issue - we could also shortcirtuit Enrich for the time being for the tramp code path.

vemv avatar Nov 05 '23 20:11 vemv

Will be on it! I spent the weekend learning more about tramp, shells, ssh & docker and sanitized how the tramp-sample-project sets up PATH, so I have something reliable to test against. If you'd like, I can include that or create a separate PR.

Whats the target date?

caadr avatar Nov 05 '23 23:11 caadr

That's awesome to hear!

Feel free to include that work as you wish. Perhaps separate PR since most likely we'll end up squashing stuff, so more commits is better.

Whats the target date?

Flexible - do not feel pressured, we can also simply cut bugfix releases later.

vemv avatar Nov 06 '23 04:11 vemv

  • If it's at hand, prefer mktemp -d -t cider.XXXXXX to /tmp

This should essentially do the same thing in most cases:

(make-nearby-temp-file "cider." :dir) ;; => /ssh:root@localhost#8022:/tmp/cider.OrkB00 

In this draft I make the copy directly at <remote-tempdir>/.cider__<script-name>__XXXXXX, because cleanup seemed simpler without a directory. The way it works right now, the file could end up in default-directory - when tramp does not find an accessible tempdir on the remote - and I didn't want the files to pile up in the project dir. There are still some holes in the cleanup process: when something goes wrong before the script is executed it will stay. Making the script delete itself is probably not the best approach, but I like that it is "self-contained" and it was fun playing around :)

  • If so, ~setq-local~ bind cider-enrich-classpath nil and proceed

This is happening in cider--update-jack-in-cmd by checking if the command contains one of the scripts names. To keep that simple but reliable, I prefixed the files with enriched_[lein|clojure].sh.

So far, this draft works as expected with a minimal init.el on my pi and in clojure / lein docker containers, but it'll need more thorough testing, considering the moving parts.

EDIT: did some manual testing with a renamed /tmp and a read-only project dir and cider is falling back correctly.

Let me know if I'm moving in the right direction and I'll split it up, add tests, docstrings.

caadr avatar Nov 12 '23 22:11 caadr

Happy to see these iterations 🙌

Please don't rename lein.sh → enrich_lein.sh as it makes some UIs more verbose, and it also would require a PR against https://github.com/melpa/melpa which is extra friction.

These files are also "APIs" that users can reference from .zshrc.

Cheers - V

vemv avatar Nov 13 '23 22:11 vemv

Aye, of course, reverted the filenames.

caadr avatar Nov 14 '23 01:11 caadr

There's a potential problem (with handling ssh remotes in general):

All of this assumes, that tramp and the remote PATH are set up to work together. It will fail early in the process otherwise.

This would happen e.g. when one connects via ssh + relies on a PATH setup in the remotes .bashrc or .profile + uses default tramp. More specifically, it they don't have this line in their tramp config, which makes tramp grab the env from a login shell:

(add-to-list 'tramp-remote-path 'tramp-own-remote-path)

For more background, see: (info "(tramp) Remote progams") and the ramblings at https://github.com/clojure-emacs/cider/blob/92c9694161fa7638b5e9d05bb797ffefde613237/dev/tramp-sample-project/Dockerfile.template#L4

A couple of ways to approach this:

  • rebinding tramp-remote-path somewhere high up in the callchain (only if it is untouched)
  • informing the user that they might have this problem and how to fix it, when resolving a remote command fails
  • reverting to the old behaviour of cider--resolve-command, that would just no-op on remotes and let the user run into an error message like java not found... when the server tries to start, which would give them an idea that something is wrong with their PATH setup.

caadr avatar Nov 17 '23 10:11 caadr