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 toolchain support for non-java executables #154

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix toolchain support for non-java executables #154

wants to merge 1 commit into from

Conversation

anthonyvdotbe
Copy link
Contributor

#118 introduced toolchain support. However, due to the hard-coded "java", this doesn't work for non-java executables such as jlink (i.e. #148)

rcuprak
rcuprak approved these changes Jun 30, 2022
View reviewed changes
Copy link

@rcuprak rcuprak left a comment

Choose a reason for hiding this comment

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

I tested the fix and it corrects the blocking issue #148

@anthonyvdotbe
Copy link
Contributor Author

@abhinayagarwal @jperedadnr will you please review? (mentioning you since you reviewed the original PR as well)

@alerosmile
Copy link

This should probably be added to other places where executable is still in use. Detecting the JDK home for preparePaths() does not work using toolchains.

@anthonyvdotbe
Copy link
Contributor Author

@alerosmile the sole goal of this PR is to fix the regression introduced in #118 If you believe toolchain support is incomplete or should be improved, please file a new issue and/or PR instead.

OmSarabeSolutions
OmSarabeSolutions approved these changes Sep 28, 2024
View reviewed changes
Copy link

@OmSarabeSolutions OmSarabeSolutions left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yes this change fixes the goal javafx:jlink when using Toolchains.
Tested with java 22, javafx 23.
Can you merge it please ?

@anthonyvdotbe
Copy link
Contributor Author

@jperedadnr @abhinayagarwal @johanvos would you mind merging and releasing a new version to Maven Central please? This is a trivial fix that has been confirmed to work by 2 different people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmSarabeSolutions OmSarabeSolutions OmSarabeSolutions approved these changes

@rcuprak rcuprak rcuprak approved these changes

@jperedadnr jperedadnr Awaiting requested review from jperedadnr

@abhinayagarwal abhinayagarwal Awaiting requested review from abhinayagarwal

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

Successfully merging this pull request may close these issues.
4 participants
@anthonyvdotbe @alerosmile @rcuprak @OmSarabeSolutions