jdaugherty commented on code in PR #16224:
URL: https://github.com/apache/grails-core/pull/16224#discussion_r3897906295
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:
##########
@@ -148,6 +150,10 @@ class GlobalGrailsClassInjectorTransformation implements
ASTTransformation, Comp
pluginVersion = resolvePluginVersion(classNode,
projectVersion?.toString())
addPluginVersionProperty(classNode, pluginVersion)
compileBeansDsl(classNode, source)
+ String generatedAutoConfigurationName =
classNode.getNodeMetaData(
+
GrailsBeansASTTransformation.GENERATED_AUTO_CONFIGURATION_NAME_METADATA)
+ AutoConfigurationImportsWriter.register(
Review Comment:
`compileBeansDsl` just above deliberately loads
`GrailsBeansASTTransformation` reflectively and documents the
`ClassNotFoundException` branch, but this hunk adds direct references -
`GENERATED_AUTO_CONFIGURATION_NAME_METADATA` is built via `getName()`, so it is
not inlined as a compile-time constant and the field read loads the class, as
do the `AutoConfigurationImportsWriter` calls (including `reconcile`, which
runs for every source unit). If grails-beans-dsl is ever absent from the
compile classpath, every project source now fails with `NoClassDefFoundError`,
where previously the beans property was just left alone. Either that situation
is impossible, in which case the reflective load and its guard comment in
`compileBeansDsl` are dead ceremony that could go, or it is possible, and these
calls should take the same reflective route. Worth making the file tell one
story either way.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -324,9 +330,26 @@ private ClassNode createAutoConfigurationSibling(ClassNode
pluginClass, Annotati
sibling.addAnnotations(siblingAnnotations);
pluginClass.getAnnotations().removeAll(siblingAnnotations);
+ // The name is settled here and nowhere else. The global transform
consumes this metadata
+ // and registers it using its Eclipse-aware compilation target
resolution.
+
pluginClass.putNodeMetaData(GENERATED_AUTO_CONFIGURATION_NAME_METADATA,
siblingName);
+ if (!isEclipseSourceUnit(source)) {
+ AutoConfigurationImportsWriter.register(siblingName,
targetDirectory(source), source, compilationUnit);
Review Comment:
In the standard-compiler path this call and the global transform's
metadata-driven `register` both run for the same descriptor in the same
compilation: the implicit path reaches here through `compileBeansDsl`, and the
global transform then registers again from the node metadata. The double write
is absorbed by the short-circuit in `register`, but the hand-authored branch
has no such guard, so a module whose hand-authored file is missing the
generated class gets the warning emitted twice per compile. Reproduced by
compiling a descriptor with an implicit DSL `beans` closure through a
`CompilationUnit` with `targetDirectory` set and a hand-authored file listing
only another class: `errorCollector.warnings` holds the identical message twice.
Consider deduplicating per compilation, e.g. in the hand-authored branch of
`register`, gate the warning on `registeredBy(compilation).add(className)` so
the second call stays silent.
##########
grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy:
##########
@@ -336,7 +336,7 @@ class GlobalGrailsClassInjectorTransformationSpec extends
Specification {
sourceFile,
'''
@org.springframework.boot.autoconfigure.AutoConfiguration
- class DslBeansGrailsPlugin {
+ class DslBeansGrailsPlugin extends
grails.plugins.Plugin {
Review Comment:
Adding `extends grails.plugins.Plugin` retargets this test at the sibling
path, and the previous fixture's behavior - a descriptor not extending `Plugin`
whose DSL-shaped `beans` closure is claimed onto the class itself - loses its
only coverage here. Consider keeping the old fixture as its own test alongside
this one; the non-Plugin case could also assert that no imports file is
generated, which pins the 'only the sibling generated for a plugin descriptor
is registered' scope.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/AutoConfigurationImportsWriter.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * 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;
+import org.codehaus.groovy.syntax.SyntaxException;
+
+/**
+ * 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
+ * @param source the source being compiled, used for warnings and write
errors
+ * @param compilation what scopes names registered before their class
files are written
+ * @return {@code true} when the file was written
+ */
+ public 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.");
+ }
+ write(new File(targetDirectory, IMPORTS_LOCATION),
Collections.emptySet(), source);
+ 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)) {
Review Comment:
This compares `written` against the post-prune set, so a prune that adds
nothing new is never persisted. With the file holding
`com.example.GreetingAutoConfiguration` and
`com.example.StaleAutoConfiguration`, and neither class file present,
`register('com.example.GreetingAutoConfiguration', ...)` drops the stale entry
from `entries`, then finds `written.equals(entries)`, returns `false`, and
leaves the stale entry on disk - the startup failure the comment above says
this pruning prevents. In a Gradle build `reconcile` repairs it moments later,
but callers that only go through the local transform (this class's own spec
exercises exactly that path) keep the stale entry.
Snapshot the on-disk contents before the `removeIf` and compare `written`
against that instead; the `entries.contains(className)` clause then becomes
redundant.
--
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]
