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

Support specifying extra VM arguments for the Java Language server #800

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zbelial
Copy link

@zbelial zbelial commented Jan 14, 2022

  • eglot.el (eglot-java-vmargs): New defcustom.
    (eglot-eclipse-jdt): Specify VM arguments.

eglot.el Outdated Show resolved Hide resolved
eglot.el Outdated Show resolved Hide resolved
eglot.el Outdated Show resolved Hide resolved
@skangas
Copy link
Collaborator

skangas commented Jan 14, 2022

Thanks, if @joaotavora agrees I think we could merge this with the above changes.

@skangas
Copy link
Collaborator

skangas commented Jan 14, 2022

This patch is large enough to need a copyright assignment to the Free Software Foundation. Have you already done that, or would you like to start that process now?

@skangas skangas added the copyright-assignment Waiting for copyright assignment label Jan 14, 2022
@zbelial
Copy link
Author

zbelial commented Jan 14, 2022

This patch is large enough to need a copyright assignment to the Free Software Foundation. Have you already done that, or would you like to start that process now?

I have not done any copyright assignment to the Free Software Foundation, but I'd like to start it. Could you give me some hints about how to do it? Thanks.

@skangas
Copy link
Collaborator

skangas commented Jan 14, 2022

I have not done any copyright assignment to the Free Software Foundation, but I'd like to start it. Could you give me some hints about how to do it? Thanks.

Great, thank you for your contribution. Here are the instructions:

Please email the following information to [email protected], and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]
Emacs

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]

[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]

[For the copyright registration, what country are you a citizen of?]

[What year were you born?]

[Please write your email address here.]

[Please write your postal address here.]

[Which files have you changed so far, and which new files have you written
so far?]

@joaotavora
Copy link
Owner

joaotavora commented Jan 14, 2022

@skangas just a heads up about the JDT support.

When I added it very early on in the product, I was doing something I don't like to do. THere should be no complex catering for specific language servers in eglot.el itself except for entries in eglot-server-programs. JDT (and the somewhat quirky Rust rls), got something of a free pass (nothing against rls, it was something of a pioneer in this field).

Anyway avoiding language specific things in Eglot is exactly what LSP is about. In Eglot, it's precisely what differentiated it, in a good way, I think, from lsp-mode. There, there is a whole ecosystem of lsp-langx.el libraries and there is a trio of players: 1. major mode, 2. lsp client and 3. lsp-language-specific-code. In Eglot, there are only the first 2 and I think we should go towards that objective always, as we put eglot.el in he code.

Long story short, all this code should be in java-mode.el in Emacs. Or in some separate eglot-jdt.el library. I don't understand why Java is the only language where setting up a language server is such a pain. In any other language it's usually a question of launching a program, here, it's a very very horrible pain. None of this code belongs in Eglot.

That said, this does solve user problems. But here I would really consider making a separate deprecated eglot-jdt.el library, clearly specify that it is an exception and interim solution, and put all this cruft there. To not break user experience, this can be put at the end of eglot.el:

(provide 'eglot) ;; as normal
(require 'eglot-jdt nil t)

I'd probably also delete the rls specific support for its quirks in the process. rust-analyser (https://rust-analyzer.github.io/) seems much better.

@skangas
Copy link
Collaborator

skangas commented Jan 14, 2022

@joaotavora Thanks, that's very useful. See also my separate question about this in the discussion, which you basically answered fully here. I can see the logic in your arguments, and I can only agree.

Your plan to move this into a separate file is good, and I think we could do that before or after merging this PR. Once we have moved this out into a separate library, we can start working on basically dumping that support code on whatever major mode(s) that wants to take it. Perhaps they will be more inclined to agree once we have eglot in core.

Regarding rls, I don't mind dropping that. I wonder if we should do that without asking someone else to take it over first? If rls is not very important these days, I think we could just dump it.

@theothornhill
Copy link
Collaborator

It seems rust-analyzer is where the cool kids go these days anyway

@skangas
Copy link
Collaborator

skangas commented Jan 14, 2022

It seems like rust-analyzer is the officially blessed one: rust-lang/rfcs#2912

@zbelial
Copy link
Author

zbelial commented Jan 14, 2022

FYI, someone has created a package eglot-java which improves eglot's java support and provides some additional useful functions, maybe the author @yveszoundi is interested in helping split java support into a separate library?

@joaotavora
Copy link
Owner

FYI, someone has created a package eglot-java which improves eglot's java support and provides some additional useful functions, maybe the author @yveszoundi is interested in helping split java support into a separate library?

OH really, that's good news. Then we should definitely advertise in the README.md (with the context that it is an exception), and maybe just delete all the java stuff in eglot.el? Does eglot-java replace eglot's JDT thing completely, or does it still rely on it? I.e. @zbelial why are you not using it?

@zbelial
Copy link
Author

zbelial commented Jan 15, 2022

OH really, that's good news. Then we should definitely advertise in the README.md (with the context that it is an exception), and maybe just delete all the java stuff in eglot.el? Does eglot-java replace eglot's JDT thing completely, or does it still rely on it? I.e. @zbelial why are you not using it?

Currently it depends on eglot's eglot--eclipse-jdt-contact(this seems the only hard dependency in terms of java support. It detects where JDT is installed and sets CLASSPATH before calling eglot--eclipse-jdt-contact. )

That dependency is the only reason why I do not use it, since it still does not support specifying vm arguments.

@skangas
Copy link
Collaborator

skangas commented Jan 15, 2022

Thanks for the updates here. Do you mind squashing the three commits into one? That makes it easier to review and merge.

@zbelial
Copy link
Author

zbelial commented Jan 15, 2022

Thanks for the updates here. Do you mind squashing the three commits into one? That makes it easier to review and merge.

Sure. I have squashed those three commits into one now. It seems to me that squash is done successfully, but this is the first time that I use rebase squash, is the latest commit ok?

@skangas
Copy link
Collaborator

skangas commented Jan 15, 2022

Sure. I have squashed those three commits into one now. It seems to me that squash is done successfully, but this is the first time that I use rebase squash, is the latest commit ok?

The latest commit is fine. I think we are just waiting for the copyright assignment here. Thanks!

* eglot.el (eglot-eclipse-jdt-vmargs): New defcustom.
(eglot-eclipse-jdt): Apply VM arguments in eglot-eclipse-jdt-vmargs.
@skangas
Copy link
Collaborator

skangas commented Jan 15, 2022

I just noticed that #251 is trying to do something similar as this.

@zbelial
Copy link
Author

zbelial commented Jan 16, 2022

I hadn't noticed that before. That pr and this one demonstrate that some jdt users really need some way to configure jdt :)

@skangas
Copy link
Collaborator

skangas commented Jan 16, 2022

Thinking a bit more about this, given that this is basically is only relevant until this gets moved elsewhere (see #809), I'm not sure we would want to advertize it as much as making it a defcustom implies. So could we please move this to a defvar instead?

@joaotavora
Copy link
Owner

joaotavora commented Jan 16, 2022 via email

@yveszoundi
Copy link

yveszoundi commented Apr 3, 2022

@zbelial , I made the relevant changes in eglot-java today. It should be available in upcoming MELPA builds.

  • Catch up with recent eglot code changes (removed functions)
  • Add ability to pass extra JVM arguments to the LSP server (eglot-java-eclipse-jdt-args customizable option)

/cc @skangas @manuel-uberti

Other relevant references

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

Successfully merging this pull request may close these issues.

5 participants