Build refactoring and cleanups (moving from build scripts to convention plugins) #14764
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…elp now omits gradle's default boilerplate.
…ptions: version.release and version.suffix.
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
|
notes from two core system:
|
|
Thank you.
Yup, I did try to check everything as I went along (regenerate, etc.).
"Alternatively, the generated gradle.properties is also supported since it declares project properties (equivalent to -Pkey=value)." gradle.properties just picks up project properties - it's an equivalent of saying -Pkey=value on command line. So yes, these will be respected just fine. I had some hopes we'd be able to make gradle.properties versioned, eventually, but I haven't found a way around setting the number of worker jvms. It's fine any way you set it.
Yes, this one is a pain because the formatter is Eclipse-based and downloads half the internet... Again - haven't found a way to work around it but I love the fact i don't need to care about formatting groovy code and it takes care of it for me. |
This was referenced Jun 10, 2025
Open
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
breskeby
reviewed
Jun 12, 2025
| if (!isIdea) { | ||
| plugins.withType(JavaPlugin).configureEach { | ||
| SourceSet main = sourceSets.main | ||
| main.java.srcDirs = rootProject.files("build-tools/build-infra/src/main/java") |
There was a problem hiding this comment.
referencing rootProject is deprecated and will break running gradle with configuration cache and isolated project I think.
Suggested change
| main.java.srcDirs = rootProject.files("build-tools/build-infra/src/main/java") | |
| main.java.srcDirs = layout.getSettingsDirectory().files("build-tools/build-infra/src/main/java") |
There was a problem hiding this comment.
There are lots and lots of these. It has to be a follow-up in smaller batches, otherwise it'll become too large of a patch to do at once.
breskeby
reviewed
Jun 12, 2025
| } | ||
|
|
||
| tasks.withType(SpotlessTask).configureEach { | ||
| it.projectDir.set(rootProject.file("build-tools/build-infra")) |
breskeby
reviewed
Jun 12, 2025
| } | ||
| } | ||
|
|
||
|
|
||
| // TODO: not sure whether this should live in benchmarks, but for now let it be. | ||
| configure(project(":lucene:benchmark")) { |
There was a problem hiding this comment.
this is quite an anti pattern in gradle. Maybe out of scope but definitely something to fix later. this will break isolated projects and configuration cache I think and should live in the benchmark gradle script.
There was a problem hiding this comment.
I never understood why. When did it become an anti-pattern? Because from the start configuring projects like this was one of the features and strengths of this build system - pulling the common logic in one place and applying it to those projects that need it.
I wonder, what is the alternative?
There was a problem hiding this comment.
One negative consequence I found when maintaining a fork of Solr (using the same AOP style Gradle build) at a company is that locally removing a module means there are a number of build scripts that fail because they can't resolve the removed module. Ideally, the AOP technique would be graceful to a non-existent module. I understand if nobody-cares.
There was a problem hiding this comment.
Right - if there are project-specific blocks (not just 'all-java-projects' but project-path specific blocks) then indeed this will require a review of all build files. I don't know if this is solvable. It's an orthogonal approach to having to configure most things per-project. A matter of taste, I guess.
breskeby
reviewed
Jun 12, 2025
| downloadTasks.each { | ||
| it.configure { | ||
| group = "Data set download" | ||
| doFirst { |
There was a problem hiding this comment.
I wonder if this ext.name should be better reworked to be a property in Download task in addition to the log message.
There was a problem hiding this comment.
I agree with you but again - leaving for smaller improvements later. I also find the new gradle conventions extremely counterintuitive. To me, the groovy code reads. Once you get into properties, providers and lazy evaluation, things become so much more complicated, at least to me.
There was a problem hiding this comment.
What are "the new gradle conventions" that you speak of? I wish I was aware of whatever the conventions should be since in my experience there are too many styles and I don't know which to prefer or when not to.
breskeby
reviewed
Jun 12, 2025
| @@ -15,20 +15,22 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| def resources = scriptResources(buildscript) | |||
| def resources = rootProject.file("gradle/documentation/check-broken-links") | |||
There was a problem hiding this comment.
Suggested change
| def resources = rootProject.file("gradle/documentation/check-broken-links") | |
| def resources = layout.settingsDirectory.file("gradle/documentation/check-broken-links").get().asFile |
breskeby
reviewed
Jun 12, 2025
| sourceSets.matching{ it.name ==~ /main\d+/ }.all{ SourceSet sourceSet -> | ||
| project.afterEvaluate { | ||
| plugins.withType(JavaPlugin).configureEach { | ||
| project.afterEvaluate { |
There was a problem hiding this comment.
sourceSets.matching.configureEach is lazy. you should not need the project.afterEvaluate here
breskeby
reviewed
Jun 12, 2025
| @@ -15,15 +15,20 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| // Configure release artifact signing. | |||
| allprojects { project -> | |||
There was a problem hiding this comment.
(excessive) usage of allprojects within convention plugins should be avoided. It will break the ability to move towards isolated project. Instead conventions plugins should be applied on the actual project. Be careful with using allprojects and subprojects usages across the build
There was a problem hiding this comment.
It will break the ability to move towards isolated project
What does that mean? Perhaps meaning, moving the build plugin to something isolated/reusable?
breskeby
reviewed
Jun 12, 2025
There was a problem hiding this comment.
Hope you don't mind me chiming in as this PR catched my attention. I'm Rene and maintain the elasticsearch gradle build and have been involved with Gradle directly since 0.4. I added a few remarks.
In general I think this is a great step forward into a better build structure. In general I think those convention plugins are still quite heavy. by that I mean they contain a lot imperative build logic. Usually those convention plugins are meant to be small and simple. More evolved logic (IMO everything that involves nested if statements and above) might better live in binary plugins. Those are also way better testable than convention plugins.
Not saying all this work should be part of this PR but just to be kept in mind when touching build logic.
I know who you are, @breskeby (from mailing lists googling for answers...) :) Thank you for jumping in! I would love to draw from your experience because - I admit - I am quite lost with those new gradle developments... the first versions had a lot of appeal to me but seemed a lot more intuitive... A graph of tasks/ projects and stuff applied across this graph. One of the things you mentioned is probably the primary thing I struggle with - the subprojects/ allprojects block. The entire idea of Lucene's build logic was based on applying separate concerns across all affected projects. How are you expected to do it without these features? What is the "proper way"?
One of the reasons for using convention plugins was to keep those "build aspects" separate and easy to identify. I do plan to move to java code, eventually, but this is a technicality - it doesn't solve the problems/ issues I currently have trouble with solving (like the insanity of tracking lazy evaluation cross-dependencies)... If you do have time to help out, it would be great if you took an example small bit and tried to fix it "the right way" - perhaps after this patch is committed, so we all can learn how it should be done and follow your tracks. It'd be also great if I (or we) could ask for your review on smaller gradle patches - this would be a great way for me to improve. |
|
I'll merge this as is. Listed all smaller (?) follow-up tasks in #14779. |
github-project-automation
bot
moved this from Open
to Merged
in OpenSearch Lucene & Core Performance Tracking
Please keep this for longer time, as the Jenkins build randomization can only work on environment variables. There maybe tweaks to pass this on command line in Jenkins's gradle options, but I don't want to touch that now. In addition I have this locally with my shell customization. To test other Java versions, I can quickly open a shell window with all environment variables including RUNTIME_JAVA_HOME. Loosing that is a big desaster to me! Uwe |
It is also slow when you run it later. It doenloads a lot of shit yes, but execution is also slow. You won't notice later unless you change anything in build infrastructure. An what's worst: If you have whitespace in your user home directory (common on windows, but we test this also on ASF Jenkins), it spams you with this insane amout of bullshit: It does not fail it just prints this bullshit possibly coming from Eclipse's OSGI mega-bullshit! I'd rather disable this Groovy/Gradle code formatting - sorry for this, but this took me ages on battery power today in train while debugging the other windows build failures on Jenkins. Whenever I changed a single line of Groovy code it printed the code above and sit 5 minutes waiting eating my battery power, |
No worries at all. These build options all work with env variables already but their naming is identical to the build option name (so lowercased, with dots, etc.). Maybe this could be improved/ changed in the plugin itself, I'll think about it. No problem with keeping whatever aliases we like though. |
Ok, I'll add an option to turn it off, locally. I agree - it's slow and huge. But it's the only way to verify/apply formatting automatically that I know about. #14789 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have been working for over two months trying to convert the existing gradle build into a "more modern" gradle build setup. The motivation for this is mostly to increase the speed of build execution but there are also deprecations and looming changes in gradle that will bite us hard if we don't try to clean up.
I have been doing this on my own because it's a large effort that requires frequent backtracking... and I also didn't have a clear vision where I wanted it to go. I think it's in a better form now than it is on main so I'd like you to review and consider switching over to this new setup.
It isn't all sweet. In fact, it's a bit disheartening. Read on, sorry for the lengthy introduction.
I really like the "aspect-oriented" setup of build files, where each file was configuring a certain aspect of the build: for example spotless or forbiddenapis checker applied across all projects. I really wanted to somehow preserve this but also speed up the initialization and evaluation phases. I thought gradle's "convention plugins" would be a good fit because they're true "gradle plugins" (are precompiled, live in a separate project) but allow us to keep the liberal style of build scripts.
I set out to convert all those existing build scripts we currently have into convention plugins. Two months later, things generally work well - these scripts are precompiled, the syntax highlighting and suggestions work better, there are fewer headaches with script classpath... but the initialization time nearly doubled instead of going down. This first-run generates some stubs and compiles all convention plugins. Once they're compiled, things seem to be a bit faster (even from a cold start, without a daemon). But the difference isn't huge - maybe one or two seconds (it takes 8 seconds to get to the execution phase on my machine). The first run though (from a clean checkout) is twice longer (24 seconds instead of 12 on my machine)...
I don't know why it's so slow. My long-term plan is to move some of those convention plugins to Java, which should be faster to compile and execute (...and read?). This patch is already large and I didn't want it to grow beyond comprehension - most of the code remains identical as in main, it is just moved, reshuffled or slightly modernized (to use build options, lazy evaluation APIs, etc.).
Now, the slowdown was a bummer but there are also benefits. I already mentioned a few: these convention plugins live under a separate composite gradle project and use a more reasonable infrastructure (dependencies, cross-visibility to Java classes, etc.). It should make it much easier to gradually improve this over time compared to build scripts. I also really like the replacement for "propertyOrDefault" - a custom plugin that implements "build options" - tweakable build parameters like
tests.itersortests.jvmswith the difference being that now one can list all the configurable parameters of each Lucene project and see their current values (and the source they come from). It works like a charm on this branch and I can think of a few things to make it even better. Just look at this:So, please try to switch to the PR's branch and run whatever you typically run. See if it works. Share your thoughts.
Below is a more structured record of larger changes included in this patch.
buildOptionsplugin instead ofpropertyOrDefaultmethodAll "propertyOrDefault*" methods have been removed and an external
plugin for configuring various build options is now used. This is
the plugin (blame me for any shortcomings):
https://github.com/carrotsearch/gradle-build-infra#plugin-comcarrotsearchgradlebuildinfrabuildoptionsbuildoptionsplugin
this plugin will source the value of a given build option from
several places, in order:
-Dfoo=value),-Pfoo=value),foo=value ./gradlew ...)build-options.local.propertiesproperty file(for your personal, local tweaks),
build-options.propertiesproperty file.It works much like before - you can override any build option
temporarily by passing, for example,
-Ptests.verbose=true. But youcan also make such changes locally persistent by placing them
in your
build-options.local.propertiesfile. Alternatively,the generated
gradle.propertiesis also supported since it declaresproject properties (equivalent to
-Pkey=value). At some pointin the future, we may want to keep gradle.properties versioned though,
so it's probably a good idea to switch over to
build-options.local.properties.The biggest gain from using build options is that you can now see
all declared options and their values, including their source (where their
value comes from). To see what I mean, try running these:
Lucene 'base version' string moved
Their default values are in
build-options.properties. You can overrideany of
version.suffix,version.baseorversion.releaseusing any of the build option plugin's methods. If you'd like to bump
Lucene's base version, it now lives in the versioned
build-options.properties.Tweaks to properties and options
runtime.java.homeis a build option now but the support forRUNTIME_JAVA_HOMEenv. variable has been implemented for backwardcompatibility.
tests.neverUpToDateoption is renamedtests.rerun.The value of
trueindicates test tasks will re-run even ifnothing has changed.
Changes to project structure
build-tools/missing-docletis now a regular project within Lucene (notan included composite project). This simplifies the build and ensures we apply
the same checks to this subproject as we do to everything else.
all gradle build scripts are converted to convention plugins (in groovy) and
live under
dev-tools/build-infra/src/main/groovy. The long-termplan is to move some or all of these groovy plugins to Java (so that the code is
easier to read and debug for non-groovians).
tidyis now applied to groovy/gradle scripts (formatting is enforced).Security manager support
removed, without any replacement.
Other notable changes
The legacy
precommittask has been removed; use gradle'scheck.Removed dependency on jgit entirely. This is replaced by forking the system's git
in porcelain mode (which should be stable and portable). This logic is implemented
in this plugin.
An additional benefit is that all features of git should now work (including worktrees).
Added support for owasp API keys in the form of
validation.owasp.apikeybuild option. Owasp check isstill very, very slow. We should probably just drop it.
I've changed the default on
gradle.ge(Gradle Enterprise, develocity) tofalseon non-CI builds.
I've tried to clean up lots and lots of gradle/groovy code related to eager initialization of
tasks as well as update deprecated APIs. This isn't always straightforward and I might have broken
some things...
Fixes to existing issues
gradlew clean checkwill work now ("gradlew clean check" results in internal gradle error "Unable to make progress running work." #13567)