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

jdt-language-server: init at 0.67.0 #99330

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

Conversation

@matt-snider
Copy link
Contributor

@matt-snider matt-snider commented Oct 1, 2020

Motivation for this change

It doesn't seem like there are any Java language servers in nixpkgs so far

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@matt-snider
Copy link
Contributor Author

@matt-snider matt-snider commented Oct 1, 2020

Also addresses #97814

@matt-snider
Copy link
Contributor Author

@matt-snider matt-snider commented Dec 9, 2020

This is several version behind the current (0.66.0) so I'll have to update it. That being said, I also am not sure if a few things are correct - the /tmp runtime directory, the JDK version, etc. It works for me, but is a bit buggy in some projects. This might be due to my particular setup though.

@matt-snider
Copy link
Contributor Author

@matt-snider matt-snider commented Dec 10, 2020

Looking into this more, my issue was that I had set GRADLE_HOME incorrectly.

# incorrect
export GRADLE_HOME="${pkgs.gradle}";

# correct
export GRADLE_HOME="${pkgs.gradle}/lib/gradle";

Also, I think this should probably be set to use JDK15 because with JDK11 it doesn't work in JDK15 projects (probably no project with a JDK version > 11).

@aanderse
Copy link
Member

@aanderse aanderse commented Dec 15, 2020

@fzakaria are you able to review this? Pretty please with a cherry on top. 🍒

# jdt-language-server needs to write to config location, so we create a temp
# directory each time and copy the config_linux/ file there
runtime_dir=/tmp/jdt-language-server
Copy link
Contributor

@fzakaria fzakaria Dec 15, 2020

What's this used for ? Should it be XDG_CACHE or something similar?

Copy link
Contributor Author

@matt-snider matt-snider Dec 22, 2020

Yeah, so it's not documented that well, and I wasn't sure if this should be the project directory or some cache directory. I found some examples of how it's run in other packages:

  • emacs-lsp/lsp-java passes $user-emacs-directory/workspace (I think this would be ~/.config/emacs/workspace, but I'm not an emacs user so not sure)
  • vscode-java seems to use the workspace directory, which I think isn't exactly the project, but closer to that than a global cache directory.

I actually wonder if it makes more sense to omit of the arguments passed to the jar, since various users and applications may want to do this differently.

Copy link
Contributor Author

@matt-snider matt-snider Dec 22, 2020

Correction to the above - it might make sense to omit arguments after -configuration and allow the user to specify them.

I believe -configuration should be specified by packagers, since it's just a platform-specific directory (e.g. config_linux/, config_mac/).

pkgs/development/tools/jdt-language-server/default.nix Outdated Show resolved Hide resolved
@fzakaria
Copy link
Contributor

@fzakaria fzakaria commented Dec 15, 2020

I'm not sure where GRADLE_HOME comes into the picture here ?

I started to look at the submission but haven't tested it locally yet; I'm dealing with some personal issues at the moment (COVID) so i'm a bit behind everything :(

My 2cents on the matter though that the description:

It works for me, but is a bit buggy in some projects. This might be due to my particular setup though.

Does not inspire confidence in submission. Should a package be included if that's the current state ? Maybe it needs the broken attribute then.

Anyways thank you for the contribution and I will try give it a more in-depth review this week.

@aanderse
Copy link
Member

@aanderse aanderse commented Dec 16, 2020

Thank you @fzakaria! I hope everything is alright. Keep safe, no rush, life always comes first. ❤️

@matt-snider
Copy link
Contributor Author

@matt-snider matt-snider commented Dec 22, 2020

@fzakaria Thank you for having a look! I hope everything is alright on your end as well.

I'm not sure where GRADLE_HOME comes into the picture here ?

This was just a response to my previous comment that I was having issues, but I had set GRADLE_HOME incorrectly in the environment where I started the language server.

It works for me, but is a bit buggy in some projects. This might be due to my particular setup though.

Does not inspire confidence in submission. Should a package be included if that's the current state ? Maybe it needs the broken attribute then.

This is true, but I believe said bugginess & uncertainty is a result of two factors. (1) jdt-language-server is unfortunately not well documented and I've had to rely on digging through code to fix issues along the way, and (2) most people use jdt-language-server through vscode-java, but I use neovim and LanguageClient-neovim, for which there isn't tonnes of good info on how it should be integrated. I've been using this setup for a few months now, and it mostly works. Issues that I have, I believe are due to my setup and the language server itself being not that great, and not due to packaging. I wish there were another good option for a Java language server, but it doesn't seem there is.

@matt-snider matt-snider force-pushed the jdt-language-server branch from 29527cc to b8d412e Dec 22, 2020
@matt-snider matt-snider changed the title jdt-language-server: init at 0.62.0 jdt-language-server: init at 0.67.0 Dec 22, 2020
@matt-snider
Copy link
Contributor Author

@matt-snider matt-snider commented Dec 22, 2020

I just updated my fork and pushed that, and also upgraded to 0.67.0. Everything except for the version upgrade is how I've been running it for the past month or so, I just hadn't updated the PR. Sorry about that.

@jlesquembre
Copy link
Contributor

@jlesquembre jlesquembre commented Dec 22, 2020

I'm working in something similar:
#107424
I wanted to do the options more configurable, still working on adding gradle support .
Probably we can combine both PRs

Copy link
Contributor

@fzakaria fzakaria left a comment

The derivation at this point looks sensible enough if it's correct.
I wasn't able to try it however so mostly a visual inspection 👍

Thank you for your well wishes earlier... COVID sucks :(

, stdenv
, fetchurl
, makeWrapper
, jdk
Copy link
Contributor

@fzakaria fzakaria Dec 23, 2020

let's specify the JDK version here specifically.

Copy link
Contributor Author

@matt-snider matt-snider Jan 16, 2021

I'll set it to jdk15 since that is now in master. This is relevant to your other comment below about whether it needs to be the same version as the user's project. It doesn't, but it does need to be >= to it. So if we had this at jdk 13, then jdk 15 projects wouldn't work.

# directory each time and copy the config_{linux,mac}/ folders there
runtime_dir=''${XDG_CACHE_HOME:-$HOME/.cache}/jdt-language-server
makeWrapper ${jdk}/bin/java $out/bin/jdt-language-server \
Copy link
Contributor

@fzakaria fzakaria Dec 23, 2020

Does the language server need to be running with the same version the user is coding in ?
Gradle and Maven try using JAVA_HOME if it's set otherwise use the default JDK in the derivation.

Copy link
Contributor Author

@matt-snider matt-snider Jan 16, 2021

Yeah so as commented above, it needs to be the same version or higher. I don't know if there is a benefit to the user being able to specify the version used here. For now it's fixed to jdk15. What do you think?

pkgs/development/tools/jdt-language-server/default.nix Outdated Show resolved Hide resolved
# directory each time and copy the config_{linux,mac}/ folders there
runtime_dir=''${XDG_CACHE_HOME:-$HOME/.cache}/jdt-language-server
makeWrapper ${jdk}/bin/java $out/bin/jdt-language-server \
Copy link
Contributor

@jlesquembre jlesquembre Jan 9, 2021

IMO, just using makeWrapper make the binary too inflexible, I'd prefer to have a wrapper where I can override some of the values (e.g.: log level). My attempt for such wrapper: https://github.com/NixOS/nixpkgs/pull/107424/files#diff-cf18665ca6814490f3609b1c0c15fad90da2fd9e4d2867619e1f48ba92f2bd59

Copy link
Contributor Author

@matt-snider matt-snider Jan 16, 2021

@jlesquembre Thanks for chiming in. I took a look at your PR and made a few comments. I think the approach with the script is pretty nice, but the downside is, each new option requires a change to the script so it isn't that flexible. I think it's pretty common to use JAVA_OPTS for this kind of thing, so in this version, the user can specify JAVA_OPTS="-Xmx1g -Dlog.level=ERROR". The other options (-configuration and -data) can actually be overridden by just passing as arguments to bin/jdt-language-server.

@fzakaria Could you maybe weigh in on this? @jlesquembre has created #107424 which adds some things to this PR.

Copy link
Contributor

@jlesquembre jlesquembre Jan 17, 2021

I didn't do clear enough how to override the options with the script. I updated the comment in the script, hopefully is more clear now. With the script versin you can override any option (-data, -Xms1g, ...) without changing the script, (e.g.: jdt-ls.sh -data ... will override the -data value). I also added a new option -java-opts, which is basically doing the same that your JAVA_OPTS, but as a CLI argument (I find it a bit more convenient to use an argument when calling it from inside my editor ). If you are on java 9+ , instead of JAVA_OPTS, you could use JDK_JAVA_OPTIONS, which is supported by the jdk. One last question, I don't get how you can override the -data in your version, since you are using makeWrapper, calling bin/jdt-language-server -data ... will not expand to java ... -data /dir1 -data /dir2?

Copy link
Contributor Author

@matt-snider matt-snider Jan 18, 2021

With the script versin you can override any option (-data, -Xms1g, ...) without changing the script, (e.g.: jdt-ls.sh -data ... will override the -data value).

I know but this isn't what I meant. I meant that adding new options -Xsome-other-option would require you updating the script to support them. Now that you've added the -java-opts parameter, this has been addressed. Still, it is perhaps a little inconsistent to treat some options as special (e.g. -Xms, -Xmx, -Dlog.level), and allow them directly to be passed, while others must be passed via -java-opts. There are two different ways to specify them: jdt-ls.sh -java-opts "-Xms1g" or jdt-ls.sh -Xms1g. Not a major issue, just pointing that out.

If you are on java 9+ , instead of JAVA_OPTS, you could use JDK_JAVA_OPTIONS, which is supported by the jdk.

This is good to know, I wasn't aware. If we are sure this will be run with jdk15 then we can actually switch to that option. Otherwise, we should keep JAVA_OPTS.

One last question, I don't get how you can override the -data in your version, since you are using makeWrapper, calling bin/jdt-language-server -data ... will not expand to java ... -data /dir1 -data /dir2

makeWrapper forwards the arguments so it does expand to that! Look at the shell script produced by my derivation (do nix-build -A jdt-language-server; cat result/bin/jdt-language-server).

#! /nix/store/kl6lr3czkbnr6m5crcy8ffwfzbj8a22i-bash-4.4-p23/bin/bash -e
mkdir -p ${XDG_CACHE_HOME:-$HOME/.cache}/jdt-language-server
install -Dm 1777 -t ${XDG_CACHE_HOME:-$HOME/.cache}/jdt-language-server/config /nix/store/snncbxah6ihw50wbzhrj6jazypbzcxn4-jdt-language-server-0.67.0/share/config/*
exec "/nix/store/v5bq0y1fraw4648b48vfx5c4snfvsk8j-openjdk-15.0.1-ga/bin/java"  -Declipse.application=org.eclipse.jdt.ls.core.id1 -Dosgi.bundles.defaultStartLevel=4 -Declipse.product=org.eclipse.jdt.ls.core.product -Dlog.level=ALL -noverify $JAVA_OPTS -jar /nix/store/snncbxah6ihw50wbzhrj6jazypbzcxn4-jdt-language-server-0.67.0/share/java/plugins/org.eclipse.equinox.launcher_1.6.0.v20200915-1508.jar --add-modules=ALL-SYSTEM --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED -configuration "${XDG_CACHE_HOME:-$HOME/.cache}/jdt-language-server/config" -data "${XDG_CACHE_HOME:-$HOME/.cache}/jdt-language-server/data$PWD" "$@"

Copy link
Contributor

@jlesquembre jlesquembre Jan 18, 2021

makeWrapper forwards the arguments so it does expand to that!

I get that adding an argument it's ok, but how to overwrite an argument? Maybe I'm missing something, but if the (simplified) version of the script produced by your derivation (result/bin/jdt-language-server) is:

exec "java" -Dlog.level=ALL -jar launcher.jar -data $HOME/.cache/jdt.ls/data "$@"

running

jdt-language-server -Dlog.level=INFO -data /tmp/jdtls_data -foo bar

expands to

exec "java" -Dlog.level=ALL -jar launcher.jar -data $HOME/.cache/jdt.ls/data -Dlog.level=INFO -data tmp/jdtls_data -foo bar

been able to add the -foo arguments it's nice, but it's not possible to overwrite the -Dlog.level nor the -data arguments

I know but this isn't what I meant. I meant that adding new options -Xsome-other-option would require you updating the script to support them. Now that you've added the -java-opts parameter, this has been addressed. Still, it is perhaps a little inconsistent to treat some options as special (e.g. -Xms, -Xmx, -Dlog.level), and allow them directly to be passed, while others must be passed via -java-opts. There are two different ways to specify them: jdt-ls.sh -java-opts "-Xms1g" or jdt-ls.sh -Xms1g. Not a major issue, just pointing that out.

I agree that having multiple ways to update some arguments can be confusing. I'm not opposed to remove -Xms and -Xmx, maybe is better that way. I wanted to provide some sensible defaults, while providing an option to overwrite those values, but maybe I'm over-engineering it.
But I still think it would be a good idea to keep the -java-opts AND the JAVA_OPTS (or JDK_JAVA_OPTIONS). You will use only one, but depending where you call the command, one is more convenient. For example, in a vim script, feels more natural to use the argument, while in a shell script the env variable looks fine.

Copy link
Contributor Author

@matt-snider matt-snider Jan 18, 2021

Thanks for your reply.

been able to add the -foo arguments it's nice, but it's not possible to overwrite the -Dlog.level nor the -data arguments

I'm not sure if we are talking about two different things. All I meant was that the rightmost argument will override previous values, so the second instance of -data wins. You can try this out by setting a different value and seeing that jdt ls creates the files there.

Copy link
Contributor

@jlesquembre jlesquembre Jan 18, 2021

Now I get it, thanks for clarifying it. I didn't know that the second argument wins. Is that always the case? Not sure if we can rely on that behavior, I didn't find anything about it in jdt.ls documentation.
What about JAVA_OPTS? It also looks like that the second option has preference here, but it seems fragile, I think that relying on the order of the options is not a good idea.

@matt-snider matt-snider force-pushed the jdt-language-server branch from b8d412e to 7e9a80b Jan 16, 2021
@matt-snider
Copy link
Contributor Author

@matt-snider matt-snider commented Jan 16, 2021

@fzakaria Thank you for reviewing this and sorry about the very late response. Things got a bit busy.

# -Declipse.application=org.eclipse.jdt.ls.core.id1
# -Dosgi.bundles.defaultStartLevel=4
# -Declipse.product=org.eclipse.jdt.ls.core.product
Copy link
Contributor

@jlesquembre jlesquembre Jan 17, 2021

I think that the documentation is not up-to-date here. The config.ini file provided upstream, already sets those value, I guess we can remove those options.

@matt-snider matt-snider force-pushed the jdt-language-server branch from 7e9a80b to 3592510 Jan 18, 2021
--add-flags "--add-modules=ALL-SYSTEM" \
--add-flags "--add-opens java.base/java.util=ALL-UNNAMED" \
--add-flags "--add-opens java.base/java.lang=ALL-UNNAMED" \
--add-flags "-configuration \"${runtimePath}/config\"" \
Copy link
Contributor

@jlesquembre jlesquembre Jan 18, 2021

Since the logs are also written here, shouldn't be use a different directory per project? In case we have 2 instances of the LS running at the same time

@matt-snider matt-snider mentioned this pull request Feb 7, 2021
10 tasks
@winston0410
Copy link
Contributor

@winston0410 winston0410 commented Jul 24, 2021

Is there any update one this one? I need jdtls and it seems like this PR is working yet not merged?

@hqurve hqurve mentioned this pull request Aug 2, 2021
11 tasks
@Jumziey
Copy link

@Jumziey Jumziey commented Sep 12, 2021

I just tried out the implementation and it seems to work fine in nixos, just updated the version for 1.3.0. version. Only just fired it up yet and tried a little bit of mashing, can follow up.

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

Successfully merging this pull request may close these issues.

None yet

6 participants