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.

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

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

adrech
Copy link
Contributor

@adrech adrech commented Nov 12, 2023

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.


  • The commits are consistent with our contribution guidelines
  • You've added (very few) tests 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)

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.

@adrech adrech changed the title Remote cider-jack-in breaks with cider-enrich-classpath: `No such file or directory Remote cider-jack-in breaks with cider-enrich-classpath: No such file or directory Nov 1, 2023
@vemv
Copy link
Member

vemv commented Nov 1, 2023

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
Copy link
Member

vemv commented Nov 5, 2023

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.

@adrech
Copy link
Contributor Author

adrech commented Nov 5, 2023

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?

@vemv
Copy link
Member

vemv commented Nov 6, 2023

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.

@adrech
Copy link
Contributor Author

adrech commented Nov 12, 2023

  • 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.

@adrech adrech marked this pull request as draft November 12, 2023 22:23
@adrech adrech force-pushed the fix-remote-enrich-classpath-init branch 4 times, most recently from d7370f4 to 404224c Compare November 13, 2023 21:38
@vemv
Copy link
Member

vemv commented Nov 13, 2023

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

@adrech
Copy link
Contributor Author

adrech commented Nov 14, 2023

Aye, of course, reverted the filenames.

@adrech adrech force-pushed the fix-remote-enrich-classpath-init branch 2 times, most recently from bab04c9 to f656504 Compare November 14, 2023 11:03
…lojure-emacs#3567

Fixes `cider-jack-in-clj` in tramp buffers throwing:
```
Could not start nREPL server: %s (%S)
bash: /remote/path/to/cider/clojure.sh: No such file or directory\n" "exited abnormally with code 127")
```
in cases where cider is not installed in the same directory on the
remote.

To fix this, we create temporary copy of the enrich-classpath script
named `.cider__<clojure.sh|lein.sh>__<random>` on the remote before
starting the server. The possible locations of the script are, in this
order:
- tramp-tempdir (usually "/tmp")
- clojure-project-dirj
- default-directory

If the script can't be created for any reason, the server is started with
`cider-enrich-classpath` set to nil.

Note: the temporary script will remove itself after use, but stick around when
something goes wrong before the remote process is started.
@adrech adrech force-pushed the fix-remote-enrich-classpath-init branch from d0c3e57 to 44622e5 Compare November 16, 2023 18:09
`auto-save-visited-mode` will not create any tempfiles anyways, but only
save the current file.
We don't need it (yet) and Emacs 26 does not support
`split-string-shell-command`.
Don't want to fiddle with shell-quoting unless really necessary.
@adrech adrech force-pushed the fix-remote-enrich-classpath-init branch 2 times, most recently from 8a4bd5d to 72cab2f Compare November 16, 2023 19:25
@adrech adrech force-pushed the fix-remote-enrich-classpath-init branch from 72cab2f to 8183c2b Compare November 16, 2023 19:33
@adrech
Copy link
Contributor Author

adrech commented Nov 17, 2023

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants