jamesfredley commented on code in PR #16077:
URL: https://github.com/apache/grails-core/pull/16077#discussion_r3699713038
##########
end-to-end/spring-dependency-management/build.gradle:
##########
@@ -30,22 +30,26 @@
// * io.spring.dependency-management is applied directly and imports
grails-bom as
// a Maven BOM, exactly as a migrated Grails 7 build would, so that the
Spring DM
// plugin is the source of truth for managed versions here.
+//
+// It is an end-to-end test because a Maven BOM import is only satisfiable by
a real
+// published pom - see the note on the include in settings.gradle.
plugins {
+ id 'groovy'
id 'org.apache.grails.buildsrc.properties'
id 'org.apache.grails.buildsrc.compile'
id 'org.apache.grails.buildsrc.vulnerability-scan'
+ id 'org.apache.grails.gradle.grails-web'
+ id 'org.apache.grails.gradle.grails-gsp'
}
version = '0.1'
-group = 'functionaltests'
-
-apply plugin: 'org.apache.grails.gradle.grails-web'
-apply plugin: 'org.apache.grails.gradle.grails-gsp'
+group = 'springdm'
Review Comment:
**Optional nit:** `group` changed from `functionaltests` to `springdm`. No
functional need jumps out for the move. Fine either way; only worth reverting
if you want a pure rename/move diff.
##########
end-to-end/spring-dependency-management/build.gradle:
##########
@@ -63,7 +69,7 @@ dependencyManagement {
// the grails-bom import, so the security-patched logback isn't applied
automatically. A migrated Grails 7
// app hitting the same CVE would override the Spring-managed version property
directly; reproduce that here,
// sourcing the number from dependencies.gradle so it stays the single source
of truth.
-apply from: rootProject.layout.projectDirectory.file('dependencies.gradle')
+apply from: rootProject.layout.projectDirectory.file('../dependencies.gradle')
ext['logback.version'] = bomDependencyVersions['logback.version']
// Same situation for the Jackson 3 security override (CVE-2026-59889) - see
jackson3.version in dependencies.gradle.
ext['jackson-bom.version'] = bomDependencyVersions['jackson3.version']
Review Comment:
**Optional:** Logback/jackson property overrides correctly document that
Spring Boot version properties can win over the grails-bom import.
With the real `mavenBom` still here (unlike #16073's final approach),
coordinate-level management from grails-bom should pin most other coords. A
green `HelloControllerSpec` still will not notice silent property wins on e.g.
groovy/log4j2/commons-codec.
If you want belt-and-suspenders without rewriting the fixture: a small
`verify…` task on `check` asserting a few resolved versions (at least
`org.apache.grails:*` → `projectVersion`, plus these logback/jackson pins). Not
a merge blocker.
##########
end-to-end/spring-dependency-management/build.gradle:
##########
@@ -81,6 +87,18 @@ dependencies {
integrationTestImplementation
'org.apache.grails:grails-testing-support-http-client'
}
+// The core build's gradle/functional-test-config.gradle is deliberately not
applied here, for the
+// same reasons as in legacy-commands: its dependency substitution enumerates
+// rootProject.subprojects, which only makes sense inside the core build - and
substituting
+// projects is the very thing this project must not do, since the point is to
import a published
+// grails-bom pom. Its remaining job, the per-suite skip flags keyed off
grails-test-examples-*
+// project names, has no meaning in this build, which is driven by its own
workflow.
apply {
- from
rootProject.layout.projectDirectory.file('gradle/functional-test-config.gradle')
+ from rootProject.layout.projectDirectory.file('gradle/test-config.gradle')
+}
+
+tasks.withType(Test).configureEach {
+ // Allow extra headroom for slow start-up under CI load (the client
default is 60s), matching
+ // what gradle/functional-test-config.gradle gave this application in the
core build.
+ systemProperty('grails.http.client.timeout', '120')
Review Comment:
**Optional follow-up:** Timeout carry-over looks good.
Related optional idea from #16073: a tiny verification task that fails on
version drift. The IT proves the app boots under Spring DM against the
published BOM; it does not prove managed versions match the tree under test.
Easy to add later if you prefer not to grow this PR.
##########
settings.gradle:
##########
@@ -609,29 +609,6 @@ project(':grails-test-examples-jetty').projectDir =
file('grails-test-examples/j
project(':grails-test-examples-undertow').projectDir =
file('grails-test-examples/undertow')
project(':grails-test-examples-latency').projectDir =
file('grails-test-examples/latency')
-// The Spring Dependency Management example imports grails-bom as a Maven BOM
through
-// io.spring.dependency-management, which resolves BOM imports with its own
detached
-// configurations. Those bypass the local-project substitution rules in
-// gradle/functional-test-config.gradle, so the import only works when
-// org.apache.grails:grails-bom:<projectVersion> is already published to a
repository.
-// During a release (reproducible build, SOURCE_DATE_EPOCH set) the version
being staged
-// is not published anywhere yet, so the example cannot resolve its
dependencies -
-// exclude it from the build graph entirely. It still builds and runs on every
regular
-// CI build, where the -SNAPSHOT BOM is available from the Apache snapshots
repository.
-//
-// A newly created release branch is the same condition: the moment
projectVersion is
-// bumped to a version that has never been published, the BOM import silently
resolves
-// to nothing, every managed version comes back empty, and the whole build
fails with
-// "Could not find <group>:<artifact>:" - before CI can ever publish the
snapshot that
-// would fix it. Set -PbomSnapshotNotPublished (or add it to
gradle.properties) on such
-// a branch to exclude the example until its first snapshot publish succeeds,
then drop
-// the flag again. The flag is presence-based; its value is ignored.
-def bomSnapshotNotPublished =
providers.gradleProperty('bomSnapshotNotPublished').isPresent()
Review Comment:
Removing this from core is the load-bearing part of the bootstrap fix - core
no longer needs a published BOM (or `bomSnapshotNotPublished`) to configure.
**Merge-up (track explicitly):** when this change lands on `8.1.x` /
`9.0.x`, delete `bomSnapshotNotPublished` from those branches'
`gradle.properties` and any remaining comments. Until the example is gone from
core on those lines the flag is still needed; after the forward-port it is dead
config.
##########
end-to-end/README.md:
##########
@@ -35,12 +35,21 @@ applications at via `GRAILS_REPO_URL`.
| `legacy-g7-command-plugin` | A **standalone build**, not part of this one.
Compiles against published Grails 7 / Groovy 4 to produce a genuine precompiled
`grails.dev.commands.ApplicationCommand` binary. |
| `legacy-commands-plugin` | A Grails 8 plugin whose legacy commands are
recompiled under Groovy 5. |
| `legacy-commands` | A Grails 8 application that consumes both and runs their
commands through the registry. |
+| `spring-dependency-management` | A Grails 8 application that manages its
versions with the legacy `io.spring.dependency-management` plugin instead of
the Grails Gradle plugin's native `platform(grails-bom)`, as an upgraded Grails
7 application does. |
`legacy-g7-command-plugin` is deliberately excluded from `settings.gradle`. An
included build would
substitute `org.apache.grails:grails-core` for this repository's Groovy 5
project, which is exactly
the substitution the fixture exists to avoid — it must be compiled by a real
Grails 7 toolchain for
its trait-woven bytecode to prove anything.
+`spring-dependency-management` is here because Spring DM imports a BOM as a
**Maven** BOM, resolving
+it in its own detached configuration. That bypasses any project substitution,
so the import can only
+ever be satisfied by a published
`org.apache.grails:grails-bom:<projectVersion>` — which is precisely
+what this build already provides. In the core build it had to be excluded
whenever nothing had been
+published yet (a reproducible release build, or a fresh release branch whose
version has never been
+published), and otherwise silently fell back to whatever the Apache snapshot
repository happened to
+hold rather than the working tree.
Review Comment:
Docs read well - why it lives here, publish steps, and `DO_NOT_CACHE_TESTS`
are exactly what a future reader needs. No change requested.
##########
end-to-end/gradle/test-config.gradle:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Set DO_NOT_CACHE_TESTS=1 to force test tasks to re-run on every invocation
without needing
+// --rerun-tasks. Useful for repeatedly running the same test command while
chasing flaky tests
+// across runs. The core build, grails-gradle and grails-forge all honour the
same variable.
+def doNotCacheTests = System.getenv('DO_NOT_CACHE_TESTS')?.toBoolean()
+
+tasks.withType(Test).configureEach {
+ useJUnitPlatform()
+
+ // Honor DO_NOT_CACHE_TESTS=1 so developers can repeatedly invoke the same
test command
+ // without --rerun-tasks (and without recompiling everything else). It
matters more in this
+ // build than in the core one: republishing grails-core does not change
these projects'
+ // inputs in any way Gradle can see, so re-running against freshly
published artifacts is
+ // exactly the case that otherwise comes back UP-TO-DATE or FROM-CACHE.
+ outputs.cacheIf { !doNotCacheTests }
+ outputs.upToDateWhen { !doNotCacheTests }
Review Comment:
**Optional:** Matching core's `DO_NOT_CACHE_TESTS` pattern is the right call
- republished `build/local-maven` is invisible to Gradle inputs, so FROM-CACHE
is a real footgun locally.
For CI, consider also setting `DO_NOT_CACHE_TESTS=1` on the workflow `check`
step. Not required on a clean runner that always publishes first; cheap
insurance now that this workflow is the sole home of the Spring DM BOM-import
fixture.
Also worth confirming the E2E job is a **required** status on `8.0.x` - if
optional, this path can go red without blocking merge.
##########
end-to-end/README.md:
##########
@@ -81,6 +90,13 @@ cd end-to-end
Re-run the publish whenever you change something in the core build that these
tests exercise;
nothing here can detect that for you, because the whole point is that the
build boundary is real.
+For the same reason a re-run against freshly published artifacts often comes
back `UP-TO-DATE` —
+nothing Gradle can see about these projects changed. `DO_NOT_CACHE_TESTS`
(honoured here exactly as
+in the core build, see `DEVELOPMENT.md`) forces the test tasks to run anyway:
+
+```shell
+DO_NOT_CACHE_TESTS=1 ./gradlew check
+```
Review Comment:
Good local guidance. **Optional companion:** set the same env var on the CI
`check` step so the "republish then re-run" case cannot come FROM-CACHE in
automation either.
##########
end-to-end/settings.gradle:
##########
@@ -103,10 +104,26 @@ rootProject.name = 'grails-end-to-end'
include(
'legacy-commands',
'legacy-commands-plugin',
+ // Belongs here rather than in grails-test-examples: it imports
grails-bom as a Maven BOM
+ // through io.spring.dependency-management, which resolves imports in
its own detached
+ // configuration. That bypasses any project substitution, so the
import can only ever be
+ // satisfied by a published
org.apache.grails:grails-bom:<projectVersion> - which is what
+ // the repository above is. In the core build it had to be excluded
whenever nothing had
+ // been published yet (a reproducible release build, or a fresh
release branch whose
+ // version has never been published), and otherwise resolved against
whatever the Apache
+ // snapshot repository held rather than the working tree.
+ 'spring-dependency-management',
Review Comment:
Agree with the placement and the comment. Correct home for a fixture that
must import a published `grails-bom` pom through Spring DM's detached
configuration.
**Optional PR-body note:** leaving core means this fixture no longer rides
the Win/macOS/J25 functional matrix or SiteMesh 2 path filters. Acceptable for
a published-artifact boundary test; just call the trade-off out so nobody
assumes matrix parity with `grails-test-examples/*`.
##########
end-to-end/spring-dependency-management/build.gradle:
##########
@@ -55,6 +59,8 @@ grails {
dependencyManagement {
imports {
+ // Resolved by Spring DM's own detached configuration, so it is a
genuine Maven BOM import
+ // of the pom the core build published into build/local-maven - not a
substituted project.
mavenBom "org.apache.grails:grails-bom:${projectVersion}"
Review Comment:
This is the right thing to keep. The fixture exists to exercise a real
Spring DM `mavenBom` import of `grails-bom`, which only a published pom can
satisfy. Putting that behind `build/local-maven` + `exclusiveContent` is why
end-to-end is the correct home (vs hand-managing versions from
`dependencies.gradle` as in #16073).
##########
.github/workflows/end-to-end.yml:
##########
@@ -20,7 +20,10 @@
# substitution - that is what makes the tests end-to-end. The repository is
the same
# build/local-maven that grails-forge points its generated applications at,
populated by
# publishAllPublicationsToTestCaseMavenRepoRepository, so the suite exercises
real poms and
-# module metadata including the CLI companion artifacts.
+# module metadata including the CLI companion artifacts. That is a hard
requirement for some of
+# these projects rather than a nicety: spring-dependency-management imports
grails-bom as a Maven
+# BOM through io.spring.dependency-management, which resolves it in its own
detached configuration
+# and so can only ever consume a published pom.
Review Comment:
Good callout in the workflow header - this is now a hard requirement for the
suite, not a nicety.
**Optional:** on the `Run the end-to-end tests` step, set
`DO_NOT_CACHE_TESTS: '1'` so a build-cache hit cannot skip tests after the
publish steps above. Low probability on clean GHA runners; low cost.
##########
end-to-end/settings.gradle:
##########
@@ -103,10 +104,26 @@ rootProject.name = 'grails-end-to-end'
include(
'legacy-commands',
'legacy-commands-plugin',
+ // Belongs here rather than in grails-test-examples: it imports
grails-bom as a Maven BOM
+ // through io.spring.dependency-management, which resolves imports in
its own detached
+ // configuration. That bypasses any project substitution, so the
import can only ever be
+ // satisfied by a published
org.apache.grails:grails-bom:<projectVersion> - which is what
+ // the repository above is. In the core build it had to be excluded
whenever nothing had
+ // been published yet (a reproducible release build, or a fresh
release branch whose
+ // version has never been published), and otherwise resolved against
whatever the Apache
+ // snapshot repository held rather than the working tree.
+ 'spring-dependency-management',
)
// legacy-g7-command-plugin is deliberately NOT included here. It compiles
against published
// Grails 7 / Groovy 4 artifacts, and an included build would substitute
// org.apache.grails:grails-core for this repo's Groovy 5 project - which is
exactly the
// substitution the fixture exists to avoid. It stays a standalone build, run
under its own JDK
// (legacy-g7-command-plugin/.sdkmanrc), and legacy-commands consumes the jar
it produces.
+
+// Due to https://github.com/gradle/gradle/issues/2986 , we can't change the
global exclude for profiles, it must be done everywhere
+for (String pattern in DirectoryScanner.defaultExcludes) {
+ if (pattern.contains('gitignore') || pattern.contains('gitattributes')) {
+ DirectoryScanner.removeDefaultExclude(pattern)
+ }
+}
Review Comment:
**Optional / out of band:** Same Gradle #2986 `DirectoryScanner` workaround
as root (also added in `build-logic` / `grails-gradle` / `grails-forge`).
Legitimate for each independent settings script - Ant state is per-build - and
not required for the Spring DM move itself.
Fine to keep for consistency. If you want a thinner review surface, the
non-`end-to-end` copies could be a tiny chore PR; not worth blocking this one.
##########
build-logic/settings.gradle:
##########
@@ -91,3 +92,10 @@ project(':build-logic').projectDir = file('plugins')
include 'grails-docs-core'
project(':grails-docs-core').projectDir = file('docs-core')
+
+// Due to https://github.com/gradle/gradle/issues/2986 , we can't change the
global exclude for profiles, it must be done everywhere
+for (String pattern in DirectoryScanner.defaultExcludes) {
+ if (pattern.contains('gitignore') || pattern.contains('gitattributes')) {
+ DirectoryScanner.removeDefaultExclude(pattern)
+ }
+}
Review Comment:
**Optional:** Same note as on `end-to-end/settings.gradle` - fine for
consistency across independent builds, not required for the Spring DM move. No
objection to keeping.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
