desruisseaux commented on PR #1144: URL: https://github.com/apache/maven-compiler-plugin/pull/1144#issuecomment-5968985466
Can we keep this pull request in draft mode? It will be a large effort to review. Some concerns that I had at a first look: * It seems to do at least two things: 1) scan for dependencies, and 2) check ABI compatibility. Only 1 is necessary for correctness, while 2 is a finer-grain change detection compared to timestamps. However, it is not clear to me that the added cost of ABI analysis is a win compared to the cost of some false positives when using only timestamps. * The ABI is formatted as a `String`, then SHA-512 is computed from that `String`. For example, `ACC_PUBLIC` integer value is formatted as the `"public"` string. This adds unnecessary processing compared to computing SHA-512 on a sequence of bytes were binary values are kept binary. Note: this issue disappear if we omit item 2 above. * This pull requests duplicates low-level details that could be expressed with higher-level standard API. For example, the `ACC_PUBLIC`, etc. constants defined in `ClassAnalyzer` are already available in `java.lang.reflect.Modifier`. * This pull request depends on `jdk.compiler` module. But all modules starting with `jdk.` are not part of the Java specification and are not guaranteed to be available everywhere. For example, I don't know if the Eclipse compiler implements those interfaces. * This pull request adds another feature in the mix: detecting the annotation processor type by scanning two non-standard property files. One of them is Gradle-specific, the other one I'm not sure where it come from. What about the following implementation strategy? 1. In a separated pull request, setup Maven Compiler plugin as a multi-release project using Maven Compiler 4.0.0-beta-5 (I can volunteer for that). 2. Start with scanning dependencies only. Leave ABI analysis, SHA-512 and detection of annotation processor type to future pull requests. 3. Use `java.lang.classfile` only. No ASM. It means that dependency tracking would not be available before Java 24, but I think that it is okay. It only means that safe compilation will be slower on those platforms, nothing more. 4. Instead of doing an analysis during compilation, do the analysis of `.class` files after compilation. It would remove the dependency to the `jdk.compiler` module and would allow incremental compilation to work with any compilers and also in forked mode (not supported by the current pull request). -- 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]
