Fantastic work Ralph! Please see my comments below: On Tue, Dec 28, 2021 at 9:39 AM Ralph Goers <[email protected]> wrote: > Most of the components that were generating test jars have been split into two modules - the main component, > which only builds the jar, and a -test component that builds a -test jar and then runs the unit tests.
For completeness and consistency across all modules *and* to (hopefully) mitigate the problem you have mentioned in #2, I think we should place all tests to their own modules. Even though we all agree that this is ugly, IIRC, our most recent Maven and JPMS research pointed us in this direction. I also propose backporting every "moving tests to a module" change to `release-2.x`. Otherwise, it will be really difficult to synchronize changes on both branches in the future. > 1. log4j-core uses toolchains on MacOS to use Java 17. There is a bug in javac on Macs that causes compilation > to fail. It is fixed in Java 17 but was not back-ported to Java 11. For one, I am fine if we set the build (not target!) JDK requirement to Java 17 for all platforms to work around this problem. This said, I am confused on why we still need Maven toolchains in `master`. Shouldn't we simply remove all Maven toolchains configuration and configure maven-compiler-plugin and maven-enforcer plugin to require 17 for build and target 11? > 2. In general the test jars have a second compilation step to create the module-info.java. Unfortunately, the unit > tests cannot be modularized because: > a. the unit tests open the main module (i.e. they use the same package names). > b. the test jar requires the main module. > c. the unit tests require the test module. > This creates a circularity because the unit tests depend on the test module and it requires the same packages > as the unit tests. Will this still pose a problem if we move all tests to their own module? > 3. I tried building entirely with Java 17 but there were some unit tests in log4j-core-test that fail. There is one class > that is trying to modify a final variable in the Constant class and it seems that the technique it is using is no > longer valid. > 4. log4j-jul must use Junit 4. It seems Junit 5 is hard-wired to use java.util.logging and so it isn’t possible to set the > system property in time to have the unit tests use Log4j’s LogManager. Do we have tickets for these two? > 5. JsonTemplateLayout is still creating a test-jar. I haven’t validated if anything needs it. If it does than it will need to > be split to have a -test module as well. JTL test-jar is used by log4j-perf. I have created LOG4J2-3308 <https://issues.apache.org/jira/browse/LOG4J2-3308> to address this. > 6. I split the annotation processor into its own module - log4j-plugin-processor. Note that with modular compiles > annotation processors are no longer added automatically so this should be clearer. > 7. I have not added module-info.java files to anything beyond log4j-api, log4j-plugins, and log4j-core. That will be > coming next. > 8. log4j-api-test has a few tests that seem to randomly fail. I didn’t look into them. Again, great work Ralph! Thanks so much for pushing `master` further.
