Skip to content

Comments

java-language-server: init at 0.2.38#131262

Merged
SuperSandro2000 merged 5 commits intoNixOS:masterfrom
hqurve:jlt
Aug 10, 2021
Merged

java-language-server: init at 0.2.38#131262
SuperSandro2000 merged 5 commits intoNixOS:masterfrom
hqurve:jlt

Conversation

@hqurve
Copy link
Contributor

@hqurve hqurve commented Jul 23, 2021

Motivation for this change

Something I would like, also its a packaging request closes #128999

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/)
  • 21.11 Release Notes (or backporting 21.05 Relase notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@hqurve
Copy link
Contributor Author

hqurve commented Jul 23, 2021

Tested on linux using neovim with lspconfig

require'lspconfig'.java_language_server.setup {
    cmd = {"/nix/store/spnkfdwa630vfv3zqdwix0qa2zy5752x-java-language-server-0.2.38/share/java/java-language-server/lang_server_linux.sh"},
    on_attach=on_attach,
    root_dir= require'lspconfig/util'.path.dirname,
}

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jul 23, 2021
@hqurve hqurve force-pushed the jlt branch 2 times, most recently from 2b9d0b6 to bfd1f4d Compare July 23, 2021 18:10
@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 131262 at bfd1f4db run on x86_64-linux 1

1 package built successfully:
  • java-language-server
1 suggestion:
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/development/tools/java/java-language-server/default.nix:68:3:

       |
    68 |   installPhase = ''
       |   ^
    

@hqurve
Copy link
Contributor Author

hqurve commented Aug 2, 2021

@houstdav000 @winston0410 Would you mind testing the derivation in your editor of choice? It works for me on neovim (with lspconfig) on nixos but I dont know if I may have missed something.

All you would have to do is copy the java-language-server/default.nix to some location in your configuration and then add environment.systemPackages = [(pkgs.callPackage ./path/to/file {})] to your configuration. You can of course add it to a shell.nix too but this is just an example.

By default, there is an executable called java-language-server (which in turn calls dist/lang_server_{platform}.sh) on your path.

Thanks

@winston0410
Copy link
Contributor

winston0410 commented Aug 2, 2021

@hqurve I am using the exact same config with you, and it is working correctly!👍👍

Btw I have asked the java reddit, and they seems to prefer jdtls over this language server. I will be grateful if you could package that as well.

@winston0410
Copy link
Contributor

This is the related issue for jdtls, I am not good enough in nix to understand why it is stopped right now.

#97814

@hqurve
Copy link
Contributor Author

hqurve commented Aug 2, 2021

@hqurve I am using the exact same config with you, and it is working correctly!+1+1

Great

Btw I have asked the java reddit, and they seems to prefer jdtls over this language server. I will be grateful if you could package that as well.

This is the related issue for jdtls, I am not good enough in nix to understand why it is stopped right now.
#97814

There are currently two PRs, #99330 and #107424, opened for this; although that its for an older version (I think the current one is 1.2.0). Regardless, I would look at it (even if its just for fun)

@roberth roberth added the 6.topic: java Including JDK, tooling, other languages, other VMs label Aug 8, 2021
@roberth roberth requested a review from raboof August 8, 2021 14:30
@raboof
Copy link
Member

raboof commented Aug 9, 2021

Result of nixpkgs-review pr 131262 run on x86_64-linux 1

1 package built:
  • java-language-server

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. 2 small comments. I wonder if we shouldn't put energy into packaging jdtls instead, if that is preferred over this, though :)

Comment on lines 84 to 88
cat << _EOF > $out/bin/java-language-server
#!${runtimeShell}
$out/share/java/java-language-server/lang_server_${platform}.sh "\$@"
_EOF
chmod +x $out/bin/java-language-server
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this needs to be a script and can't be a symlink?

Copy link
Contributor Author

@hqurve hqurve Aug 9, 2021

Choose a reason for hiding this comment

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

Yeah, I mentioned it in the comment in the code, but I guess I should expand. Suppose it was a symlink and consider this line in lang_server_linux.sh. According to my testing and this answer, $0 would refer to the symlink and not the target file. Therefore, dirname $0 would evalate to /nix/store/...java-language-server/bin instead of ...../java-language/server/share/java/java-language-server and as you can see this would cause issues in the rest of the code.

Initially, I tried to symlink it but when I noticed this, I only saw two solutions, have a script like how it is done in the AUR or patch the files. I wasn't sure how to properly patch the files in a simple manner, so I resorted to a script.

Please let me know if there is better way to solve this issue (perhaps using symlinks)

Copy link
Member

@raboof raboof Aug 9, 2021

Choose a reason for hiding this comment

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

Doh, I should have read more closely :).

perhaps wrapProgram/makeWrapper might be slightly nicer, but that's a matter of taste.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didnt know such a tool existed

buildPhase = ''
runHook preBuild

mvn package -Dmaven.repo.local=$out/.m2 -DskipTests
Copy link
Member

Choose a reason for hiding this comment

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

This is slightly different from the example at https://nixos.org/manual/nixpkgs/unstable/#double-invocation - any reason to use $out/.m2 rather than just $out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at it too, none that I could think of. (Perhaps its an artifact of when I was having issues trying to get it to build) Ill change it to just $out.

Also, the reason to skip the tests, the build instructions say to skip them and indeed, I was getting an error when I let them run. (Ill attempt to build it to show you the error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran the tests and I believe all of the errors were caused by a module not being exported

java.lang.IllegalAccessError: superclass access check failed: class org.javacs.ReusableCompiler$ReusableContext (in unnamed module @0xb6a0f7) cannot access class com.sun.tools.javac.
util.Context (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.util to unnamed module @0xb6a0f7

I believe this problem could be mitigated by adding

<arg>--add-exports</arg>
<arg>jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>

to each of the dependency compilerArgs in pom.xml but ... ill wait to see what you say

@hqurve
Copy link
Contributor Author

hqurve commented Aug 9, 2021

Yeah, I agree jdtls should have higher priority than this, but by the time when I realized, it was already too late.

@SuperSandro2000 SuperSandro2000 merged commit ca2cc06 into NixOS:master Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: java Including JDK, tooling, other languages, other VMs 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

java-language-server: Package Request

6 participants