Copilot commented on code in PR #1075:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1075#discussion_r3665325604
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1559,8 +1559,25 @@ private Set<File> computeStaleSources(
}
try {
- staleSources.addAll(scanner.getIncludedSources(rootFile,
outputDirectory));
- } catch (InclusionScanException e) {
+ Set<File> includedSources =
scanner.getIncludedSources(rootFile, outputDirectory);
+ // The stale source scanner assumes that every source produces
an output file. Filter its result only
+ // when the compiler provides an individual source-to-output
mapping; aggregate outputs are ambiguous.
+ if (outputStyle ==
CompilerOutputStyle.ONE_OUTPUT_FILE_PER_INPUT_FILE) {
+ for (File source : includedSources) {
+ String relativePath =
+
rootFile.toPath().relativize(source.toPath()).toString();
+ boolean outputExists =
mapping.getTargetFiles(outputDirectory, relativePath).stream()
Review Comment:
`Path#toString()` is platform-dependent (`\` on Windows, `/` on Unix). If
`mapping.getTargetFiles(...)` expects a normalized (usually `/`-separated)
relative path, this can break output detection on Windows and cause
empty/truncated sources to be misclassified. Consider normalizing the relative
path to the format used by the mapping/scanner (e.g., replace separator chars
consistently or use a mapping API that accepts `Path`).
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1559,8 +1559,25 @@ private Set<File> computeStaleSources(
}
try {
- staleSources.addAll(scanner.getIncludedSources(rootFile,
outputDirectory));
- } catch (InclusionScanException e) {
+ Set<File> includedSources =
scanner.getIncludedSources(rootFile, outputDirectory);
+ // The stale source scanner assumes that every source produces
an output file. Filter its result only
+ // when the compiler provides an individual source-to-output
mapping; aggregate outputs are ambiguous.
+ if (outputStyle ==
CompilerOutputStyle.ONE_OUTPUT_FILE_PER_INPUT_FILE) {
+ for (File source : includedSources) {
+ String relativePath =
+
rootFile.toPath().relativize(source.toPath()).toString();
+ boolean outputExists =
mapping.getTargetFiles(outputDirectory, relativePath).stream()
+ .anyMatch(File::exists);
+ // A zero-byte compilation unit legitimately produces
no class. Keep it stale if an output
+ // exists, however, so that truncating an existing
source is still detected as a change.
+ if (Files.size(source.toPath()) != 0 || outputExists) {
+ staleSources.add(source);
+ }
+ }
Review Comment:
`outputExists` is computed for every included source (mapping lookup +
filesystem `exists()` checks) even when the source is non-empty. Since the
mapped-output check is only needed for 0-byte sources, compute the file size
first and only perform the mapping/exists check when `size == 0` to avoid
unnecessary IO and potential slowdown on large source sets.
##########
src/test/resources/unit/compiler-empty-source-change-detection-test/plugin-config.xml:
##########
@@ -0,0 +1,37 @@
+<!--
+ ~ 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.
+ -->
+
+<project>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration>
+ <compileSourceRoots>
+
<compileSourceRoot>${basedir}/target/test-classes/unit/compiler-empty-source-change-detection-test/src/main/java</compileSourceRoot>
+ </compileSourceRoots>
+ <compilerId>javac</compilerId>
+ <release>17</release>
Review Comment:
Hardcoding `<release>17</release>` makes the regression test require running
under a JDK that supports `--release 17`. If this branch/plugin line is
expected to test under multiple JDKs (or older baselines), this can cause CI
failures unrelated to the change-detection logic. Prefer using an existing
test/property-driven release level (or omit `<release>` if not needed for the
scenario) to keep the test environment-agnostic.
--
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]
