codeconsole commented on code in PR #16094: URL: https://github.com/apache/grails-core/pull/16094#discussion_r3781983182
########## grails-common/src/main/groovy/org/grails/aot/RegistrableTypes.java: ########## @@ -0,0 +1,251 @@ +/* + * 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.aot; Review Comment: Moved in 79bd5b9: `org.grails.aot` -> `org.apache.grails.common.aot`. ########## grails-core/src/main/groovy/org/grails/spring/beans/aot/AbstractBeanDefinitionExcludeFilter.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.spring.beans.aot; Review Comment: Done across the PR in 79bd5b9: ``` org.grails.aot -> org.apache.grails.common.aot org.grails.spring.beans.aot -> org.apache.grails.core.aot org.grails.plugins.web.controllers.aot -> org.apache.grails.controllers.aot org.grails.gsp.aot -> org.apache.grails.gsp.aot org.grails.plugins.web.taglib.aot -> org.apache.grails.gsp.taglib.aot org.grails.web.mime.aot -> org.apache.grails.mimetypes.aot grails.plugin.scaffolding.aot -> org.apache.grails.scaffolding.aot org.grails.web.databinding.aot -> org.apache.grails.web.databinding.aot org.grails.web.mapping.aot -> org.apache.grails.web.mapping.aot org.grails.datastore.gorm.aot -> org.apache.grails.datamapping.aot grails.gorm.validation.aot -> org.apache.grails.datamapping.validation.aot ``` Each module contributes its own package. The Gradle plugin classes stay in `org.grails.gradle.plugin.aot` beside the rest of that plugin, since that jar is a build plugin rather than something an application puts on its module path. Say if you want those moved as well. ########## grails-core/src/main/groovy/grails/boot/GrailsBanner.groovy: ########## @@ -152,52 +368,136 @@ class GrailsBanner implements Banner { case VersionOption.SPRING_SECURITY: ['Spring Security': findVersion('org.springframework.security.core.SpringSecurityCoreVersion')] break + case VersionOption.CONTAINER: + findContainerVersion() + break case VersionOption.TOMCAT: - ['Tomcat': findVersion('org.apache.catalina.util.ServerInfo')] + ['Tomcat': findTomcatVersion()] break case VersionOption.JETTY: - ['Jetty': findVersion('org.eclipse.jetty.util.Jetty')] + ['Jetty': findJettyVersion()] break case VersionOption.UNDERTOW: - ['Undertow': findVersion('io.undertow.Undertow')] + ['Undertow': findUndertowVersion()] break default: null } } as Map<String, String> + versions.findAll { String label, String version -> version != null } + } + + /** + * The servlet container the application is running on, and the version it records. + * + * <p>An application runs on one container: choosing another is done by excluding the starter + * for this one, so two are not on the classpath together. They are therefore tried in the order + * they are commonly used and the first one found is the answer -- an application on Tomcat + * never goes looking for Jetty.</p> + * + * <p>On a container that records no version, or on none of these, this is empty and the banner + * leaves the line out rather than saying it does not know.</p> + */ + protected Map<String, String> findContainerVersion() { + String tomcat = findTomcatVersion() + if (tomcat != null) { + return ['Tomcat': tomcat] + } + String jetty = findJettyVersion() + if (jetty != null) { + return ['Jetty': jetty] + } + String undertow = findUndertowVersion() + if (undertow != null) { + return ['Undertow': undertow] + } + return [:] } /** - * Finds the implementation version of the specified class. + * Tomcat's version, read from the resource it ships rather than only from its manifest. * - * @param className the fully qualified class name - * @return the implementation version, or 'unknown' if not found + * <p>A resource survives being repackaged into an executable jar or built into an image, where + * the manifest's attributes are no longer attached to the package -- which is why the manifest + * route reads as nothing in exactly the two places a version is most worth having.</p> */ - private static String findVersion(String className) { + protected String findTomcatVersion() { + findVersionInResource('org/apache/catalina/util/ServerInfo.properties', 'server.number') + ?: findVersion('org.apache.catalina.util.ServerInfo') + } + + protected String findJettyVersion() { + findVersion('org.eclipse.jetty.util.Jetty') + } + + protected String findUndertowVersion() { + findVersion('io.undertow.Undertow') + } + + /** + * A version a library records in a resource it ships, read without loading any of its classes. + * + * <p>The manifest route only works while a jar is a plain entry on the classpath. Repackaged + * into an executable jar its attributes are no longer attached to the package, and an image has + * no jars at all -- which is why a container version read that way reads as nothing in exactly + * the two places it is most worth having. A resource is still a resource in both.</p> + * + * @param resource the classpath location of the resource to read + * @param key the property within it that carries the version + * @return the version, or {@code null} where the resource or the property is absent + */ + protected static String findVersionInResource(String resource, String key) { + InputStream stream = GrailsBanner.classLoader.getResourceAsStream(resource) + if (stream == null) { + return null + } try { - def pkg = Class.forName(className).package - return pkg?.implementationVersion ?: 'unknown' + Properties properties = new Properties() + stream.withCloseable { properties.load(it) } + return properties.getProperty(key) + } + catch (IOException ignored) { + return null + } + } + + /** + * The version a library records in the manifest of the jar it ships in. + * + * <p>Loaded without being initialised. A version is read <em>about</em> a library rather than + * <em>from</em> it, and running a static initialiser to find one lets the library do whatever it + * does on the way -- Spring Security logs a line of its own from there, which arrived in the + * middle of the banner, between the mark and the very versions it was being read for. The + * manifest is attached to the package when the class is loaded, and loading is all this + * needs.</p> + * + * @param className the fully qualified name of a class the library ships + * @return the version, or {@code null} where the class is absent or records none + */ + protected static String findVersion(String className) { Review Comment: Changed in 437e627. A version asked for by name reads `unknown`; only the defaults are left out. The lookups moved out of `createBannerVersions` into `versionsFor(key, env)`, because deciding this per option needs the option that produced a label. `GrailsBannerNativeMarkSpec` covers both directions. Upgrade guide and what's-new updated to match. ########## grails-core/src/main/groovy/grails/boot/config/GrailsEnvironmentPostProcessor.java: ########## @@ -92,6 +101,43 @@ public void postProcessEnvironment(ConfigurableEnvironment environment, SpringAp } } + /** + * Colours the output of an image running at a terminal, which it otherwise cannot tell it has. + * + * <p>Spring Boot decides by asking for the console, and an image answers that it has none even + * when it is being watched at a terminal. So the same application whose start-up is coloured + * under {@code bootRun} arrives plain once it is built, for a reason that has nothing to do with + * the terminal it is running at.</p> + * + * <p>What the environment names as the terminal is read instead, which an image does carry. + * That does not distinguish output being watched from output being redirected -- nothing in an + * image does, which is the whole difficulty -- so a shell that redirects to a file still gets + * the escapes. It does distinguish a shell from the places that name no terminal at all: a + * build, a container, a service manager, where the output is only ever read later and stays + * plain. An application that has said either way is left alone.</p> + */ + private void colourTheOutputOfAnImageThatHasATerminal(ConfigurableEnvironment environment) { Review Comment: Removed in 437e627. `GrailsEnvironmentPostProcessor` is byte-identical to the merge base again and the spec is gone, which takes `isImage()` and `terminal()` with it. An image that should be coloured can be told so with `spring.output.ansi.enabled`, which needs nothing here. ########## grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/GroovyPagesGrailsPlugin.groovy: ########## @@ -166,27 +187,22 @@ class GroovyPagesGrailsPlugin extends Plugin { } } - def deployed = !Metadata.getCurrent().isDevelopmentEnvironmentAvailable() groovyPageLocator(CachingGrailsConventionGroovyPageLocator) { bean -> bean.lazyInit = true if (customResourceLoader) { resourceLoader = groovyPageResourceLoader } - if (deployed) { - Resource defaultViews = applicationContext?.getResource('gsp/views.properties') - - if (defaultViews != null) { - if (!defaultViews.exists()) { - defaultViews = applicationContext?.getResource('classpath:gsp/views.properties') - } - } - - if (defaultViews?.exists()) { - precompiledGspMap = { PropertiesFactoryBean pfb -> - ignoreResourceNotFound = true - locations = [defaultViews] as Resource[] - } - } + // Where the pages compiled at build time are listed. Attached whatever the + // surroundings, because whether to read from it is decided where a page is looked + // up, at run time, and only there is the answer knowable: deciding it here settles + // it while the definition is being generated, in the directory the application was + // built in, where a development environment is available -- so an image would be + // built believing it has to compile its pages, which is the one thing it cannot do. + // Named rather than resolved, so that what is written down is a location to look in + // and not a path on the machine that did the building. + precompiledGspMap = { PropertiesFactoryBean pfb -> Review Comment: Gated again in 67abf890, on `!developmentMode` — which already carries the AOT override, so the manifest is attached for a deployed application and for one whose code is being written out, and not for a project on disk. The locations stay named rather than resolved; that was the part that had to change. `GroovyPagesGrailsPluginPrecompiledSpec` asserts both directions: the property is absent from the `groovyPageLocator` definition in development mode, present when deployed. So a stale `gsp/views.properties`, or one shipped inside a plugin jar, no longer reaches the locator in dev. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy: ########## @@ -1115,6 +1268,150 @@ ${importStatements} } } + /** + * Wires up the cache the JDK can write for an application, so the next start reads what a + * training run worked out rather than working it out again. + * + * <p>Three steps, because the cache is only usable against the layout it was trained on: the + * archive is extracted, the extracted application is run and asked for its pages, and what the + * run recorded is left beside it. An application asks for this with + * {@code grails.aotCache.enabled}, and says which of its pages matter.</p> + */ + protected void configureAotCache(Project project) { + AotCacheExtension extension = ((ExtensionAware) project.extensions.getByName('grails')) + .extensions.create('aotCache', AotCacheExtension) + extension.enabled.convention(false) + extension.paths.convention([]) + extension.jvmArguments.convention(['-Dspring.aot.enabled=true', '-Dgrails.env=production']) + extension.port.convention(TRAINING_PORT) + extension.startTimeoutSeconds.convention(TRAINING_START_TIMEOUT_SECONDS) + + project.pluginManager.withPlugin(SPRING_BOOT_PLUGIN) { + TaskProvider<?> bootJar = project.tasks.named('bootJar') + Provider<Directory> application = project.layout.buildDirectory.dir('aot-cache/application') + Provider<JavaLauncher> launcher = trainingLauncher(project) + + TaskProvider<Exec> extract = project.tasks.register('extractAotCacheApplication', Exec) { Exec task -> + task.group = BasePlugin.BUILD_GROUP + task.description = 'Extracts the application, which is the form the cache is read against' + task.onlyIf { extension.enabled.get() } + task.dependsOn(bootJar) + // Named so the extraction is skipped when the archive it came from has not moved, + // rather than repeated on every run because nothing said what it produced. + task.inputs.file(project.provider { archiveOf(bootJar) }) + task.outputs.dir(application) + task.doFirst { + File destination = application.get().asFile + project.delete(destination) Review Comment: Fixed in 67abf890. `extractAotCacheApplication` is now an `ExtractApplicationTask`: `@InputFile` archive, `@OutputDirectory` destination, `@Input` java executable, with injected `FileSystemOperations` and `ExecOperations`. Nothing reaches `Project` at execution time. `archiveOf` is now `archiveFileOf` returning `Provider<RegularFile>`, which the trace and training tasks take as well, so nothing calls `TaskProvider.get()` while the build is configured. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy: ########## @@ -836,6 +869,39 @@ ${importStatements} it.destinationDirectory = project.layout.buildDirectory.dir('assetCompile/assets') } } + configureAssetsOnTheClasspath(project) + } + + /** + * Packages the compiled assets where an executable jar can read them. + * + * <p>The asset pipeline plugin puts them at the root of whatever archive is built, which is + * where a war serves its web content from and is therefore right for a war. An executable jar + * has no web content: it serves assets by reading them off the classpath, and its classpath is + * {@code BOOT-INF/classes} -- so the same assets, at the same place, in a jar rather than a war, + * are packaged but unreachable, and every asset a page asks for is a 404 while the page itself + * renders. Adding them under the classpath directory is what makes them found.</p> + * + * <p>Only for {@code bootJar}. A war already serves them from the root, and putting them on its + * classpath as well would ship the same bytes twice.</p> + */ + private void configureAssetsOnTheClasspath(Project project) { + project.pluginManager.withPlugin(SPRING_BOOT_PLUGIN) { + // Read after the build script has run, and by the task the pipeline registers rather + // than by the plugin that registers it: the asset pipeline's plugin id has changed + // once already, and the task name has not. + project.afterEvaluate { Review Comment: Replaced in 67abf890 with a `FileCollection` over `tasks.matching { it.name == 'assetCompile' }`, still guarded by `pluginManager.withPlugin`. Worth noting `matching {}.configureEach {}` on its own is not equivalent: it fires when the task is realized, which can be after `bootJar` has already run, and the assets end up missing from the jar. `AssetClasspathPackagingSpec` catches that. ########## grails-core/src/main/groovy/org/grails/spring/beans/AbstractResourceLocatorPostProcessor.java: ########## @@ -61,10 +63,25 @@ public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) t } GenericBeanDefinition definition = new GenericBeanDefinition(); definition.setAbstract(true); - definition.getPropertyValues().add("searchLocations", this.searchLocations); + definition.getPropertyValues().add("searchLocations", searchLocationsToInherit()); registry.registerBeanDefinition(BEAN_NAME, definition); } + /** + * The locations to be inherited, which while code is being generated are none. + * + * <p>These are directories on the machine this runs on, and a child definition merges them in. + * Generating code for that child writes them into it, so an application would carry the + * directory it was built in and look for its resources there -- a path that says where it was + * built and, wherever it then runs, is not where its resources are. A generated application + * reads them from its own contents instead, which is what is left when there is nowhere named + * to look.</p> + */ + private List<String> searchLocationsToInherit() { Review Comment: Extracted in d81902f: `org.apache.grails.common.aot.AheadOfTimeProcessing.isGeneratingCode()`, beside the other shared ahead-of-time code as you suggested. It went from four sites to one — `AbstractResourceLocatorPostProcessor`, `GroovyPagesGrailsPlugin`, `AbstractDatastoreInitializer`, and `UrlMappingsGrailsPlugin`, the last of which no longer needs it at all (see the thread on that file). GORM reaches it through `grails-datastore-core`'s existing `api` dependency on `grails-common`, so no new module dependency. On the search locations: resolution falls through to the locator's resource loader, which is the application context, so a child with none still finds a packaged resource. Spec added in b22be60 asserting exactly that. ########## grails-core/src/main/groovy/org/grails/spring/beans/aot/VarargsBeanRegistrationAotProcessor.java: ########## @@ -0,0 +1,180 @@ +/* + * 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.spring.beans.aot; + +import java.lang.reflect.Array; +import java.lang.reflect.Executable; +import java.util.Collection; +import java.util.List; +import java.util.function.Predicate; + +import org.jspecify.annotations.Nullable; + +import org.springframework.aot.generate.GenerationContext; +import org.springframework.beans.factory.aot.BeanRegistrationAotContribution; +import org.springframework.beans.factory.aot.BeanRegistrationAotProcessor; +import org.springframework.beans.factory.aot.BeanRegistrationCode; +import org.springframework.beans.factory.aot.BeanRegistrationCodeFragments; +import org.springframework.beans.factory.aot.BeanRegistrationCodeFragmentsDecorator; +import org.springframework.beans.factory.config.ConstructorArgumentValues; +import org.springframework.beans.factory.config.ConstructorArgumentValues.ValueHolder; +import org.springframework.beans.factory.support.RegisteredBean; +import org.springframework.beans.factory.support.RootBeanDefinition; +import org.springframework.javapoet.CodeBlock; +import org.springframework.util.ClassUtils; + +/** + * Gathers a variable-argument constructor argument into the array it feeds, ahead of time. + * + * <p>A bean declared through the plugin DSL passes its arguments positionally, and a constructor + * that ends in a variable-argument parameter is called the way the language allows: one value where + * the parameter is an array, or a collection where it is an array of that element type. Building the + * bean, Spring adapts the argument to the parameter. Reading the definition to generate code for it, + * Spring does not: it looks the argument up by the parameter's type, and a lone {@code String} does + * not answer to {@code String[]}.</p> + * + * <p>The argument is then missed and resolved as a dependency instead, and an array of a type nobody + * publishes as a bean resolves to an empty array rather than failing. So the bean is built, and + * built wrong: a datastore that maps no classes, or a servlet registration with no URL mapping, + * which then falls back to mapping everything. Nothing is logged, and the bean that goes wrong is + * rarely the one that reports it -- the first symptom is a page that 404s or a domain class that + * says it is not one.</p> + * + * <p>Gathering the argument into an array here means the generator writes out {@code new String[] + * {"*.gsp"}}, which the lookup does find. Only an argument that is already usable as the array is + * left alone, and an argument that would need its elements converted is left to the resolution that + * exists today rather than guessed at here.</p> + * + * @since 8.0 + */ +public class VarargsBeanRegistrationAotProcessor implements BeanRegistrationAotProcessor { + + @Override + @Nullable + public BeanRegistrationAotContribution processAheadOfTime(RegisteredBean registeredBean) { + Executable executable = resolveExecutable(registeredBean); + if (executable == null || !executable.isVarArgs()) { + return null; + } + Class<?>[] parameterTypes = executable.getParameterTypes(); + RootBeanDefinition beanDefinition = registeredBean.getMergedBeanDefinition(); + Object gathered = gatherTrailingArgument(beanDefinition.getConstructorArgumentValues(), parameterTypes); + if (gathered == null) { + return null; + } + return BeanRegistrationAotContribution.withCustomCodeFragments( + codeFragments -> new VarargsCodeFragments(codeFragments, gathered)); + } + + /** + * The constructor or factory method the generator will write the call to. + * + * <p>Resolution reads the bean class and its members, so a bean whose class cannot be resolved + * fails here rather than at the point of use. It is not this processor's place to report that: + * generation carries on and fails where it means something.</p> + */ + @Nullable + private Executable resolveExecutable(RegisteredBean registeredBean) { + try { + return registeredBean.resolveConstructorOrFactoryMethod(); + } + catch (Throwable ignored) { Review Comment: Narrowed in 437e627 to `catch (Exception)`, with a debug line naming the bean. An `Error` is a JVM out of memory or a class that will not link, and neither is a bean without a variable-argument constructor. -- 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]
