[LOG4J2-2561] JEP223 version detection fix for JDK 9 and up#260
[LOG4J2-2561] JEP223 version detection fix for JDK 9 and up#260rgoers merged 6 commits intoapache:masterfrom
Conversation
|
Can you add a jira issue for this as well? https://issues.apache.org/jira/browse/LOG4J2 |
ulrichenslin
changed the title
|
Jira LOG4J2-2561 added for this PR. |
remkop
left a comment
remkop
left a comment
There was a problem hiding this comment.
Looks ok to me. Do we have any unit tests for this, and if so, can we add a test for post-Java 9 version strings? If not, can we add some tests? (Of course making sure we reset the java.version property to its original value after the test or if the test fails.)
|
Added test for the detection testing. @remkop Thanks for the advise on reseting the java.version. I missed that in my initial test. |
|
update: looking at this on my phone, I missed that the PR also includes a test. Looks good to me. |
|
Oh, you just added that? Ok, thanks! |
| @@ -134,7 +134,13 @@ private static boolean isPreJava8() { | |||
| final String version = System.getProperty("java.version"); | |||
There was a problem hiding this comment.
Can we make this method delegate to a package-private static boolean isPreJava8(String javaVersion) which we can invoke directly from the test? The reflective method is likely to cause failures if we refactor this code in the future, and setting the java.version system property may have other consequences which I'd prefer to avoid.
There was a problem hiding this comment.
I agree, but wanted to make the code change as small as possible, since it is a very minor change.
If we want to be more adventures, we should move the org.apache.logging.log4j.util.Constants#getMajorVersion to core, and just have everything call that. Then the version detection is centralised and can we can test it as a public method.
How to you advise we move forward on this.
There was a problem hiding this comment.
Personally, I dislike checking for versions. I much prefer checking for features. But seeing that this was already implemented I wouldn't expect you to change that.
I am a bit confused by your question though. It looks like Constants has a getMajorVersion method that has the same problem. I would recommend you just fix that and then have AbstractStringLayout reference Constants.JAVA_MAJOR_VERSION. I don't know why you would need to move it from the API to core. If you want to make it able to be unit testable create a new method in Constants that is package private called getMajorVersion(String version) and then have getMajorVersion just be return getMajorVersion(System.getProperty("java.version");
There was a problem hiding this comment.
Please see updated PR. This is a cleaner implementation.
Version testing moved to org.apache.logging.log4j.util.ConstantsTest.testJdkVersionDetection
carterkozak
left a comment
carterkozak
left a comment
There was a problem hiding this comment.
This implementation looks a lot cleaner, thank you!
| import java.lang.reflect.InvocationTargetException; | ||
| import java.lang.reflect.Method; | ||
| import java.nio.charset.Charset; | ||
| import java.util.Properties; |
There was a problem hiding this comment.
Could you please fix the unused imports in this file?
| public class ConstantsTest { | ||
|
|
||
| @Test | ||
| public void testJdkVersionDetection() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { |
There was a problem hiding this comment.
nit: No need to declare these exception types now that we're calling getMajorVersion directly.
There was a problem hiding this comment.
The code has been cleaned up.
|
The tests on the CI is failing, but the failures are not related to the changes made in this PR. How do we move forward? |
|
@jvz Thanks for approving the PR. To get this ported to 2.11, do I just create another PR with the same change on a 'release-2.x' branch, with the same Jira number? |
|
That's one simple way to do it. I don't recall if I've merged any backported commits myself, though many of the other developers here will merge a commit to both branches concurrently. As for merging, if anyone else wants to do so, feel free. I'm out of town right now and won't have a chance to merge everything for a few more days. |
|
I'd appreciate a new PR. I have tried various options like cherry picking but never had much luck. I usually end up doing it by hand. |
5 participants
Fix version detection for JDK 9 and up