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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

jeffryang24
Copy link

@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
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
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
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
Author

Choose a reason for hiding this comment

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

I see, noted 👍

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
3 participants