Skip to content

Extract shared BOM build conventions#15827

Open
jamesfredley wants to merge 5 commits into
8.0.xfrom
fix/bom-build-script-cleanups
Open

Extract shared BOM build conventions#15827
jamesfredley wants to merge 5 commits into
8.0.xfrom
fix/bom-build-script-cleanups

Conversation

@jamesfredley

@jamesfredley jamesfredley commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR reduces duplicated Gradle BOM build logic by moving the shared publication, POM customization, constraint extraction, and release snapshot validation conventions into org.apache.grails.buildsrc.bom-conventions.

The BOM build scripts now keep only BOM-specific inputs such as descriptions, dependency declarations, and variant-specific dependency sets. The shared plugin owns the common task wiring and POM mutation behavior used by the seven Grails BOM projects.

Changes

  • Add BomConventionsPlugin in build-logic/plugins and register it as org.apache.grails.buildsrc.bom-conventions.
  • Apply the shared plugin in all seven BOM projects.
  • Preserve BOM-specific dependency and platform declarations in each BOM script.
  • Reduce duplicated Groovy module override maps in dependencies.gradle while leaving one literal coordinate list for Dependabot visibility.
  • Remove unreachable BOM override branches for Micronaut/Hibernate variants.
  • Address Copilot feedback by using the configured gradleBuildProjects group mapping when registering Gradle-build artifacts for POM property replacement.
  • Add TestKit coverage for shared BOM plugin wiring and Gradle-build artifact POM property generation.

Verification

  • JAVA_HOME=/tmp/opencode/jdk21 ./gradlew :build-logic-root:build-logic:compileGroovy :build-logic-root:build-logic:test
  • JAVA_HOME=/tmp/opencode/jdk21 ./gradlew :build-logic-root:build-logic:test --tests org.apache.grails.buildsrc.BomConventionsPluginSpec
  • Generated all seven BOM POMs with -PincludeMicronautProjects.
  • Ran extractConstraints for all seven BOMs with -PincludeMicronautProjects.
  • Compared generated constraints against a clean origin/8.0.x baseline: no differences.
  • Compared generated POMs against the baseline: only the intended base BOM improvement differs, where Gradle-build artifacts now use generated version properties instead of inline 8.0.0-SNAPSHOT versions.

Notes

  • LSP diagnostics are unavailable locally for .groovy / .gradle because no Groovy/Gradle LSP is configured.
  • The full aggregate gate currently has unrelated baseline failures on origin/8.0.x (GraphQL static compile errors and MongoDB timeout flakes), reproduced in a clean baseline worktree.

Centralize duplicated BOM publication, constraint extraction, and POM customization logic in a build-logic convention plugin. Apply it across the BOM modules while keeping generated POM and constraints output byte-identical to the current 8.0.x baseline.

Assisted-by: Hephaestus:openai/gpt-5.5 codex-review
Copilot AI review requested due to automatic review settings July 5, 2026 00:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request extracts repeated Grails BOM build logic into a shared build-logic Gradle plugin, then applies it across the various BOM subprojects to reduce duplication and centralize publication / POM / constraints behavior.

Changes:

  • Add org.apache.grails.buildsrc.bom-conventions plugin to encapsulate common BOM conventions (publication hooks, constraints extraction wiring, snapshot validation, POM customization).
  • Update Grails BOM subprojects (default/base/hibernate*/micronaut*) to use the shared plugin and remove duplicated build script logic.
  • Refactor dependencies.gradle to generate Groovy module dependency maps via a shared helper and remove now-unreachable override branches.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
grails-bom/micronaut/build.gradle Switch Micronaut BOM to shared bom-conventions plugin; remove duplicated publication/constraints logic.
grails-bom/hibernate7/build.gradle Switch Hibernate 7 BOM to shared bom-conventions plugin; remove duplicated publication/constraints logic.
grails-bom/hibernate7-micronaut/build.gradle Switch Hibernate 7 + Micronaut BOM to shared bom-conventions plugin; remove duplicated publication/constraints logic.
grails-bom/hibernate5/build.gradle Switch Hibernate 5 BOM to shared bom-conventions plugin; remove duplicated publication/constraints logic.
grails-bom/hibernate5-micronaut/build.gradle Switch Hibernate 5 + Micronaut BOM to shared bom-conventions plugin; remove duplicated publication/constraints logic.
grails-bom/default/build.gradle Switch default BOM to shared bom-conventions plugin; remove duplicated publication/constraints logic.
grails-bom/base/build.gradle Switch base BOM to shared bom-conventions plugin; remove duplicated publication/constraints logic.
dependencies.gradle Extract Groovy module dependency map generation into a helper and simplify override branching.
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/BomConventionsPlugin.groovy New shared BOM conventions plugin implementing extracted common logic.
build-logic/plugins/build.gradle Register the new plugin and add required module dependency for BOM task classes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.5526%. Comparing base (ef09bcd) to head (7749b58).
⚠️ Report is 49 commits behind head on 8.0.x.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #15827        +/-   ##
==================================================
+ Coverage     49.4833%   49.5526%   +0.0692%     
- Complexity      16883      16927        +44     
==================================================
  Files            1998       1999         +1     
  Lines           93672      93753        +81     
  Branches        16400      16419        +19     
==================================================
+ Hits            46352      46457       +105     
+ Misses          40169      40133        -36     
- Partials         7151       7163        +12     

see 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Use the configured Gradle-build project group mappings when registering POM property replacements, and add focused TestKit coverage for the shared BOM conventions plugin.

Assisted-by: Hephaestus:openai/gpt-5.5 codex-review

@jdaugherty jdaugherty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made several comments inline, but my major issue is we should never use .matching {} - this gradle code is non-lazy.

task.dependsOn('generateMetadataFileForMavenPublication', 'generatePomFileForMavenPublication')
}

project.tasks.named('check') { Task task ->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds new lifecycle behavior that was not present in any of the extracted BOM scripts: check now runs extractConstraints, which also runs both publication generators and resolves the full BOM graph. grails-doc:generateBomDocumentation already invokes extractConstraints explicitly. It also makes a previously configuration-cache-compatible BOM check fail: ./gradlew :grails-base-bom:check --configuration-cache --rerun-tasks reports unsupported serialization of ConfigurationContainer, DependencyHandler, and Project. Could we remove this check dependency and keep constraint extraction explicit?

}

private static void configureCoordinates(Project project) {
project.version = project.findProperty('projectVersion')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extracted scripts used version = projectVersion, which fails with MissingPropertyException when projectVersion is not defined. findProperty returns null here, leaving project.version as unspecified; projectCoordinateProperties() and the POM customization then register coordinates like org.apache.grails:grails-gradle-plugins:unspecified, and version property replacement silently stops matching with no error. Suggest failing fast, e.g. project.property('projectVersion') or an explicit check.

if (project.extensions.extraProperties.get('isReleaseBuild') &&
project.extensions.extraProperties.get('isPublishedExternal')) {
project.tasks.matching { Task task ->
task.name in ['generateMetadataFileForMavenPublication', 'generatePomFileForMavenPublication']

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior change from the extracted scripts: they used afterEvaluate { tasks.named(...).configure { ... } }, which throws UnknownTaskException if the publication tasks are missing. tasks.matching { }.configureEach { } silently no-ops if these tasks are never created (e.g. the publish plugin renames its publication), so on a real release the snapshot validation could silently not run. Since this guard only matters when isReleaseBuild && isPublishedExternal, consider asserting the tasks exist in that case rather than matching by name.

dep.version[0].value = extractedConstraint.versionPropertyReference
pomProperties.put(extractedConstraint.versionPropertyName, inlineVersion)
}
} else if (!inlineVersion) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else if (!inlineVersion) is always true at this point — carried over from the original scripts, but since this code was rewritten anyway, a plain else would be clearer.

import org.apache.grails.gradle.tasks.bom.PropertyNameCalculator

@CompileStatic
class BomConventionsPlugin implements Plugin<Project> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename this to GrailsBomPlugin

project.extensions.configure(JavaPlatformExtension) { JavaPlatformExtension extension ->
extension.allowDependencies()
}
project.pluginManager.apply('org.apache.grails.buildsrc.publish')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not used aggregate plugins to date. These should be left on the individual projects (publish & sbom)

}

private static void applyDependencyDefinitions(Project project) {
project.apply([from: project.rootProject.layout.projectDirectory.file('dependencies.gradle')])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between this and before is it was applied in the buildscript so it was available in all contexts. I think this is ok, but it's important to note the difference.

Comment thread dependencies.gradle Outdated
// These files are split to facilitate separation of build vs application dependencies. These are the application dependencies.
static Map<String, String> groovyModuleDependencies(String groovyVersion) {
[
'', '-ant', '-astbuilder', '-cli-commons', '-cli-picocli', '-console', '-contracts', '-datetime',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is harder to read and should be reverted. the standard of this file is to list the dependencies. these extractions make it harder to maintain

Address review feedback against .matching {} and keep BOM publication
conventions on configureEach/named-style configuration.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]
@testlens-app

testlens-app Bot commented Jul 10, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 7749b58
▶️ Tests: 19099 executed
⚪️ Checks: 59/59 completed


Learn more about TestLens at testlens.app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants