On Mon, Jan 3, 2022 at 5:28 PM Ralph Goers <[email protected]> wrote:
> > > > On Jan 3, 2022, at 5: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 would recommend against this for the following reasons: > 1. It is very clear which modules create test jars if they are the only > ones that > have a Maven test module. > 2. Having a Maven module that has nothing in src/main means you don’t have > an artifact. This module would have to be declared as type “pom” and skip > the > deploy step. > 3. When the source and unit tests reside in the same Maven module it seems > no module-info.java is required for the tests and everything can be tested > on > the module path as a JPMS module. Maven determines whether to populate > the module path and test as a module based on the presence of a > module-info.java > in src/main. If the project has no source I don’t believe it will test it > as a JPMS > module, which is something we very much want to do. > Sorry, I think I have been a victim of my own incorrect wording. I rather wanted to suggest "placing all tests _exposed as test JARs_ to their own modules". I think we both agree on this. Please, correct me if I am wrong. > > 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. > > To be honest, I haven’t been able to cherry-pick much of anything across > the > branches in quite a while. At this point with the work Matt has been doing > on > dependency injection and splitting things out into separate modules almost > everything has problems with cherry-picking. > Hrm... Sadly, what you say makes sense. > > >> 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? > > The default for the build is Java 11. You can’t simply switch the Java > version of > the compilerin the middle of the build without using toolchains. > I don't get it, why do we need to switch the Java version of the compiler in the middle of the build? I thought this is all we need: <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> <configuration> <release>11</release> </configuration> </plugin> Doesn't this enable us to use any JDK >= 11 and still target Java 11? That is, one can still use Java 17 and yet produce Java 11 bytecode. > >> 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? > > See my answer above as to why moving all tests to their own module is > a bad idea. > I will slightly change my question: if we move only tests exposed as test-JARs to their own module, does this still pose a problem? > >> 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? > > I could create a ticket for Junit 5 but I don’t see the point. At this > point it > would be a great deal of effort for them to fix it just for us. > I meant creating LOG4J2 tickets for #3 and #4 you have mentioned. They look like pretty isolated problems so people can pick them up.
