Find compatible GradleJavaHome and notify client in case of incompatibility #165
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdneo
reviewed
Jul 8, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
Tanish-Ranjan
changed the title
|
To check if the selected JDK is compatible or not we are now comparing the feature release of the Java Version. Previously, if we retrieved the latest compatible JDK version of say |
jdneo
reviewed
Jul 15, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/gradle/GradleApiConnector.java
Outdated
Show resolved
Hide resolved
jdneo
reviewed
Jul 16, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/utils/JavaUtils.java
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
jdneo
reviewed
Jul 17, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/gradle/GradleApiConnector.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/gradle/GradleApiConnector.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
|
The JDK lookup for gradle seems to be different from the way it is listed in the docs. Priority order I found out:
|
jdneo
reviewed
Jul 17, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
jdneo
reviewed
Jul 22, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
jdneo
reviewed
Jul 22, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
jdneo
reviewed
Jul 23, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/services/LifecycleService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/utils/JavaUtils.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/utils/JavaUtils.java
Show resolved
Hide resolved
server/src/main/java/com/microsoft/java/bs/core/internal/utils/JavaUtils.java
Outdated
Show resolved
Hide resolved
jdneo
reviewed
Jul 23, 2024
...c/test/java/com/microsoft/java/bs/core/internal/server/BuildTargetServerIntegrationTest.java
Outdated
Show resolved
Hide resolved
|
Hi @Tanish-Ranjan, The PR has some conflicts with the develop branch now. Could you please resolve them? |
Tanish-Ranjan
force-pushed
the
fix/76-notify-incompatible-java-home
branch
from
8ed0125 to
bb3df6d
Compare
jdneo
reviewed
Jul 29, 2024
server/src/main/java/com/microsoft/java/bs/core/internal/gradle/GradleApiConnector.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/microsoft/java/bs/core/internal/server/IntegrationTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/microsoft/java/bs/core/internal/server/IntegrationTest.java
Outdated
Show resolved
Hide resolved
jdneo
reviewed
Jul 30, 2024
...rc/test/java/com/microsoft/java/bs/core/internal/server/LifecycleServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
|
Prioritizing Gradle default behavior for java home detection with 3b68887 |
Changes: - Added a ClientNotifier.java, a utility class for sending notifications to the client. - LifecycleService now keeps a reference to the BuildClient for sending notifications. - Updated Launcher.java to set BuildClient on LifecycleService.java - Added method in utils.java to return the lease compatible Java version. - Added unit tests in BuildTargetServerIntegrationTest.java to check if the compatibility notifications are returned properly.
Changes: - Added method to retrieve the GradleJavaHome for the given project. - Added a new utility class JavaUtils. It contains methods to extract the java version from the JDK file and check java version compatibility. - Added new test class JavaUtilsTest containing unit tests for methods in JavaUtils. - Updated LifecycleService to check for GradleJavaHome compatibility before checking JDKs in the preferences.
Changes: - Updated LifecycleService#getJdkToLaunchDaemon to use feature release for check with selected version. - Refactored JavaVersion variables for consistency in naming. - Added unit test for retrieving the oldest compatible java version. - Updated tests for UserJavaHome to prevent the use of GradleJavaHome for receiving the correct error message on client.
Changes: - Removed methods for adding originId and taskId to the notification in ClientNotifier. - Refactored sendNotification to be static and accept the BuildClient directly in the parameter. - Added a new test case to JavaUtilsTest verifying if IOException is properly thrown in case of non-existent executable.
…egating to ClientNotifier.
…s and flattened the logic. Changes: - Updated JavaDocs in GradleApiConnector. - Removed getGradleJavaHome from GradleApiConnector. - Added getBuildEnvironment method to extract the BuildEnvironment variable directly from the GradleApiConnector. - Made JavaUtils constructor private and added throws and @nonnull annotations to prevent unwanted exceptions. - Broke down and flattened the logic for getSuitableJdk from LifecycleService into two separate methods. - Added getGradleVersion method in LifecycleService to extract the gradle version for the given project. - Updated getSuitableJdk to getGradleCompatibleJdk to contain only the java home selection logic. - Added a new method setGradleJavaHome which calls the previous methods to select and set the java home and send notification to the client upon failure.
Changes: - Removed BuildTargetServerIntegrationTest - Added IntegrationTest. Base class for integration tests containing the setup. - Added BuildTargetServiceIntegrationTest. Subclass of IntegrationTest responsible for handling build target integration tests. - Added LifecycleServiceIntegrationTest. Subclass of IntegrationTest responsible for handling lifecycle integration tests.
Changes: - Moved methods from IntegrationTest to LifecycleService and BuildTargetService IntegrationTest to keep only the common methods needed by both integration tests.
…compatibility. Changes: - Added new method to check compatibility of Java Home with a probe build in GradleApiConnector - Refactored LifecycleService#setGradleJavaHome to prioritize the default gradle configuration and send a notification to the client if the default behavior is changed. - Added new test for java home detection with gradle default behavior.
Tanish-Ranjan
force-pushed
the
fix/76-notify-incompatible-java-home
branch
from
3b68887 to
cad7692
Compare
This was referenced Aug 24, 2024
This was referenced Sep 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addressing issue 75 and issue 76