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

extend/pathname: use absolute path to java in write_jar_script #8163

Merged
merged 1 commit into from Aug 4, 2020

Conversation

alebcay
Copy link
Member

@alebcay alebcay commented Aug 1, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This fixes #8154. The solution is not as simple as replacing java with ${JAVA_HOME}/bin/java because the brace expansion of ${JAVA_HOME} will occur before JAVA_HOME is assigned a value. Thus, we have to explicitly export JAVA_HOME, or we could try something like JAVA_HOME=...; exec "${JAVA_HOME}/bin/java" .... In any case, my point is that piggybacking off of write_env_script as it currently exists doesn't seem to be possible.

To be clear, this is really only an issue for Linux users. On macOS, /usr/bin/java is a stub that will execute the Java that JAVA_HOME points to, and /usr/bin/java should always be on PATH.

As always, I'm open to suggestions, especially on style, especially on that export JAVA_HOME=... Ruby glob. It's not looking too pretty.

@MikeMcQuaid
Copy link
Member

The only issue I can see with this is that a java in the PATH will not be respected. Is this a problem? I'm not sure it is either way but a PATHy solution would be to consider adding this java to the end of the PATH in this script so we know there's at least one. Thoughts?

@alebcay
Copy link
Member Author

alebcay commented Aug 3, 2020

I believe on macOS, the wrapper/stub will always prefer the Java at JAVA_HOME, if defined, over any system Java that is installed/selected in /Library. I think you're right for Linux, though. I don't know if it's expected that specifying a JAVA_HOME would override a Java on PATH or if PATH should take precedence.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Can always be tweaked further based on user feedback 👍🏻

@MikeMcQuaid MikeMcQuaid merged commit bbd32a6 into Homebrew:master Aug 4, 2020
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 20, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

write_jar_script does not work properly if using Java not on PATH
3 participants