Copilot commented on code in PR #16224:
URL: https://github.com/apache/grails-core/pull/16224#discussion_r3862482374
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:
##########
@@ -194,6 +195,12 @@ class GlobalGrailsClassInjectorTransformation implements
ASTTransformation, Comp
// create or update grails-plugin.xml
generatePluginXml(pluginClassNode, pluginVersion,
transformedClassNames, pluginXmlFile)
}
+
+ // The generated auto-configurations register themselves as they are
created, but a descriptor
+ // that was deleted, or that no longer has a beans closure, creates
nothing and so says nothing
+ // about the entry it used to leave behind. This runs for every source
unit of a Grails
+ // project, which is what makes the entry go when the class it names
does.
+ AutoConfigurationImportsWriter.reconcile(compilationTargetDirectory,
compilationUnit)
Review Comment:
This reconciliation hook only runs when the Groovy compiler visits a project
source unit. An incremental build caused solely by deleting a descriptor can
remove its class outputs without presenting any source unit here, so the stale
imports entry remains; similarly, adding or deleting the hand-authored resource
does not invalidate `compileGroovy`, so the documented opt-in can leave the
generated file absent (or duplicated) until a clean/source rebuild.
Reconciliation and hand-authored-file switching need to be wired to a build
step whose inputs include both the source set and this resource, rather than
relying only on an AST visit.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -324,9 +326,18 @@ private ClassNode createAutoConfigurationSibling(ClassNode
pluginClass, Annotati
sibling.addAnnotations(siblingAnnotations);
pluginClass.getAnnotations().removeAll(siblingAnnotations);
+ // The name is settled here and nowhere else, so this is where it can
be registered.
+ AutoConfigurationImportsWriter.register(siblingName,
targetDirectory(source), source, compilationUnit);
+
return sibling;
}
+ /** The compiler's output directory, which is where generated metadata
belongs. */
+ private static File targetDirectory(SourceUnit source) {
+ CompilerConfiguration configuration = source == null ? null :
source.getConfiguration();
+ return configuration == null ? null :
configuration.getTargetDirectory();
Review Comment:
This bypasses the repository's existing Groovy-Eclipse target resolution.
For an `EclipseSourceUnit`, `CompilerConfiguration.targetDirectory` may be null
or project-relative, which is why
`GlobalGrailsClassInjectorTransformation.resolveCompilationTargetDirectory`
delegates to `GroovyEclipseCompilationHelper` (lines 462-468). Here that makes
`register` silently do nothing or write relative to the compiler process, so
Eclipse builds omit or misplace the imports file. Pass the already-resolved
target into this transformation or provide equivalent Eclipse-aware resolution
without introducing the existing dependency cycle.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/AutoConfigurationImportsWriter.java:
##########
@@ -0,0 +1,263 @@
+/*
+ * 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.
+ */
+package org.grails.compiler.beans;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.WeakHashMap;
+
+import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.control.messages.WarningMessage;
+
+/**
+ * Registers a generated auto-configuration in
+ * {@code
META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports}.
+ *
+ * <p>The class a {@code beans} closure compiles to is created during
compilation and is not a source
+ * file anyone can open. Leaving its registration to be written by hand made a
plugin whose beans are
+ * silently never registered the ordinary consequence of not knowing the class
exists - and the name
+ * to write is one only the compiler knows, since it follows from the
descriptor's name and package.
+ * Writing it where the class is created is the only point at which that name
is known for certain.
+ *
+ * <p>A module that keeps the file by hand at {@value
#SOURCE_IMPORTS_LOCATION} keeps it: generating
+ * a second copy would put the same resource at the same path twice, and
folding its entries into a
+ * copy under the build directory would lose them the moment anyone deleted
the file that was, until
+ * then, where they were written down. Such a module is warned when the
generated class is missing
+ * from it and is otherwise left alone, so nothing that builds today builds
differently - deleting
+ * the hand-authored file is what opts in, and is safe once it holds nothing
but what is generated.
+ *
+ * <p>That one conventional location is all a compiler can look in: a source
set's resource
+ * directories are a build-tool notion and are not among the things the
compiler is told, so a module
+ * that relocates them keeps a file this cannot see and gets a second copy
generated, which the build
+ * then reports as two resources at one path. {@code FactoriesFileWriter} reads
+ * {@code META-INF/grails.factories} from the same fixed location for the same
reason. Handling a
+ * relocated one needs the build to say where it is.
+ *
+ * <p>Hand-authored entries have to remain possible: a module may register a
class from another jar,
+ * one annotated with a composed annotation, or one carrying no annotation at
all, the imports file
+ * being the registration and {@code @AutoConfiguration} only supplying
ordering.
+ *
+ * @since 8.0
+ */
+public final class AutoConfigurationImportsWriter {
+
+ public static final String IMPORTS_LOCATION =
+
"META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports";
+
+ static final String SOURCE_IMPORTS_LOCATION = "src/main/resources/" +
IMPORTS_LOCATION;
+
+ /** Set by the Grails Gradle plugin on the compiler's fork options; see
GrailsAppBaseDirProvider. */
+ private static final String BASE_DIR_PROPERTY = "base.dir";
+
+ private static final String COMMENT_START = "#";
+
+ private static final String CLASS_FILE_EXTENSION = ".class";
+
+ /**
+ * What each compilation has registered so far, so an entry survives the
pruning below before the
+ * class file backing it has been written - class generation runs long
after this does, and two
+ * descriptors recompiling together would otherwise prune each other.
Weakly keyed on the
+ * compilation, which is what makes the state per-build rather than
per-JVM in a reused daemon.
+ */
+ private static final Map<Object, Set<String>> REGISTERED_BY_COMPILATION =
+ Collections.synchronizedMap(new WeakHashMap<>());
+
+ private AutoConfigurationImportsWriter() {
+ }
+
+ /**
+ * Adds {@code className} to the generated imports file under {@code
targetDirectory}, together
+ * with anything an earlier source unit of the same compilation registered
there. A module that
+ * keeps the file by hand is warned instead, and its file is left as the
only one.
+ *
+ * @param className the generated auto-configuration's binary name
+ * @param targetDirectory the compilation output directory, or {@code
null} when the compiler did
+ * not supply one - in which case there is nowhere
to write and the class
+ * stays registerable by hand
+ * @return {@code true} when the file was written
+ */
+ static boolean register(String className, File targetDirectory, SourceUnit
source, Object compilation) {
+ if (className == null || className.isEmpty() || targetDirectory ==
null) {
+ return false;
+ }
+
+ File sourceDirectory = findSourceDirectory(targetDirectory);
+ File handAuthored = sourceDirectory == null ? null : new
File(sourceDirectory, SOURCE_IMPORTS_LOCATION);
+ if (handAuthored != null && handAuthored.isFile()) {
+ Set<String> handAuthoredEntries = new TreeSet<>();
+ readEntries(handAuthored, handAuthoredEntries);
+ if (!handAuthoredEntries.contains(className)) {
+ warn(source, className + " is generated from a beans closure
but is not listed in " +
+ SOURCE_IMPORTS_LOCATION + ", so Spring Boot will not
read it. Add it there, or delete " +
+ "that file once it holds nothing that is not generated
and it will be written for you.");
+ }
+ return false;
+ }
+
+ Set<String> registeredHere = registeredBy(compilation);
+ registeredHere.add(className);
+
+ File importsFile = new File(targetDirectory, IMPORTS_LOCATION);
+ Set<String> entries = new TreeSet<>();
+ readEntries(importsFile, entries);
+
+ // A descriptor that was renamed, deleted, or given a different
autoConfigurationName leaves
+ // an entry naming a class that is no longer generated, and Spring
Boot fails to start on an
+ // auto-configuration it cannot load. Anything this compilation
registered is kept regardless:
+ // its class file is written in a later phase than this one runs in.
+ entries.removeIf(entry -> !registeredHere.contains(entry) &&
!isGeneratedHere(targetDirectory, entry));
+
+ Set<String> written = new TreeSet<>(entries);
+ written.addAll(registeredHere);
+ if (written.equals(entries) && importsFile.isFile() &&
entries.contains(className)) {
+ return false;
+ }
+ entries = written;
+
+ return write(importsFile, entries);
+ }
+
+ /**
+ * Writes the entries, or deletes the file when none are left - an empty
imports file is a
+ * resource that says nothing, and leaving one behind is the sort of thing
that shows up in a
+ * reproducible-build diff.
+ */
+ private static boolean write(File importsFile, Set<String> entries) {
+ try {
+ if (entries.isEmpty()) {
+ return Files.deleteIfExists(importsFile.toPath());
+ }
+ Files.createDirectories(importsFile.toPath().getParent());
+ // Sorted and newline-terminated, so recompiling the same sources
rewrites the same bytes.
+ Files.write(importsFile.toPath(), (String.join("\n", entries) +
"\n")
+ .getBytes(StandardCharsets.UTF_8));
+ return true;
+ }
+ catch (IOException notWritable) {
+ // The class is still generated and still registerable by hand, so
failing compilation
+ // over the convenience of not having to would be the worse trade.
+ return false;
Review Comment:
An `IOException` here is silently discarded, and the caller ignores the
false result, so compilation succeeds while the generated auto-configuration is
not registered—the exact silent failure this writer is intended to prevent.
Report the failure through the `SourceUnit` warning/error collector or fail
compilation instead of returning without diagnostics.
##########
grails-beans-dsl/src/test/groovy/org/grails/compiler/beans/AutoConfigurationImportsWriterSpec.groovy:
##########
@@ -0,0 +1,268 @@
+/*
+ * 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.
+ */
+package org.grails.compiler.beans
+
+import org.codehaus.groovy.control.CompilationUnit
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.codehaus.groovy.control.Phases
+import spock.lang.Specification
+import spock.lang.TempDir
+
+/**
+ * The class a {@code beans} closure compiles to is generated rather than
written, so nobody can
+ * list it in {@code AutoConfiguration.imports} without first knowing it
exists. These drive a real
+ * compilation with a target directory, which is the only thing that makes the
file observable.
+ */
+class AutoConfigurationImportsWriterSpec extends Specification {
+
+ private static final String IMPORTS =
+
'META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports'
+
+ @TempDir
+ File projectDir
+
+ private File targetDir
+ private String previousBaseDir
+
+ void setup() {
+ targetDir = new File(projectDir, 'build/classes/groovy/main')
+ targetDir.mkdirs()
+ previousBaseDir = System.setProperty('base.dir',
projectDir.absolutePath)
+ }
+
+ void cleanup() {
+ if (previousBaseDir == null) {
+ System.clearProperty('base.dir')
+ }
+ else {
+ System.setProperty('base.dir', previousBaseDir)
+ }
+ }
+
+ void 'the generated sibling registers itself'() {
+ when:
+ compile(plugin('Greeting'))
+
+ then: 'the name only the compiler knows is written where Spring Boot
reads it'
+ importsEntries() == ['com.example.GreetingAutoConfiguration']
+ }
+
+ void 'autoConfigurationName registers under the name it asks for'() {
+ when:
+ compile(plugin('Renamed', "autoConfigurationName =
'LegacyAutoConfiguration'"))
+
+ then: 'what is registered is the name the class is actually generated
under'
+ importsEntries() == ['com.example.LegacyAutoConfiguration']
+ }
+
+ void 'siblings from separate source units accumulate rather than replacing
one another'() {
+ when: 'two descriptors compile separately, as they do in a real build'
+ compile(plugin('First'))
+ compile(plugin('Second'))
+
+ then:
+ importsEntries() == ['com.example.FirstAutoConfiguration',
'com.example.SecondAutoConfiguration']
+ }
+
+ void 'a renamed descriptor does not leave its old entry behind'() {
+ given: 'a descriptor compiles, is renamed, and the stale output is
cleaned - an incremental build'
+ compile(plugin('Greeting'))
+ new File(targetDir,
'com/example/GreetingAutoConfiguration.class').delete()
+ new File(targetDir, 'com/example/GreetingGrailsPlugin.class').delete()
+
+ when:
+ compile(plugin('Farewell'))
+
+ then: 'an entry naming a class that is no longer generated fails
Spring Boot at startup'
+ importsEntries() == ['com.example.FarewellAutoConfiguration']
+ }
+
+ void 'a descriptor left untouched by an incremental build keeps its
entry'() {
+ given: 'two descriptors, then only the one about to be rebuilt is
cleaned, as Gradle does'
+ compile(plugin('Greeting'))
+ compile(plugin('Farewell'))
+ new File(targetDir,
'com/example/FarewellAutoConfiguration.class').delete()
+
+ when:
+ compile(plugin('Farewell'))
+
+ then: 'the one that was not rebuilt is still generated, so it is still
registered'
+ importsEntries() == ['com.example.FarewellAutoConfiguration',
'com.example.GreetingAutoConfiguration']
+ }
+
+ void 'two descriptors recompiling together do not prune one another'() {
+ given: 'both were built before, and both stale outputs are cleaned'
+ compile(plugin('Greeting'))
+ compile(plugin('Farewell'))
+ new File(targetDir,
'com/example/GreetingAutoConfiguration.class').delete()
+ new File(targetDir,
'com/example/FarewellAutoConfiguration.class').delete()
+
+ when: 'they compile as one unit, so neither class file exists while
the other registers'
+ compileTogether([plugin('Greeting'), plugin('Farewell')])
+
+ then:
+ importsEntries() == ['com.example.FarewellAutoConfiguration',
'com.example.GreetingAutoConfiguration']
+ }
+
+ void 'a descriptor deleted with nothing to replace it takes its entry with
it'() {
+ given: 'the descriptor is gone, so nothing generates a sibling and
register is never called'
+ compile(plugin('Greeting'))
+ new File(targetDir,
'com/example/GreetingAutoConfiguration.class').delete()
+ new File(targetDir, 'com/example/GreetingGrailsPlugin.class').delete()
+
+ when: 'the compilation reconciles, which it does whether or not
anything was generated'
+ AutoConfigurationImportsWriter.reconcile(targetDir, null)
+
+ then: 'the file goes with the last entry - an empty imports file is a
resource saying nothing'
+ importsEntries() == []
+ !new File(targetDir, IMPORTS).exists()
+ }
+
+ void 'reconciling leaves an entry whose class is still generated'() {
+ given:
+ compile(plugin('Greeting'))
+
+ when:
+ AutoConfigurationImportsWriter.reconcile(targetDir, null)
+
+ then:
+ importsEntries() == ['com.example.GreetingAutoConfiguration']
+ }
+
+ void 'reconciling does not create a file for a module that generates
nothing'() {
+ when: 'a module with no beans closure anywhere, which is most of them'
+ AutoConfigurationImportsWriter.reconcile(targetDir, null)
+
+ then:
+ !new File(targetDir, IMPORTS).exists()
+ }
+
+ void 'reconciling leaves a hand-authored file alone'() {
+ given: 'no generated file, which is how a module keeping its own looks
from here'
+ File handAuthored = new File(projectDir,
"src/main/resources/${IMPORTS}")
+ handAuthored.parentFile.mkdirs()
+ handAuthored.text = 'com.elsewhere.FromAnotherJar\n'
+
+ when:
+ AutoConfigurationImportsWriter.reconcile(targetDir, null)
+
+ then:
+ handAuthored.readLines() == ['com.elsewhere.FromAnotherJar']
+ !new File(targetDir, IMPORTS).exists()
+ }
+
+ void 'a module that keeps the file by hand keeps it'() {
+ given: 'a hand-authored file, which may hold entries no compilation
can discover'
+ File handAuthored = new File(projectDir,
"src/main/resources/${IMPORTS}")
+ handAuthored.parentFile.mkdirs()
+ handAuthored.text =
'com.example.GreetingAutoConfiguration\ncom.elsewhere.FromAnotherJar\n'
+
+ when:
+ compile(plugin('Greeting'))
+
+ then: 'nothing is generated beside it, which would put the same
resource at the same path twice'
+ !new File(targetDir, IMPORTS).exists()
+
+ and: 'and the entries it alone knows about are untouched'
+ handAuthored.readLines().contains('com.elsewhere.FromAnotherJar')
+ }
+
+ void 'a hand-authored file missing the generated class is warned about'() {
+ given:
+ File handAuthored = new File(projectDir,
"src/main/resources/${IMPORTS}")
+ handAuthored.parentFile.mkdirs()
+ handAuthored.text = 'com.elsewhere.FromAnotherJar\n'
+
+ when:
+ compile(plugin('Greeting'))
+
+ then: 'silently registering nothing is the failure this exists to
prevent'
+ warnings().any {
+ it.contains('com.example.GreetingAutoConfiguration') &&
it.contains(IMPORTS)
+ }
+ }
+
+ private List<String> collectedWarnings = []
+
+ private List<String> warnings() {
+ collectedWarnings
+ }
+
+ private List<String> importsEntries() {
+ File file = new File(targetDir, IMPORTS)
+ file.exists() ? file.readLines().findAll { it.trim() &&
!it.startsWith('#') } : []
+ }
+
+ private static String plugin(String name, String grailsBeansMembers = '') {
+ """
+ package com.example
+
+ import grails.compiler.beans.GrailsBeans
+ import grails.plugins.Plugin
+ import org.springframework.boot.autoconfigure.AutoConfiguration
+
+ @GrailsBeans(${grailsBeansMembers})
+ @AutoConfiguration
+ class ${name}GrailsPlugin extends Plugin {
+ def beans = {
+ bean('${name.uncapitalize()}Greeting', String) { 'hello' }
+ }
+ }
+ """
+ }
+
+ private CompilerConfiguration compileTogether(List<String> sources) {
+ CompilationUnit unit = newUnit()
+ sources.eachWithIndex { String source, int index ->
unit.addSource("Together${index}.groovy", source) }
+ run(unit)
+ }
+
+ /** A real compilation, since only a target directory makes the generated
file observable. */
+ private CompilerConfiguration compile(String source) {
+ CompilationUnit unit = newUnit()
+ unit.addSource("Source${System.identityHashCode(source)}.groovy",
source)
+ run(unit)
+ }
+
+ private CompilationUnit newUnit() {
+ CompilerConfiguration configuration = new CompilerConfiguration()
+ configuration.targetDirectory = targetDir
+ new CompilationUnit(configuration, null, new
GroovyClassLoader(getClass().classLoader, configuration))
+ }
+
+ /**
+ * Compiled through to OUTPUT, which is the phase that writes the class
files. Stopping earlier
+ * would leave the output directory empty, and what is still generated
there is exactly what
+ * decides whether an entry is kept.
+ */
+ private CompilerConfiguration run(CompilationUnit unit) {
+ try {
+ unit.compile(Phases.OUTPUT)
+ }
+ catch (Exception ignored) {
+ // the generated file is what is under test, not the class
+ }
Review Comment:
Suppressing every compilation exception lets these tests pass after the
metadata is written even when the generated sibling cannot actually compile or
be emitted. That defeats the stated real-compilation coverage and can hide
regressions in the behavior being registered; allow `unit.compile` failures to
fail the spec.
--
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]
