jdaugherty commented on code in PR #16019:
URL: https://github.com/apache/grails-core/pull/16019#discussion_r3671179734
##########
grails-core/build.gradle:
##########
@@ -51,6 +51,8 @@ dependencies {
api 'jakarta.persistence:jakarta.persistence-api'
api 'jakarta.annotation:jakarta.annotation-api'
+ api project(':grails-beans-dsl')
Review Comment:
Correcting my earlier comment on this line: this `api` is right, and it
should stay.
`grails-core` is on every Grails project's classpath, so declaring the beans
DSL as `api` here is precisely how it should reach downstream projects — the
same thing `grails-core` already does for several other modules. The DSL is
effectively part of that default surface rather than an optional extra, and
`GlobalGrailsClassInjectorTransformation.compileBeansDsl` depends on exactly
that to make the implicit `def beans = { }` convention work for third-party
plugin authors.
What follows from it is that **no other module needs to declare
`grails-beans-dsl` at all**. I've left a note on each of the seven that
currently does. My previous comment asked for the nine declarations to "agree
on one story" — the story is this line, and the other seven should be deleted.
##########
grails-cache/build.gradle:
##########
@@ -49,6 +49,7 @@ dependencies {
api "org.codehaus.gpars:gpars:$gparsVersion"
api
"com.googlecode.concurrentlinkedhashmap:concurrentlinkedhashmap-lru:$concurrentlinkedhashmapLruVersion"
+ implementation project(':grails-beans-dsl')
Review Comment:
Drop this line — `grails-core` declares `api project(':grails-beans-dsl')`,
so the DSL is already on this module's compile classpath. See my note on
`grails-core/build.gradle:54`.
##########
grails-i18n/build.gradle:
##########
@@ -41,6 +41,8 @@ dependencies {
api project(':grails-web-core')
api 'org.apache.groovy:groovy'
+ implementation project(':grails-beans-dsl')
Review Comment:
Drop this line — `grails-core` declares `api project(':grails-beans-dsl')`,
so the DSL is already on this module's compile classpath. See my note on
`grails-core/build.gradle:54`.
##########
grails-domain-class/build.gradle:
##########
@@ -38,6 +38,7 @@ dependencies {
implementation platform(project(':grails-bom'))
+ implementation project(':grails-beans-dsl')
Review Comment:
Drop this line — `grails-core` declares `api project(':grails-beans-dsl')`,
so the DSL is already on this module's compile classpath. See my note on
`grails-core/build.gradle:54`.
##########
grails-url-mappings/build.gradle:
##########
@@ -43,6 +43,8 @@ dependencies {
api 'org.apache.groovy:groovy'
api 'org.springframework.boot:spring-boot-servlet'
+ implementation project(':grails-beans-dsl')
Review Comment:
Drop this line — `grails-core` declares `api project(':grails-beans-dsl')`,
so the DSL is already on this module's compile classpath. See my note on
`grails-core/build.gradle:54`.
##########
grails-beans-dsl-plugin-example/build.gradle:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id 'groovy'
+ id 'java-library'
+ id 'org.apache.grails.buildsrc.properties'
+ id 'org.apache.grails.buildsrc.dependency-validator'
+ id 'org.apache.grails.buildsrc.compile'
+ id 'org.apache.grails.buildsrc.vulnerability-scan'
+ id 'org.apache.grails.buildsrc.autoconfiguration-imports'
+ id 'org.apache.grails.gradle.grails-code-style'
+ id 'org.apache.grails.gradle.grails-jacoco'
+}
+
+version = projectVersion
+group = 'org.apache.grails'
Review Comment:
Alongside the directory move (see my note on `settings.gradle`), this file
needs to adopt the `grails-test-examples` conventions:
* `group = 'examples'` — no module under `grails-test-examples/` uses
`org.apache.grails`; that group is for published framework modules, and these
aren't published.
* Swap `gradle/test-config.gradle` for
`gradle/functional-test-config.gradle`, and add `testRuntimeOnly
'org.junit.platform:junit-platform-launcher'` since the latter doesn't supply
it.
* Drop `implementation project(':grails-beans-dsl')` — `grails-core`'s `api`
covers it.
* Drop `org.apache.grails.gradle.grails-code-style` and
`org.apache.grails.gradle.grails-jacoco`: no other module in that directory
applies either, and keeping them would put example code into the violation and
coverage reports. Happy to be overruled if that's intentional, but it should be
a deliberate precedent rather than a carry-over.
* Keep `groovy`/`java-library` and
`org.apache.grails.buildsrc.autoconfiguration-imports` — the latter is the
point of the module and is entirely path-independent, so the move doesn't
affect it.
Also worth updating the comment at line 41, which refers to the sibling by
its current name.
##########
grails-beans-dsl-example/build.gradle:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id 'groovy'
+ id 'java-library'
+ id 'org.apache.grails.buildsrc.properties'
+ id 'org.apache.grails.buildsrc.dependency-validator'
+ id 'org.apache.grails.buildsrc.compile'
+ id 'org.apache.grails.buildsrc.vulnerability-scan'
+ id 'org.apache.grails.buildsrc.autoconfiguration-imports'
+ id 'org.apache.grails.gradle.grails-code-style'
+ id 'org.apache.grails.gradle.grails-jacoco'
+}
+
+version = projectVersion
+group = 'org.apache.grails'
Review Comment:
Alongside the directory move (see my note on `settings.gradle`), this file
needs to adopt the `grails-test-examples` conventions:
* `group = 'examples'` — no module under `grails-test-examples/` uses
`org.apache.grails`; that group is for published framework modules, and these
aren't published.
* Swap `gradle/test-config.gradle` for
`gradle/functional-test-config.gradle`, and add `testRuntimeOnly
'org.junit.platform:junit-platform-launcher'` since the latter doesn't supply
it.
* Drop `implementation project(':grails-beans-dsl')` — `grails-core`'s `api`
covers it.
* Drop `org.apache.grails.gradle.grails-code-style` and
`org.apache.grails.gradle.grails-jacoco`: no other module in that directory
applies either, and keeping them would put example code into the violation and
coverage reports. Happy to be overruled if that's intentional, but it should be
a deliberate precedent rather than a carry-over.
* Keep `groovy`/`java-library` and
`org.apache.grails.buildsrc.autoconfiguration-imports` — the latter is the
point of the module and is entirely path-independent, so the move doesn't
affect it.
##########
grails-databinding/build.gradle:
##########
@@ -38,6 +38,7 @@ dependencies {
implementation platform(project(':grails-bom'))
+ implementation project(':grails-beans-dsl')
Review Comment:
Drop this line — `grails-core` declares `api project(':grails-beans-dsl')`,
so the DSL is already on this module's compile classpath. See my note on
`grails-core/build.gradle:54`.
##########
grails-gsp/grails-sitemesh3/build.gradle:
##########
@@ -52,6 +52,7 @@ dependencies {
api project(':grails-core')
implementation 'org.apache.groovy:groovy'
+ implementation project(':grails-beans-dsl')
Review Comment:
Drop this line — `grails-core` declares `api project(':grails-beans-dsl')`,
so the DSL is already on this module's compile classpath. See my note on
`grails-core/build.gradle:54`.
##########
grails-mail/build.gradle:
##########
@@ -76,10 +76,14 @@ dependencies {
compileOnly 'org.springframework.boot:spring-boot-autoconfigure' //
@AutoConfiguration, @ConditionalOnProperty, @ConfigurationProperties
compileOnly 'org.apache.groovy:groovy' // Provided as this is a Grails
plugin
compileOnly 'org.slf4j:slf4j-api' // @Slf4j
+ compileOnly project(':grails-beans-dsl') // comp: @GrailsBeans
Review Comment:
Drop this line too. `compileOnly` was the closest to correct of the three
scopes in use, but it is still unnecessary — `grails-core` declares `api
project(':grails-beans-dsl')`, so the DSL arrives on this module's compile
classpath already. See my note on `grails-core/build.gradle:54`.
##########
grails-beans-dsl/build.gradle:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id 'groovy'
+ id 'java-library'
+ id 'org.apache.grails.buildsrc.properties'
+ id 'org.apache.grails.buildsrc.dependency-validator'
+ id 'org.apache.grails.buildsrc.compile'
+ id 'org.apache.grails.buildsrc.publish'
+ id 'org.apache.grails.buildsrc.sbom'
+ id 'org.apache.grails.buildsrc.vulnerability-scan'
+ id 'org.apache.grails.gradle.grails-code-style'
+ id 'org.apache.grails.gradle.grails-jacoco'
+}
+
+version = projectVersion
+group = 'org.apache.grails'
Review Comment:
This should be a nested group — `org.apache.grails.beandsl` rather than flat
`org.apache.grails`.
The repo already partitions published coordinates this way, including for
single-module projects, so there is direct precedent:
| group | example |
| --- | --- |
| `org.apache.grails.data` | `grails-datastore-core`, `grails-data-simple`,
… |
| `org.apache.grails.web` | `grails-web-core`, `grails-web-mvc`,
`grails-encoder`, … |
| `org.apache.grails.testing` | `grails-testing-support-core`,
`grails-test-suite-*`, … |
| `org.apache.grails.events` / `.async` / `.forge` / `.gradle` |
multi-module |
| `org.apache.grails.i18n` / `.databinding` / `.codecs` / `.common` /
`.bootstrap` | **single-module** |
Since the DSL is intended to be on every project's classpath by default (via
`grails-core`'s `api`), giving it its own namespace makes that status legible
in the coordinates rather than burying it in the flat group.
Worth checking the knock-on: `gradle/publish-root-config.gradle` gates on
project *name*, so that list is unaffected, but any BOM constraint or
documentation that spells out the coordinates needs to follow the new group.
##########
settings.gradle:
##########
@@ -107,6 +107,9 @@ def skipMicronautProjects = explicitlySkipMicronaut ||
(!buildJdkSupportsMicrona
include(
'grails-bootstrap',
+ 'grails-beans-dsl',
+ 'grails-beans-dsl-example',
Review Comment:
These two should not be root projects — they belong under
`grails-test-examples/` and should follow those conventions.
The move is more than a `git mv`; two of the steps are load-bearing:
1. **The rename is mandatory, not cosmetic.** Root `build.gradle:51-67`
populates `testProjects` by matching the project *name* prefix
`grails-test-examples`, not the directory. If the directories move but the
names don't, the module lands outside `testProjects` while applying
`functional-test-config.gradle`, whose `evaluationDependsOn` fan-out
(`gradle/functional-test-config.gradle:20-22`) filters on exactly that set — a
project that evaluation-depends on itself.
2. **`gradle/test-config.gradle` → `gradle/functional-test-config.gradle`.**
All 111 existing modules under `grails-test-examples/` use the latter; none use
the former. Note it does *not* contribute
`org.junit.platform:junit-platform-launcher`, which `test-config.gradle` does —
so both modules need it declared explicitly, the same way
`grails-test-examples/hibernate7/standalone-hibernate/build.gradle:44-46` does,
or the Spock specs won't launch.
Suggested shape, following the flat-bucket pattern
(`grails-test-examples/<dir>` → `:grails-test-examples-<dir>`, as used by
`app1`, `cache`, `gsp-layout`, `config-report`):
| now | proposed directory | proposed project |
| --- | --- | --- |
| `grails-beans-dsl-example/` | `grails-test-examples/beans-dsl/` |
`:grails-test-examples-beans-dsl` |
| `grails-beans-dsl-plugin-example/` |
`grails-test-examples/beans-dsl-plugin/` |
`:grails-test-examples-beans-dsl-plugin` |
The `-example` suffix should go — no module under `grails-test-examples/`
carries it, the directory already says it.
That means removing lines 111-112 here, then adding both names to the flat
`include(...)` block further down this file and a matching `projectDir` line to
the block after it — the flat bucket is not auto-scanned, unlike the
`hibernate5`/`hibernate7` `listFiles` loops.
One consequence worth being deliberate about: these two modules are the only
consumers of the new `autoconfiguration-imports` convention plugin, and
`FarewellGrailsPluginAutoDiscoverySpec` is the only test anywhere asserting the
generated sibling's FQCN. Once they sit under `grails-test-examples/`, that
coverage stops running in core-only CI (`-PonlyCoreTests`). Applying the
convention plugin to at least one real converted module — which I suggested
separately on `AutoConfigurationImportsPlugin` — would cover that gap properly.
--
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]
