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

Emacs-pet is broken against Eglot in Emacs 31 #50

Open
vedang opened this issue Dec 1, 2024 · 1 comment
Open

Emacs-pet is broken against Eglot in Emacs 31 #50

vedang opened this issue Dec 1, 2024 · 1 comment

Comments

@vedang
Copy link

vedang commented Dec 1, 2024

Description
emacs-pet advices the Eglot internal function eglot--executable-find. This function existed till Eglot version 1.16, but has been completely removed in Eglot version 1.17 -- Eglot now just uses executable-find, along with the compat library.

Reproduction steps

  1. Download the latest Emacs (master version). I use the latest available version from here: https://github.com/jimeh/emacs-builds/releases
  2. Open a Python project which uses poetry (or any other tool) for managing virtual environments.
  3. See the pet-verify-setup reports everything correctly.
  4. Run eglot-ensure and see that binaries in the venv are not found.

Expected behavior
Eglot should find the appropriate binaries from the venv.

PET version
3.1.0
Emacs version
31.0.50
Desktop (please complete the following information):

  • OS: macOS
  • Version 15.1.1

System tools versions

  • dasel 2.8.1
  • sqlite3 3.39.2
  • yq not installed
  • toml not installed

Additional context
none
@linear Linear
Copy link

linear bot commented Dec 1, 2024

ALL-15 Emacs-pet is broken against the latest version of Eglot (1.17)

vedang added a commit to vedang/emacs-pet that referenced this issue Dec 1, 2024
@vedang
Fix: Hook into eglot-ensure, instead of eglot--executable-find
7300274 
`eglot--executable-find` was an internal function that has been
removed in the latest version of Eglot (1.17). Due to this,
`emacs-pet` no longer works with Eglot correctly, since it's advice is
not used anymore. wyuenho#50

In this commit, we fix the issue by making the following changes:

1. Extract the internals of `pet-executable-find` into an independent
function `pet-adjust-path-executable-find`.
   - This function creates a buffer-local variants of `exec-path` and
   `tramp-remote-path` variables, and add venv paths found by pet.
   Eglot can now find the path to the right executables correctly.
   - We also remove calls to `setenv PATH`, since we don't need to
   modify a  global system setting to make venv executables available
   to Eglot.

2. Remove all references to unused or non-existent eglot variables.
   - `eglot--executable-find` no longer exists.
   - `eglot--uri-to-path` is not used by `pet`.

3. Modify `pet-executable-find` to use our new function
`pet-adjust-paths-executable-find`.

4. Advice `eglot-ensure`, which is an Eglot public API that is always called
when starting an LSP server.
   - We are no longer depending on internal variables / functions for
   this, and so the Eglot code is now a bit more future-proof.

I've tested this change as follows:

1. Tested against Python projects using the following types of
scaffolding:
   - poetry
   - Manual venv creation
2. Tested against Eglot versions 1.17 and 1.16 (Emacs versions 31 and 29)
3. I could not get `make test` to run correctly, because of native
compilation / Emacs 31 issues.
@vedang vedang mentioned this issue Dec 1, 2024
Open
vedang added a commit to vedang/emacs-pet that referenced this issue Dec 2, 2024
@vedang
Fix: Hook into eglot-ensure, instead of eglot--executable-find
87dd4bc 
`eglot--executable-find` was an internal function that has been
removed in the latest version of Eglot (1.17). Due to this,
`emacs-pet` no longer works with Eglot correctly, since it's advice is
not used anymore. wyuenho#50

In this commit, we fix the issue by making the following changes:

1. Extract the internals of `pet-executable-find` into an independent
function `pet-adjust-path-executable-find`.
   - This function creates a buffer-local variants of `exec-path` and
   `tramp-remote-path` variables, and add venv paths found by pet.
   Eglot can now find the path to the right executables correctly.
   - We also remove calls to `setenv PATH`, since we don't need to
   modify a  global system setting to make venv executables available
   to Eglot.

2. Remove all references to unused or non-existent eglot variables.
   - `eglot--executable-find` no longer exists.
   - `eglot--uri-to-path` is not used by `pet`.

3. Modify `pet-executable-find` to use our new function
`pet-adjust-paths-executable-find`.

4. Advice `eglot-ensure`, which is an Eglot public API that is always called
when starting an LSP server.
   - We are no longer depending on internal variables / functions for
   this, and so the Eglot code is now a bit more future-proof.
   - We use the `:before` variant of advice, because `eglot-ensure`
   does not take any arguments and we don't need the flexibility of
   the `:around` version. Instead, we check if the major mode is
   `python` to decide if we should trigger the advice.

I've tested this change as follows:

1. Tested against Python projects using the following types of
scaffolding:
   - poetry
   - Manual venv creation
2. Tested against Eglot versions 1.17 and 1.16 (Emacs versions 31 and 29)
3. I could not get `make test` to run correctly, because of native
compilation / Emacs 31 issues.
vedang added a commit to vedang/emacs-pet that referenced this issue Dec 2, 2024
@vedang
Fix tests for pet-eglot--executable-find-advice
1c0a400 
Remove these tests and add new tests for
`pet-eglot--adjust-paths-advice`.

Closes: wyuenho#50
vedang added a commit to vedang/emacs-pet that referenced this issue Dec 12, 2024
@vedang
Use pet-executable-find in the eglot-server-programs closure
3566a6f 
`eglot--executable-find` was an internal function that has been
removed in the latest version of Eglot (1.17). Due to this,
`emacs-pet` no longer works with Eglot correctly, since it's advice is
not used anymore. wyuenho#50

In this commit, we fix the issue by making the following changes:

1. Remove all references to unused or non-existent eglot variables.
   - `eglot--executable-find` no longer exists.
   - `eglot--uri-to-path` is not used by pet.

2. Advice `eglot--lookup-mode` to use the pet version of
`eglot-server-programs`.
   - Provide a new closure to eglot to find LSP executables.
   - Use `pet-executable-find` in this closure.
@wyuenho wyuenho changed the title Emacs-pet is broken against the latest version of Eglot (1.17) Emacs-pet is broken against Eglot in Emacs 31 Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@vedang