gnodet-bot commented on code in PR #1154: URL: https://github.com/apache/maven-compiler-plugin/pull/1154#discussion_r4181452689
########## src/it/abi-incremental-basic/verify.groovy: ########## @@ -0,0 +1,38 @@ +/* + * 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 incremental messages +def logFile = new File( basedir, 'build.log' ) +assert logFile.exists() +def content = logFile.text + +// Step 1 should be a full build +assert content.contains( 'full build' ) || content.contains( 'Compiling 3 source files' ) + +// Step 3 should show incremental behavior (not recompiling all 3 files) +// The Graph strategy should detect changes +assert content.contains( 'incremental' ) || content.contains( '1 file' ) Review Comment: 💡 **Weak assertion:** This condition: ```groovy assert content.contains('incremental') || content.contains('1 file') ``` would pass even if the graph strategy fell back to timestamp (which also logs "incremental" in some paths). A stronger assertion would check for the graph-specific log message, e.g.: ```groovy assert content.contains('graph: incremental') || content.contains('graph: full build') ``` Similarly, line 34's `contains('full build')` would match the timestamp strategy's messages too. Consider matching the exact graph-strategy wording (`graph: full build`, `graph: incremental`) to ensure the IT actually validates that the `graph` strategy was activated, not just that _some_ incremental strategy ran. The cascade IT (`abi-incremental-cascade/verify.groovy`) has a similar issue — it only checks `!content.contains('COMPILATION ERROR')`, which passes trivially even if the graph strategy never activated. ########## src/main/java24/org/apache/maven/plugin/compiler/incremental/BytecodeAnalyzer.java: ########## @@ -0,0 +1,160 @@ +/* + * 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, Review Comment: 💡 **Defensive coding inconsistency:** The root `BytecodeAnalyzer` (JDK <24) guards against malformed descriptors in `descriptorToReadable`: ```java int semi = desc.indexOf(';'); yield semi > 0 ? toJavaName(desc.substring(1, semi)) : desc; ``` But this java24 override does: ```java case 'L' -> toJavaName(desc.substring(1, desc.indexOf(';'))); ``` If `indexOf` returns -1 (malformed descriptor), this throws `StringIndexOutOfBoundsException`. Same pattern in `parseParams`: the root version guards `indexOf(';', i)` with `semi >= 0 ? semi + 1 : params.length()`, while this version does `i = params.indexOf(';', i) + 1` which would set `i = 0` on a missing `;`, causing an infinite loop. In practice this can't happen — JDK 24+ `ClassFile` API rejects corrupt bytecode before these utility methods are called, and the comment in the root version acknowledges this. But having different defensive behavior between the two copies is a maintenance hazard. Consider either making both defensive or extracting the shared utility methods into `ClassAnalyzer` (which both versions already extend/use). -- 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]
