You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/08/18 22:26:16 UTC
[GitHub] [maven-surefire] CMoH opened a new pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components
CMoH opened a new pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387
Trivial fix for jira issue https://issues.apache.org/jira/browse/SUREFIRE-1939
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r748755474
##########
File path: surefire-booter/src/main/java/org/apache/maven/surefire/booter/SystemUtils.java
##########
@@ -88,7 +88,7 @@ public static boolean endsWithJavaPath( String jvmExecPath )
public static File toJdkHomeFromJvmExec( String jvmExecutable )
Review comment:
Do we have unit test for it ... ?
If not please create - it should be easy
Unit test should coverage all cases described in java doc for this method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] CMoH commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components
Posted by GitBox <gi...@apache.org>.
CMoH commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r748766515
##########
File path: surefire-booter/src/main/java/org/apache/maven/surefire/booter/SystemUtils.java
##########
@@ -88,7 +88,7 @@ public static boolean endsWithJavaPath( String jvmExecPath )
public static File toJdkHomeFromJvmExec( String jvmExecutable )
Review comment:
I feel the fix is more complete now. Thanks for the comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] CMoH commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components
Posted by GitBox <gi...@apache.org>.
CMoH commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r773444482
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SystemUtilsTest.java
##########
@@ -98,6 +98,15 @@ public void incorrectJdkPath()
assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
}
+ @Test
+ public void incorrectJdkPathShouldNotNPE()
Review comment:
Well, the reason I submitted this is because unit tests fail, due to this: on my machine JAVA_HOME is `/opt/openjdk-bin-11/`. Unit test `incorrectJdkPath()` works as follows:
```
@Test
public void incorrectJdkPath()
{
File jre = new File( System.getProperty( "java.home" ) );
File jdk = jre.getParentFile();
File incorrect = jdk.getParentFile();
assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
}
```
This leads to `incorrect=/`, which reaches `toJdkHomeFromJvmExec("/")`, which calls again `getParent("/")`, which yields null, and next `getName()` is called on this null. It is clear from the implementation of `toJdkHomeFromJvmExec()` that returning null is fine when this jvmExec is not found. I believe this is the case here.
Also, the test is about incorrect JDK paths, and such a path is an incorrect JDK path. However, one incorrect case crashes with a non-informative NPE. I would say a "cannot find jvm executable" error on the return-null path would be more helpful for users that misconfigured their systems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r773116452
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SystemUtilsTest.java
##########
@@ -98,6 +98,15 @@ public void incorrectJdkPath()
assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
}
+ @Test
+ public void incorrectJdkPathShouldNotNPE()
Review comment:
is `jvmExecutable` set to `/opt/jdk` instead of `/opt/jdk/bin/java` in your case? this test is written to make `jdk=/opt` and `jre=/opt/jdk/` which I can understand you want a case with a single segment path but is not aligned on the description and only possible if the jdk is extracted at the root of the filesystem, is it your case? Also it means the jvmExecutable is not the executable so something is wrong higher in the processing and must be fixed preventing this NPE later so I would rather chase this cause instead of swallowing it.
Any inputs on these points?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r789267882
##########
File path: surefire-booter/src/test/java/org/apache/maven/surefire/booter/SystemUtilsTest.java
##########
@@ -98,6 +98,15 @@ public void incorrectJdkPath()
assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
}
+ @Test
+ public void incorrectJdkPathShouldNotNPE()
Review comment:
@CMoH
We can accept this fix for the unit test but more important would be to back track the callers in `AbstractSurefireMojo` and see if this NPE may happen and may have some influence, and where if any.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] CMoH commented on pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components
Posted by GitBox <gi...@apache.org>.
CMoH commented on pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#issuecomment-903189013
The MacOS failure seems unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] CMoH commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components
Posted by GitBox <gi...@apache.org>.
CMoH commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r748766331
##########
File path: surefire-booter/src/main/java/org/apache/maven/surefire/booter/SystemUtils.java
##########
@@ -88,7 +88,7 @@ public static boolean endsWithJavaPath( String jvmExecPath )
public static File toJdkHomeFromJvmExec( String jvmExecutable )
Review comment:
No, I didn't feel the need to add a new unit test since the existing unit test fails with NPE under the described conditions (i.e JAVA_HOME set to a directory 2 levels up from root, `/opt/openjdk-bin-11.0.11_p9` in my case).
However, I have added a system-independent unit test to showcase the situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org