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

Searching JDK version by first token with dash is error-prone #87

Open
Vampire opened this issue Nov 17, 2023 · 3 comments · May be fixed by #88
Open

Searching JDK version by first token with dash is error-prone #87

Vampire opened this issue Nov 17, 2023 · 3 comments · May be fixed by #88

Comments

@Vampire
Copy link
Contributor

Vampire commented Nov 17, 2023

If you for example use the mxschmitt/action-tmate@v3 action to get a login onto a GitHub Action runner,
and call /usr/libexec/java_home -V you will see that the paths of the JDKs are like /Users/runner/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.9-9/x64/Contents/Home.

If in such or similar situation the findJDKDylib algorithm is followed,
it misses to find jdk1., then cuts as the first - and then again at the first /.
This results in the "version" Hotspot_jdk which then of course fails the major version check.

The algorithm should be improved to check for further occurrences, or validate that digits are following or similar.

@Vampire
Copy link
Contributor Author

Vampire commented Nov 17, 2023

Actually even checking all dashes would not help in this case, as for the shown example 9 would then be determined as version.
It should probably actually call the executable to retrieve its version like for the JRE search.

@rschmunk
Copy link
Contributor

I've thought for a while that actually asking the executable what its version is would be the best route.

@Vampire
Copy link
Contributor Author

Vampire commented Nov 19, 2023

Expect a PR soon, I made that and some other improvements locally already. :-)

Vampire added a commit to Vampire/appbundler that referenced this issue Nov 20, 2023
@Vampire Vampire linked a pull request Nov 20, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants