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 2020/04/27 12:26:26 UTC
[GitHub] [maven-surefire] akomakom opened a new pull request #288: SUREFIRE-1234 Set java home from toolchain
akomakom opened a new pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288
When a toolchain is used to select a test jvm, also set the `JAVA_HOME` environment variable to the selected jvm instead of inheriting the environment variable from the invoking shell.
(Note: this PR is based on branch from #287 )
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620416097
@akomakom
Thx. We are in master.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom edited a comment on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
akomakom edited a comment on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620089356
> You think that the java process won't set/override the JAVA_HOME for itself even if we did not specify it in the list of env for the inheritance?
I can tell you from local testing, that without this change I get the main jvm's JAVA_HOME. I'm using a test in an external project that prints out the variable:
```java
@Test
public void printJavaHome() throws Exception {
System.out.println("JAVA_HOME=" + System.getenv("JAVA_HOME"));
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620067158
But the inheritance of the env is not done from the toolchain to the jvm. The toolchain is used only for one purpose, to get the path of the JDK installation on the disk, see [this](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L2546).
The SHELL is calling `/path/to/jdk/bin/java -jar` etc.
The only place where the JAVA_HOME can be inherited in to the SHELL is [this](https://github.com/apache/maven-shared-utils/blob/master/src/main/java/org/apache/maven/shared/utils/cli/Commandline.java#L402) - external library - not surefire however we are able to exclude it. And the environment is merged in [here](https://github.com/apache/maven-shared-utils/blob/master/src/main/java/org/apache/maven/shared/utils/cli/Commandline.java#L243).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom edited a comment on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
akomakom edited a comment on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620089356
> You think that the java process won't set/override the JAVA_HOME for itself even if we did not specify it in the list of env for the inheritance?
I can tell you from local testing, that without this change I get the main jvm's JAVA_HOME. I'm using a test that prints out the variable:
```java
@Test
public void printJavaHome() throws Exception {
System.out.println("JAVA_HOME=" + System.getenv("JAVA_HOME"));
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620240243
We have a [utility](https://github.com/apache/maven-surefire/blob/15a631b811fb1deadfdaaa56a76dff5b3275629c/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L2539) `SystemUtils#toJdkHomeFromJvmExec()`. Everything is there. It's only a question to put:
```
if ( !environmentVariables.containsKey( "JAVA_HOME" ) )
{
environmentVariables.put( "JAVA_HOME", jdkHome.getAbsolutePath() );
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom commented on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620221768
> We have the same problem in the option when [if ( isNotEmpty( jvm ) )](https://github.com/apache/maven-surefire/blob/15a631b811fb1deadfdaaa56a76dff5b3275629c/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L2523).
> We should set JAVA_HOME in this option too but it is clearer to do it in a new PR.
Actually I don't believe that this is necessary (I considered this earlier). a `jvm` is a full path to the executable, and `JAVA_HOME` may be non-trivial to infer (ie on Ubuntu, /usr/bin/java may be 3 symlinks removed from the actual binary). If someone is setting `jvm` to a full path, they can also set `<JAVA_HOME>/full/path</JAVA_HOME>` explicitly since they are already hardcoding paths.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620182261
I forgot to ask you for a unit test. it;s easy. Just prepare the `DefaultJavaToolChain` and call it with `Whitebox.invokeMethod`. We should expect a path in the assertion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620186330
Here is the Jira ticket created.
https://issues.apache.org/jira/browse/SUREFIRE-1783
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620070432
You think that the java process won't set/override the JAVA_HOME for itself if we did not specify it in the list of env for the inheritance?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#discussion_r416140830
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoToolchainsTest.java
##########
@@ -47,7 +49,7 @@
* Test for {@link AbstractSurefireMojo}. jdkToolchain parameter
*/
@RunWith( PowerMockRunner.class )
-@PrepareForTest( {AbstractSurefireMojo.class} )
+@PrepareForTest( {AbstractSurefireMojo.class, DefaultJavaToolChain.class} )
Review comment:
for your info, the annotation `PrepareForTest` is used with a class which is mocked using the method `mockStatic`. This does not happen in your new test method `shouldChangeJavaHomeFromToolchain`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620240243
We have a [utility](https://github.com/apache/maven-surefire/blob/15a631b811fb1deadfdaaa56a76dff5b3275629c/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L2539) `SystemUtils#toJdkHomeFromJvmExec()`. Everything is there. It's only a question to put:
```
if ( !environmentVariables.containsKey( "JAVA_HOME" ) )
{
environmentVariables.put( "JAVA_HOME", defaultJavaToolChain.getJavaHome() );
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom commented on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620230177
> @akomakom but the people would expect some service from us. They normally do not look into our code forcing the jvm this way. We can set the JAVA_HOME unless already set, this is what i mean. Then the setting of JAVA_HOME in user's config would always take the precedense, as you described.
So how do you plan to reliably determine `JAVA_HOME` from `jvm`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom commented on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
akomakom commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620170999
I'm no stranger to force pushing :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom commented on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
akomakom commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620089356
> You think that the java process won't set/override the JAVA_HOME for itself even if we did not specify it in the list of env for the inheritance?
I can tell you from local testing, that without this change I get the main jvm's JAVA_HOME. I'm using a test that prints out the variable:
```java
@Test
public void JAVA_HOME() throws Exception {
System.out.println("JAVA_HOME=" + System.getenv("JAVA_HOME"));
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620070432
You think that the java process wil not set/override the JAVA_HOME for itself?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620070432
You think that the java process won't set/override the JAVA_HOME for itself even if we did not specify it in the list of env for the inheritance?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620202503
We have the same problem in the option when [if ( isNotEmpty( jvm ) )](https://github.com/apache/maven-surefire/blob/15a631b811fb1deadfdaaa56a76dff5b3275629c/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L2523).
We should set JAVA_HOME in this option too but it is clearer to do it in a new PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom edited a comment on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom edited a comment on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620230177
> @akomakom but the people would expect some service from us. They normally do not look into our code forcing the jvm this way. We can set the JAVA_HOME unless already set, this is what i mean. Then the setting of JAVA_HOME in user's config would always take the precedense, as you described.
So how do you plan to reliably determine `JAVA_HOME` from `jvm`?
I'd think that mentioning that JAVA_HOME is not affected by setting `jvm` in the docs is sufficient.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620070432
You think that the java process won't set/override the JAVA_HOME for itself if we did not specify it in the list of env?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom commented on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620267640
Good to know, will open a separate PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620223653
@akomakom but the people would expect some service from us. They normally do not look into our code forcing the jvm this way. We can set the JAVA_HOME unless already set, this is what i mean. Then the setting of JAVA_HOME in user's config would always take the precedense, as you described.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #288: SUREFIRE-1234 Set java home from toolchain
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#issuecomment-620169605
I will merge #287 but then you have to override this PR. The first two commits will be in conflict with one squashed commit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom commented on a change in pull request #288: [SUREFIRE-1783] Fork JVM defined by Toolchain should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom commented on a change in pull request #288:
URL: https://github.com/apache/maven-surefire/pull/288#discussion_r416141616
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoToolchainsTest.java
##########
@@ -47,7 +49,7 @@
* Test for {@link AbstractSurefireMojo}. jdkToolchain parameter
*/
@RunWith( PowerMockRunner.class )
-@PrepareForTest( {AbstractSurefireMojo.class} )
+@PrepareForTest( {AbstractSurefireMojo.class, DefaultJavaToolChain.class} )
Review comment:
Right, I tried following your advice but couldn't figure out how to do it that way. I'll remove this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org