sbglasius commented on code in PR #16376:
URL: https://github.com/apache/grails-core/pull/16376#discussion_r4073234165
##########
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:
@matrei confirmed this empirically in his review (finding 1): `--info` shows
`Started Gradle worker daemon ... maxHeapSize=1500m, keepAliveMode=SESSION` for
the guide, and `Stopped 53 worker daemon(s)` only at the end of the build. So
the guide JVM is held for the remainder of the session, not returned when the
task ends.
Leaving this open rather than resolving it - it's corroborated, not
addressed. Either route closes it: send the guide through `javaexec` like
groovydoc (the `UserGuideBuilder` split already makes that small), or adjust
the wording in `PublishGuideWorkAction`'s javadoc and the PR description so it
doesn't claim a short-lived JVM. His point that the Groovy compile workers
already behave this way at `-Xmx2G` is fair mitigation for the first option.
##########
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:
Flagging that @matrei's nit 5 looks adjacent to this but is a different
point - he compares the two *mechanisms* (`GradleUtils.findProperty` at
execution time vs. `providers.gradleProperty`) and concludes "both work",
preferring the provider form for configuration-cache reasons. I agree on the
mechanism.
What's still open here is the *precedence* the provider form gives it: as a
convention, `-PguideMaxHeapSize` loses to any explicit
`publishGuide.maxHeapSize = ...`, which contradicts the javadoc above and
`gradle.properties` ("either beats the build script"). Keeping the provider is
fine -
`maxHeapSize.set(providers.gradleProperty('guideMaxHeapSize').orElse(<build
script value>))`, or reading it in the task action, restores the documented
precedence without giving up the configuration-cache property.
Latent either way, since nothing sets it today. The second half stands on
its own though: `gradle.properties` says the guide heap is "raised ... by
grails-doc", and `grails-doc/build.gradle` never sets it.
--
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]
