You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "jvz (via GitHub)" <gi...@apache.org> on 2023/11/12 00:33:25 UTC
[PR] Work towards supporting Java 17 builds (logging-log4j2)
jvz opened a new pull request, #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964
If you add a dependency on GraalJS to `log4j-core-test`, that brings down the test failure count to 14 when run with Java 21. There are some tests that have to be disabled for later versions due to Java changes such as the removal of custom security managers (throws an exception right now in Java 21) and the removal of Nashorn (we need an explicit JavaScript `ScriptEngine` dependency in Java 17+). It's possible that Nashorn is available as a standalone dependency somewhere, but I haven't looked yet. When run without a JavaScript engine, we get 36 test failures and 2 test errors, all of which are script-related tests. Thus, this gets everything working again except for JavaScript `ScriptEngine` use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1390309413
##########
log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/StatusLoggerExtension.java:
##########
@@ -146,10 +148,16 @@ public void handleException(final ExtensionContext context, final Throwable thro
.withThrowable(data.getThrowable())
.withLocation(data.getStackTraceElement())
.log("{} {}",
- DateTimeFormatter.ISO_LOCAL_TIME.format(Instant.ofEpochMilli(data.getTimestamp())),
+ formatLocalTime(data.getTimestamp()),
data.getMessage());
});
}
+
+ private static String formatLocalTime(final long epochMilli) {
+ final Instant instant = Instant.ofEpochMilli(epochMilli);
+ final LocalDateTime localDateTime = LocalDateTime.ofInstant(instant, ZoneId.systemDefault());
+ return localDateTime.format(DateTimeFormatter.ISO_LOCAL_TIME);
+ }
Review Comment:
Turns out this change didn't work, so I fixed it and forgot to push to 2.x directly, so here you go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#issuecomment-1808761322
> @jvz, I miss a clear scope for this PR. To me it feels like (could very well be due to my lack of understanding) changes are at arbitrary places and doesn't much contribute to the PR goal: Java 17 support.
The scope of this PR is to fix a ton of tests that fail in Java 17 or Java 21. Most of those tests were failing due to breaking encapsulation rules of Java such as trying to redefine a static final field somewhere. Believe me, I wouldn't be making changes to 2.x that aren't in scope of anything important! I'm all-in on the main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#issuecomment-1809925996
> > @jvz, I miss a clear scope for this PR. To me it feels like (could very well be due to my lack of understanding) changes are at arbitrary places and doesn't much contribute to the PR goal: Java 17 support.
>
> The scope of this PR is to fix a ton of tests that fail in Java 17 or Java 21. Most of those tests were failing due to breaking encapsulation rules of Java such as trying to redefine a static final field somewhere. Believe me, I wouldn't be making changes to 2.x that aren't in scope of anything important! I'm all-in on the main branch.
Okay. I would really appreciate it if you can update the description of the ticket accordingly. What is the goal? What is failing? The list of failures addressed, etc. One last remark: I would only address 17 and, if needed, worry about 21 in a follow-up ticket.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1397673300
##########
log4j-core-its/src/test/java/org/apache/logging/log4j/core/async/perftest/ResponseTimeTest.java:
##########
@@ -141,7 +141,7 @@ public static void main(final String[] args) throws Exception {
runLatencyTest(logger, WARMUP_DURATION_MILLIS, WARMUP_COUNT, loadMessagesPerSec, idleStrategy,
warmupServiceTmHistograms, warmupResponseTmHistograms, threadCount);
System.out.println("-----------------Warmup done. load=" + loadMessagesPerSec);
- if (!Constants.ENABLE_DIRECT_ENCODERS || !Constants.ENABLE_THREADLOCALS) {
+ if (!Constants.ENABLE_DIRECT_ENCODERS || !org.apache.logging.log4j.util.Constants.isUseThreadLocals()) {
Review Comment:
Yes, that was the idea here. In main, that's something we're trying to clean up more for the properties enhancement, but it requires a lot of refactoring, hence why it's limited to main.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1391563925
##########
log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/StatusLoggerExtension.java:
##########
@@ -146,10 +148,16 @@ public void handleException(final ExtensionContext context, final Throwable thro
.withThrowable(data.getThrowable())
.withLocation(data.getStackTraceElement())
.log("{} {}",
- DateTimeFormatter.ISO_LOCAL_TIME.format(Instant.ofEpochMilli(data.getTimestamp())),
+ formatLocalTime(data.getTimestamp()),
data.getMessage());
});
}
+
+ private static String formatLocalTime(final long epochMilli) {
+ final Instant instant = Instant.ofEpochMilli(epochMilli);
+ final LocalDateTime localDateTime = LocalDateTime.ofInstant(instant, ZoneId.systemDefault());
+ return localDateTime.format(DateTimeFormatter.ISO_LOCAL_TIME);
+ }
Review Comment:
I pushed this change to both `2.x` and `main` (it was also breaking #1961), so you can rebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1390431301
##########
log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/StatusLoggerExtension.java:
##########
@@ -146,10 +148,16 @@ public void handleException(final ExtensionContext context, final Throwable thro
.withThrowable(data.getThrowable())
.withLocation(data.getStackTraceElement())
.log("{} {}",
- DateTimeFormatter.ISO_LOCAL_TIME.format(Instant.ofEpochMilli(data.getTimestamp())),
+ formatLocalTime(data.getTimestamp()),
data.getMessage());
});
}
+
+ private static String formatLocalTime(final long epochMilli) {
+ final Instant instant = Instant.ofEpochMilli(epochMilli);
+ final LocalDateTime localDateTime = LocalDateTime.ofInstant(instant, ZoneId.systemDefault());
+ return localDateTime.format(DateTimeFormatter.ISO_LOCAL_TIME);
+ }
Review Comment:
Somehow I always get the time API wrong. It should be:
```
DateTimeFormatter.ISO_LOCAL_TIME.withZone(ZoneId.systemDefault()).format(Instant.ofEpochMilli(data.getTimestamp())),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1402775588
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Constants.java:
##########
@@ -120,4 +115,44 @@ static int getMajorVersion(final String version) {
return 0;
}
}
+
+ /**
+ * {@code true} if we think we are running in a web container, based on the boolean value of system property
+ * "log4j2.is.webapp", or (if this system property is not set) whether the {@code javax.servlet.Servlet} class
+ * is present in the classpath.
+ */
+ public static boolean isWebApp() {
+ return WEB_APP.getAsBoolean();
+ }
+
+ @InternalApi
+ public static void setWebApp(final boolean webApp) {
+ WEB_APP.setAsBoolean(webApp);
+ }
+
+ @InternalApi
+ public static void resetWebApp() {
+ WEB_APP.reset();
+ }
Review Comment:
We can use `@RestrictedApi` from error prone as that's something the compiler can enforce (at least when using error prone). But right, this whole concept of these two values shouldn't really be public as it's an internal detail used by some tests. Should think of another way to do this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#issuecomment-1821560341
@jvz,
When you fix the tests under Java 17, can you close #1851 and fix apache/logging-parent#62 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1401860900
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Constants.java:
##########
@@ -120,4 +115,44 @@ static int getMajorVersion(final String version) {
return 0;
}
}
+
+ /**
+ * {@code true} if we think we are running in a web container, based on the boolean value of system property
+ * "log4j2.is.webapp", or (if this system property is not set) whether the {@code javax.servlet.Servlet} class
+ * is present in the classpath.
+ */
+ public static boolean isWebApp() {
+ return WEB_APP.getAsBoolean();
+ }
+
+ @InternalApi
+ public static void setWebApp(final boolean webApp) {
+ WEB_APP.setAsBoolean(webApp);
+ }
+
+ @InternalApi
+ public static void resetWebApp() {
+ WEB_APP.reset();
+ }
Review Comment:
I am a little bit sensitive about adding public methods in `log4j-api`. Even if the `o.a.l.l.util` package is marked as internal, it **isn't** (it contains `Supplier` for example).
Is this something we are willing to support in `3.x`? It seems to me that methods like `Constants#isWebApp()` and `Constants#isUseThreadLocals()` should be caller sensitive or at least depend on the classloader where the `ThreadLocal`s will be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1390309587
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/ClockFactoryTest.java:
##########
@@ -16,32 +16,15 @@
*/
package org.apache.logging.log4j.core.util;
-import java.lang.reflect.Field;
-
-import org.apache.commons.lang3.reflect.FieldUtils;
-import org.apache.logging.log4j.core.async.AsyncLogger;
-import org.apache.logging.log4j.core.impl.Log4jLogEvent;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.condition.EnabledOnJre;
-import org.junit.jupiter.api.condition.JRE;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertSame;
-// as of Java 12, final fields can no longer be overwritten via reflection
-@EnabledOnJre({ JRE.JAVA_8, JRE.JAVA_9, JRE.JAVA_10, JRE.JAVA_11 })
Review Comment:
No longer requiring reflection here, so we get more tests to run on Java 17, too! Yay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#issuecomment-1807261101
Fixes #1851.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#issuecomment-1823926687
@jvz,
I'll cherry-pick some changes from here to provide a minimal fix to tests on JRE 17.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1390309480
##########
log4j-core-test/pom.xml:
##########
@@ -379,6 +379,22 @@
</annotationProcessorPaths>
</configuration>
</plugin>
+ <!--
+ https://junit-pioneer.org/docs/environment-variables/#warnings-for-reflective-access
+ -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <argLine>
+ <!-- for environment variable access -->
+ --add-opens java.base/java.util=ALL-UNNAMED
+ --add-opens java.base/java.lang=ALL-UNNAMED
+ <!-- for URLStreamHandlerFactory access -->
+ --add-opens java.base/java.net=ALL-UNNAMED
Review Comment:
We have some tests that try to modify internals of `URLStreamHandlerFactory` to test something related to handling `URL`s. Yet another sign we could use our own `Resource`-style API. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1391506158
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Constants.java:
##########
@@ -22,24 +22,20 @@
* @since 2.6.2
*/
public final class Constants {
+ private static final LazyBoolean WEB_APP = new LazyBoolean(() -> isWebApp(PropertiesUtil.getProperties()));
Review Comment:
It's to make testing this stuff not require breaking encapsulation or other hacks which aren't allowed in newer versions of Java. This is largely the same basic refactoring as was done in main for these two properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#issuecomment-1818869481
@jvz,
I'll apply the Palantir formatter to `main`. As Carter suggested, it should be pretty easy to merge this PR if you do the same here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1391509246
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/Constants.java:
##########
@@ -76,8 +76,10 @@ public final class Constants {
* {@code true} if we think we are running in a web container, based on the boolean value of system property
* "log4j2.is.webapp", or (if this system property is not set) whether the {@code javax.servlet.Servlet} class
* is present in the classpath.
+ * @deprecated use {@link org.apache.logging.log4j.util.Constants#isWebApp()}
*/
- public static final boolean IS_WEB_APP = org.apache.logging.log4j.util.Constants.IS_WEB_APP;
+ @Deprecated
Review Comment:
This one is already here. I deprecated it because the method should be invoked instead. Nothing in our codebase is using this field, but I left it in place for backward compatibility (well, the BND plugin would make me bump this package to 3.0.0 if I removed it to be honest lol)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1391507645
##########
log4j-core-test/pom.xml:
##########
@@ -379,6 +379,22 @@
</annotationProcessorPaths>
</configuration>
</plugin>
+ <!--
+ https://junit-pioneer.org/docs/environment-variables/#warnings-for-reflective-access
+ -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <argLine>
+ <!-- for environment variable access -->
+ --add-opens java.base/java.util=ALL-UNNAMED
+ --add-opens java.base/java.lang=ALL-UNNAMED
+ <!-- for URLStreamHandlerFactory access -->
+ --add-opens java.base/java.net=ALL-UNNAMED
Review Comment:
Also seems like the tests using this are related to `ResolverUtil` which is going away in 3.x as we no longer do class path scanning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1392342855
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/Constants.java:
##########
@@ -76,8 +76,10 @@ public final class Constants {
* {@code true} if we think we are running in a web container, based on the boolean value of system property
* "log4j2.is.webapp", or (if this system property is not set) whether the {@code javax.servlet.Servlet} class
* is present in the classpath.
+ * @deprecated use {@link org.apache.logging.log4j.util.Constants#isWebApp()}
*/
- public static final boolean IS_WEB_APP = org.apache.logging.log4j.util.Constants.IS_WEB_APP;
+ @Deprecated
Review Comment:
I think we are allowed to break binary backward compatibility in non-API[1] classes. Otherwise, doesn't this conflict with the rationale of a major release? Maybe this is indeed a gray area and we should discuss further. Though my stance is, as long as it is improving the architecture and not breaking the API[1], go ahead.
[1] For me, API translates to the `log4j-api` and the file-based configuration API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#issuecomment-1811060733
I ran 2.x branch in Java 17 and Java 21 by adjusting the enforcer version range in the pom. Based on the numerous failing and erring tests, I iterated through them by fixing the errors and came to this. I didn't refactor all the static properties because that's something we're working on more so in main, and I only wanted to update the minimum required to get the tests updated.
At this point, just reuse whatever you'd like to incorporate to 2.x; I'll continue working on main.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#issuecomment-1823597273
I'm working on the main branch; this PR is a draft of ideas needed to get Java 17 working for tests. I won't be making more changes to this branch as I don't have the time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1391507145
##########
log4j-core-its/src/test/java/org/apache/logging/log4j/core/async/perftest/ResponseTimeTest.java:
##########
@@ -141,7 +141,7 @@ public static void main(final String[] args) throws Exception {
runLatencyTest(logger, WARMUP_DURATION_MILLIS, WARMUP_COUNT, loadMessagesPerSec, idleStrategy,
warmupServiceTmHistograms, warmupResponseTmHistograms, threadCount);
System.out.println("-----------------Warmup done. load=" + loadMessagesPerSec);
- if (!Constants.ENABLE_DIRECT_ENCODERS || !Constants.ENABLE_THREADLOCALS) {
+ if (!Constants.ENABLE_DIRECT_ENCODERS || !org.apache.logging.log4j.util.Constants.isUseThreadLocals()) {
Review Comment:
I didn't want to refactor all the use of constants in 2.x as that's something related to the context-scoped properties feature introduced in 3.x. However, if we have tests that would otherwise need to, say, modify a static final field, then yes, I'd refactor this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz closed pull request #1964: Work towards supporting Java 17 builds
URL: https://github.com/apache/logging-log4j2/pull/1964
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1390722031
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Constants.java:
##########
@@ -22,24 +22,20 @@
* @since 2.6.2
*/
public final class Constants {
+ private static final LazyBoolean WEB_APP = new LazyBoolean(() -> isWebApp(PropertiesUtil.getProperties()));
Review Comment:
@jvz, mind helping me to understand what these changes have to do with Java 17 support, please?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/Constants.java:
##########
@@ -76,8 +76,10 @@ public final class Constants {
* {@code true} if we think we are running in a web container, based on the boolean value of system property
* "log4j2.is.webapp", or (if this system property is not set) whether the {@code javax.servlet.Servlet} class
* is present in the classpath.
+ * @deprecated use {@link org.apache.logging.log4j.util.Constants#isWebApp()}
*/
- public static final boolean IS_WEB_APP = org.apache.logging.log4j.util.Constants.IS_WEB_APP;
+ @Deprecated
Review Comment:
Why are we adding deprecated fields?
##########
log4j-core-its/src/test/java/org/apache/logging/log4j/core/async/perftest/ResponseTimeTest.java:
##########
@@ -141,7 +141,7 @@ public static void main(final String[] args) throws Exception {
runLatencyTest(logger, WARMUP_DURATION_MILLIS, WARMUP_COUNT, loadMessagesPerSec, idleStrategy,
warmupServiceTmHistograms, warmupResponseTmHistograms, threadCount);
System.out.println("-----------------Warmup done. load=" + loadMessagesPerSec);
- if (!Constants.ENABLE_DIRECT_ENCODERS || !Constants.ENABLE_THREADLOCALS) {
+ if (!Constants.ENABLE_DIRECT_ENCODERS || !org.apache.logging.log4j.util.Constants.isUseThreadLocals()) {
Review Comment:
Why is this `Constants` class exempt to the `LazyBoolean` refactoring?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1392337318
##########
log4j-core-its/src/test/java/org/apache/logging/log4j/core/async/perftest/ResponseTimeTest.java:
##########
@@ -141,7 +141,7 @@ public static void main(final String[] args) throws Exception {
runLatencyTest(logger, WARMUP_DURATION_MILLIS, WARMUP_COUNT, loadMessagesPerSec, idleStrategy,
warmupServiceTmHistograms, warmupResponseTmHistograms, threadCount);
System.out.println("-----------------Warmup done. load=" + loadMessagesPerSec);
- if (!Constants.ENABLE_DIRECT_ENCODERS || !Constants.ENABLE_THREADLOCALS) {
+ if (!Constants.ENABLE_DIRECT_ENCODERS || !org.apache.logging.log4j.util.Constants.isUseThreadLocals()) {
Review Comment:
So we are converting constants that are subject to modification in tests to `Lazy*` instances and leaving out the rest intact?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Work towards supporting Java 17 builds (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1964:
URL: https://github.com/apache/logging-log4j2/pull/1964#discussion_r1390476103
##########
log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/StatusLoggerExtension.java:
##########
@@ -146,10 +148,16 @@ public void handleException(final ExtensionContext context, final Throwable thro
.withThrowable(data.getThrowable())
.withLocation(data.getStackTraceElement())
.log("{} {}",
- DateTimeFormatter.ISO_LOCAL_TIME.format(Instant.ofEpochMilli(data.getTimestamp())),
+ formatLocalTime(data.getTimestamp()),
data.getMessage());
});
}
+
+ private static String formatLocalTime(final long epochMilli) {
+ final Instant instant = Instant.ofEpochMilli(epochMilli);
+ final LocalDateTime localDateTime = LocalDateTime.ofInstant(instant, ZoneId.systemDefault());
+ return localDateTime.format(DateTimeFormatter.ISO_LOCAL_TIME);
+ }
Review Comment:
Oh that makes sense; something needs to be converted to a time zone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org