Implement X11Requirement for Linux #3483

sjackman · 2017-11-26T17:01:23Z

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 tests with your changes locally?

Move the XQuartz implementation to extend/os/mac/requirements.

ilovezfs · 2017-11-26T17:02:56Z

Library/Homebrew/requirements/x11_requirement.rb

@@ -5,8 +5,7 @@ class X11Requirement < Requirement
  attr_reader :min_version

  fatal true
-  cask "xquartz"
-  download "https://xquartz.macosforge.org"
+  default_formula "linuxbrew/xorg/xorg"


having a default formula in a non-core tap seems like asking for trouble

Yeh, I'd rather avoid linuxbrew tap usage for now until we're closer to merging the projects.

Okay. How about default_formula "linuxbrew/xorg/xorg" in extend/os/linux/requirements/x11_requirement.rb?

How does that change anything? It's still a non-core (i.e. not Linuxbrew/homebrew-core) tap.

MikeMcQuaid · 2017-11-26T18:15:14Z

Library/Homebrew/extend/os/mac/requirements/x11_requirement.rb

@@ -0,0 +1,49 @@
+require "requirement"
+
+Object.send(:remove_const, :X11Requirement)


This is a bit weird.

This line removes the class X11Requirement so that extend/os/mac/requirements/x11_requirement.rb can redefine it. Creating a new class with a new name like XQuartzRequirement and then assigning it with X11Requirement = XQuartzRequirement causes a test to fail because be_an_instance_of(X11Requirement) is false. We could make X11Requirement be a base class, and have XQuartzRequirement inherit from it. Alternatively, we could remove this test. Your thoughts?

I replaced it with X11Requirement = XQuartzRequirement.

MikeMcQuaid · 2017-11-26T18:15:24Z

Library/Homebrew/extend/os/mac/requirements/x11_requirement.rb

+  def initialize(name = "x11", tags = [])
+    @name = name
+    if /(\d\.)+\d/ =~ tags.first
+      @min_version = Version.create(tags.shift)


Do XQuartz versions map to X11/Xorg versions at all? Would be good to share the version logic if possible.

The About window in my xquartz says: "XQuartz 2.7.11 (xorg-server 1.18.4)"

I haven't yet found a need in Linuxbrew for a version check in the X11Requirement. This code is here primarily so that this Requirement exposes the same version API as the XQuartz Requirement, so that the tests pass. I can instead remove this version logic and make that particular test :needs_macos.

The point is when it's satisfied by something other than the default formula (which will be forced to be up to date regardless), which I'm assuming remains untested.

Before the xorg formula existed, I used Linuxbrew with the host's X11 implementation. When installing a bottle that depends on :x11, the default_formula is used. The host's Xorg may be used when installing something from source that depends on :x11.

Headless Linux servers don't usually have the X server installed (/usr/bin/X), just the X client libraries and executables. The X server is run on your local machine (which is not necessarily running Linuxbrew), whereas the X client is run on the headless server (which is running Linuxbrew). For this reason, I'm checking the version of a X client executable (/usr/bin/xdpyinfo) rather than the X server. The X client libraries are needed to build software that depends on X11, but the X server is not needed at build time.

The most recent complete release of Xorg X11R7.7 shipped with xdpyinfo 1.3.0.
The previous release of Xorg X11R7.6 shipped with xdpyinfo 1.2.0.
See https://www.x.org/releases/X11R7.7/changelog.html

That's the xdpyinfo version rather than the X11 version, though; it's not particularly useful in this case. Can you see if there's anything else that provides the actual X11 version even with the X server not installed? If not, I think this should just assume the server is not headless and the Linux requirement can always override that.

The katamari 7.7 had server version 1.12.2 and xdpyinfo 1.3.0. So I think starting with Xorg -version >= 1.12.2 makes sense, and then if the Xorg command isn't available, fall back on xdpyinfo -version > = 1.3.0.

Linux:

openzfs ~ # Xorg -version 2>&1|grep Server X.Org X Server 1.19.3 openzfs ~ # xdpyinfo -version xdpyinfo 1.3.2

macOS with XQuartz 2.7.11:

iMac-TMP:~ joe$ Xorg -version 2>&1|grep Server X.Org X Server 1.18.4 iMac-TMP:~ joe$ xdpyinfo -version xdpyinfo 1.3.2

@sjackman are you planning on actually using the satisfy block from this upstreamed code or just continuing to use

satisfy build_env: false do Formula["linuxbrew/xorg/xorg"].installed? end

in Linuxbrew itself?

Yes, once it's merged into Homebrew/brew, and then after that, merged into Linuxbrew/brew.

MikeMcQuaid · 2017-11-27T08:22:35Z

Library/Homebrew/extend/os/linux/requirements/x11_requirement.rb

@@ -0,0 +1,3 @@
+class X11Requirement
+  default_formula "linuxbrew/xorg/xorg"


I'd rather avoid any linuxbrew tap references until we are further along with the migration to merge Linuxbrew/brew and Homebrew/brew.

I've removed extend/os/linux/requirements/x11_requirement.rb

MikeMcQuaid · 2017-12-01T12:25:16Z

@sjackman My apologies but I misunderstood my conversation previously with @ilovezfs. Instead of needing a version tag that can be passed through let's just make the requirement enforce a minimum X -version and, if not available, xdpyinfo -version within the requirement to a value you're happy with. I'd suggest after that (doesn't need to be part of this PR) we remove the existing version that's passed through as part of the requirement to make it a no-op and then remove it from formulae. Sorry about all the back and forth 😭

ilovezfs · 2017-12-01T12:33:35Z

minor thing: I don't think X command prints a version for XQuartz but Xorg command does.

iMac-TMP:~ joe$ X -version
iMac-TMP:~ joe$ Xorg -version

X.Org X Server 1.18.4
Release Date: 2016-07-19
X Protocol Version 11, Revision 0
Build Operating System: Darwin 14.0 x86_64 
Current Operating System: Darwin iMac-TMP.local 15.6.0 Darwin Kernel Version 15.6.0: Sun Jun  4 21:43:07 PDT 2017; root:xnu-3248.70.3~1/RELEASE_X86_64 x86_64
Build Date: 25 October 2016  10:12:52PM
 
Current version of pixman: 0.34.0
	Before reporting problems, check http://wiki.x.org
	to make sure that you have the latest version.
iMac-TMP:~ joe$

MikeMcQuaid · 2017-12-02T10:27:15Z

Library/Homebrew/test/x11_requirement_spec.rb

@@ -20,7 +20,7 @@
      expect(subject).not_to eql(other)
    end

-    it "returns false if the minimum version differs" do
+    it "returns false if the minimum version differs", :needs_macos do


Can you instead modify this test so it does something on Linux?

I've added a test for Linux.

MikeMcQuaid · 2017-12-03T09:28:07Z

Library/Homebrew/test/spec_helper.rb

@@ -61,6 +61,10 @@
    skip "Needs official command Taps." unless ENV["HOMEBREW_TEST_OFFICIAL_CMD_TAPS"]
  end

+  config.before(:each, :needs_linux) do


I'd rather we didn't have these as they show up as pending tests on macOS (I realise the inverse is the case on Linux). Here's a solution for pending tests (that I think we should do for Linux and, I guess, macOS): rspec/rspec-core#2377 (comment) but in this particular case I think an if/else within the test itself is better as they are testing essentially the same functionality. CC @reitermarkus for RSpec thoughts.

I've tweaked the macOS underlying code so this may no longer be needed.

I've tweaked the macOS underlying code so this may no longer be needed.

Meaning the PR is good as is, or you'd prefer the if/else instead?

Ah, I think you're referring to 8b9ac2b x11_requirement: remove custom minimum version. I'll rebase onto master.

sjackman · 2017-12-04T00:35:34Z

I've rebased this PR onto master.

sjackman · 2017-12-04T00:54:07Z

Thanks for the reorganization of the macOS X11Requirement, Mike.

MikeMcQuaid · 2017-12-04T08:42:22Z

Library/Homebrew/extend/os/mac/requirements/x11_requirement.rb

+
+  def initialize(name = "x11", tags = [])
+    @name = name
+    # no-op on version specified as a tag argument


Let's do the same thing on the generic X11 requirement; they should have the same interface. I think the only thing they should probably differ on is the satisfy and min_version stuff (and would be good to have a min_version method there too).

These two initialize functions are identical. What do you mean here exactly?

Generic: https://github.com/sjackman/homebrew/blob/9f7278899e24fa3567db7e15050a63e8f3242679/Library/Homebrew/requirements/x11_requirement.rb#L8

macOS: https://github.com/sjackman/homebrew/blob/9f7278899e24fa3567db7e15050a63e8f3242679/Library/Homebrew/extend/os/mac/requirements/x11_requirement.rb#L12

Apologies, I misread this.

MikeMcQuaid · 2017-12-04T08:43:48Z

Library/Homebrew/requirements/x11_requirement.rb

@@ -17,32 +13,26 @@ def initialize(name = "x11", tags = [])
  end

  def min_version


Maybe a min_xdpyinfo_version method too would be good.

MikeMcQuaid · 2017-12-04T08:44:08Z

Library/Homebrew/requirements/x11_requirement.rb

-    return Version::NULL unless OS.mac?
-
-    MacOS::XQuartz.minimum_version
+    Version.new "1.12.2"


Don't think this actually needs a Version.new given you're always comparing them to Versions below.

MikeMcQuaid · 2017-12-04T08:44:50Z

Library/Homebrew/requirements/x11_requirement.rb

-    s
-  end
-
-  def <=>(other)


Want to keep this method in some form for API compatability

MikeMcQuaid · 2017-12-04T08:45:02Z

Library/Homebrew/requirements/x11_requirement.rb

-    min_version <= MacOS::XQuartz.version
-  end
-
-  def message


Want to keep this method in some form.

sjackman · 2017-12-04T18:04:48Z

Fixed up.

MikeMcQuaid · 2017-12-06T11:52:28Z

Library/Homebrew/extend/os/mac/requirements/x11_requirement.rb

+
+  def initialize(name = "x11", tags = [])
+    @name = name
+    # no-op on version specified as a tag argument


Apologies, I misread this.

MikeMcQuaid · 2017-12-06T11:53:23Z

Library/Homebrew/requirements/x11_requirement.rb

-    s = "XQuartz #{min_version} (or newer) is required to install this formula."
-    s += super
-    s
+    "X11 is required to install this formula.\n"


Insert #{min_version} in here somewhere and may as well += super, too.

MikeMcQuaid · 2017-12-06T11:53:35Z

Library/Homebrew/requirements/x11_requirement.rb

-
-  def inspect
-    "#<#{self.class.name}: #{name.inspect} #{tags.inspect}>"
-  end


Let's keep this method as-is.

ilovezfs · 2017-12-06T12:23:45Z

Library/Homebrew/requirements/x11_requirement.rb

-    next false unless MacOS::XQuartz.installed?
-    min_version <= MacOS::XQuartz.version
+    if which "Xorg"
+      version = Utils.popen_read "Xorg", "-version", err: :out


@sjackman I think this needs to pass the return value of which "Xorg" to Utils.popen_read, otherwise when env filtering is enabled, it will end up with command not found: Xorg. Ditto xdpyinfo below.

I believe that both which and Utils.popen_read will experience the same PATH. Either they'll both succeed, or they'll both fail. Do you believe that one of the two experiences env filtering, while the other does not? When Xorg and xdpyinfo are in the standard system path, both which and Utils.popen_read should work with env filtering enabled.

I believe that both which and Utils.popen_read will experience the same PATH.

They will not. My comment was not speculative but based on testing. which is Homebrew's custom which, which brings back in the ORIGINAL_PATH.

Other requirements use Homebrew's which for this reason. There's no guarantee things used to satisfy requirements are in the env filtered PATH.

Please point me to the relevant code.
which uses PATH
https://github.com/Homebrew/brew/blob/master/Library/Homebrew/utils.rb#L309
popen_read uses PATH
https://github.com/Homebrew/brew/blob/master/Library/Homebrew/utils/popen.rb#L18
What you're saying may well be true, but it's not evident from the code.

Requirement#which and Requirment#which_all

Fixed. Thanks for the feedback, Joe.

ilovezfs · 2017-12-06T12:44:19Z

@sjackman once this is merged, non-formulae (i.e. x11 other than linuxbrew/xorg/xorg) will be allowed to satisfy the x11 requirement. But as far as I can tell, Linuxbrew doesn't have an ENV.x11 implementation. So won't non-formulae satisfiers cause a problem until ENV.x11 is actually implemented?

It's not relevant to whether this PR gets merged in Homebrew/brew now as-is, but it does seem like in its current form it can't be merged into Linuxbrew/brew.

sjackman · 2017-12-06T19:13:53Z

With stdenv, the default no-op dev x11; end is sufficient. Linuxbrew/brew has an implementation of ENV.x11 for superenv, but superenv hasn't yet been contributed back to Homebrew/brew. It's coming.

Move the XQuartz implementation to extend/os/mac/requirements.

MikeMcQuaid · 2017-12-08T15:47:40Z

Thanks again @sjackman!

sjackman · 2017-12-08T15:50:55Z

Thanks again for merging, Mike! Thanks for the feedback, @ilovezfs!

MikeMcQuaid · 2017-12-08T18:03:57Z

$ brew tests --only=x11_requirement 
Using recorded test runtime
1 processes for 1 specs, ~ 1 specs per process
/usr/local/Homebrew/Library/Homebrew/extend/os/mac/requirements/x11_requirement.rb:42: warning: already initialized constant X11Requirement
/usr/local/Homebrew/Library/Homebrew/requirements/x11_requirement.rb:3: warning: previous definition of X11Requirement was here

@sjackman can you address this (today if possible of I may need to revert)? Check out how JavaRequirement avoids this.

sjackman · 2017-12-08T18:07:52Z

Yes, I can address it now.

sjackman · 2017-12-08T18:46:13Z

Fixed.

ilovezfs reviewed Nov 26, 2017

View reviewed changes

MikeMcQuaid requested changes Nov 26, 2017

View reviewed changes

sjackman force-pushed the x11 branch 3 times, most recently from 9fef2d4 to 0a21425 Compare November 27, 2017 06:22

MikeMcQuaid reviewed Nov 27, 2017

View reviewed changes

sjackman force-pushed the x11 branch 2 times, most recently from 11aecb5 to 615fd54 Compare November 27, 2017 20:11

sjackman force-pushed the x11 branch from 615fd54 to e14ac14 Compare December 1, 2017 22:59

MikeMcQuaid reviewed Dec 2, 2017

View reviewed changes

sjackman force-pushed the x11 branch 3 times, most recently from 8c58355 to 7e49e62 Compare December 3, 2017 00:07

MikeMcQuaid reviewed Dec 3, 2017

View reviewed changes

sjackman force-pushed the x11 branch 3 times, most recently from 1949bad to 189f322 Compare December 4, 2017 00:35

sjackman force-pushed the x11 branch from 189f322 to dae92d5 Compare December 4, 2017 00:40

sjackman force-pushed the x11 branch from dae92d5 to 9f72788 Compare December 4, 2017 07:12

MikeMcQuaid requested changes Dec 4, 2017

View reviewed changes

sjackman force-pushed the x11 branch from 9f72788 to 266f6fc Compare December 4, 2017 18:00

MikeMcQuaid requested changes Dec 6, 2017

View reviewed changes

ilovezfs reviewed Dec 6, 2017

View reviewed changes

sjackman force-pushed the x11 branch 2 times, most recently from 8e42095 to ce27a33 Compare December 7, 2017 16:27

Implement X11Requirement for Linux

804393e

Move the XQuartz implementation to extend/os/mac/requirements.

sjackman force-pushed the x11 branch from ce27a33 to 804393e Compare December 7, 2017 16:45

MikeMcQuaid approved these changes Dec 8, 2017

View reviewed changes

MikeMcQuaid merged commit 5055c31 into Homebrew:master Dec 8, 2017

sjackman deleted the x11 branch December 8, 2017 15:50

sjackman mentioned this pull request Dec 8, 2017

X11Requirement: Inherit from generic class [macOS] #3548

Merged

5 tasks

Homebrew locked and limited conversation to collaborators May 4, 2018

		@@ -0,0 +1,49 @@
		require "requirement"

		Object.send(:remove_const, :X11Requirement)

		@@ -0,0 +1,3 @@
		class X11Requirement
		default_formula "linuxbrew/xorg/xorg"

		@@ -17,32 +13,26 @@ def initialize(name = "x11", tags = [])
		end

		def min_version

Implement X11Requirement for Linux #3483

Implement X11Requirement for Linux #3483

Conversation

sjackman commented Nov 26, 2017 • edited Loading

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 27, 2017 • edited Loading

Choose a reason for hiding this comment

sjackman Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovezfs Dec 1, 2017 • edited Loading

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

MikeMcQuaid commented Dec 1, 2017

ilovezfs commented Dec 1, 2017 • edited Loading

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 Dec 4, 2017

sjackman commented Dec 4, 2017

Choose a reason for hiding this comment

sjackman Dec 4, 2017 • edited Loading

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 Dec 4, 2017

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

ilovezfs commented Dec 6, 2017

sjackman commented Dec 6, 2017

MikeMcQuaid commented Dec 8, 2017

sjackman commented Dec 8, 2017

MikeMcQuaid commented Dec 8, 2017

sjackman commented Dec 8, 2017

sjackman commented Dec 8, 2017

sjackman commented Nov 26, 2017 •

edited

Loading

sjackman Nov 27, 2017 •

edited

Loading

sjackman Nov 27, 2017 •

edited

Loading

sjackman Nov 27, 2017 •

edited

Loading

sjackman Dec 1, 2017 •

edited

Loading

ilovezfs Dec 1, 2017 •

edited

Loading

ilovezfs commented Dec 1, 2017 •

edited

Loading

sjackman Dec 4, 2017 •

edited

Loading