jamesfredley commented on code in PR #16059: URL: https://github.com/apache/grails-core/pull/16059#discussion_r3674970247
########## .github/workflows/end-to-end.yml: ########## @@ -0,0 +1,140 @@ +# 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. + +# The end-to-end suite is its own Gradle build, so the core build never reaches it and +# `./gradlew build` at the root is unaffected. +# +# It resolves Grails from the artifacts the core build publishes rather than by project +# 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. +# +# It also needs two JDKs, which is the other reason it gets its own workflow: the Grails 7 +# fixture must be compiled on Java 17 (the minimum for a Grails 7 app, so the binary matches +# what a real Grails 7 plugin is built with), while the core build and the Grails 8 +# application consuming the fixture need 21. The Grails 8 side simply tracks the repository's +# root .sdkmanrc - it has to run on whatever the core build it consumes runs on - and only the +# fixture carries its own pin. The steps below read both out of those files rather than relying +# on Gradle toolchain auto-detection. +name: "End to End" +on: + workflow_dispatch: + push: + branches: + - '[0-9]+.[0-9]+.x' + # The legacy command compatibility work these tests cover is still in review. Run the suite + # on its branch so the result is visible on the pull request; drop this entry once it merges. + - 'feat/8.0.x-legacy-command-compat' + paths: + - 'end-to-end/**' + - 'grails-core/**' + - 'grails-core-cli-legacy/**' + - 'grails-console/**' + - 'grails-gradle/**' + - '.github/workflows/end-to-end.yml' + pull_request: + paths: + - 'end-to-end/**' + - 'grails-core/**' + - 'grails-core-cli-legacy/**' + - 'grails-console/**' + - 'grails-gradle/**' + - '.github/workflows/end-to-end.yml' +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} +jobs: + endToEnd: + name: "End to End Tests (end-to-end build only)" + if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} + runs-on: ubuntu-24.04 + steps: + - name: "๐ฅ Checkout repository" + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: "โ๏ธ Determine JDKs from .sdkmanrc" + # Read both pins out of the files that already declare them, so the workflow cannot + # drift from what a developer gets with `sdk env`. + id: jdks + run: | + set -euo pipefail + fixture_java=$(grep -E '^java=' end-to-end/legacy-g7-command-plugin/.sdkmanrc | cut -d= -f2) + build_java=$(grep -E '^java=' .sdkmanrc | cut -d= -f2) + echo "fixture-java=${fixture_java%%.*}" >> "$GITHUB_OUTPUT" + echo "build-java=${build_java%%.*}" >> "$GITHUB_OUTPUT" Review Comment: `${fixture_java%%.*}` truncates `17.0.18-librca` to `17`, and the same for `21.0.7-librca` -> `21`. So the comment two lines up - "so the workflow cannot drift from what a developer gets with `sdk env`" - is not what the code does: it pins only the major, and the actual JDK is whatever Liberica major the runner image currently ships. A runner update silently changes the JDK both the fixture and the suite are built on, which is precisely the drift this step exists to prevent. `release-verify.yml` already has the shape you want here - it keeps the patch with `${SDKMAN_JAVA%-*}`, stripping only the vendor suffix. Same thing here would make the comment true: ```bash echo "fixture-java=${fixture_java%-*}" >> "$GITHUB_OUTPUT" echo "build-java=${build_java%-*}" >> "$GITHUB_OUTPUT" ``` If the full patch version is deliberately not pinned because `setup-java` cannot always satisfy an exact Liberica patch, then say that in the comment rather than claiming no-drift. ########## .github/workflows/end-to-end.yml: ########## @@ -0,0 +1,140 @@ +# 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. + +# The end-to-end suite is its own Gradle build, so the core build never reaches it and +# `./gradlew build` at the root is unaffected. +# +# It resolves Grails from the artifacts the core build publishes rather than by project +# 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. +# +# It also needs two JDKs, which is the other reason it gets its own workflow: the Grails 7 +# fixture must be compiled on Java 17 (the minimum for a Grails 7 app, so the binary matches +# what a real Grails 7 plugin is built with), while the core build and the Grails 8 +# application consuming the fixture need 21. The Grails 8 side simply tracks the repository's +# root .sdkmanrc - it has to run on whatever the core build it consumes runs on - and only the +# fixture carries its own pin. The steps below read both out of those files rather than relying +# on Gradle toolchain auto-detection. +name: "End to End" +on: + workflow_dispatch: + push: + branches: + - '[0-9]+.[0-9]+.x' + # The legacy command compatibility work these tests cover is still in review. Run the suite + # on its branch so the result is visible on the pull request; drop this entry once it merges. + - 'feat/8.0.x-legacy-command-compat' + paths: + - 'end-to-end/**' + - 'grails-core/**' + - 'grails-core-cli-legacy/**' + - 'grails-console/**' + - 'grails-gradle/**' + - '.github/workflows/end-to-end.yml' + pull_request: + paths: + - 'end-to-end/**' + - 'grails-core/**' + - 'grails-core-cli-legacy/**' + - 'grails-console/**' + - 'grails-gradle/**' + - '.github/workflows/end-to-end.yml' +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} +jobs: + endToEnd: + name: "End to End Tests (end-to-end build only)" + if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} + runs-on: ubuntu-24.04 + steps: + - name: "๐ฅ Checkout repository" + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: "โ๏ธ Determine JDKs from .sdkmanrc" + # Read both pins out of the files that already declare them, so the workflow cannot + # drift from what a developer gets with `sdk env`. + id: jdks + run: | + set -euo pipefail + fixture_java=$(grep -E '^java=' end-to-end/legacy-g7-command-plugin/.sdkmanrc | cut -d= -f2) + build_java=$(grep -E '^java=' .sdkmanrc | cut -d= -f2) + echo "fixture-java=${fixture_java%%.*}" >> "$GITHUB_OUTPUT" + echo "build-java=${build_java%%.*}" >> "$GITHUB_OUTPUT" + echo "Grails 7 fixture JDK: ${fixture_java}" + echo "end-to-end build JDK (from root .sdkmanrc): ${build_java}" + - name: "โ๏ธ Setup JDKs" + # Both, in one step. The last version listed becomes the default JAVA_HOME (the Grails 8 + # side); the fixture step below switches to JAVA_HOME_17_X64 for its single invocation. + uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 + with: + distribution: liberica + java-version: | + ${{ steps.jdks.outputs.fixture-java }} + ${{ steps.jdks.outputs.build-java }} + - name: "๐๏ธ Restore dependency jar cache" + uses: actions/cache@v4 + with: + # Cache only downloaded dependency jars and wrapper distributions, never Grails build outputs. + # Keyed by branch version so each release branch maintains its own warm cache. + path: | + ~/.gradle/caches/modules-2 + ~/.gradle/wrapper + key: gradle-deps-${{ runner.os }}-${{ github.base_ref || github.ref_name }}-${{ hashFiles('**/dependencies.gradle', '**/gradle-wrapper.properties') }} + restore-keys: | + gradle-deps-${{ runner.os }}-${{ github.base_ref || github.ref_name }}- + - name: "๐ Setup Gradle" + uses: gradle/actions/setup-gradle@50e97c2cd7a37755bbfafc9c5b7cafaece252f6e # v6.1.0 + with: + cache-disabled: true # dependency jars are cached by the explicit branch-keyed step above + develocity-access-key: ${{ secrets.DEVELOCITY_ACCESS_KEY }} + - name: "๐ฆ Setup: publish grails-gradle to the local repository the tests resolve from" + # Both builds publish into the same build/local-maven, and both are needed: the Grails BOM + # constrains org.apache.grails.gradle artifacts. grails-forge depends on this same pair of + # publish tasks for the applications its tests generate. + working-directory: 'grails-gradle' + run: ./gradlew publishAllPublicationsToTestCaseMavenRepoRepository -PskipTests --stacktrace + - name: "๐ฆ Setup: publish Grails to the local repository the tests resolve from" + run: ./gradlew publishAllPublicationsToTestCaseMavenRepoRepository -PskipTests --stacktrace + - name: "๐ฆ Setup: build the precompiled Grails 7 / Groovy 4 fixture" + working-directory: 'end-to-end/legacy-g7-command-plugin' + env: + JAVA_HOME: ${{ env.JAVA_HOME_17_X64 }} + run: ./gradlew jar --stacktrace + - name: "๐ Verify the fixture really was built on Grails 7 / Groovy 4" + working-directory: 'end-to-end/legacy-g7-command-plugin' + # A fixture silently built by the wrong toolchain would still pass the suite while + # proving nothing, so fail loudly here instead. + run: | + set -euo pipefail + jar=$(ls build/libs/*.jar) + unzip -p "$jar" META-INF/MANIFEST.MF | tr -d '\r' > /tmp/fixture-manifest + cat /tmp/fixture-manifest + grep -q '^Grails-Compile-Version: 7\.' /tmp/fixture-manifest + grep -q '^Groovy-Compile-Version: 4\.' /tmp/fixture-manifest Review Comment: Two problems that compound, and together they defeat the stated purpose of the verify step. **`JAVA_HOME_17_X64` hardcodes the very version the step above just derived.** The `17` in that variable name is a literal. If `legacy-g7-command-plugin/.sdkmanrc` ever moves off 17, `steps.jdks.outputs.fixture-java` follows it and `setup-java` provisions the new major - but `env.JAVA_HOME_17_X64` is then unset, `JAVA_HOME` resolves to empty, and the fixture builds on the default JDK (21) instead. Nothing fails. Deriving the variable name would fix it: ```yaml env: JAVA_HOME: ${{ env[format('JAVA_HOME_{0}_X64', steps.jdks.outputs.fixture-java)] }} ``` **And the verify step cannot catch that.** Its comment says "a fixture silently built by the wrong toolchain would still pass the suite while proving nothing, so fail loudly here instead" - but it only greps `Grails-Compile-Version` and `Groovy-Compile-Version`, which come from the resolved BOM, not from the JDK. A fixture compiled on 21 against Grails 7.0.14 / Groovy 4.0.32 still prints `7.` and `4.` and sails through. That matters more than usual here because `legacy-g7-command-plugin/build.gradle` sets no `release`, `sourceCompatibility` or toolchain (deliberately - that was the point of the move), so the class file version is determined *entirely* by `JAVA_HOME`. The check that would actually be loud is the bytecode major: ```bash unzip -p "$jar" legacy/g7/commands/HelloG7PrecompiledCommand.class | od -An -t u1 -j 6 -N 2 # expect major 61 for Java 17 ``` Stamp the JDK into the manifest at jar time the same way Grails/Groovy are, and assert it here. ########## .github/workflows/end-to-end.yml: ########## @@ -0,0 +1,140 @@ +# 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. + +# The end-to-end suite is its own Gradle build, so the core build never reaches it and +# `./gradlew build` at the root is unaffected. +# +# It resolves Grails from the artifacts the core build publishes rather than by project +# 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. +# +# It also needs two JDKs, which is the other reason it gets its own workflow: the Grails 7 +# fixture must be compiled on Java 17 (the minimum for a Grails 7 app, so the binary matches +# what a real Grails 7 plugin is built with), while the core build and the Grails 8 +# application consuming the fixture need 21. The Grails 8 side simply tracks the repository's +# root .sdkmanrc - it has to run on whatever the core build it consumes runs on - and only the +# fixture carries its own pin. The steps below read both out of those files rather than relying +# on Gradle toolchain auto-detection. +name: "End to End" +on: + workflow_dispatch: + push: + branches: + - '[0-9]+.[0-9]+.x' + # The legacy command compatibility work these tests cover is still in review. Run the suite + # on its branch so the result is visible on the pull request; drop this entry once it merges. + - 'feat/8.0.x-legacy-command-compat' + paths: + - 'end-to-end/**' + - 'grails-core/**' + - 'grails-core-cli-legacy/**' + - 'grails-console/**' + - 'grails-gradle/**' + - '.github/workflows/end-to-end.yml' + pull_request: + paths: + - 'end-to-end/**' + - 'grails-core/**' + - 'grails-core-cli-legacy/**' + - 'grails-console/**' + - 'grails-gradle/**' + - '.github/workflows/end-to-end.yml' Review Comment: These path filters are now the only thing that decides whether this suite runs at all - the root build no longer reaches these projects, so nothing else will catch a break. They are missing inputs the build genuinely consumes: - `gradle/**` - `end-to-end/legacy-commands/build.gradle` applies `../gradle/grails-extension-gradle-config.gradle` directly - `build-logic/**` - `end-to-end/settings.gradle` includes it, and the projects apply its `org.apache.grails.buildsrc.*` plugins - `gradle-bootstrap/**` - it generates this build's wrappers, including the fixture's via `legacyG7Wrapper` - `dependencies.gradle` / `grails-bom/**` - the suite resolves `org.apache.grails:grails-bom:$projectVersion` from `build/local-maven` - `gradle.properties` and the root `.sdkmanrc` - the workflow reads the latter to pick its JDK A PR touching only those merges without this suite ever running. Same list applies to the `push:` filters above. ########## .github/workflows/end-to-end.yml: ########## @@ -0,0 +1,140 @@ +# 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. + +# The end-to-end suite is its own Gradle build, so the core build never reaches it and +# `./gradlew build` at the root is unaffected. +# +# It resolves Grails from the artifacts the core build publishes rather than by project +# 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. +# +# It also needs two JDKs, which is the other reason it gets its own workflow: the Grails 7 +# fixture must be compiled on Java 17 (the minimum for a Grails 7 app, so the binary matches +# what a real Grails 7 plugin is built with), while the core build and the Grails 8 +# application consuming the fixture need 21. The Grails 8 side simply tracks the repository's +# root .sdkmanrc - it has to run on whatever the core build it consumes runs on - and only the +# fixture carries its own pin. The steps below read both out of those files rather than relying +# on Gradle toolchain auto-detection. +name: "End to End" +on: + workflow_dispatch: + push: + branches: + - '[0-9]+.[0-9]+.x' + # The legacy command compatibility work these tests cover is still in review. Run the suite + # on its branch so the result is visible on the pull request; drop this entry once it merges. + - 'feat/8.0.x-legacy-command-compat' Review Comment: Self-documented as temporary - flagging it only so it does not ride along. Once this merges into `feat/8.0.x-legacy-command-compat` and that merges to `8.0.x`, drop the branch entry. Worth noting the `pull_request` trigger already covers the visibility this was added for, so it may be droppable now. ########## grails-core/src/test/groovy/org/apache/grails/core/cli/ApplicationCommandProviderSpec.groovy: ########## @@ -298,18 +312,50 @@ class ApplicationCommandProviderSpec extends Specification { !errorOutput.toString('UTF-8').contains('ships Grails 7 commands') } - def "continues command discovery when a factory resource is malformed"() { + def "reports a malformed factory resource and continues command discovery"() { given: String malformedFactory = 'grails.dev.commands.ApplicationCommand=' + '\\' + 'uInvalid' URL plugin = createFactoryJar('malformed-plugin.jar', malformedFactory) useFactoryResources([plugin]) + ByteArrayOutputStream errorOutput = new ByteArrayOutputStream() when: - ApplicationContextCommandRegistry registry = new ApplicationContextCommandRegistry() + ApplicationContextCommandRegistry registry = captureStandardError(errorOutput) { + new ApplicationContextCommandRegistry() + } then: registry.missingCommandHint == null noExceptionThrown() + errorOutput.toString('UTF-8').contains('Unable to read factory declarations') + errorOutput.toString('UTF-8').contains('malformed-plugin.jar') + errorOutput.toString('UTF-8').contains('META-INF/grails.factories') Review Comment: Two things about how this warning is captured. **It asserts the message but not the cause.** The point of the `GrailsFactoriesLoader` change is that a malformed resource stops vanishing *and* says why - the `IllegalArgumentException("Malformed \\uxxxx encoding")` is the actionable half for the user chasing missing commands. Dropping the throwable argument from the `log.warn` would leave all three of these assertions green. Worth asserting the exception type or its message text too. **Swapping global `System.err` is not safe here.** `captureStandardError` mutates process-global state, and this module runs tests with `maxParallelForks > 1` - a concurrently executing feature in the same fork writing to stderr lands in this buffer, and anything this one emits vanishes from the other's. It also couples the assertion to logback's console appender configuration rather than to the log event. A logback `ListAppender` attached to the `GrailsFactoriesLoader` logger (and detached in `cleanup`) tests the actual contract and is fork-safe. `LegacyCommandRegistryLoadingSpec` already has `attachProviderAppender` doing exactly this - same approach would work here. ########## grails-core/src/cli/groovy/org/apache/grails/core/cli/ApplicationContextCommandRegistry.groovy: ########## @@ -225,9 +234,6 @@ class ApplicationContextCommandRegistry { if (current instanceof VirtualMachineError) { throw (VirtualMachineError) current } Review Comment: This drops the `ThreadDeath` rethrow I asked for on #16011, and the same deletion lands in `ApplicationCommandDiagnostics`, `LegacyApplicationCommandProvider` and `ApplicationContextCommandFactory`, with the eight tests that fed `new ThreadDeath()` switched to `OutOfMemoryError`. I am fine with it on the merits - `Thread.stop()` throws `UnsupportedOperationException` on the Java 21 baseline, so `ThreadDeath` can no longer be thrown by the JVM, the branch is unreachable, and the type is deprecated for removal. `VirtualMachineError`, the guard that actually fires, is untouched at all four boundaries, which is the part that mattered. But it narrows a guard that was added at review request, and the description did not mention it - I only found it by reading the diff. I have added it to the PR body. Nothing further needed here. ########## grails-core/src/test/groovy/org/apache/grails/core/cli/ApplicationCommandProviderSpec.groovy: ########## @@ -98,7 +95,24 @@ class ApplicationCommandProviderSpec extends Specification { then: registry.findCommand('counting-modern') instanceof ConstructorCountingApplicationCommand - ConstructorCountingApplicationCommand.constructorCalls == registryClassLoaderConstructions + 1 + ConstructorCountingApplicationCommand.constructorCalls == 1 + } + + def "instantiates the same command class once across registry and context classloaders"() { + given: + SharedCountingApplicationCommand.constructorCalls = 0 + URL plugin = createFactoryJar( + 'shared-counting-command.jar', + 'example.OtherFactory=example.OtherImplementation', + "${ApplicationCommand.name}=${SharedCountingApplicationCommand.name}") + useFactoryResources([plugin]) + + when: + ApplicationContextCommandRegistry registry = new ApplicationContextCommandRegistry() + + then: + registry.findCommand('counting-shared') instanceof SharedCountingApplicationCommand + SharedCountingApplicationCommand.constructorCalls == 1 Review Comment: This test does not exercise the case its name claims, and it passes with the production change reverted. `useFactoryResources` only swaps the **thread context classloader** (a `URLClassLoader` over the generated jars, parented to the test's own loader). The registry's other loader is `ApplicationContextCommandRegistry.classLoader`, i.e. the plain test classpath, which never sees `shared-counting-command.jar`. So only one of the two scans ever finds the declaration, and `constructorCalls == 1` holds under the old per-classloader instantiate-as-you-go code just as well as under the new collect-then-instantiate code. That is unfortunate, because avoiding the double construction is the entire motivation for restructuring `ApplicationContextCommandRegistry`. To make it bite, the class has to be reachable from *both* loaders - e.g. put the factories resource on the registry loader too (the command class itself is already on the test classpath, so both scans would resolve the same `Class`), then assert `constructorCalls == 1`. Reverting to the old code should then produce 2. The sibling at :80 has the same limitation, but it at least got stricter with this change (dropping the `registryClassLoaderConstructions` baseline), so it is only this new one that is asserting nothing. ########## .github/workflows/end-to-end.yml: ########## @@ -0,0 +1,140 @@ +# 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. + +# The end-to-end suite is its own Gradle build, so the core build never reaches it and +# `./gradlew build` at the root is unaffected. +# +# It resolves Grails from the artifacts the core build publishes rather than by project +# 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. +# +# It also needs two JDKs, which is the other reason it gets its own workflow: the Grails 7 +# fixture must be compiled on Java 17 (the minimum for a Grails 7 app, so the binary matches +# what a real Grails 7 plugin is built with), while the core build and the Grails 8 +# application consuming the fixture need 21. The Grails 8 side simply tracks the repository's +# root .sdkmanrc - it has to run on whatever the core build it consumes runs on - and only the +# fixture carries its own pin. The steps below read both out of those files rather than relying +# on Gradle toolchain auto-detection. +name: "End to End" +on: + workflow_dispatch: + push: + branches: + - '[0-9]+.[0-9]+.x' + # The legacy command compatibility work these tests cover is still in review. Run the suite + # on its branch so the result is visible on the pull request; drop this entry once it merges. + - 'feat/8.0.x-legacy-command-compat' + paths: + - 'end-to-end/**' + - 'grails-core/**' + - 'grails-core-cli-legacy/**' + - 'grails-console/**' + - 'grails-gradle/**' + - '.github/workflows/end-to-end.yml' + pull_request: + paths: + - 'end-to-end/**' + - 'grails-core/**' + - 'grails-core-cli-legacy/**' + - 'grails-console/**' + - 'grails-gradle/**' + - '.github/workflows/end-to-end.yml' +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} +jobs: + endToEnd: + name: "End to End Tests (end-to-end build only)" + if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} + runs-on: ubuntu-24.04 + steps: + - name: "๐ฅ Checkout repository" + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: "โ๏ธ Determine JDKs from .sdkmanrc" + # Read both pins out of the files that already declare them, so the workflow cannot + # drift from what a developer gets with `sdk env`. + id: jdks + run: | + set -euo pipefail + fixture_java=$(grep -E '^java=' end-to-end/legacy-g7-command-plugin/.sdkmanrc | cut -d= -f2) + build_java=$(grep -E '^java=' .sdkmanrc | cut -d= -f2) + echo "fixture-java=${fixture_java%%.*}" >> "$GITHUB_OUTPUT" + echo "build-java=${build_java%%.*}" >> "$GITHUB_OUTPUT" + echo "Grails 7 fixture JDK: ${fixture_java}" + echo "end-to-end build JDK (from root .sdkmanrc): ${build_java}" + - name: "โ๏ธ Setup JDKs" + # Both, in one step. The last version listed becomes the default JAVA_HOME (the Grails 8 + # side); the fixture step below switches to JAVA_HOME_17_X64 for its single invocation. + uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 + with: + distribution: liberica + java-version: | + ${{ steps.jdks.outputs.fixture-java }} + ${{ steps.jdks.outputs.build-java }} + - name: "๐๏ธ Restore dependency jar cache" + uses: actions/cache@v4 + with: + # Cache only downloaded dependency jars and wrapper distributions, never Grails build outputs. + # Keyed by branch version so each release branch maintains its own warm cache. + path: | + ~/.gradle/caches/modules-2 + ~/.gradle/wrapper + key: gradle-deps-${{ runner.os }}-${{ github.base_ref || github.ref_name }}-${{ hashFiles('**/dependencies.gradle', '**/gradle-wrapper.properties') }} + restore-keys: | + gradle-deps-${{ runner.os }}-${{ github.base_ref || github.ref_name }}- + - name: "๐ Setup Gradle" + uses: gradle/actions/setup-gradle@50e97c2cd7a37755bbfafc9c5b7cafaece252f6e # v6.1.0 + with: + cache-disabled: true # dependency jars are cached by the explicit branch-keyed step above + develocity-access-key: ${{ secrets.DEVELOCITY_ACCESS_KEY }} + - name: "๐ฆ Setup: publish grails-gradle to the local repository the tests resolve from" + # Both builds publish into the same build/local-maven, and both are needed: the Grails BOM + # constrains org.apache.grails.gradle artifacts. grails-forge depends on this same pair of + # publish tasks for the applications its tests generate. + working-directory: 'grails-gradle' + run: ./gradlew publishAllPublicationsToTestCaseMavenRepoRepository -PskipTests --stacktrace + - name: "๐ฆ Setup: publish Grails to the local repository the tests resolve from" + run: ./gradlew publishAllPublicationsToTestCaseMavenRepoRepository -PskipTests --stacktrace + - name: "๐ฆ Setup: build the precompiled Grails 7 / Groovy 4 fixture" + working-directory: 'end-to-end/legacy-g7-command-plugin' + env: + JAVA_HOME: ${{ env.JAVA_HOME_17_X64 }} + run: ./gradlew jar --stacktrace + - name: "๐ Verify the fixture really was built on Grails 7 / Groovy 4" + working-directory: 'end-to-end/legacy-g7-command-plugin' + # A fixture silently built by the wrong toolchain would still pass the suite while + # proving nothing, so fail loudly here instead. + run: | + set -euo pipefail + jar=$(ls build/libs/*.jar) + unzip -p "$jar" META-INF/MANIFEST.MF | tr -d '\r' > /tmp/fixture-manifest + cat /tmp/fixture-manifest + grep -q '^Grails-Compile-Version: 7\.' /tmp/fixture-manifest + grep -q '^Groovy-Compile-Version: 4\.' /tmp/fixture-manifest + - name: "๐ Setup TestLens" + uses: testlens-app/setup-testlens@d96a555133c275a00949d2cc77b70fe9a4242ebf # v1.9.2 + - name: "๐งช Run the end-to-end tests" + # Only the end-to-end build. grails-core's own unit and functional suites are the CI + # workflow's job; nothing here re-runs them. The publish steps above are setup, not tests. + working-directory: 'end-to-end' + run: ./gradlew check --continue --stacktrace Review Comment: `check` alone leaves the moved sources outside the project's violation gate. `end-to-end` applies neither `GrailsCodeStylePlugin` nor `GrailsCodeAnalysisPlugin`, and the root `aggregateStyleViolations` / `aggregateAnalysisViolations` only walk root subprojects - which these no longer are. So CodeNarc/Checkstyle/PMD/SpotBugs no longer see `legacy-commands`, `legacy-commands-plugin`, or the fixture, and `./gradlew clean aggregateViolations` at the root will report clean regardless of what is in them. That is a real regression from the move rather than a pre-existing gap: before this PR these projects were in that graph. Either apply the style/analysis convention plugins in `end-to-end/build.gradle` and run `./gradlew check codeStyle` here, or state in `end-to-end/README.md` that the suite is deliberately outside the violation surface and why. ########## end-to-end/legacy-commands/build.gradle: ########## @@ -72,9 +81,17 @@ dependencies { testImplementation 'org.spockframework:spock-core' } +// The core build's gradle/functional-test-config.gradle is deliberately not applied here. Its +// dependency substitution enumerates rootProject.subprojects, which only makes sense inside the +// core build; here includeBuild('..') substitutes the org.apache.grails coordinates instead. 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. Review Comment: This is the comment class the PR description says it corrected, and this one is inverted: "here `includeBuild('..')` substitutes the org.apache.grails coordinates instead" is exactly what this build does *not* do. `end-to-end/settings.gradle` deliberately has no `includeBuild('..')` - it resolves `org.apache.grails` from `build/local-maven` through `exclusiveContent`, which is the whole reason the suite is end-to-end and the reason the CLI companion capability problem goes away. A future reader taking this comment at face value would conclude composite substitution is in play and reason about the fixture's isolation completely backwards. Suggest: "...which only makes sense inside the core build; here the org.apache.grails coordinates resolve from the published artifacts in build/local-maven instead." ########## grails-core/src/cli/groovy/org/apache/grails/core/cli/ApplicationContextCommandRegistry.groovy: ########## @@ -43,22 +43,56 @@ class ApplicationContextCommandRegistry { ClassLoader registryClassLoader = ApplicationContextCommandRegistry.classLoader ClassLoader contextClassLoader = Thread.currentThread().contextClassLoader - addApplicationCommands(registryClassLoader) + addApplicationCommands(registryClassLoader, contextClassLoader) + + Set<String> handledFactoryKeys = loadCommandProviders(registryClassLoader, contextClassLoader) + missingCommandHint = ApplicationCommandDiagnostics.detectMissingCommandHint( + registryClassLoader, contextClassLoader, handledFactoryKeys) + } + + private void addApplicationCommands(ClassLoader registryClassLoader, ClassLoader contextClassLoader) { + Map<Class<? extends ApplicationCommand>, String> commandOrigins = new LinkedHashMap<>() + addApplicationCommandClasses(commandOrigins, registryClassLoader) // If this is reflectively loaded from the delegating cli, we need to make sure the context class loader is Review Comment: Good change, and a real behavioural fix rather than a refactor: collecting `Class` -> origin across both loaders before instantiating anything means a command reachable through both is constructed once instead of twice-then-discarded, which for a command with constructor side effects was an observable bug. Folding `instantiateCommand` and `instantiate` into one generified `instantiate(Class<? extends T>)` is the right cleanup alongside it. Same for the `Ordered` propagation in `LegacyApplicationCommandAdapter` - registration is first-wins, so two Grails 7 plugins shipping the same command name were previously resolved by jar scan order, and a G7 command that used Spring ordering to win a collision now keeps winning it. My only issue is that neither appeared in the PR description, which read as if this PR were build placement plus four small fixes. I have added both to the body. The `Ordered` branch also needs its adapter spec updated - see the top-level comment. -- 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]
