-
Notifications
You must be signed in to change notification settings - Fork 611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build: gradlew avoid downloader #2419
Conversation
More specific than needed, and some JDK/configs may complain about an incompatibility with --release.
I'll merge this in a couple days if I don't hear back. |
Could we have that for Windows, too. I don't know what your exact problem with '--source 11' was. Basically it would be better to use We can remove all that source/target/release completely, as we compile the code as-is. It it won't compile anymore if something changes incompatibly. In Lucene's APIJAR generator we also do not pass any version to "java": https://github.com/apache/lucene/blob/main/gradle/generation/extract-jdk-apis.gradle#L42-L75 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have that in Lucene, too?
I forgot the Windows one; I'm glad I tagged you for review :-) Maybe you could review that? Admittedly the --source issue was specific to a custom JVM variant where I work. Maybe --release alone would work; I didn't check. Any way, I'd rather remove needless specificity. I'll do a Lucene PR after this. No CHANGES.txt for this minor build improvement stuff IMO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have not tested this, but looks fine. The "IF NOT EXIST" code is identical to the few lines below, so I assume it works correctly.
@@ -87,7 +89,7 @@ set CLASSPATH=%GRADLE_WRAPPER_JAR% | |||
IF NOT EXIST "%APP_HOME%\gradle.properties" ( | |||
@rem local expansion is needed to check ERRORLEVEL inside control blocks. | |||
setlocal enableDelayedExpansion | |||
"%JAVA_EXE%" %JAVA_OPTS% --source 11 "%APP_HOME%/buildSrc/src/main/java/org/apache/lucene/gradle/GradlePropertiesGenerator.java" "%APP_HOME%\gradle\template.gradle.properties" "%APP_HOME%\gradle.properties" | |||
"%JAVA_EXE%" %JAVA_OPTS% "%APP_HOME%/buildSrc/src/main/java/org/apache/lucene/gradle/GradlePropertiesGenerator.java" "%APP_HOME%\gradle\template.gradle.properties" "%APP_HOME%\gradle.properties" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling this should be backslashes, but it's ok (was the same before).
* avoid WrapperDownloader if have the JAR * don't specify --source More specific than needed, and some JDK/configs may complain about an incompatibility with --release. (cherry picked from commit fd3f7d2)
* avoid WrapperDownloader if have the JAR * don't specify --source More specific than needed, and some JDK/configs may complain about an incompatibility with --release. From apache/solr#2419
* avoid WrapperDownloader if have the JAR * don't specify --source More specific than needed, and some JDK/configs may complain about an incompatibility with --release. From apache/solr#2419
* avoid WrapperDownloader if have the JAR * don't specify --source More specific than needed, and some JDK/configs may complain about an incompatibility with --release. From apache/solr#2419
--source 11
to either script; it's unnecessary and I found there to be an issue in a JDK config where this is (supposedly) incompatible with--release
, which I don't see specified here (shrug).