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

Upgrade to Java 11 #39

Closed
valentjn opened this issue May 7, 2020 · 8 comments
Closed

Upgrade to Java 11 #39

valentjn opened this issue May 7, 2020 · 8 comments
Assignees
Labels
1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature 3-fixed Issue resolution: Issue has been fixed on the develop branch
Milestone

Comments

@valentjn
Copy link
Owner

valentjn commented May 7, 2020

Is your feature request related to a problem? Please describe.
Oracle's support for Java 8 is scheduled to be dropped by the end of 2020. Other Java projects such as the Eclipse platform are moving to Java 11, and LTEX must do this as well.

Describe the solution you'd like
Some kind of transition process has to be worked out. #38 shows what happens if we don't do this. Nag screens could help push users towards Java 11. In addition, some documentation on the upgrade process is necessary, and it should be stated prominently in the readme.

Describe alternatives you've considered
Dropping Java altogether, see #5. However, this seems like an even bigger task.

Additional context
None.

@valentjn valentjn added the 1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature label May 7, 2020
@valentjn
Copy link
Owner Author

valentjn commented Jun 1, 2020

I think solving this issue has become easier with LTEX 5.0.0 as it downloads Java if necessary. The version being downloaded is already Java 11. Currently, Java is only downloaded if it can't be found on the system.

So we could reverse this in the future: Only use the system-wide Java if it's 11+, otherwise download Java 11 and show a nag notification that the user needs to update their Java installation to prevent LTEX from downloading it after each update. If the download fails for whatever reason, we could fallback to the Java 8 installation on the system and show an even more urgent message that LTEX will stop working if the user doesn't update their installation.

valentjn added a commit that referenced this issue Jul 19, 2020
@valentjn valentjn self-assigned this Jul 27, 2020
@valentjn
Copy link
Owner Author

valentjn commented Jul 27, 2020

@valentjn valentjn pinned this issue Aug 11, 2020
@tobiasdiez
Copy link

One way forward would be to bundle Java ltex-ls via jlink. This gives a minimal Java runtime needed to run the language tool server, no extra download of the JDK necessary. Moreover, you have complete control which Java version you want to ship, leading to reproducible user environments. The disadvantage is that the build process is a bit more involved. For an example github action workflow, have a look at https://github.com/JabRef/jabref/blob/2d92025db43082434d9fe45693420fcd4808b959/.github/workflows/deployment.yml#L85.

@valentjn
Copy link
Owner Author

valentjn commented Oct 3, 2020

@tobiasdiez Thanks for the suggestion, and thanks for the link. I didn't know about jlink. However, I think it was introduced with Java 9, so I guess it can't be used for the migration.

But I'm not sure about the advantages. vscode-ltex doesn't download a JDK, only a JRE, which is only 40MB in size. The large thing to be downloaded is ltex-ls, which includes just the necessary JARs, but is still 180MB. It wouldn't matter to me if LTEX had to download one file of 220MB, or two files of 220MB combined (it does matter for an application like JabRef though). Plus, the versions for offline installation of LTEX already are single-file.

The download URL of the JRE (AdoptOpenJDK/Adoptium on GitHub) is hardcoded into vscode-ltex, so there's already full control over the JRE version. This is not true if the user has a working version of Java installed. However, the system-wide Java is only used by vscode-ltex if ltex-ls successfully starts. Otherwise, the hard-coded JRE will be downloaded and used instead. I suppose this is the reason that only few issues here are about Java itself.

I did try it though, but ltex-ls migrated from Gradle to Maven, and Apache's outdated Maven jlink plugin (stuck at 3.0.0-alpha-1 since 2017) immediately produces a NullPointerException due to MJLINK-4. The bug is supposed to be resolved in 3.0.0-alpha-2-SNAPSHOT, but fetching this version manually (and depending on a snapshot version) doesn't sound like a sustainable solution for LTEX.

@tobiasdiez
Copy link

However, I think it was introduced with Java 9, so I guess it can't be used for the migration.

I'm not sure what you mean with migration in this context. The point of jlink is to take only the modules from the jdk (and other dependencies if they are modularized), and bundle them together in a custom java runtime that includes everything you need to run ltex but not more. So you can link it again Java 9, or Java 15, or whatever Java version you want to use.

Strictly speaking, there are no JRE's anymore. What AdoptOpenJDK provides is a JDK bundled with jlink using a static list of modules to include (see https://stackoverflow.com/questions/61232071/is-adoptopenjdks-jre-11-the-same-as-using-jlink-on-jdk-adding-all-dependencie). So if you use jlink for ltex-ls, you will get something that is smaller in size than ltex + jre. Chances are that it has even less than the 180MB because it can strip parts of the dependencies that are not needed (that depends on the dependencies through).

I've have no experience with jlink under Maven. But you should be able to add the snapshot repository and then use the following version.
https://repository.apache.org/content/repositories/snapshots/org/apache/maven/plugins/maven-jlink-plugin/3.0.0-alpha-2-SNAPSHOT/
It's still a snapshot, so that's indeed a bit unfortunate.

If you are fine with downloading the JRE separately, then there might be no need for jlink indeed. But then I also don't get the point of this ticket as you already shipping Java 11...

@valentjn
Copy link
Owner Author

valentjn commented Oct 3, 2020

I didn't formulate the description of this issue clearly enough. LTEX currently supports running with Java 8+. This issue is a feature request to upgrade this requirement to Java 11+.

The fact/feature that LTEX downloads Java 11 for you if no Java 8+ has been found on the system (a) doesn't change that LTEX still supports Java 8 if it's already installed, and (b) is newer than the issue (as written above; LTEX used to not download anything). The issue will be closed as soon as LTEX requires Java 11 to run (whether pre-installed or automatically downloaded doesn't matter), which will be on November 1.

I appreciate your input though, and I'll give it another shot :)

@valentjn
Copy link
Owner Author

valentjn commented Oct 12, 2020

@tobiasdiez After spending a few hours to convince Maven to run the snapshot version of maven-jlink-plugin (and succeeding), it turns out the structure of LanguageTool prohibits the use of jlink (Modules language.da and language.pl export package org.languagetool.language to module morfologik.fsa.builders). Apparently, different JAR modules must not have classes that are in the same package, which is exactly what LT does at least with its org.languagetool.language package, e.g., language.da and language.pl.

As the advantages of using jlink over the current situation are small for LTEX, as this would be quite a big non-backward-compatible refactoring for LT, and as there's no guarantee that jlink will even work after fixing this, I think I'll just leave it as it is.

@valentjn valentjn added this to the 8.0.0 milestone Oct 17, 2020
@valentjn
Copy link
Owner Author

As the upgrade is already in LTEX LS's develop (valentjn/ltex-ls@80875bdc6d), this can be closed now. The first version to require Java 11 will be LTEX 8.0.0.

@valentjn valentjn added the 3-fixed Issue resolution: Issue has been fixed on the develop branch label Oct 27, 2020
@valentjn valentjn unpinned this issue Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature 3-fixed Issue resolution: Issue has been fixed on the develop branch
Projects
None yet
Development

No branches or pull requests

2 participants