LUCENE-9266 remove gradle wrapper jar from source

[madrob]

ASF Release Policy states that we cannot have binary JAR files checked
in to our source releases, a few other projects have solved this by
modifying their generated gradlew scripts to download a copy of the
wrapper jar.

This implementation is heavily based on the one found in KAFKA-1714

[dweiss]

I like the direction, Mike. If we can keep gradle wrapper jar in the repository (see that comment on legal jira) then this patch could be easily extended to make the whole download check conditional and only applicable if you're running a stand-alone source release bundle. This is definitely a follow-up work.

I can try to help out with Windows testing and maybe cosmetics; let me know if you want me to file a pr against your repo or attach a patch when you're ready.

[dweiss]

Realistically, I don't think anybody will. Rather, they'll retry the script. If something doesn't work with move it's probably fatal anyway.

…

On Tue, Mar 31, 2020 at 9:34 PM Mike Drob ***@***.***> wrote: @madrob commented on this pull request. ________________________________ In buildSrc/src/main/java/org/apache/lucene/gradle/WrapperDownloader.java: > + File temp = Files.createTempFile("gradle-wrapper", null).toFile(); + try (ReadableByteChannel in = Channels.newChannel(url.openStream()); + FileOutputStream out = new FileOutputStream(temp)) { + out.getChannel().transferFrom(in, 0, maxSize); + } + + try (FileInputStream fis = new FileInputStream(temp)) { + MessageDigest sha256 = MessageDigest.getInstance("SHA-256"); + + // Convert binary digest to hex checksum value + // This will strip leading zeroes, deal with that when we need to + String sha256sum = new BigInteger(1, sha256.digest(fis.readAllBytes())).toString(16); + + if (args[2].equals(sha256sum)) { + if (!temp.renameTo(jar)) { + System.err.println("Failed renaming wrapper jar, leaving download at " + temp); The user might be able to manually move it into place since we've already validated the checksum. I'll update the error. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

[dweiss]

changes.patch.txt

Hi Mike. I've restructured the downloader class a bit so that gradle wrapper version and checksum are aligned with the target file - makes it a single place to change things (instead of messing with the scripts). I also simplified error messaging a bit, hope it's ok.

More interestingly I discovered a hideous problem with CLASSPATH environment variable: this is used by java in single-source mode and was locking the wrapper file (on Windows), effectively preventing any changes (in case of overwriting due to a wrong checksum, for example). Corrected.

Looks good to me to commit this in (removing the gradle-wrapper.jar file). Anything else can be a follow-up.

[madrob]

Your patch moved the *nix invocation of WrapperDownloader to before we set JAVA_CMD, when it still needed to be after. I switched that back, but kept your change of setting the CLASSPATH after we download the new gradle-wrapper.jar. I'll push a squashed/rebased version of everything here for one last look before committing.

In your Windows patch you use backslashes for GRADLE_WRAPPER_JAR but forward slashes for the JAVA_EXE command - is that fine/expected/necessary?

[dweiss]

bq. Your patch moved the *nix invocation of WrapperDownloader to before we set JAVA_CMD, when it still needed to be after. I switched that back, but kept your change of setting the CLASSPATH after we download the new gradle-wrapper.jar

I should have checked, sorry. I had a million things to do today.

bq. In your Windows patch you use backslashes for GRADLE_WRAPPER_JAR but forward slashes for the JAVA_EXE command - is that fine/expected/necessary?

Neither matters on modern Java versions. You can switch them to windows path separators if you wish but you don't have to.

[madrob]

From @gerlowskija on Slack:

Initially it failed for me with:
./gradlew --info tasks Error: Could not find or load main class buildSrc.src.main.java.org.apache.lucene.gradle.WrapperDownloader.java
But that turned out to be because I was using Java8. Might be worth doing some sort of java -version check somewhere, but maybe not worth the trouble.

Sure. But while people are developing on a mix of master/8x, there’s gonna be Java version mixups pretty frequently, so people are gonna start seeing that opaque error message unless we give them a better one.

A better error message would be nice here, but I also don't want to figure out how to parse the output. Maybe the easiest solution is to add another class with an empty main method and if that fails we assume the java launcher is too old?

[dweiss]

I would really want to limit the number of forked java processes to the absolute minimum in those scripts. Each invocation of java adds a bit of penalty to startup and these accumulate. I would assume people are running with Java 11+ and if it fails for them so be it...

If you really want to be nice here then I would rewrite the Downloader to be compatible with Java 8, then compile that class with javac and run it as regular class. The first thing you do in the downloader then is you can System.exit(-1) if java version < 11.

It can be also left for a separate patch/ improvement.

[madrob]

Committed in e25ab42