gnodet-bot commented on code in PR #1154: URL: https://github.com/apache/maven-compiler-plugin/pull/1154#discussion_r4188410548
########## src/it/abi-incremental-basic/verify.groovy: ########## @@ -0,0 +1,44 @@ +/* + * 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 + * + * http://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. + */ + +// All class files should exist +assert new File( basedir, 'target/classes/api/Model.class' ).exists() +assert new File( basedir, 'target/classes/impl/Helper.class' ).exists() +assert new File( basedir, 'target/classes/impl/Service.class' ).exists() + +// Graph state is stored in maven-status dir (not inside classes/ to avoid polluting JARs) +assert new File( basedir, 'target/maven-status/maven-compiler-plugin/classes/incremental-state' ).exists() + +// Build log should show graph-strategy-specific incremental messages +def logFile = new File( basedir, 'build.log' ) +assert logFile.exists() +def content = logFile.text + +// Step 1 should be a full build via the graph strategy +assert content.contains( 'graph: full build' ) + +// Step 3 should show incremental behavior via the graph strategy (not recompiling all 3 files) +assert content.contains( 'graph: incremental' ) + +// ABI manifest should be written to target/ for downstream reactor modules +assert new File( basedir, 'target/abi-fingerprints' ).exists() +def manifest = new File( basedir, 'target/abi-fingerprints' ).text +assert manifest.contains( 'api.Model=' ) +assert manifest.contains( 'impl.Helper=' ) +assert manifest.contains( 'impl.Service=' ) Review Comment: 🔴 **Critical:** This IT asserts that `target/abi-fingerprints` exists and contains class→fingerprint entries, but the `graph` strategy does not produce ABI fingerprints — `abiFingerprint` was removed from the java24 `ClassAnalysis` record in this force-push, and `GraphIncrementalBuild` has never written an ABI manifest file. This IT will fail at verification. These assertions belong in PR #1155 (the `abi` strategy), or should be removed from this PR entirely. ```suggestion ``` ########## src/main/java/org/apache/maven/plugin/compiler/incremental/BytecodeAnalyzer.java: ########## @@ -0,0 +1,221 @@ +/* + * 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 + * + * http://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.apache.maven.plugin.compiler.incremental; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Set; + +/** + * Facade for analyzing compiled {@code .class} files to extract type references + * and compute bytecode-level ABI fingerprints. + * + * <p>This class is part of a multi-release JAR. The root implementation (loaded on + * JDK < 24) always returns {@code false} from {@link #isAvailable()} — ABI + * fingerprinting is not supported on JDK 17–23. The {@code META-INF/versions/24/} + * override (loaded automatically by the JVM on JDK 24+) provides the real + * implementation backed by the standard {@code java.lang.classfile} API. + * + * <p>Callers must check {@link #isAvailable()} before calling {@link #analyze}. + * When unavailable, the ABI incremental strategy falls back to the timestamp strategy. + * + * <p>The {@link ClassAnalysis} record carries the class name, ABI fingerprint, + * human-readable canonical form, and two classified sets of type references: + * {@link ClassAnalysis#signatureTypes()} (API surface) and + * {@link ClassAnalysis#implementationTypes()} (method body instructions only). + */ +public final class BytecodeAnalyzer { + + /** + * Prefix used to distinguish module-info qualified names from regular class names. + * A type name starting with this prefix is a module name, not a class name. + */ + public static final String MODULE_PREFIX = "module:"; + + /** + * Analysis result for a single {@code .class} file. + * + * @param className fully-qualified class name (dot-separated) + * @param abiFingerprint 16-character hex SHA-256 prefix of the ABI canonical form + * @param abiCanonical human-readable representation of the public API surface + * @param signatureTypes types appearing in public API surface (method/field descriptors, + * supertype, interfaces, exception types, annotation types) + * @param implementationTypes types appearing only in method body bytecode instructions + * (INVOKEVIRTUAL, NEW, CHECKCAST, field owners, etc.) + * @param annotationTypes fully-qualified names of annotation types present on the class + * or its members, used for annotation processor cascade decisions + * @param moduleName Java module name, or empty string if unnamed or non-modular + * @param isModuleInfo {@code true} if this represents {@code module-info.class} + * @param sourceFileName simple source file name from the {@code SourceFile} class file + * attribute (e.g. {@code "Foo.java"}); empty string if absent + */ + public record ClassAnalysis( + String className, + String abiFingerprint, + String abiCanonical, Review Comment: ⚠️ **Inconsistency with java24 override:** The java24 `BytecodeAnalyzer.ClassAnalysis` record had `abiFingerprint` and `abiCanonical` removed in this force-push, but this root version still declares them. The two MRJAR versions of `ClassAnalysis` now have incompatible shapes. Since the root is a stub that throws on all methods, this won't cause a runtime error — but it's stale code that contradicts the java24 version. The `graph` strategy never uses these fields. ```suggestion * @param signatureTypes types appearing in public API surface (method/field descriptors, * supertype, interfaces, exception types, annotation types) * @param implementationTypes types appearing only in method body bytecode instructions * (INVOKEVIRTUAL, NEW, CHECKCAST, field owners, etc.) * @param annotationTypes fully-qualified names of annotation types present on the class * or its members, used for annotation processor cascade decisions * @param moduleName Java module name, or empty string if unnamed or non-modular * @param isModuleInfo {@code true} if this represents {@code module-info.class} * @param sourceFileName simple source file name from the {@code SourceFile} class file * attribute (e.g. {@code "Foo.java"}); empty string if absent */ public record ClassAnalysis( String className, Set<String> signatureTypes, Set<String> implementationTypes, Set<String> annotationTypes, String moduleName, boolean isModuleInfo, String sourceFileName) {} ``` ########## src/main/java/org/apache/maven/plugin/compiler/incremental/BytecodeAnalyzer.java: ########## @@ -0,0 +1,221 @@ +/* + * 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 + * + * http://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.apache.maven.plugin.compiler.incremental; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Set; + +/** + * Facade for analyzing compiled {@code .class} files to extract type references + * and compute bytecode-level ABI fingerprints. + * + * <p>This class is part of a multi-release JAR. The root implementation (loaded on + * JDK < 24) always returns {@code false} from {@link #isAvailable()} — ABI + * fingerprinting is not supported on JDK 17–23. The {@code META-INF/versions/24/} + * override (loaded automatically by the JVM on JDK 24+) provides the real + * implementation backed by the standard {@code java.lang.classfile} API. + * + * <p>Callers must check {@link #isAvailable()} before calling {@link #analyze}. + * When unavailable, the ABI incremental strategy falls back to the timestamp strategy. + * + * <p>The {@link ClassAnalysis} record carries the class name, ABI fingerprint, + * human-readable canonical form, and two classified sets of type references: + * {@link ClassAnalysis#signatureTypes()} (API surface) and + * {@link ClassAnalysis#implementationTypes()} (method body instructions only). Review Comment: 💡 **Stale Javadoc:** The class-level Javadoc still references ABI fingerprints ("compute bytecode-level ABI fingerprints", "ABI fingerprint, human-readable canonical form", "ABI incremental strategy"). The `graph` strategy doesn't use ABI fingerprints — it tracks all class-level dependencies and cascades on any change. This Javadoc should be updated to describe the actual behavior. ```suggestion /** * Facade for analyzing compiled {@code .class} files to extract type references. * * <p>This class is part of a multi-release JAR. The root implementation (loaded on * JDK < 24) always returns {@code false} from {@link #isAvailable()} — * bytecode analysis is not supported on JDK 17–23. The {@code META-INF/versions/24/} * override (loaded automatically by the JVM on JDK 24+) provides the real * implementation backed by the standard {@code java.lang.classfile} API. * * <p>Callers must check {@link #isAvailable()} before calling {@link #analyze}. * When unavailable, the incremental strategy falls back to the timestamp strategy. * * <p>The {@link ClassAnalysis} record carries the class name and two classified sets * of type references: {@link ClassAnalysis#signatureTypes()} (API surface) and * {@link ClassAnalysis#implementationTypes()} (method body instructions only). */ ``` -- 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]
