Add Package Manager Friendly Start Up Script #2005
Conversation
|
Can one of the admins verify this patch? |
|
@languitar, as you are the maintainer of the AUR jdtls version and I'm trying to bring it to Homebrew, your feedback is highly appreciated (at least from my point of view). |
|
On first sight this looks good. Apart from the magic of finding java etc. this looks a lot like what I use in my AUR package. |
There was a problem hiding this comment.
I'm not a maintainer but noticed a couple of things:
- I think
pythonmight still refer to python2 on some systems, which would break because at leastpathlibwouldn't be available. Some afaik also don't create one at all - but onlypython3(?) - The
Running from the command linesection in the readme would probably need an update. - I think on windows it is still not uncommon to not have python at all?
- This would remove the option to easily configure heap size and GC. Not sure if that's a common use case, just want to point it out.
- I suspect the version check would fail if using early access releases. Not sure if this is common enough to support - but just to mention it.
❮ ./.local/jdks/jdk-17ea-panama/bin/java -version
openjdk version "17-panama" 2021-09-14
OpenJDK Runtime Environment (build 17-panama+3-167)
OpenJDK 64-Bit Server VM (build 17-panama+3-167, mixed mode, sharing)
~
❮ ./.local/jdks/jdk-19ea-panama/bin/java -version
openjdk version "19-panama" 2022-09-20
OpenJDK Runtime Environment (build 19-panama+1-13)
OpenJDK 64-Bit Server VM (build 19-panama+1-13, mixed mode, sharing)
I am not sure about the output of builds other than OpenJDK?
With that in mind - I think a start-up script would in general be a nice addition.
schrieveslaach
force-pushed
the
start-up-script
branch
from
8474769
to
2bb1cbc
Compare
|
@mfussenegger, @languitar, thanks for your feedback. I'll look into this. I already added |
schrieveslaach
force-pushed
the
start-up-script
branch
from
2bb1cbc
to
9220919
Compare
schrieveslaach
force-pushed
the
start-up-script
branch
2 times, most recently
from
184aaff
to
bef2456
Compare
|
@languitar, @mfussenegger, I've updated this PR and I took your comments into account. Do you mind to have another look? |
There was a problem hiding this comment.
Many users have noted this would be really good to have, so I think it's worth adding. @testforstephen let me know what you think about this.
Note that the script is not currently packaged in the final tarball at org.eclipse.jdt.ls.product/distro/jdt-language-server-1.9.0-202202221741.tar.gz, but I guess this is fine as it's meant as more a developer utility.
There was a problem hiding this comment.
I understand adding a startup script would bring some value. As a Java developer, I'm more used to add wrapper scripts using maven wrapper way, that's more clean and no extra dependencies.
However, if the target audience of this PR is mainly for the automated Package Manager, using python might be fine. To keep the implementation simple, i'd like to remove the extra python dependencies for defusedxml and semver. And also don't handle lombok.jar in the startup of language server, since Lombok support is out of scope of eclipse.jdt.ls project.
schrieveslaach
force-pushed
the
start-up-script
branch
from
bef2456
to
086cfd1
Compare
|
I updated the PR and addressed the suggestions
@languitar, moved to pathlib
@rgrunber, actually I intended to ensure that the script will be part of the final
@testforstephen, I remove the Python packages and removed the Lombok option (I think I'll write a Neovim plugin to resolve Lombok that can be used in conjunction with @mfussenegger's I've added some other command line option to specify additional Java properties to be able to specify the Lombok Java agent.
@testforstephen, is it completely out of scope (I'm asking out of curiosity)? I've discovered an issues while loading the lombok Java agent and I would like to report those. |
schrieveslaach
force-pushed
the
start-up-script
branch
2 times, most recently
from
87c48ca
to
d05c137
Compare
schrieveslaach
force-pushed
the
start-up-script
branch
2 times, most recently
from
795609f
to
e51c34c
Compare
|
@testforstephen looks like issues have been addressed here. Have a look and let me know if you're fine with merging. |
There was a problem hiding this comment.
Once the license header issue @rgrunber commented has been addressed, it's OK to merge.
schrieveslaach
force-pushed
the
start-up-script
branch
from
e51c34c
to
6b0f166
Compare
|
@schrieveslaach thanks for taking the time to address all the suggestions. |
|
Thanks for merging. 🙂 |
|
|
||
| def main(args): | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("--validate-java-version", default=True, action=argparse.BooleanOptionalAction) |
There was a problem hiding this comment.
argparse.BooleanOptionalAction is only available in python 3.9. So essentially everyone using python version < 3.9, get the above error.
There was a problem hiding this comment.
Issue has been raised in #2153
Introducing a package manager friendly start up script to make the handling of jdt.ls easier (fixes #1823 and fixes #1934).
Why did I choose Python and not bash? Basically to address the following concern:
The script has been written to be platform agnostic so that it can be used on Window, Linux, and Mac. So there is no need to write separate scripts for each platform.
It also adds some logic to bootstrap Lombok out of the box. This feature could be hidden behind a command line option.