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.

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

Fix: Support JVM version string from old (< v8.9) and new (>= v8.9) gradle version #683

Conversation

jeffryang24
Copy link
Contributor

@jeffryang24 jeffryang24 commented Sep 5, 2024

Issue #, if available: This PR should resolve #682

Description of changes:

  • Check both JVM and Launcher JVM prefix for the gradle[w] -version command output. Gradle < v8.9 was using JVM: x.x.x, while Gradle >= v8.9 has changed it to Launcher JVM: x.x.x.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jeffryang24 jeffryang24 requested a review from a team as a code owner September 5, 2024 15:14
@github-actions github-actions bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. area/workflow/java_gradle and removed pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Sep 5, 2024
@jeffryang24 jeffryang24 changed the title Fix: Support version string from old and new gradle version Fix: Support JVM version string from old (< v8.9) and new (>= v8.9) gradle version Sep 5, 2024
@@ -83,5 +83,5 @@ def _get_jvm_string(self, gradle_path):

for line in stdout.splitlines():
l_dec = decode(line)
if l_dec.startswith("JVM"):
if l_dec.startswith("JVM") or l_dec.startswith("Launcher JVM"):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This could be simplified to l_dec.contains("JVM")

Copy link
Contributor Author

@jeffryang24 jeffryang24 Sep 12, 2024

Choose a reason for hiding this comment

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

I see, I will change it with .contains(...) alike method, probably use x in y style. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with in operator to check the JVM substring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @hnnasit , is there something that I need to do so this PR can be merged? Thank you 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jeffryang24, our CI checks are failing consistently (and randomly out of the blue). The failures are unrelated to your changes, but we're working on fixing them. At the moment, there is no action needed from your side. Thanks for waiting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, noted 👍

@mndeveci mndeveci added this pull request to the merge queue Nov 15, 2024
Merged via the queue into aws:develop with commit 2a2d6b7 Nov 15, 2024
131 checks passed
@jeffryang24 jeffryang24 deleted the fix/support-old-and-new-gradle-jvm-version-label branch November 25, 2024 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: sam build can't check the JVM version on gradle >= v8.9
4 participants