Skip to content

Switch the build to JDK 11 #1369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Aug 4, 2023
Merged

Switch the build to JDK 11 #1369

merged 5 commits into from
Aug 4, 2023

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Mar 15, 2023 •
edited
Loading

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 under the java8-tests profile,
  • 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.

@ppkarwasz ppkarwasz force-pushed the java17 branch 2 times, most recently from 3f70792 to 57fc110 Compare March 16, 2023 05:43
vy
vy approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job Piotr! I definitely support this change!

I am still not happy with all the use-Java-8-to-do-X infection all over the place. If we can move JMX et al. to its own repository in a follow-up story, can we remove the 8 dependency all together and only employ it only in CI while testing?

@ppkarwasz
Copy link
Contributor Author

@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).

@vy
Copy link
Member

vy commented Mar 16, 2023

@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
  2. 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.

@ppkarwasz
Copy link
Contributor Author

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.

@ppkarwasz ppkarwasz force-pushed the java17 branch 2 times, most recently from dd4874a to b7189ae Compare July 25, 2023 20:06
@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Jul 26, 2023 •
edited
Loading

@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 replaced the maven-bundle-plugin with maven-jar-plugin in log4j-jmx-gui. This artifact does not need to be an OSGI bundle.

@ppkarwasz ppkarwasz marked this pull request as draft July 31, 2023 04:44
@ppkarwasz ppkarwasz mentioned this pull request Jul 31, 2023
Closed
@ppkarwasz ppkarwasz added this to the 2.20.1 milestone Aug 2, 2023
@ppkarwasz ppkarwasz marked this pull request as ready for review August 2, 2023 21:53
@ppkarwasz ppkarwasz changed the title Switch the build to JDK 17 Switch the build to JDK 11 Aug 3, 2023
@ppkarwasz
Copy link
Contributor Author

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.

@ppkarwasz ppkarwasz merged commit 1fdd808 into apache:2.x Aug 4, 2023
@ppkarwasz ppkarwasz deleted the java17 branch August 4, 2023 07:35
@LuciferYang LuciferYang mentioned this pull request Oct 22, 2023
Closed
LuciferYang added a commit to apache/spark that referenced this pull request Oct 23, 2023
@LuciferYang
[SPARK-45625][BUILD] Upgrade log4j to 2.21.0
dbf2f49 
### What changes were proposed in this pull request?
This pr aims upgrade log4j from 2.20.0 to 2.21.0.

### Why are the changes needed?
Support for the zstd compression algorithm has been added in the new version: apache/logging-log4j2#1508 | apache/logging-log4j2#1514
Meanwhile, the new version starts to use Java 11 for building, and the runtime version is still compatible with Java 8: apache/logging-log4j2#1369
The new version also brings some bug fixes, such as:
- Fixed logging of java.sql.Date objects by appending it before Log4J tries to call java.util.Date.toInstant() on it: apache/logging-log4j2#1366
- Fixed concurrent date-time formatting issue in PatternLayout: apache/logging-log4j2#1485
- Fixed buffer size in Log4jFixedFormatter date time formatter: apache/logging-log4j2#1418
- Fixed the propagation of synchronous action failures in RollingFileManager and FileRenameAction: apache/logging-log4j2#1445 | apache/logging-log4j2#1549
- Fixed RollingFileManager to propagate failed synchronous actions correctly: apache/logging-log4j2#1445
and more.

The complete release note is as follows:
- https://github.com/apache/logging-log4j2/releases/tag/rel%2F2.21.0

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GitHub Actions

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #43478 from LuciferYang/SPARK-45625.

Authored-by: yangjie01 <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vy vy vy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.21.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ppkarwasz @vy