SOLR-16641 - Generate gradle.properties from gradlew #1320

colvinco · 2023-01-31T09:27:55Z

https://issues.apache.org/jira/browse/SOLR-16641

Description

Stop generating gradle.properties and use a checked-in template instead. If gradle.properties isn't present when gradlew runs, it will be created from the gradle.properties.template. The properties will be available for the first execution of gradle on a clean checkout.

Also added notes on the need to have perl and python3 for gradlew check to succeed.

Solution

The gradlew already had hooks that altered the arguments passed in when gradle.properties didn't exist. I've changed them to create the gradle.properties instead, and removed the localSettings task from gradle.

Tests

Without this PR

Delete gradle.properties
Run ./gradlew updateLicenses and it will always result in a circular dependency and fail.
Apply the PR and run ./gradlew updateLicenses again and it will generate the gradle.properties and succeed.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

uschindler · 2023-01-31T11:22:52Z

Hi @dweiss,
I am not fully sure if this is a good idea to do it like that.
In reality there should be an option in gradle to apply settings at runtime (e.g., using the global build script which sets up project structure) and allow to load custom/default settings and/or our own settings.

magibney · 2023-01-31T12:51:44Z

There was some related discussion recently on SOLR-16634 -- at the moment I'm inclined to favor @dsmiley's suggestion to possibly move localSettings into the gradlew wrapper script.

gradle/validation/jar-checks.gradle

colvinco · 2023-01-31T19:09:16Z

I've moved the generation of the localSettings to gradlew as discussed on the Jira issue.
I've also removed the task dependencies between localSettings and other tasks as they're redundant when executed through gradlew.
I've also removed the CI steps that called localSettings

gradlew

dweiss · 2023-01-31T19:25:45Z

In reality there should be an option in gradle to apply settings at runtime (e.g., using the global build script which sets up project structure) and allow to load custom/default settings and/or our own settings.

In reality, gradle folks are not really responsive to such requests... I know. I wish it was an option too.

magibney · 2023-01-31T19:30:57Z

btw I wasn't intending to throw the scope of this PR by mentioning extracting gradle.properties generation into the gradlew script. I think it's still an open question whether it makes sense to pursue that. But it definitely makes sense to fix the circular dependency (as you did in aabacb56472acb573ad60b8b5d21913e3a8d0af5).

Maybe it'd make sense to proceed with a different PR for the gradlew script quesiton? (sorry for the mixed messages).

colvinco · 2023-01-31T19:35:27Z

np. I don't mind doing a separate PR if it makes it easier to discuss and get the aabacb5 fix back in this one, or just let this one brew.
I don't think there's a huge rush to fix this so happy to do it either way

dsmiley

Yay; glad to see you working on this @colvinco ! It's a big deal! It'll improve the user getting-started experience (and even for experienced devs that forget localSettings LOL) and improve CI maintenance (e.g. working on Jenkins, GitHub Actions, crave.io). I think this PR/JIRA issue should more obviously declare this. It's a 100x more important than a "circular task dependency" in the build.

I'm surprised to see the "localSettings" gradle task live on. Surely it could be replaced with a bash script?

dweiss · 2023-01-31T19:56:11Z

I'm surprised to see the "localSettings" gradle task live on. Surely it could be replaced with a bash script?

Depends. Some values there are computed from Java defaults. I agree a modification to launch scripts is probably easier to work with though, especially because it can pick up those defaults on the first run.

colvinco · 2023-01-31T20:32:54Z

It's also having to "write it twice" for the Windows .bat as long as it's still supported for dev. (Though with WSL becoming more prevalent that's a problem that will solve itself eventually).

dsmiley · 2023-01-31T21:28:08Z

I suggest for the Windows side, just check if there is no gradle.properties and log a warning pointing at the unix script to help the user craft one themselves. This is just an idea; I think we can get away with being less thorough in some fashion for Windows as it's used much less and the need for this file is iffy. We're doing something rather unusual in Lucene & Solr -- insisting on gradle.properties when countless Gradle projects have no such mandate.

A simplifying alternative would be to check gradle.properties into the project (but still marked ignored in .gitignore) and with Gradle's own defaults for number of workers & tests, or choose low ones. Advise users to edit them according to their machine. We are overthinking this stuff today! This has dev maintenance costs we pay right now. Generating this file is over-engineered IMO.

colvinco · 2023-02-01T09:17:27Z

A simplifying alternative would be to check gradle.properties into the project (but still marked ignored in .gitignore) and with Gradle's own defaults for number of workers & tests, or choose low ones. Advise users to edit them according to their machine. We are overthinking this stuff today! This has dev maintenance costs we pay right now. Generating this file is over-engineered IMO.

I would create a gradle.properties.template file that lives in source control and copy it to the ignored gradle.properties path. That way it's easy to update the template without messing around with gitignore.

I've taken a closer look at the properties file. Currently I see that only two of the settings are generated dynamically, and the rest are all hardcoded.

        // Approximate a common-sense default for running gradle/tests with parallel
        // workers: half the count of available cpus but not more than 12.
        def cpus = Runtime.runtime.availableProcessors()
        def maxWorkers = (int) Math.max(1d, Math.min(cpus * 0.5d, 12))
        def testsJvms = (int) Math.max(1d, Math.min(cpus * 0.5d, 12))
...
# Maximum number of parallel gradle workers.
org.gradle.workers.max=${maxWorkers}

# Maximum number of test JVMs forked per test task.
tests.jvms=${testsJvms}

Getting the number of "processors" (depending on whether hyperthreading counts...) shouldn't be a problem on bash, https://stackoverflow.com/questions/6481005/how-to-obtain-the-number-of-cpus-cores-in-linux-from-the-command-line, so I could still seed in those values.

I'm using Ubuntu in WSL:

me:~$ getconf _NPROCESSORS_ONLN
16
me:~$ nproc
16
me:~$ grep -c ^processor /proc/cpuinfo
16
me:~$ grep ^cpu\\scores /proc/cpuinfo | uniq |  awk '{print $4}'
8

For Windows it's also possible: https://pureinfotech.com/check-many-cores-processor-windows-10/

wmic cpu get NumberOfCores,NumberOfLogicalProcessors /format:value
NumberOfCores=8
NumberOfLogicalProcessors=16

Runtime.runtime.availableProcessors() gives the same answer as nproc, but it was halved (and capped to 12). So I could just use the number of cores, or divide nproc.

Doing the string replacement when copying the file in bash shouldn't be a problem, I'll see if I can do it sensibly in batch as well.

gradle.properties.template

colvinco · 2023-02-01T10:45:01Z

Hopefully we're getting there now :)
I didn't bother with trying to do variable substitution from batch as I didn't see a good + easy option. But bash will set the two parameters that were being set before.

colvinco · 2023-02-01T10:50:20Z

The check on crave.io is failing to apply my commit... I'm guessing that's because I've been force pushing to my branch and it's now a bit out of sync? https://github.com/apache/solr/actions/runs/4063490165/jobs/6995782967

error: patch failed: gradlew.bat:80
error: gradlew.bat: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: SOLR-16632: Add core name to periodic delete related log messages (#1311)
Applying: SOLR-16641 - Generate gradle.properties from template
Patch failed at 0002 SOLR-16641 - Generate gradle.properties from template
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

I can create a clean PR on a fresh branch to test that, unless there's another reason for it to happen?

colvinco · 2023-02-01T18:36:37Z

It's been a bit of a rollercoaster ;)

risdenk

I'm happy with this with some minor cleanup (optional)

.gitignore

buildSrc/src/main/java/org/apache/lucene/gradle/GradlePropertiesGenerator.java

gradle/template.gradle.properties

gradle/generation/local-settings.gradle

gradle/template.gradle.properties

dweiss

LGTM.

dweiss · 2023-02-02T08:17:53Z

buildSrc/src/main/java/org/apache/lucene/gradle/GradlePropertiesGenerator.java

+    var replacements = Map.of("org.gradle.workers.max", maxWorkers, "tests.jvms", testsJvms);
+
+    // Java properties doesn't preserve comments, so going line by line instead
+    // The properties file isn't big, nor is the map of replacements, so not worrying about speed


This will work for simple values (and property names) but not when property values are more complex characters - these should (in theory at least) be escaped and/or encoded to plain ascii. I don't think it's a problem for now.

I am also not really happy with this parsing.

My idea would be: As the template is always applied (no problem with windows or any othe roperating system), we can make the template a real template and add two search/replace markers in the file. We should then load the whole file into a String (UTF-8 encoding!!!) and then do String#replace('@PLACEHOLDER@', value). Those replaces can be done using replacements.entries().forEach(e -> fileContent = fileContent.replace(e.getKey(), e.getValue()) (don't use replaceAll, as it uses regex and we have no forbiddenapis here to check this!!!!).

So I would add @MAX_WORKERS@ and @TEST_JVMS@ as map keys and replace those tags. This is like Ant did it for long time :-)

dweiss · 2023-02-02T08:21:37Z

help/localSettings.txt

-Local developer settings
-========================
-
-The first invocation of any task in Solr gradle build will generate


I think there is some value in leaving (some content) of this file under help/ - so that folks know that this file can be tweaked and is generated?

And keep it wired into gradlew :helpLocalSettings?

Sorry - I thought I removed that comment. This is duplicated in the template now so I think it's fine to be removed.

Ah bad timing. I can revert that last commit I just put in, or leave it. It might still be a useful signpost

uschindler

See my comment, I don't like the explicit parsing of the properties file. Just see it as a plain text template file and search/replace some ANT-like @VAR@ patterns in it: Load as string, replace tokens, save string (as UTF-8).

In general I am still not really happy with the whole setup. At moment in Lucene I have see this problem very often: You add some new features (like the MR JAR and adding new Java versions) and then many people have the template file expanded in their checkout and ten build fails, because it is not updated.

Not sure how to solve this without Gradle allowing to set some of the properties at runtime.

I have some ideas, but actually we should really open a bug reort that they finally fix the stupidness with toolkits, those properties files and allowing to set those properties in the gradle.settings file using code (which is committed to repo).

gradle/template.gradle.properties

uschindler · 2023-02-02T08:58:22Z

buildSrc/src/main/java/org/apache/lucene/gradle/GradlePropertiesGenerator.java

+    var replacements = Map.of("org.gradle.workers.max", maxWorkers, "tests.jvms", testsJvms);
+
+    // Java properties doesn't preserve comments, so going line by line instead
+    // The properties file isn't big, nor is the map of replacements, so not worrying about speed


I am also not really happy with this parsing.

My idea would be: As the template is always applied (no problem with windows or any othe roperating system), we can make the template a real template and add two search/replace markers in the file. We should then load the whole file into a String (UTF-8 encoding!!!) and then do String#replace('@PLACEHOLDER@', value). Those replaces can be done using replacements.entries().forEach(e -> fileContent = fileContent.replace(e.getKey(), e.getValue()) (don't use replaceAll, as it uses regex and we have no forbiddenapis here to check this!!!!).

So I would add @MAX_WORKERS@ and @TEST_JVMS@ as map keys and replace those tags. This is like Ant did it for long time :-)

gradle/template.gradle.properties

janhoy

Checked out and tested. Verified that props file was created. On my M1 Pro mac with 10 cores the jvms were set to 5. Looks good!

gradle/template.gradle.properties

buildSrc/src/main/java/org/apache/lucene/gradle/GradlePropertiesGenerator.java

uschindler

Works as first step.
Could you make a PR for Lucene, too (please refer to discussions here).

madrob · 2023-02-02T12:44:22Z

buildSrc/src/main/java/org/apache/lucene/gradle/GradlePropertiesGenerator.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.gradle;


Just because this was based on a Lucene class doesn't mean this is a Lucene class. Should probably be packaged under o.a.solr.gradle?

+1 -- analogous to this namespace issue. Not sure whether there are practical implications to the naming in this case, but regardless, solr package makes sense here.

I thought about it too but thought we could leave it for another day. Although it would be natural to change when we're touching this area.

I only put it there because it's adjacent to the https://github.com/apache/solr/blob/1d76b053c4a6fb742a97f4250b332d755680d00e/buildSrc/src/main/java/org/apache/lucene/gradle/WrapperDownloader.java that's also being used in gradlew. In actual fact, neither of them belong in buildSrc, because they aren't used by the gradle scripts themselves. I just didn't want to do something different.

I could move them both to https://github.com/apache/solr/tree/1d76b053c4a6fb742a97f4250b332d755680d00e/gradle
Either into a new directory within it, or put the WrapperDownloader in https://github.com/apache/solr/tree/1d76b053c4a6fb742a97f4250b332d755680d00e/gradle/wrapper and the new generator in https://github.com/apache/solr/tree/1d76b053c4a6fb742a97f4250b332d755680d00e/gradle/generation?

Remove the package completely. As this is a command line class only and it is not even compiled, theres no need to have a package.

I think the reason it was originally placed there was so that it lived together with other "sources" - so that any validation checks, formatting, etc. would apply. I don't think we actually do it to buildSrc (haven't checked) but we could, in which case it'd be easier to maintain it as part of buildSrc than in some ad-hoc location.

Can we spin that refactoring out in a separate PR and merge this one, now that it is stable?

I would keep the lucene namespace as at moment this makes merging cross source trees easier (I would like to port this over to Lucene, so cherry picking this commit works from there if the path names are similar). At moment I try to port most lucene and solr gradle build imüprovements to the other project, so aligned path names makes this easier.

Does someone want to merge it then? :)

janhoy · 2023-02-03T15:37:53Z

Please add a line to CHANGES.txt for 9.2 release. It can be useful for folks to get alerted about this change.

risdenk · 2023-02-06T15:19:27Z

Thanks @colvinco!

uschindler · 2023-02-06T16:05:02Z

Thanks, I created apache/lucene#12131 for Apache Lucene (cherrypicking was easy). Please have a look.

colvinco force-pushed the SOLR-16641 branch from 0248579 to a2944cd Compare January 31, 2023 09:38

uschindler requested a review from dweiss January 31, 2023 11:21

colvinco force-pushed the SOLR-16641 branch from a4a7597 to 8335779 Compare January 31, 2023 14:23

dweiss approved these changes Jan 31, 2023

View reviewed changes

gradle/validation/jar-checks.gradle Outdated Show resolved Hide resolved

risdenk approved these changes Jan 31, 2023

View reviewed changes

colvinco marked this pull request as draft January 31, 2023 17:08

colvinco force-pushed the SOLR-16641 branch from 9495f8a to aabacb5 Compare January 31, 2023 19:04

risdenk requested changes Jan 31, 2023

View reviewed changes

gradlew Outdated Show resolved Hide resolved

colvinco force-pushed the SOLR-16641 branch from 106d6c8 to b36acb3 Compare January 31, 2023 19:13

dsmiley reviewed Jan 31, 2023

View reviewed changes

colvinco changed the title ~~SOLR-16641 - Fix circular task dependency for gradle.properties~~ SOLR-16641 - Generate gradle.properties from gradlew Jan 31, 2023

colvinco force-pushed the SOLR-16641 branch from b36acb3 to 25ba9aa Compare February 1, 2023 10:40

SOLR-16641 - Generate gradle.properties from template

e9aeeac

colvinco force-pushed the SOLR-16641 branch from 25ba9aa to e9aeeac Compare February 1, 2023 10:41

colvinco commented Feb 1, 2023

View reviewed changes

gradle.properties.template Outdated Show resolved Hide resolved

colvinco requested a review from dweiss February 1, 2023 10:48

colvinco marked this pull request as ready for review February 1, 2023 10:48

colvinco mentioned this pull request Feb 1, 2023

SOLR-16641 - Generate gradle.properties from template #1322

Closed

Fix GENERATOR_STATUS

ae62cb2

risdenk approved these changes Feb 1, 2023

View reviewed changes

colvinco added 2 commits February 1, 2023 20:27

Fix up formatting and move localSettings help into the properties file

a888803

Tweak template comment

5a901b5

risdenk approved these changes Feb 1, 2023

View reviewed changes

colvinco requested a review from uschindler February 1, 2023 20:34

colvinco commented Feb 1, 2023

View reviewed changes

gradle/template.gradle.properties Outdated Show resolved Hide resolved

dweiss approved these changes Feb 2, 2023

View reviewed changes

Tweak template comment and bring back a simple helpLocalSettings

d9d6e83

uschindler requested changes Feb 2, 2023

View reviewed changes

uschindler reviewed Feb 2, 2023

View reviewed changes

gradle/template.gradle.properties Outdated Show resolved Hide resolved

janhoy approved these changes Feb 2, 2023

View reviewed changes

gradle/template.gradle.properties Outdated Show resolved Hide resolved

colvinco added 2 commits February 2, 2023 09:23

Use string replacement instead

f84d494

Remove toolchain properties from template

4a1f767

janhoy reviewed Feb 2, 2023

View reviewed changes

buildSrc/src/main/java/org/apache/lucene/gradle/GradlePropertiesGenerator.java Show resolved Hide resolved

Exit if file exists

1d76b05

colvinco requested a review from uschindler February 2, 2023 10:11

uschindler approved these changes Feb 2, 2023

View reviewed changes

colvinco mentioned this pull request Feb 2, 2023

Generate gradle.properties from gradlew apache/lucene#12123

Closed

madrob reviewed Feb 2, 2023

View reviewed changes

Update CHANGES.txt

89fdbe4

risdenk self-assigned this Feb 6, 2023

risdenk merged commit 50a0648 into apache:main Feb 6, 2023

colvinco deleted the SOLR-16641 branch February 6, 2023 15:21

risdenk pushed a commit that referenced this pull request Feb 6, 2023

SOLR-16641 - Generate gradle.properties from gradlew (#1320)

8b3825d

uschindler mentioned this pull request Feb 6, 2023

Port over gradle setting generator from Solr apache/lucene#12131

Merged

SOLR-16641 - Generate gradle.properties from gradlew #1320

SOLR-16641 - Generate gradle.properties from gradlew #1320

Conversation

colvinco commented Jan 31, 2023 • edited

Description

Solution

Tests

Checklist

uschindler commented Jan 31, 2023

magibney commented Jan 31, 2023

colvinco commented Jan 31, 2023

dweiss commented Jan 31, 2023

magibney commented Jan 31, 2023

colvinco commented Jan 31, 2023

dsmiley left a comment

Choose a reason for hiding this comment

dweiss commented Jan 31, 2023

colvinco commented Jan 31, 2023

dsmiley commented Jan 31, 2023

colvinco commented Feb 1, 2023

colvinco commented Feb 1, 2023

colvinco commented Feb 1, 2023

colvinco commented Feb 1, 2023

risdenk left a comment

Choose a reason for hiding this comment

dweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler left a comment

Choose a reason for hiding this comment

uschindler Feb 2, 2023 • edited

Choose a reason for hiding this comment

janhoy left a comment

Choose a reason for hiding this comment

uschindler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janhoy commented Feb 3, 2023

risdenk commented Feb 6, 2023

uschindler commented Feb 6, 2023

colvinco commented Jan 31, 2023 •

edited

uschindler Feb 2, 2023 •

edited

uschindler Feb 2, 2023 •

edited