Add custom URL patterns for downloading gradle distribution and gradle wrapper

[dweiss]

This patch adds a way to specify custom locations for gradle wrapper and gradle distribution. The assumptions were:

1. minimize surface of this change to a single class emulating gradle-wrapper's logic to "install" the gradle distribution specified in gradle-wrapper.properties, if it's not installed,
2. custom wrapper and distribution URLs passed via env variables, with ${gradleVersion} replaceable pattern (works like gradle's official distributionUrl).
3. ensure the downloaded distribution matches the expected checksums; the install path should match the "official" URL and thus be transparent to other scripts and checks in Lucene.

I've tested this locally, for example:

$ find ~/tmp/gradle
.
./9.7.0
./9.7.0/gradle-9.7.0-bin.zip
./9.7.0/gradle-wrapper.jar

$ rm -rf ~/.gradle/wrapper
$ LUCENE_GRADLE_WRAPPER_URL='file:///home/dweiss/tmp/gradle/${gradleVersion}/gradle-wrapper.jar'   LUCENE_GRADLE_DISTRIBUTION_URL='file:///home/dweiss/tmp/gradle/${gradleVersion}/gradle-${gradleVersion}-bin.zip' ./gradlew tasks

This is a different take but similar in spirit to these related issues: #16521, #16483.

[dsmiley]

BTW it would have been nice if you iterated on my attempt at this; we are both committers here. Can you please list how this PR differs from my attempt?

[dweiss]

In short, it is similar but not the same. It is different enough that I thought I'd file a separate PR instead of pushing to your branch (but I can, it's up to you).

I wanted to keep this thing entirely separate (so that no extra stuff needs to go into gradle-wrapper.properties, for example) and optional (so that unless you specify those env properties, nothing changes for anybody else). The logic we have now works fine for many people and it's a result of multiple iterations of changes, I don't think we should change it, to be honest.

So everything related to this "intranet-mirror" is self-contained in a single, separate class. Also, contrary to your patch, this one simulates gradle wrapper's logic pretty closely - including the same lock files, sha checksums matching the "official" distribution and a separate variable for fetching both the wrapper and the distribution code.

I initially made some shortcuts (launched gradle directly, without gradle-wrapper.jar) but it wasn't good - there are manifest entries in gradle-wrapper.jar that would have to be passed to the bootstrap class (Enable-Native-Access: ALL-UNNAMED)... it was way more complicated.

[dweiss]

I guess the big question is if this sort of solution would work for you, David (with two env variables). I think it should be fairly convenient (you can even have those distribution zips mirrored locally, like the above example shows - this way the network is never accessed).

[dsmiley]

I like that the changes can be env-var driven.

I hoped we would either replace or maybe add a little to our WrapperDownloader. As an ASF project + gradle build, we need something -- just trying to keep that something narrow. I don't see why this new IntranetGradle does both wrapper downloading and gradle distribution downloading.

Maybe you disagree but if I'm going to set env vars to choose my gradle wrapper in some way, I'd rather no sha256 be verified, and/or allow me to have yet another env var to specify whatever the sha is for it. I think most of all I want to make the wrapper distribution something in my control/env. I can have it on my machine somewhere but the build here needs to know where it is. If I specify my gradle wrapper, Lucene shouldn't further insist what version and/or sha256 it is. I'm elecrting to choose the wrapper.

[dsmiley]

should have a comment to clarify what this is doing, maybe with an example

[dweiss]

I'll go through this code again if we decide to commit this approach - I just wanted to open a separate PR so that we can discuss this alternative take on the problem. It is machine generated code; I'll go through the patch and remove odd comments etc. if we decide this is the way to go.

[dweiss]

I hoped we would either replace or maybe add a little to our WrapperDownloader. As an ASF project + gradle build, we need something -- just trying to keep that something narrow. I don't see why this new IntranetGradle does both wrapper downloading and gradle distribution downloading.

I wanted to keep it separate from WrapperDownloader because... well, it is hacky. WrapperDownloader is simple, that other class is hairy because gradle is dumb not to offer this by default... This is the only reason - to keep this separate just to signal that it's something we do that we shouldn't really be doing (gradle should be allowing it).

Maybe you disagree but if I'm going to set env vars to choose my gradle wrapper in some way, I'd rather no sha256 be verified, and/or allow me to have yet another env var to specify whatever the sha is for it.

Everyone is so obsessed about security these days that I thought this wouldn't hurt. It ensures gradle distribution you point at (even if it's hosted somewhere else) is identical to that expected from the official server. I personally don't care that much about it but it seems like a reasonable thing to do (?).

I think most of all I want to make the wrapper distribution something in my control/env. I can have it on my machine somewhere but the build here needs to know where it is. If I specify my gradle wrapper, Lucene shouldn't further insist what version and/or sha256 it is. I'm elecrting to choose the wrapper.

Get it. I think it's going to hurt you if they change something in an incompatible way - the wrapper jar is part of each distribution (major or minor) and it does change from time to time (what they change - I've no idea but the checksum changes [1]). Typically, this jar would be versioned alongside the rest of the sources - the decision to download it dynamically was driven by ASF requirements not to ship binaries with the source code, so the downloading/ sha check is a workaround but it still works as if the wrapper were versioned - for each commit it is fixed to a particular binary.

The simplest way to dodge checksum checking would be a small adjustment to gradlew scripts in which - if the env URLs are provided, we use them, download and install the jar/gradle distro and skip the original WrapperDownloader (which does checksum checking)... This said, I think it's better to keep those checksums - this will avoid potential odd surprises in the future. If you live behind a corporate firewall then it's fairly easy to create a directory structure for each gradle version needed + the corresponding wrapper (what I tried to show in the description of this patch). This is a one-time effort, the directory structure stays the same on disk and you can keep the env pointer URLs constant (they can use the replaceable gradleVersion part).

[1] https://gradle.org/release-checksums/

[dweiss]

I've added a separate env variable - LUCENE_GRADLE_VERIFY_CHECKSUMS=false to ignore all checksums... Take a look at commit 47ac309. What I stated in my previous comment still holds though - I don't think it's a good thing to ignore them. I think they should be verified and the correct (expected) gradle-wrapper.jar should be used.

[dsmiley]

If you live behind a corporate firewall then it's fairly easy to create a directory structure for each gradle version needed + the corresponding wrapper (what I tried to show in the description of this patch). This is a one-time effort, the directory structure stays the same on disk and you can keep the env pointer URLs constant (they can use the replaceable gradleVersion part).

I'd rather not do obscure local machine stuff (that I may forget how to do) every time Lucene/Solr bumps the Gradle version. It's annoying. Keeping an edited gradle-wrapper.properties with a corporate distributionUrl hasn't been too annoying to maintain. It's at least a known/documented thing. But also aligning the gradle wrapper has felt like a needless chore. The wrapper is not paired with the distribution requiring version alignment.

[dweiss]

Keeping an edited gradle-wrapper.properties with a corporate distributionUrl hasn't been too annoying to maintain.

I'm sorry but I disagree with you here. This file is outside of Lucene's control, it shouldn't be modified. When upstream changes gradle version you'll get conflicts on any pull/merge. It seems weird to accept it.

The wrapper is not paired with the distribution requiring version alignment.

Of course it is - that's why they publish SHA checksums for each and every release:
https://gradle.org/release-checksums/

It may not change from version to version but each version expects its aligned wrapper.

But also aligning the gradle wrapper has felt like a needless chore.

Just to note - perhaps it's not obvious - this patch uses env variables but they don't have to be parameterized with gradle version. So you can declare fixed locations for the gradle-wrapper or even the distribution...

LUCENE_GRADLE_DISTRIBUTION_URL=file:///home/myhome/tmp/my-gradle-distribution.zip
LUCENE_GRADLE_WRAPPER_URL=file:///home/myhome/tmp/gradle-wrapper.jar
LUCENE_GRADLE_VERIFY_CHECKSUMS=false

I think it's a bad idea... but if you want to - you can do it with this patch.

[dweiss]

Also - perhaps at the risk of stating the obvious - you don't need to run gradlew scripts at all. If you'd rather use your own gradle distribution(s) then they will work too - the version must match but otherwise you can run gradle directly (without any gradle-wrapper fiddling):

~/.gradle/wrapper/dists/gradle-9.7.0-bin/d4tj7w02tcgubx9zk9hbippn6/gradle-9.7.0/bin/gradle check -x test

We are strict about the gradle version used... maybe it'd be simpler to add an option to relax this check and allow compatible major versions too. Knowing how much changes from version to version I would never use it myself but it's also something to consider if you have such strong feelings about gradle-wrapper.

[dsmiley]

It may not change from version to version but each version expects its aligned wrapper.

That's a highly presumptuous claim. And my experience is contrary to your presumption: I've seen no issues locally (misc projects at work) with even Gradle major version divergence between wrapper & distribution.

The wrapper is on their "release train" and gets released without necessarily any changes. I wish they didn't bother doing that given how stable & backwards compatible it is... but whatever.

[dweiss]

That's a highly presumptuous claim. And my experience is contrary to your presumption: I've seen no issues locally (misc projects at work) with even Gradle major version divergence between wrapper & distribution.

That something works doesn't mean it doesn't have the potential to break. Think of it this way - when you want to install the gradle wrapper, you use a certain gradle version to do so. When you update from a particular distribution, that distribution's wrapper will be written in your project - each one carries its own binary and writes it when needed.

Here's what gradle says about it [1] - you trust that their wording of "usually" means "always", I don't.

[1] https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:upgrading_wrapper

[dsmiley]

Shrug... fair but the consequence is low. If it works, it works... (no need to upgrade) and if it doesn't then upgrade.

[dweiss]

Like I mentioned, if you don't care about it then this should be a one-time operation:

LUCENE_GRADLE_DISTRIBUTION_URL=[corporate gradle distro mirror]
LUCENE_GRADLE_WRAPPER_URL=file:///home/myhome/tmp/gradle-wrapper.jar
LUCENE_GRADLE_VERIFY_CHECKSUMS=false

Is this something you'd be comfortable with (and would it be convenient in your workflow)? If so, I can review the changes in this patch or commit it back to your original PR.

[dweiss]

I've cleaned up LLM verbosity and added some sanity checking.

[dsmiley]

I'll try this out tomorrow.
Boy I wish we could convince the Gradle folks to prioritize an ASF compatible solution.

[dsmiley]

It's dubious to me if the weight here is worth its value but I leave that to you.

[dweiss]

I use this workflow when upgrading gradle. I'd like to have this class covered so I leave it in.

[dweiss]

Boy I wish we could convince the Gradle folks to prioritize an ASF compatible solution.

They don't seem to care.

[dsmiley]

Did you notice https://github.com/Glavo/gradle-wrapper-neo ? I subscribe to some of those Gradle tickets and saw the post about this. Looks like exactly what we need! Outside of Gradle doing it themselves, of course.

[dweiss]

No, I didn't know about it - good find. It's only a month old though... and looking at the code I think I like the much leaner version here (~500 lines) better - take a look at it [1] (3500 lines). I like the approach they have with a user-home configuration file though. Maybe it's more convenient than env variables? What do you think?

[1] https://github.com/Glavo/gradle-wrapper-neo/blob/main/gradle/wrapper/GradleWrapperNeo.java

[dweiss]

The one thing I don't like about neo is that it's a one-way option for everyone. I'd rather use the "official" wrapper for everyone and have a loophole for those folks who can't access official services (for various reasons).

[dsmiley]

I've sorta know someone at Gradle who works with the ASF and I crafted an email to make a case that this matter needs attention/priority it's hasn't been getting.

RE neo:

Yes I observed the line count. Neo takes actual gradle wrapper source, which is why. I prefer "lean" but if we don't maintain it then I'm indifferent.

I like the approach they have with a user-home configuration file though. Maybe it's more convenient than env variables? What do you think?

I like that too... and can be configured overlaying a project's gradle-wrapper.properties (if I recall).

[dweiss]

I've sorta know someone at Gradle who works with the ASF and I crafted an email to make a case that this matter needs attention/priority it's hasn't been getting.

Thanks! I definitely think this is something of value.

Neo takes actual gradle wrapper source, which is why. I prefer "lean" but if we don't maintain it then I'm indifferent.

I prefer the burden of maintaining that 500 lines of code I've reviewed than reviewing updates to neo, to be honest... I'm not against it but let's wait and see what evolves - maybe gradle folks will add something of their own, maybe neo get more traction in the community.

For now, I suggest to either accept this patch as is or reshape the code to use a (property) file in the user's home directory instead of env variables. I don't quite see the value of their regexp replacements - I think an URL with those replaceable gradleVersion is more consistent with what gradle wrapper does and more intuitive and I'd stick to it. The remaining logic seems very similar and there's not much to borrow.