jdaugherty commented on code in PR #16376:
URL: https://github.com/apache/grails-core/pull/16376#discussion_r4073317333
##########
build-logic/docs-core/src/main/groovy/grails/doc/gradle/PublishGuideTask.groovy:
##########
@@ -110,8 +133,11 @@ class PublishGuideTask extends DefaultTask {
propertiesFiles = objects.fileCollection()
sourceDir =
objects.directoryProperty().convention(project.layout.projectDirectory.dir('src'))
resourcesDir =
objects.directoryProperty().convention(project.layout.projectDirectory.dir('resources'))
- macros = objects.listProperty(Object).convention([])
+ macros = objects.listProperty(String).convention([])
targetDir =
objects.directoryProperty().convention(project.layout.buildDirectory.dir('docs'))
+ maxHeapSize = objects.property(String)
Review Comment:
Fixed in 725d807. You're right on both counts - a convention only applies
while nothing has been set, so a build script would have silently beaten the
command line, and the javadoc plus `gradle.properties` both promised the
opposite.
`maxHeapSize` is a plain `convention('1500m')` now and the property is
resolved in the task action, project property first, matching what groovydoc
does. Verified: `-PguideMaxHeapSize=1m` fails with "Failed to run Gradle Worker
Daemon" (so it reaches the fork), default builds the guide byte-for-byte
identically.
Also corrected the `gradle.properties` line claiming grails-doc raises the
guide heap - it does not, the guide runs on the task's own default.
##########
build-logic/docs-core/src/main/groovy/grails/doc/gradle/PublishGuideTask.groovy:
##########
@@ -124,81 +150,39 @@ class PublishGuideTask extends DefaultTask {
}
@TaskAction
- def publishGuide() {
- Properties combinedProperties = new Properties()
-
- File workingDir =
Files.createTempDirectory('grails-doc-publish-guide').toFile()
-
- File resources = resourcesDir.get().asFile
- File docProperties = new File(resources, 'doc.properties')
- if (docProperties.exists()) {
- docProperties.withInputStream { input ->
- combinedProperties.load(input)
- }
- }
-
- // Add properties from any optional properties files too.
- for (File f : propertiesFiles) {
- f.withInputStream { input ->
- combinedProperties.load(input)
- }
- }
- combinedProperties.putAll(properties.get())
- combinedProperties.putAll(propertiesWithFilePaths.get())
-
- File apiDir = targetDir.get().asFile
- apiDir.deleteDir()
- apiDir.mkdirs()
-
- def publisher = new DocPublisher(sourceDir.get().asFile, apiDir)
- publisher.ant = ant
- publisher.asciidoc = asciidoc
- publisher.workDir = workingDir
- publisher.apiDir = apiDir
- publisher.language = language.getOrElse('')
- publisher.sourceRepo = sourceRepo.getOrElse('')
- publisher.images = new File(resources, 'img')
- publisher.css = new File(resources, 'css')
- publisher.fonts = new File(resources, 'fonts')
- publisher.js = new File(resources, 'js')
- publisher.style = new File(resources, 'style')
- publisher.version = combinedProperties['grails.version']
-
- // Override doc.properties properties with their language-specific
counterparts (if
- // those are defined). You just need to add entries like es.title or
pt_PT.subtitle.
- if (language.isPresent()) {
- String lang = language.get()
- def pos = lang.size() + 1
- def languageProps = combinedProperties.findAll { k, v ->
k.startsWith("${lang}.") }
- languageProps.each { k, v -> combinedProperties[k[pos..-1]] = v }
+ void publishGuide() {
+ // Everything is read into locals first. Both the fork options and the
work parameters
+ // have members of their own named like this task's properties -
maxHeapSize,
+ // properties - and inside the configuration closures those would win.
+ String workerHeap = this.maxHeapSize.get()
+ String languageValue = this.language.getOrNull()
+ String sourceRepoValue = this.sourceRepo.getOrNull()
+ Boolean asciidocValue = this.asciidoc.get()
+ Map<String, Object> engineProperties = this.properties.get()
+ Map<String, File> filePathProperties =
this.propertiesWithFilePaths.get()
+ FileCollection propertiesFileValues = this.propertiesFiles
+ Directory sourceDirValue = this.sourceDir.get()
+ Directory resourcesDirValue = this.resourcesDir.get()
+ Directory targetDirValue = this.targetDir.get()
+ List<String> macroNames = this.macros.get()
+ Boolean verboseAntValue = this.verboseAnt.get()
+
+ WorkQueue queue = workerExecutor.processIsolation { spec ->
Review Comment:
Confirmed and documented in 725d807. I checked it rather than take it on
trust: after a `publishGuide` build the only surviving workers are the `-Xmx2G`
Groovy compile daemons - no `-Xmx1500m` process remains - so it is
session-scoped, alive from the task until the build ends, not carried into the
next build. Worth adding that the ceiling does not reserve anything: once idle
it held ~284 MB resident, not 1500m.
The class docs said the memory goes straight back to the OS, which was
wrong; they now say it is stopped when the build session ends rather than when
the task returns, and that it can still be resident while the aggregate
groovydoc runs.
I kept the worker rather than moving to `javaexec`, deliberately: the guide
is build logic and runs on the Groovy that Gradle embeds, which a worker
inherits for free and a bare JVM would force us to pin a version for. Groovydoc
needed `javaexec` only because of the Groovy 4/5 classloader conflict. Measured
peak is comfortable either way - daemon 1.7 GB + aggregate 1.9 GB + guide 0.9
GB against 7 GB. Happy to switch it if you'd rather have the two paths
identical.
##########
build-logic/plugins/src/test/groovy/org/apache/grails/buildsrc/GroovydocEnhancerPluginSpec.groovy:
##########
@@ -51,4 +89,74 @@ class GroovydocEnhancerPluginSpec extends Specification {
groovydocFiles.any { it.name == runtimeOnlyJar.name }
groovydocFiles.any { it.name == compileOnlyJar.name }
}
+
+ void 'groovydoc documents the sources with the groovy the project builds
against'() {
+ given: 'a project whose documentation classpath pins its own groovy'
+ writeGroovydocProject()
+
+ when: 'groovydoc runs'
+ def result = GradleRunner.create()
+ .withProjectDir(projectDir)
+ .withArguments('groovydoc')
+ .withPluginClasspath()
+ .build()
+
+ then: 'the task succeeds and documents the source'
+ result.task(':groovydoc').outcome == TaskOutcome.SUCCESS
+ File documented = new File(projectDir,
'build/docs/groovydoc/com/example/Documented.html')
+ documented.exists()
+
+ and: "the project's groovy generated it, not the older one Gradle runs
build logic on"
+ documented.text.contains("Generated by groovydoc (${groovyVersion})")
+ }
+
+ void 'the groovydoc heap is the forked jvm heap, not the daemon heap'() {
+ given: 'a project that would document happily on the daemon heap'
+ writeGroovydocProject()
+
+ when: 'groovydoc is given a heap no JVM can start with'
+ def result = GradleRunner.create()
+ .withProjectDir(projectDir)
+ .withArguments('groovydoc', '-PgroovydocMaxHeapSize=1m')
+ .withPluginClasspath()
+ .buildAndFail()
+
+ then: 'a JVM refused to start on it - which only a forked JVM could
report'
+ result.task(':groovydoc').outcome == TaskOutcome.FAILED
+ result.output.contains('Too small maximum heap')
Review Comment:
The message strings are gone in 725d807 - both assertions now use Gradle's
own `finished with non-zero exit value`, which proves the same thing (a
separate process was launched with that heap and died) without depending on a
JVM's phrasing.
On the network: I'd keep the fixtures. `ConfigurationMetadataPluginSpec` in
this same module already resolves real dependencies, so this is not a new
requirement for `:build-logic:test`, and without a fixture that actually
resolves and runs groovydoc there is no way to prove the fork works at all -
which is what the previous round asked for. Leaving this thread open in case
you want to weigh that differently.
--
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]
