On Mon, Jan 3, 2022 at 7:30 AM Volkan Yazıcı <[email protected]> wrote:
> 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. > I agree with Volkan. Whatever nightmare of a mess JPMS is making in master, it is no longer possible to cherry-pick what should be simple commits from release-2.x to master. Patches/diff files are not even possible, it is all manual, this is a f*g PITA for both development and maintenance as I am currently experiencing :-( This is not a good use of time. Frustrated, Gary > > > 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. >
