Fix Language::Java::java_home_cmd for Linux #5333

sjackman · 2018-11-20T01:58:53Z

/usr/libexec/java_home is specific to macOS.

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?

sjackman · 2018-11-20T02:10:46Z

Linuxbrew/brew has been using this fix since 2015-03-07.

MikeMcQuaid

While this will work on macOS the generic implementations will be broken. Would be good to have a fallback to something that works e.g. using which "java" or the JavaRequirement. Could be a chance to generally consolidate some logic with the JavaRequirement.

MikeMcQuaid · 2018-11-20T09:42:56Z

Library/Homebrew/language/java.rb

-      version_flag = " --version #{version}" if version
-      "/usr/libexec/java_home#{version_flag}"
-    end
+    def self.java_home_cmd(_version = nil); end


I don't think this is a correct implementation. See the uses of this in Homebrew/homebrew-core; they won't work after this. It would be good to use something here that makes sense there.

MikeMcQuaid · 2018-11-20T09:43:14Z

Library/Homebrew/language/java.rb


-    def self.java_home_env(version = nil)
-      { JAVA_HOME: "$(#{java_home_cmd(version)})" }
+    def self.java_home_env(_version = nil)


I don't think this is a correct implementation. See the uses of this in Homebrew/homebrew-core; they won't work after this. It would be good to use something here that makes sense there.

MikeMcQuaid · 2018-11-20T09:43:48Z

Library/Homebrew/language/java.rb

    end

-    def self.overridable_java_home_env(version = nil)
-      { JAVA_HOME: "${JAVA_HOME:-$(#{java_home_cmd(version)})}" }
+    def self.overridable_java_home_env(_version = nil)


I don't think this is a correct implementation. See the uses of this in Homebrew/homebrew-core; they won't work after this. It would be good to use something here that makes sense there.

MikeMcQuaid · 2018-11-26T22:03:35Z

Library/Homebrew/language/java.rb

@@ -2,7 +2,7 @@ module Language
  module Java
    def self.java_home_cmd(version = nil)
      version_flag = " --version #{version}" if version
-      "/usr/libexec/java_home#{version_flag}"
+      "#{HOMEBREW_BREW_FILE} java-home#{version_flag}"


A new documented command seems pretty overkill? Could perhaps do something like raise NotImplementedError on the generic/non-Mac implementation? Is this used in Linuxbrew formulae and needed to work?

write_jar_script uses Language::Java.java_home_cmd, so for that reason, it does need to work. Making it an option of an existing command doesn't seem as clean to me as a new command.

What was the existing Linuxbrew implementation of this? We make write_jar_script use a more generically named function which used java_home_cmd only on macOS.

The existing implementation of java_home_cmd returns nil, and the existing implementations of java_home_env and overridable_java_home_env return {}, which was the first version of this PR that I submitted. When JAVA_HOME is not defined, most tools will use the java interpreter found in PATH, which, when there is only a single Java interpreter installed, is good enough.

One place when I've seen this not work is when building a native JNI library, and the build script expects JAVA_HOME to be defined to find $JAVA_HOME/include.

The existing implementation of java_home_cmd returns nil, and the existing implementations of java_home_env and overridable_java_home_env return {}, which was the first version of this PR that I submitted.

I don't think those versions should be empty no-ops but I think a new command is a bit overkill in the opposite direction. The original implementation with raise NotImplementedError scattered around where appropriate feels like it may be a good call.

sjackman · 2018-11-27T19:09:45Z

The shell script created by write_jar_script looks like this:

#!/bin/bash
JAVA_HOME="$(/usr/libexec/java_home --version 1.8)" exec "/usr/local/Cellar/igv/2.4.14/libexec/igv.sh" "$@"

rather than being hard coded like for example this:

JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_73.jdk/Contents/Home exec "/usr/local/Cellar/igv/2.4.14/libexec/igv.sh" "$@"

The former is better, because the latter would break when the Java interpreter is upgraded.

I feel that providing a script that is functionally equivalent to macOS's /usr/libexec/java_home is the most straight forward approach to achieving this same functionality on Linux.

MikeMcQuaid · 2018-11-27T19:15:34Z

I feel that providing a script that is functionally equivalent to macOS's /usr/libexec/java_home is the most straight forward approach to achieving this same functionality on Linux.

I don't like the idea of having a new Homebrew command that's only relevant to make Linux formulae behave a bit more like Homebrew's. There must be a way that e.g. Debian and Fedora already handle something similar.

sjackman · 2018-11-27T19:20:13Z

This PR implements equivalent functionality to macOS, which I think is the best possible scenario. If your objection is that brew java-home is user-facing and documented, we can find a way to hide this tool so that it's internal to Homebrew.

MikeMcQuaid · 2018-11-27T19:24:04Z

@sjackman My objection is a few-fold:

we're emulating an existing macOS utility for Linuxbrew. Our implementation will not be 1:1 compatible without extensive testing (both manual and automated).
we're making applications dependent at runtime on executing Homebrew itself. If there's issues with Homebrew's Ruby code at that time: these applications will not run.
we're reinventing the wheel for a problem for which Linux package managers already have solutions in order to avoid having to change formulae on Linux
this is new code that's not been used or tested on Linuxbrew already

sjackman · 2018-11-27T20:30:32Z

I've pushed take three of this PR.
Language::Java::java_home_cmd is not implemented on Linux and raises NotImplementedError.
Language::Java::java_home uses java_home_cmd on macOS and JavaRequirement.java_home on Linux.
Language::Java::java_home_env uses Language::Java::java_home.

MikeMcQuaid

I'm liking this approach 👍. Sorry for all the back and forth.

MikeMcQuaid · 2018-11-28T15:55:56Z

Library/Homebrew/language/java.rb

+      raise NotImplementedError
+    end
+
+    def self.java_home(version = nil)


Do you see this being a new method that's called by formulae? If not, could move it somewhere else less public or add a @private at least.

9/14 uses of java_home_cmd look like…

cmd = Language::Java.java_home_cmd("1.8") ENV["JAVA_HOME"] = Utils.popen_read(cmd).chomp

A function that immediately evaluates to java_home (like the Linux implementation) seems useful.

ENV["JAVA_HOME"] = Language::Java.java_home("1.8")

That doesn't however work with the current macOS implementation of java_home, which evaluates to the string "$(/usr/libexec/java_home …)", and not the actual home.

I'll take a stab at making a useful java_home function and hiding the private one that possibly evaluates to $(…) on macOS.

On second thought, it's not worth creating a new function for just 9 uses in Homebrew/core. I'll make java_home private.

I'm game for it being public if it'd be used for that. I think 9 uses is sufficient 👍

I'm game for implementing it, but the current implementation is not sufficient. On macOS,

ENV["JAVA_HOME"] = Language::Java.java_home("1.8")

would evaluate to

ENV["JAVA_HOME"] = "$(/usr/libexec/java_home --version 1.8)"

which is incorrect. It should evaluate to

ENV["JAVA_HOME"] = "/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home"

I'm game for it being public if it'd be used for that. I think 9 uses is sufficient 👍

Language::Java.java_home is now implemented, public, and tested.

MikeMcQuaid · 2018-11-28T15:57:00Z

Library/Homebrew/test/language/java_spec.rb

@@ -1,7 +1,7 @@
 require "language/java"

 describe Language::Java do
-  describe "::java_home_env" do
+  describe "::java_home_env", :needs_macos do


This should work on Linux, no?

The current test which searches for the version string (e.g. "1.8+") in java_home does not work. I can modify the test.

Ah, the test also fails unless java is installed. I'll change it to :needs_java.

Please 👍. Tests for any new public functions would be good too.

Hmm. No, :needs_macos is more correct than :needs_java.

java_home = described_class.java_home_env("blah") expect(java_home[:JAVA_HOME]).to include("--version blah")

On macOS this test is fine, but on Linux it fails unless Java version blah is installed, because on Linux it uses JavaRequirement to check that that particular version of Java is installed.

@sjackman Fine with me as long as we have new tests for each of the generic cases that are implemented.

I've added a test for the new Language::Java.java_home method.

MikeMcQuaid · 2018-11-28T15:57:07Z

Library/Homebrew/test/language/java_spec.rb

@@ -13,7 +13,7 @@
    end
  end

-  describe "::overridable_java_home_env" do
+  describe "::overridable_java_home_env", :needs_macos do


This should work on Linux, no?

sjackman · 2018-11-29T17:55:36Z

@MikeMcQuaid Thanks for the feedback. Good to merge?

MikeMcQuaid · 2018-11-29T19:48:34Z

Library/Homebrew/language/java.rb

-      version_flag = " --version #{version}" if version
-      "/usr/libexec/java_home#{version_flag}"
+    def self.java_home_cmd(_version = nil)
+      raise NotImplementedError


A comment might be useful to explain why this is the case.

MikeMcQuaid · 2018-11-29T19:48:40Z

Library/Homebrew/language/java.rb

-    def self.java_home_cmd(version = nil)
-      version_flag = " --version #{version}" if version
-      "/usr/libexec/java_home#{version_flag}"
+    def self.java_home_cmd(_version = nil)


MikeMcQuaid · 2018-11-29T19:49:55Z

Library/Homebrew/test/spec_helper.rb

@@ -85,6 +85,10 @@
    skip "Not on macOS." unless OS.mac?
  end

+  config.before(:each, :needs_java) do
+    skip "Java not installed." unless which("java")


This will behave oddly on macOS with the java shim but Java not being installed. Might want to use similar logic to the JavaRequirement.

Good catch. Fixed.

/usr/libexec/java_home is specific to macOS. Language::Java::java_home_cmd is not implemented on Linux and raises NotImplementedError. Add private Language::Java::java_home_shell and use it instead of java_home_cmd. Add public Language::Java::java_home for use by formulae.

MikeMcQuaid

One final comment: feel free to address or merge as-is, don't feel strongly. Thanks for bearing with me here and sorry for all the back and forth. We're getting close 😍

MikeMcQuaid · 2018-11-30T15:31:25Z

Library/Homebrew/test/spec_helper.rb

+  config.before(:each, :needs_java) do
+    java_installed = if OS.mac?
+      Utils.popen_read("/usr/libexec/java_home", "--failfast")
+      $CHILD_STATUS.success?


Any reason to not use system here?

I used Utils.popen_read so that it has the standard behaviour of silencing stderr by default and showing it when HOMEBREW_STDERR is enabled.

sjackman · 2018-11-30T16:10:57Z

One final comment: feel free to address or merge as-is, don't feel strongly. Thanks for bearing with me here and sorry for all the back and forth. We're getting close 😍

Thanks for the review, Mike. Merged!

sjackman self-assigned this Nov 20, 2018

sjackman added the bug Reproducible Homebrew/brew bug label Nov 20, 2018

sjackman force-pushed the java-home branch from 9abf2cf to 55b801a Compare November 20, 2018 03:07

sjackman requested a review from MikeMcQuaid November 20, 2018 03:07

sjackman mentioned this pull request Nov 20, 2018

cannoli 0.3.0 brewsci/homebrew-bio#522

Closed

MikeMcQuaid requested changes Nov 20, 2018

View reviewed changes

sjackman force-pushed the java-home branch 2 times, most recently from a441821 to 0a36c3e Compare November 26, 2018 19:41

sjackman changed the title ~~Do not use /usr/libexec/java_home on Linux~~ Fix Language::Java::java_home_cmd for Linux Nov 26, 2018

MikeMcQuaid reviewed Nov 26, 2018

View reviewed changes

sjackman force-pushed the java-home branch 2 times, most recently from 623cbb0 to fa6c85d Compare November 26, 2018 23:06

sjackman force-pushed the java-home branch 2 times, most recently from cff9ee4 to 4467f4d Compare November 27, 2018 20:28

MikeMcQuaid reviewed Nov 28, 2018

View reviewed changes

sjackman force-pushed the java-home branch 3 times, most recently from d241cea to a7a68f0 Compare November 29, 2018 17:33

MikeMcQuaid reviewed Nov 29, 2018

View reviewed changes

sjackman force-pushed the java-home branch from a7a68f0 to e8b2269 Compare November 29, 2018 20:50

MikeMcQuaid approved these changes Nov 30, 2018

View reviewed changes

sjackman merged commit c79deae into Homebrew:master Nov 30, 2018

sjackman deleted the java-home branch November 30, 2018 16:10

sjackman mentioned this pull request Dec 18, 2018

Fixes needed for java_requirement Linuxbrew/brew#509

Closed

lock bot added the outdated PR was locked due to age label Dec 30, 2018

lock bot locked as resolved and limited conversation to collaborators Dec 30, 2018

Fix Language::Java::java_home_cmd for Linux #5333

Fix Language::Java::java_home_cmd for Linux #5333

Conversation

sjackman commented Nov 20, 2018 • edited

sjackman commented Nov 20, 2018

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman commented Nov 27, 2018

MikeMcQuaid commented Nov 27, 2018

sjackman commented Nov 27, 2018

MikeMcQuaid commented Nov 27, 2018

sjackman commented Nov 27, 2018 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman Nov 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman Nov 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman Nov 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman commented Nov 30, 2018

sjackman commented Nov 20, 2018 •

edited

sjackman commented Nov 27, 2018 •

edited

sjackman Nov 28, 2018 •

edited

sjackman Nov 28, 2018 •

edited

sjackman Nov 28, 2018 •

edited