search

scalatest / scalatest-maven-plugin

ScalaTest Maven Plugin
Apache License 2.0
34 stars 61 forks source link

Respect JAVA_HOME environment variable #69

Closed andreoss closed 4 years ago

andreoss commented 4 years ago

Closes #26, also related to #6

katrinsharp commented 4 years ago

Hi @andreoss. How is your PR different from https://github.com/scalatest/scalatest-maven-plugin/pull/43 ? Thank you.

cstroe commented 4 years ago

@katrinsharp This PR does an additional explicit lookup for the JAVA_HOME environment variable if the java.home system property is not set.

@andreoss To my knowledge, looking at the JAVA_HOME environment variable is not required. The java.home system property is always set by the JVM, so if your JVM program is running, the location of the JVM should be available from the java.home, no need to look elsewhere.

Maven launcher scripts handle JAVA_HOME to find which JVM to execute, but once scala-maven-plugin is launched, it should only look at the java.home system property.

cstroe commented 4 years ago

As it was mentioned in another PR, we should be using ToolchainManager lookups to find the JVM: https://github.com/scalatest/scalatest-maven-plugin/pull/43#issuecomment-439405249

More info on Maven Toolchains.

The main benefit:

With Maven Toolchains applied to JDK toolchain, a project can now be built using a specific version of JDK independent from the one Maven is running with.

This might help when doing JDK upgrades of projects that use scalatest-maven-plugin.

andreoss commented 4 years ago

@andreoss To my knowledge, looking at the JAVA_HOME environment variable is not required. The java.home system property is always set by the JVM, so if your JVM program is running, the location of the JVM should be available from the java.home, no need to look elsewhere.

Maven launcher scripts handle JAVA_HOME to find which JVM to execute, but once scala-maven-plugin is launched, it should only look at the java.home system property.

You might be right. But surefire seems to query for JAVA_HOME before java.home https://github.com/apache/maven-surefire/blob/master/surefire-its/src/test/java/org/apache/maven/surefire/its/fixture/SurefireLauncher.java#L77 My guess is that this approach is chosen because it's less bug-prone, since System properties is a mutable collection, and value for java.home can be altered. Unlike immutable environment variables.

Also there could be cases when using that the same JVM which was used for maven is not always desirable. The standard mvn script seems to always set JAVA_HOME, but in case maven was started from IDE it may not be so.

cstroe commented 4 years ago

@andreoss Yep, it doesn't hurt to use JAVA_HOME as a backup as far as I know. :+1:

katrinsharp commented 4 years ago

LGTM

cerveada commented 4 years ago

Hello, there seems to be some confusion

You might be right. But surefire seems to query for JAVA_HOME before java.home https://github.com/apache/maven-surefire/blob/master/surefire-its/src/test/java/org/apache/maven/surefire/its/fixture/SurefireLauncher.java#L77

This is an integration test code, not code used by surefire itself. In fact surefire uses java.home. I can share a link to simple project that I used to test that if anyone is interested.

@andreoss Yep, it doesn't hurt to use JAVA_HOME as a backup as far as I know. 👍

But that is not what is done in the PR. JAVA_HOME is the defualt and the java.home is a backup.

Please take a look at #80 where I described why it seems to me the default should be java.home.