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

Split Entries for JDK 8 and 11 #38

Closed
JoelProminic opened this issue Mar 3, 2021 · 12 comments
Closed

Split Entries for JDK 8 and 11 #38

JoelProminic opened this issue Mar 3, 2021 · 12 comments
Assignees
Labels
enhancement New feature or request in-progress
Milestone

Comments

@JoelProminic
Copy link
Contributor

JoelProminic commented Mar 3, 2021

We want to start using JDK 11 as the default JDK for Moonshine to better support the language server. However, we have some features and pending features that use the HCL Domino API, which requires JDK 8.

To support this, we want to have two entries for Java in Moonshine SDK Installer:

Label Required For Notes
OpenJDK Java Development, Editor Enhancements, Flex/ActionScript Compilation and Debug, Feathers Compilation, Apache Ant Integration, PrimeFaces Project Preview This will be JDK 11.
OpenJDK 8 Domino Support

I was surprised that the The "Java Development" feature is new - it seemed odd that this was not added before. Here is a description:

Compile Java Applications

We will need to store the two SDKs separately in Moonshine. We can discuss the options for this in a Moonshine issue.

@JoelProminic JoelProminic added the enhancement New feature or request label Mar 3, 2021
@JoelProminic JoelProminic added this to the v3.7.0 milestone Mar 3, 2021
rat-moonshine added a commit that referenced this issue Mar 4, 2021
- Changed existing JDK to download JDK11
- Added new Java Development feature
- Added version number to entries
(#39, #38)
@rat-moonshine
Copy link
Collaborator

rat-moonshine commented Mar 4, 2021

So a new feature entry is being added - Java Development. Currently this adds into this ordering, we can change this though:
image

Addition of the newer JDK 8 entry now looks like this, when the existing entry now pointed to download JDK 11:
image

Version number also added #39 .

The only problem that currently I'm seeing is download-ability. Currently, the mechanism to determine if Java installed (and adding a downloaded tick) works in following ways:

  1. Checks if already setup in Moonshine Settings (when running inside Moonshine)
  2. Check by the named directory under the MoonshineSDKs
  3. Check if the JAVA_HOME exists in system environment variable

Since I have JAVA_HOME added to environment variable it immediately marks both the JDK entries as downloaded (and need not any further action). This maybe Okay against the usual JDK entry, but for JDK8 user may requires to have it downloaded explicitly, for which we may avail some option like re-download (in case of Royale Nightly) or something.

Thoughts are welcome.

@rat-moonshine
Copy link
Collaborator

rat-moonshine commented Mar 4, 2021

Currently, we don't have a version check mechanism for Java, and neither for any other SDK other than Flex. This may create problem when:

  • User already has JAVA_HOME set to v8.0
  • We wants to push v11.0
  • SDK Installer marked the JDK-entry is present based on the JAVA_HOME present
  • JDK v11.0 entry do not have a download button

@JoelProminic
Copy link
Contributor Author

Moonshine SDK Installer will need to check the Java version at least.

If there are Java directories in MoonshineSDKs, iterate through them and:

  • Assign the latest version >= 11.0 to the OpenJDK entry
  • Assign the latest version of Java 1.8 to the OpenJDK 8 entry
  • Ignore versions < 1.8

Then, check JAVA_HOME

  • if the version is > 11.0, then assign it to OpenJDK if it is not assigned already
  • if the version is 1.8, then assign it to OpenJDK 8 if it is not assigned already
  • if the version is < 1.8, then ignore it.

If OpenJDK or OpenJDK 8 does not have a matching JDK assigned, then they should be conidered uninstalled.

rat-moonshine added a commit that referenced this issue Mar 5, 2021
rat-moonshine added a commit to Moonshine-IDE/Moonshine-IDE that referenced this issue Mar 8, 2021
- Have multiple JDK path settings
- Update JDK paths based on InstallerSharedCore detections
(Moonshine-IDE/Moonshine-SDK-Installer#38)
@rat-moonshine
Copy link
Collaborator

rat-moonshine commented Mar 8, 2021

Some updates until now:

  • Moonshine now has two pointers to setup JDK-default (v11) and JDK v8
  • MSKDI updated to detect availability of the JDK versions based on the logic it has, and discussed recently
  • Both the JDK path/settings auto-setup by MSDKI codes if applicable
  • Based on the found Java-version MSDKI set the appropriate JDK path in Moonshine settings

This still in a starting stage of the development, problems may found, and needs more tests.

@JoelProminic
Copy link
Contributor Author

JoelProminic commented Mar 8, 2021

I did a quick test with the standard use case on macOS, and the Moonshine SDK Installer use cases look good. The Installer logic also worked.

Here is a test matrix based on the logic we discussed above. Let me know if there are any missing cases, or if some cases are not possible to test:

OS MoonshineSDKs Environnment OpenJDK OpenJDK 8 Test Results
macOS JDK 8 only N/A Missing Installed Confirmed
macOS JDK 6 only N/A Missing Installed
macOS Both JDK 8 and 11 N/A Installed Installed Confirmed
macOS JDK 11 only N/A Installed Missing Confirmed
macOS No JDK N/A Missing Missing Confirmed
Windows JDK 8 only Not Set Missing Installed Confirmed
Windows JDK 8 only JDK 6 Missing Installed (8) Confirmed
Windows JDK 8 only JDK 11 Installed Installed Confirmed
Windows JDK 11 only Not Set Installed Missing Confirmed
Windows JDK 11 only JDK 8 Installed Installed Confirmed
Windows JDK 11 only JDK 15 Installed (11) Missing Confirmed
Windows Both JDK 8 and 11 Any Installed Installed Confirmed
Windows Both JDK 8 and 11 Not Set Installed Installed Confirmed
Windows No JDK JDK 6 Missing Missing Confirmed
Windows No JDK JDK 15 Installed Missing Confirmed

I excluded the cases where different versions of the JDK appear in MoonshineSDKs, since this shouldn't happen without manual intervention from the user.

The logic was not working like I expected on the Moonshine side, but we'll discuss this in Moonshine-IDE/Moonshine-IDE#755

@rat-moonshine
Copy link
Collaborator

rat-moonshine commented Mar 9, 2021

I have updated the above test-matrix with more entries I could think about, with some fixes.

However, I couldn't able to get a full tests on macOS since download problem of OpenJDK versions. While I tried to download v11 or v15 from https://adoptopenjdk.net/releases.html site, resulting tar.gz file always extracts a single file instead of a directory in my tests.

I see the resulting file has 'bin' folder inside it, if I right-click on the file and chose "Show Package Content". But extract-out those files on a directory and if try running on Terminal (i.e. java -version), macOS refused to run the executable.

For now, I leave the macOS tests at my side.

@JoelProminic
Copy link
Contributor Author

I noticed that the "Required for" list for OpenJDK 8 is: Java Development, Domino Support

I'd like to keep OpenJDK as the only requirement for Java Development, so that the user doesn't feel like they need to install both if they don't specifically need JDK 8.

@JoelProminic
Copy link
Contributor Author

In my macOS tests, I see that OpenJDK 8 now installs with this path:

~/Downloads/MoonshineSDKs/Java/openjdk-1.8.0/Contents/Home/bin/java

This causes the OpenJDK 8 check to fail, since it expects this path

~/Downloads/MoonshineSDKs/Java/openjdk-1.8.0/bin/java

The OpenJDK (11) install matches the second pattern above.

rat-moonshine added a commit that referenced this issue Mar 11, 2021
…ture

- Minor textual changes in description
(#38)
@rat-moonshine
Copy link
Collaborator

rat-moonshine commented Mar 11, 2021

I have fixed the problem where JDK 11 was unzipping properly but JDK 8 - and the resulting JDK 8 directory left an .app file like structure.

Here is some notes on the fix, I see I did some weird handling for the JDK particularly where it involves looking into Contents/Home structure - https://github.com/prominic/Moonshine-SDK-Installer/blob/features/issue_38_jdk11/MoonshineSDKInstaller/src/components/HelperInstaller.mxml#L344. Until now it was triggering for the default-JDK only, once I added JDK8 into the condition it also starts downloading in an expected way.

I'd like to keep OpenJDK as the only requirement for Java Development, so that the user doesn't feel like they need to install both if they don't specifically need JDK 8.

I have removed JDK8 'Java Development' criteria.

@JoelProminic
Copy link
Contributor Author

I think this logic is working well. It should be fine to merge this, as long as it is not a problem if the Moonshine-IDE/Moonshine-IDE#755 changes are not merged yet - that issue needs more work.

@piotrzarzycki21
Copy link
Collaborator

I'm not sure why we have miss that during our review. This seems to be ready. Can we close it ?

@JoelProminic
Copy link
Contributor Author

Yeah, the MSDKI side of this seems fine, though there are some remaining issues on the Moonshine side (Moonshine-IDE/Moonshine-IDE#755).

@piotrzarzycki21 piotrzarzycki21 modified the milestones: v3.7.0, v3.8.0 May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in-progress
Projects
Development

No branches or pull requests

3 participants