Skip to content
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

Fix gradle check with JDK17+ #5735

Merged
merged 1 commit into from
Jan 22, 2025
Merged

Fix gradle check with JDK17+ #5735

merged 1 commit into from
Jan 22, 2025
+10 −3

Conversation

ngocnhan-tran1996
Copy link
Contributor

Related gh-5599

I run ./gradlew :micrometer-osgi-test:check and the result is success on my local machine with OS is Windows 11.
I think we should check ./gradlew clean check before merging (maybe the result will be different)

@ngocnhan-tran1996 ngocnhan-tran1996 marked this pull request as draft December 7, 2024 10:40
@ngocnhan-tran1996 ngocnhan-tran1996 force-pushed the fix-gradle branch 4 times, most recently from fc6f16b to 09d8672 Compare December 7, 2024 12:24
@jonatan-ivanov
Copy link
Member

Cannot we "just" upgrade to 7.x instead of using 6.x and 7.x simultaneously?

@shakuzen
Copy link
Member

7.x requires Java 17, I believe.

@ngocnhan-tran1996
Copy link
Contributor Author

@jonatan-ivanov

Java 11 is incompatiable with version 7.x.
It will raise error when build step run.

If just upgrade version, I think we will wait until we have a plan not support prior java 17

@shakuzen
Copy link
Member

What's remaining to move this PR to ready for review? I ran ./gradlew check on my machine (Apple M1) and it passed.

@ngocnhan-tran1996 ngocnhan-tran1996 marked this pull request as ready for review December 20, 2024 13:37
@shakuzen
Copy link
Member

shakuzen commented Jan 7, 2025

Just to be clear, this will fix it as long as you are running the build with JDK 17 or higher, but the build will still fail with 11. Is that right?

@ngocnhan-tran1996
Copy link
Contributor Author

I have not yet tested on java 11. Could we can check via ci/cd?

@shakuzen
Copy link
Member

shakuzen commented Jan 8, 2025

Could we can check via ci/cd?

The CI build uses ./gradlew build, which is also the default task. It'd be good for us to understand what is different when running ./gradlew check vs ./gradlew build. When running the build task, check also gets run as far as I can tell. I really don't get why it fails when the command is check.

@marcingrzejszczak marcingrzejszczak added the waiting for feedback We need additional information before we can continue label Jan 9, 2025
@ngocnhan-tran1996
Copy link
Contributor Author

I confirm it works on java 17

@shakuzen
Copy link
Member

Yes, it fixes it when using JDK 17+ with the build, but as far as I could tell ./gradlew check is still broken when using JDK 11 with the build.

@github-actions GitHub Actions
Copy link

If you would like us to look at this PR, please provide the requested information. If the information is not provided within the next 7 days this PR will be closed.

@shakuzen shakuzen changed the title Fix gradle command Fix gradle check with JDK17+ Jan 22, 2025
@shakuzen shakuzen added type: task A general task build A change in our build-system and removed waiting for feedback We need additional information before we can continue feedback-reminder labels Jan 22, 2025
@shakuzen shakuzen added this to the 1.15.0-M2 milestone Jan 22, 2025
shakuzen
shakuzen reviewed Jan 22, 2025
View reviewed changes
gradle/libs.versions.toml
@@ -234,6 +234,7 @@ plugin-japicmp = { module = "me.champeau.gradle:japicmp-gradle-plugin", version
plugin-downloadTask = { module = "de.undercouch:gradle-download-task", version = "5.6.0" }
plugin-spotless = { module = "com.diffplug.spotless:spotless-plugin-gradle", version = "6.25.0" }
plugin-bnd = "biz.aQute.bnd:biz.aQute.bnd.gradle:6.4.0"
plugin-bndForJava17 = "biz.aQute.bnd:biz.aQute.bnd.gradle:7.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Dependabot struggles with having multiple entries in the version catalog for the same artifact. We may want to consider inlining the older version where it is used and keeping only the newer version here so it gets updated by dependabot. We can do this separate from this PR as I think we have a number of other multiple entries that need to be handled.

@shakuzen
Copy link
Member

Merging to unblock the check command on JDK17+ (which is what I expect most people building the project are using). The original issue will remain open as it is still a problem on JDK 11.

@shakuzen shakuzen merged commit 826166a into micrometer-metrics:main Jan 22, 2025
8 checks passed
@shakuzen shakuzen mentioned this pull request Jan 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shakuzen shakuzen shakuzen left review comments

Assignees
No one assigned
Labels
build A change in our build-system type: task A general task
Projects
None yet
Milestone
1.15.0-M2
Development

Successfully merging this pull request may close these issues.
4 participants
@ngocnhan-tran1996 @jonatan-ivanov @shakuzen @marcingrzejszczak