Hi Wessel, Comments inline. On 14/08/2019 17:34, Wessel van Norel wrote:
Hello,
@Ceki Gülcü Thanks for approving the 1.7 release commit.
My pleasure.
I’ve tried to also add it to the 1.8.0-SNAPSHOT branch, but I couldn’t get a build for that branch working quickly. First I had to get a jdk6 and configure the m2 toolchain, but after that it still failed to build on the module-info. After moving that to a java9 directory, as you did in master, it still didn’t work. This because now the 1.6 jdk build was failing on a --release flag given to the javac command. After that I gave up, since I’m not sure if you are still working on 1.8 or that you decided to release a 2.0 to make the breaking character of the changes more clearly.
I have attached my copy of toolschains.xml used to build SLF4J 1.8.x for sake of completeness. As you can see, I was using Java 8 to provide Java 8 (D'oh) functionality and Java 11 to provide Java 9 functionality. In SLF4J 2.0, which relies on Multi-release jars, toolschains are no longer required. You just need to Java 11 to build things. This is miuch simpler. SLF4J 2.0 build on top of 1.8.x but adds fluent-api which is kind of a big deal.
Another thing that I noted during my backporting work was the build has flaky tests, at least on my machine. At least two tests based on the MultithreadedInitializationTest sometimes fail on the last assertTrue in this abstract test. I’ll investigate this further.
Yes, that happens to me as well. The problem is relatively hard to reproduce. I tried to tackle it in the past and had always failed.
One remaining question. I moved 2 methods from MessageFormatter to Util. I thought that it was ok to do so because one of them was package private and the other was private. But you moved them back to the MessageFormatter. Is that because it was package private before? So are only private methods ok to change and all other should be considered stable?
You can move private methods around. However, I felt that Util was not a better place than MessageFormatter and moved it back. I wrote a comment about this in github which has apparently fallen though the cracks. Best, -- Ceki