You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2023/03/15 22:53:02 UTC
[PR] Switch the build to JDK 17 (logging-log4j2)
ppkarwasz opened a new pull request, #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369
This switches the build to use JDK 17 with `--release 8` (or whatever is appropriate for the module).
JDK 8 is still required for:
* running tests (it is regulated by the `surefire.jdkToolchain` property),
* providing `jconsole.jar` for `log4j-jmx-gui`.
To compile `log4j-jmx-gui` running JDK 17 a `JAVA_HOME_8_X64` environment variable must be provided and contain the location of the JDK 8 installation
--
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] Switch the build to JDK 17 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369#discussion_r1138361001
##########
log4j-api-java9/pom.xml:
##########
@@ -27,10 +27,12 @@
<name>Apache Log4j API Java 9 support</name>
<description>The Apache Log4j API (Java 9)</description>
<properties>
- <log4jParentDir>${basedir}/..</log4jParentDir>
<docLabel>API Documentation</docLabel>
- <projectDir>/api</projectDir>
+ <log4jParentDir>${basedir}/..</log4jParentDir>
Review Comment:
Yes, we should, but this is an unrelated change that needs to be applied in each POM file.
I unintentionally reordered the 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] Switch the build to JDK 17 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369#issuecomment-1471730969
@vy,
I updated the PR so that you can run the build with whatever JDK newer or equal to 11, as long as:
* you set the `JAVA_HOME_8_X64` environment variable for JConsole,
* you have a valid toolchains configuration with at least a JRE 11+ (for `log4j-jpl`).
The first condition will drop when we get rid of `log4j-jmx-gui`, the second one when Maven will generate an implicit JDK toolchain configuration (I have a PR somewhere).
--
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] Switch the build to JDK 17 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369#issuecomment-1471859030
@ppkarwasz, thanks so much for the quick response! I have some questions/remarks:
### Toolchains
It feels odd to me to require both toolchains (which is the Maven-native way of providing multiple JDKs) and an environment variable for JDK 8. I understand that you intend to move `log4j-jmx-gui` to its own repository and then we won't need the `JAVA_HOME_8_X64` environment variable anymore. Though the need for toolchains will still be there. I think, if possible, we should pick one of the two plans:
1. Embrace 17 & 8 in toolchains (no `JAVA_HOME_8_X64` environment variable!) in this PR, later on remove `log4j-jmx-gui`, and consequently remove the need for toolchains and totally switch to 17
1. Remove `log4j-jmx-gui` (this can be done in this PR!) and only rely on 17 (no toolchains!)
I am in favor of the 2nd approach here. But maybe I am missing something obvious.
### `>=11` vs `17`
I also would prefer a fixed JDK version (that is, as the name of this PR suggest, 17) rather than leaving it open. This will help us to align the build and development experience everywhere; in CI, in personal development machines, etc. The way it feels now is like a ticking bomb for _"build doesn't work on my machine while using Java X"_ posts.
--
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] Switch the build to JDK 11 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz merged PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369
--
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] Switch the build to JDK 11 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369#issuecomment-1664613818
In the end JDK 17 was too ambitious: many tests still fail there.
In its final form this PR:
- switches the build to JDK 9+ without requiring toolchains or anything else. On JDK 11 all tests pass, on JDK 17 there are some test failures (mainly due to the indiscriminate use of reflection).
- the `java8-tests` profile (used by the CI) requires a `toolchains.xml` configuration with a JDK 8 toolchain.
--
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] Switch the build to JDK 17 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369#discussion_r1138236598
##########
.github/workflows/build.yml:
##########
@@ -47,27 +47,38 @@ jobs:
with:
fetch-depth: 32
- # JDK 8 and 11 is needed for the build.
+ # JDK 17 is needed for the build.
+ # JDK 8 is used for the tests.
# Search `maven-toolchains-plugin` usages for details.
- name: Setup JDK
uses: actions/setup-java@3f07048e3d294f56e9b90ac5ea2c6f74e9ad0f98 # 3.10.0
with:
distribution: temurin
java-version: |
Review Comment:
```suggestion
# The last provided Java version will be used and available globally.
# Other Java versions can be accessed through environment variables: `JAVA_HOME_<majorVersion>_<architecture>`
java-version: |
```
##########
.github/workflows/build.yml:
##########
@@ -136,15 +143,16 @@ jobs:
with:
fetch-depth: 32
- # JDK 8 and 11 are needed for the build.
+ # JDK 17 is needed for the build.
+ # JDK 8 is just needed for JConsole.
# Search `maven-toolchains-plugin` usages for details.
- name: Setup JDK
uses: actions/setup-java@3f07048e3d294f56e9b90ac5ea2c6f74e9ad0f98 # 3.10.0
with:
distribution: temurin
java-version: |
Review Comment:
```suggestion
# The last provided Java version will be used and available globally.
# Other Java versions can be accessed through environment variables: `JAVA_HOME_<majorVersion>_<architecture>`
java-version: |
```
##########
log4j-jmx-gui/pom.xml:
##########
@@ -65,20 +65,43 @@
</instructions>
</configuration>
</plugin>
+
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>enforce-jconsole-jar</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <requireFilesExist>
Review Comment:
Can't we simplify this block by replacing it with a `requireEnvironmentVariable` rule on `JAVA_HOME_8`? This would also remove the need for `Find real JDK 8 path` CI steps.
##########
.github/workflows/build.yml:
##########
@@ -136,15 +143,16 @@ jobs:
with:
fetch-depth: 32
- # JDK 8 and 11 are needed for the build.
+ # JDK 17 is needed for the build.
+ # JDK 8 is just needed for JConsole.
Review Comment:
I am confused. Earlier 8 was needed for tests (employed via `maven-surefire-plugin` configuration), now it is needed for JConsole. Hasn't earlier JConsole also (I mean, next to tests) required it?
##########
log4j-api-java9/pom.xml:
##########
@@ -27,10 +27,12 @@
<name>Apache Log4j API Java 9 support</name>
<description>The Apache Log4j API (Java 9)</description>
<properties>
- <log4jParentDir>${basedir}/..</log4jParentDir>
<docLabel>API Documentation</docLabel>
- <projectDir>/api</projectDir>
+ <log4jParentDir>${basedir}/..</log4jParentDir>
Review Comment:
Can't we simply replace all these with `${maven.multiModuleProjectDirectory}`?
--
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] Switch the build to JDK 17 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369#issuecomment-1650937772
@vy,
I refactored this PR by replacing the dependency on `jconsole.jar` with two dummy classes, that are excluded from packaging afterwards. The new requirements for the build are:
* the build should run on JDK 11 or later without a toolchains configuration or an environment variable; it skips a couple of JRE 8 specific tests,
* there is a profile `java8-tests` that runs tests using a JRE 8 toolchain. This profile is used by the CI and requires two toolchains configurations: a JRE 8 and JRE 11 or higher (for `log4j-api-java9`, `log4j-core-java9` and `log4j-jpl`).
For simplicity I also replace the `maven-bundle-plugin` with `maven-jar-plugin` in `log4j-jmx-gui`. This artifact does not need to be an OSGI bundle.
--
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] Switch the build to JDK 17 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369#issuecomment-1472000204
The need for toolchains comes from a couple of JDK 8-specific tests in `log4j-api` and `log4j-core`: `StackLocator` and a couple of other classes has an implementation that only works on JDK 8 (or JDK <11).
If you run those tests on JDK 11, they might succeed, but they will not be testing the code they are supposed to test.
I don't see a problem in running the build with JDK 11 (unless it is for a release): if there are differences in building experience between JDK 11 and JDK 17, we should know about it.
--
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] Switch the build to JDK 17 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1369:
URL: https://github.com/apache/logging-log4j2/pull/1369#discussion_r1138462470
##########
.github/workflows/build.yml:
##########
@@ -136,15 +143,16 @@ jobs:
with:
fetch-depth: 32
- # JDK 8 and 11 are needed for the build.
+ # JDK 17 is needed for the build.
+ # JDK 8 is just needed for JConsole.
Review Comment:
`deploy` does not run tests, so we don't need JDK 8 as soon as `log4j-jmx-gui` is out of the way.
--
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