codeconsole commented on code in PR #16224: URL: https://github.com/apache/grails-core/pull/16224#discussion_r3899542630
########## 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: Confirmed and fixed in 1bce8fd. With the file holding `com.example.GreetingAutoConfiguration` and `com.example.StaleAutoConfiguration` and neither class present, `register` pruned the stale entry, found the pruned set equal to what it was about to write, returned false, and left it on disk. `register` now snapshots the file before the `removeIf` and compares against that, and the `entries.contains(className)` clause is gone as redundant. Covered by a case that fails against the previous code: the surviving class registers again, adding nothing already listed, and the stale entry does not survive the write it triggered. ########## 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: Confirmed and fixed in 1bce8fd. The warning is gated on `registeredBy(compilation).add(className)`, so the second registration of the same descriptor in one compilation is silent. The regression lives in `GlobalGrailsClassInjectorTransformationSpec` rather than this class own specification: only one of the two registrations happens in grails-beans-dsl, since the global transform is not on that module test classpath, so the duplicate cannot be reproduced there. Reverting the gate makes it report two. ########## 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: Taken as the first of the two, in 4fdc4b2, kept as its own commit so it can be dropped on its own if you would rather have the second. grails-core/build.gradle:59 declares `api project(:grails-beans-dsl)`, which is what the guard own comment gave as the reason its branch could never be taken. `compileBeansDsl` now constructs `GrailsBeansASTTransformation` directly, and the `ClassNotFoundException` branch is gone: it caught a condition the next statement would have failed on anyway, once the registering above it referenced the class directly. ########## 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: Fixed in 1bce8fd. The previous fixture is back as its own test: a descriptor named `*GrailsPlugin` that does not extend `Plugin`, asserting the closure is still consumed onto the class itself, plus the assertion you suggested that no imports file is generated for it. That second one is what pins the scope to the sibling generated for a plugin descriptor. -- 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]
