You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/11/16 00:11:26 UTC
[GitHub] [lucene] rmuir opened a new pull request, #11937: reland: 11925 javac options
rmuir opened a new pull request, #11937:
URL: https://github.com/apache/lucene/pull/11937
This is fixing 11927 for alternate toolchains. Git really showing its true colors today.
The problem with alternate toolchains is they fork differently: invoke `javac` executable rather than error prone's invocation of `java`
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
dweiss commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023550549
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
Hmm... perhaps this should be somehow fixed where the alt jvm hack is - it'd be consistent here (and everywhere). It looks like gradle prefixes jvm options with -J internally, which we don't do?
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
dweiss commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023671778
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
https://github.com/gradle/gradle/issues/22746
This is a bug in gradle - they treat jvm args differently, depending on the compiler mode.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
uschindler commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023668563
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
I think as Robert said: The internals of javacompile do not execute "javac" and instead use "java" together with some own code!?
I would change the if statement here a bit like at other places in the build so it is more generic. I will do that when Java 20 support comes, don't hurry!
Thanks Robert for fixing the alternate runtimes.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023906901
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
since you defer the evaluation, i think your change will work fine!
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
dweiss commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023961947
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
I'll backport, no problem.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
dweiss commented on PR #11937:
URL: https://github.com/apache/lucene/pull/11937#issuecomment-1316436339
> you can't really reopen a PR and github revoked access to dweiss's branch as soon as i pressed merge
well, it wasn't me. :) No idea what happened there...
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir merged pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
rmuir merged PR #11937:
URL: https://github.com/apache/lucene/pull/11937
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
dweiss commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023673686
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
> They may not work on 9.x where altJvm + error-prone is actually possible (in 10.x its not, hence impossible to test or worry about).
@rmuir altJvm is possible on 10.x as well, just pass a path to JDK17 via -Pruntime.java.home=/my/jdk/17 and it'll use that 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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
uschindler commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023893339
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
It is a5e25259468 ? Cool. Much better as it works with toolchains, too.
Can you backport this to 9.x?
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
uschindler commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023954625
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
> > @rmuir altJvm is possible on 10.x as well, just pass a path to JDK17 via -Pruntime.java.home=/my/jdk/17 and it'll use that JVM.
>
> yes, i know, but i was trying to test the grid of `altjvm=True/False X errorprone=True/False` on 10.x, you can't do altjvm=True + errorprone=True, it gets disabled due to some workaround. So I tested that on 9.x
>
> thanks for looking at it, as long as jenkins is happy we are good.
Yes altjvm does not work with errorprone due to a bug in the errorprone plugin. the issue has to do with the open modules. Maybe it is the same issue like with -J and no -J.
We disabled it. For Gradle's Toolkit, theres support in the errorprone plugin. It just does not work with our altjvm support.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023957903
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
yeah, i just wanted the change to not be a PITA in the future in case the bug got fixed and workaround removed. but i was surprised by the compileMain19 and had to hack around that in an ugly way. Dawid's patch does what i originally wanted to do, just deferred so it doesn't get overwritten.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023903448
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
> @rmuir altJvm is possible on 10.x as well, just pass a path to JDK17 via -Pruntime.java.home=/my/jdk/17 and it'll use that JVM.
yes, i know, but i was trying to test the grid of `altjvm=True/False X errorprone=True/False` on 10.x, you can't do altjvm=True + errorprone=True, it gets disabled due to some workaround. So I tested that on 9.x
thanks for looking at it, as long as jenkins is happy we are good.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #11937:
URL: https://github.com/apache/lucene/pull/11937#issuecomment-1316066302
you can't really reopen a PR and github revoked access to dweiss's branch as soon as i pressed merge, so i couldn't even update it.
at least here all the changes are in one place and work on main branch. They may not work on 9.x where altJvm + error-prone is actually possible (in 10.x its not, hence impossible to test or worry about). I'll deal with that when backporting.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a diff in pull request #11937: reland: 11925 javac options
Posted by GitBox <gi...@apache.org>.
dweiss commented on code in PR #11937:
URL: https://github.com/apache/lucene/pull/11937#discussion_r1023691111
##########
gradle/hacks/turbocharge-jvm-opts.gradle:
##########
@@ -38,4 +39,20 @@ allprojects {
jvmArgs += vmOpts
}
-}
\ No newline at end of file
+
+ // Tweak javac to not be too resource-hungry.
+ // This applies to any JVM when javac runs forked (e.g. error-prone)
+ // Avoiding the fork entirely is best.
+ tasks.withType(JavaCompile) { JavaCompile task ->
+ if (task.path == ":lucene:core:compileMain19Java") {
Review Comment:
> I would change the if statement here a bit like at other places in the build so it is more generic. I will do that when Java 20 support comes, don't hurry!
I pushed an alternative workaround that doesn't require any explicit task references and is actually more direct as to what the hell actually is happening there (with respect to the bug I filed with gradle).
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org