Skip to content

Conversation

@ecofighter
Copy link

Currently, when editing remote files, copilot.el searches for the executable on the remote system and launches the server remotely. This PR fixes that behavior by ensuring both the executable search and server launch occur locally.

  • Change executable-find second argument to nil in copilot-server-executable to search locally.
  • Change default-directory to local home directory when it is a remote directory in copilot--start-server and copilot-install-server.
    This ensures processes run with the local home directory as their working directory.
  • Remove executable existence check in copilot--start-server since it's already verified in the copilot-server-executable function.

Fixes #403

- Add `files` dependency.
- Change `executable-find` second argument to `nil` in `copilot-server-executable` to search locally.
- Remove existence check in `copilot--start-server` since already checked in
  `copilot-server-executable`.
- Update `copilot--start-server` to set `default-directory` to local home when
  in remote context.
- Set `default-directory` to local home when installing the server from a remote directory.
@ecofighter ecofighter force-pushed the search-executable-locally branch from faf032f to 3004101 Compare April 7, 2025 14:20
copilot.el Outdated
(file-exists-p copilot-server-executable))
copilot-server-executable)
((executable-find copilot-server-executable t))
((executable-find copilot-server-executable nil))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just remove this param - it's optional.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

copilot.el Outdated
(t "bin"))
copilot-server-executable)
t)))
nil)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

copilot.el Outdated
"Interactively install server."
(interactive)
(if-let* ((npm-binary (executable-find "npm")))
(if-let* ((default-directory (if (file-remote-p default-directory)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what this is trying to solve - I don't think it will have any effect on the executable-find call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, you want it to search only the local filesystem. As noted in another remark - I have some doubts about this approach as I think this might be quite surprising to some people.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 7, 2025

Change default-directory to local home directory when it is a remote directory in copilot--start-server and copilot-install-server.

Doing this might be super confusing, though, as I'm not sure anyone editing something over TRAMP would expect something like this to happen.

Removed the redundant ~nil~ argument from the ~executable-find~ function call.
- Change `copilot-install-dir` default value to use `user-emacs-directory` for constructing the cache path
- Modify `copilot-install-server` to handle `shell-file-name` correctly when using connection-local variables
@ecofighter
Copy link
Author

Change default-directory to local home directory when it is a remote directory in copilot--start-server and copilot-install-server.

Doing this might be super confusing, though, as I'm not sure anyone editing something over TRAMP would expect something like this to happen.

The change is scoped to lexical scope only. By using let, default-directory is only modified locally within these specific functions (copilot--start-server and copilot-install-server).

I've confirmed that default-directory maintains its original value and find-file and eglot works expectedly. I think the user's TRAMP editing context remains preserved in all other operations.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 7, 2025

I've confirmed that default-directory maintains its original value and find-file and eglot works expectedly. I think the user's TRAMP editing context remains preserved in all other operations.

Do you know how Eglot behaves when dealing with TRAMP? Does try to start LST servers locally or remotely?

My point was mostly that I'm not sure if when I edit something remotely I'd expect it to trigger the creation of a local process, but I haven't used TRAMP much in ages and I'm not sure what are the common practices there these days.

@ecofighter
Copy link
Author

Do you know how Eglot behaves when dealing with TRAMP? Does try to start LST servers locally or remotely?

My point was mostly that I'm not sure if when I edit something remotely I'd expect it to trigger the creation of a local process, but I haven't used TRAMP much in ages and I'm not sure what are the common practices there these days.

Eglot runs LSP servers remotely when used with TRAMP. I've tested this, and LSP servers run remotely as expected.

According to TRAMP's document (https://www.gnu.org/software/tramp/#Remote-processes), the location where processes run depends solely on the default-directory value:

TRAMP supports starting new running processes on the remote host for discovering remote file names. Emacs packages on the remote host need no specific modifications for TRAMP’s use.

This type of integration does not work with the ftp method, and does not support the pty association as specified in start-file-process.

process-file and start-file-process work on the remote host when the variable default-directory is remote:

(let ((default-directory "/ssh:remote.host:"))
  (start-file-process "grep" (get-buffer-create "*grep*")
                      "/bin/sh" "-c" "grep -e tramp *"))

But I'm also not very familiar with the current common practices around TRAMP.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 9, 2025

@ecofighter I'm guessing it'd be best if we try to run the server locally for local files and remotely for remote files (I guess for remote files we can also elect to not do anything) to be aligned with how LSP packages work in general.

@ecofighter
Copy link
Author

@ecofighter I'm guessing it'd be best if we try to run the server locally for local files and remotely for remote files (I guess for remote files we can also elect to not do anything) to be aligned with how LSP packages work in general.

I understood.
I'll close this PR and try a new approach as you said.

@ecofighter ecofighter closed this Apr 9, 2025
@ecofighter ecofighter deleted the search-executable-locally branch April 9, 2025 15:20
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.

Doesn't work with TRAMP on Win11

2 participants