jamesfredley commented on PR #15698:
URL: https://github.com/apache/grails-core/pull/15698#issuecomment-4724420533
Thanks for the thorough review, @jdaugherty and @matrei. Pushed 9a132a87fa
addressing the latest round of feedback. Summary of changes:
### Property scoping
- The PID system property is now `grails.cli.pid.file` (was the unscoped
`cli.pid.file`). It is read in `GrailsApp.run()` and set directly on the
`bootRun` task by `GrailsGradlePlugin` under the same scoped name.
### Gradle plugin lifecycle
- `configureBootRunPidFile` no longer uses `afterEvaluate`; it uses
`project.pluginManager.withPlugin('org.springframework.boot')` +
`tasks.withType(BootRun).configureEach`.
- The PID path is resolved lazily at execution time through a
`CommandLineArgumentProvider` (`RunAppPidFileProvider`), mirroring the existing
`GrailsAppBaseDirProvider` in the same file. This keeps the build-directory
resolution out of configuration time and stays configuration-cache safe.
### Hard-coded, single source of truth
- The PID file is hard-coded to `build/run-app.pid`. Removed the
command-line / system-property / application-config override variants from
`run-app`, `stop-app`, and `RunningApplicationProcess`, so the producer
(`bootRun`) and the consumers (`run-app`'s already-running guard and
`stop-app`) always agree on one location.
- `run-app` no longer passes `-Dgrails.cli.pid.file`; the Gradle plugin is
the only place that configures it.
### Docs / metadata
- Removed the internal `grails.cli.pid.file` property and the `Command Line`
group from `spring-configuration-metadata.json` (and the matching `grails-doc`
ordering). It is internal CLI/Gradle wiring, not user configuration.
### Stop marker (kept, with rationale)
- Kept the `run-app.stopping` marker. `stop-app` calls
`ProcessHandle.destroy()` on only the forked application JVM, not the Gradle
process, so the `bootRun` build exits with a non-zero child status (e.g. 143 on
Unix) rather than cancelling like a Ctrl+C of the whole build. A foreground /
separate-terminal `run-app` blocked on that build needs the marker to report a
clean stop instead of a startup failure. This is now documented in
`stop-app.groovy` and `RunningApplicationProcess`.
### Tests
- `GrailsGradlePluginToolchainSpec`: verifies `bootRun` supplies the default
`build/run-app.pid` and ignores a CLI-supplied path (hard-coded).
- `RunningApplicationProcessSpec`: trimmed the now-removed override tests;
retained PID read / liveness / stop / marker coverage.
### Verified
- `./gradlew :grails-shell-cli:test --tests
"org.grails.cli.gradle.RunningApplicationProcessSpec"`
- `./gradlew :grails-gradle-plugins:test --tests
"org.grails.gradle.plugin.core.GrailsGradlePluginToolchainSpec"`
- `./gradlew :grails-core:compileGroovy :grails-profiles-base:compileProfile`
- `./gradlew :grails-doc:generateConfigReference`
--
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]
