Skip to content
This repository was archived by the owner on Jan 16, 2025. It is now read-only.

use XQuartz when present #12607

Closed
wants to merge 17 commits into from
Closed

use XQuartz when present #12607

wants to merge 17 commits into from

Conversation

camillol
Copy link
Contributor

@camillol camillol commented Jun 5, 2012

Let's start testing this. I've built wine with it, which is a very large X11 client, and it works fine.

See also the discussion in #12552.

@MikeMcQuaid
Copy link
Member

This does make me wonder if we're going to have to start making the X11 libraries "proper" dependencies now.

@camillol
Copy link
Contributor Author

camillol commented Jun 6, 2012

@MikeMcQuaid Apple's X11 is nothing but an outdated snapshot of XQuartz. I don't see a reason to prefer it.

@MikeMcQuaid
Copy link
Member

Because we've done significantly more testing with Apple's X11 and it's the current default.

@camillol
Copy link
Contributor Author

camillol commented Jun 6, 2012

Yes, but we need to take a step back and look at why we are adding support for XQuartz in the first place. Read the discussion in #12552. Mountain Lion won't have an X11.app, you're going to need XQuartz. And we also have people trying to introduce new formulas, such as Gtk+3, that require a reasonably recent version of X11; but we want these formulas to work on 10.6 too. I really don't think adding a formula to build X11 from source is a good idea. Rather, we should rely on XQuartz to provide a recent enough X11 on all versions of OS X we support.

@MikeMcQuaid
Copy link
Member

I'm well aware of that. If Gtk+3 requires a newer version and when it is in Homebrew then we should consider it. To do so now is not a good idea; Mountain Lion has not even been released yet.

@camillol
Copy link
Contributor Author

camillol commented Jun 6, 2012

Gtk+3 is not in Homebrew because it requires a recent X11, as explained in #12552. And by the time this pull request goes through, I expect we'll be at least at 10.8.1.

@jacknagel
Copy link
Contributor

Let's see.

  • If both the stock X11 and XQuartz are present, we should prefer XQuartz. There is no good reason to make the outdated, broken version the default. Doing so only makes the users suffer, and have to hack around Homebrew internals to make things work that should instead "just work".
  • We should most definitely be testing this now. Tons of people will upgrade the day Mountain Lion is released, and they will expect that this works.

@adamv
Copy link
Contributor

adamv commented Jun 6, 2012

I'm am going to install XQuartz on my home machine and run with this patch.

I'm glad Mike presents a dissenting opinion, as it is good to keep us honest, but we're at the point where relying on any system-provided X11-related stuff is becoming a liability, c.f. the Cairo issues for instance.

@camillol
Copy link
Contributor Author

camillol commented Jun 6, 2012

How should we indicate that a minimum version of X11 is required?

depends_on :x11 => "2.6.8"
x11 "2.6.8"

ENV.x11 "2.6.8"

@adamv
Copy link
Contributor

adamv commented Jun 6, 2012

depends_on already has "change some env flags" logic, for keg-only brews, so it would not be out of the question to migrate ENV.x11 to a dependency.

A special symbol dependency as above, though; don't really want to add compile-from-source for this.

@camillol
Copy link
Contributor Author

camillol commented Jun 6, 2012

Indeed. Speaking of which, are there any other formulas that have problems with ENV.x11, besides Cairo? I'm wondering if we should separate the two aspects of "I need X11 installed" and "I want the ENV set up". If it's just Cairo, maybe we can find a workaround for that one formula.

@jacknagel
Copy link
Contributor

I'm hopeful that if we get this working right, we can get rid of the keg-only cairo altogether, since we will drop 10.5 support shortly after 10.8 is released. Also potentially pixman.

@jacknagel
Copy link
Contributor

ENV.x11 should also append the appropriate directories to PKG_CONFIG_PATH.

@adamv
Copy link
Contributor

adamv commented Jun 6, 2012

@jacknagel good point; when we build pkg-config we hard-code the System X11 folder into the path

@jacknagel
Copy link
Contributor

I installed XQuartz and this patch, and I'm rebuilding a bunch of stuff to see what needs adjusting. I'll push formula changes to a branch later.

@jacknagel
Copy link
Contributor

when we build pkg-config we hard-code the System X11 folder into the path

We should consider not doing that, and instead be more explicit about what is using X11 libs, using a special dep to adjust PKG_CONFIG_PATH.

@camillol
Copy link
Contributor Author

camillol commented Jun 6, 2012

@jacknagel you may want to restart your rebuild with the new commit, so you can test the new pkgconfig setup too.

@jacknagel
Copy link
Contributor

Don't worry, did it locally before I started.

@MikeMcQuaid
Copy link
Member

Thanks dudes. If you guys think we should move to XQuartz I'm down with that too.

@jacknagel
Copy link
Contributor

Here's a branch with @camillol's patches and some formula tweaks: https://github.com/jacknagel/homebrew/commits/xquartz

The second topmost commit contains changes I've made to things that I have built; the topmost commit is things that I anticipate will need to be changed. There are also some X11 paths embedded in patches that will need to be fixed eventually.

And a list of things I have compiled against XQuartz: https://gist.github.com/2886033

@jacknagel
Copy link
Contributor

Pushed more things into my branch; looks like we'll have to hang on to the keg-only cairo after all, as some things need cairo-gobject which XQuartz does not have. But we should be able to drop the pixman and fontconfig duplicates.

Note that all of this assumes Snow Leopard or newer, I threw Leopard under the bus in my branch.

@2bits
Copy link
Contributor

2bits commented Jun 7, 2012

Gimp would be a good one to test. Curious why that hasn't been pulled?

@2bits
Copy link
Contributor

2bits commented Jun 7, 2012

I think it would be helpful to have a wiki for Howto Install XQuartz, even if it only says

  • Close all your open programs by right clicking on them in the LaunchBar and choosing Quit
  • Download the latest version of XQuartz from http://xquartz.macosforge.org
  • Double click the dmg file you downloaded, then double click on the XQuartz.mkpg
  • Log out and log in.

because there is no Howto on the net that I could find and no instructions in their wiki. Plus I had problems in the past trying to replace the stock one in /usr/X11 when I didn't know much and thought that was the best way to crowbar it in and make it work with Homebrew. I still have PTSD from that.

@2bits
Copy link
Contributor

2bits commented Jun 7, 2012

I noticed in my testing that there are two pkgconfig paths in X. You might want to adjust ENV.x11 to support them both (I had to manually add one to my libxaw3d.rb formula to work around this).

$ find /opt/X11 -type d -name pkgconfig
/opt/X11/lib/pkgconfig
/opt/X11/share/pkgconfig

$ find /usr/X11 -type d -name pkgconfig
/usr/X11/lib/pkgconfig
/usr/X11/share/pkgconfig

The two directories within a distrobution of X do not dupe each other. So I think it's reasonable to add both. Thanks.

@2bits
Copy link
Contributor

2bits commented Jun 7, 2012

I pulled this request with brew pull 12607. I installed XQuartz and gs. I adjusted my gv commit to not depend on HB libxaw3d. I iinstalled gv. When I try to read the svn-book.pdf, I get errors from gs,

GPL Ghostscript 9.05: Error: Font Renderer Plugin ( FreeType ) return code = -1

I'm not sure what it means. A few other pdfs work without error, but I thought I'd mention it.

ghostscript does depend on /opt/X11/lib/libfreetype.6.dylib according to otool -L /usr/local/bin/gs

@camillol
Copy link
Contributor Author

camillol commented Jun 7, 2012

@2bits the instructions are:

  • download the dmg from xquartz.macosforge.org
  • install XQuartz.pkg from the dmg
  • (optional) logout and login again to make sure XQuartz is launched automatically when you run a program that needs X11 from the command line (however, we can probably ask people to run launchctl load /Library/LaunchAgents/org.macosforge.xquartz.startx.plist instead)

@jacknagel
Copy link
Contributor

I noticed in my testing that there are two pkgconfig paths in X.

Yes, I noticed this too but forgot to mention it. I don't know if anything in the share location is used by we should add it anyway; it's a valid location as far as pkg-config is concerned.

@@ -26,25 +26,31 @@ def initialize

def add spec
case spec
when :x11 then @external_deps << X11Dependency.new
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably do this like

when Symbol
  case spec
  when :x11
  ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do a case spec inside a case spec? Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well really it should be factored out into a private method; add is getting long already.

when Symbol then add_symbol(spec)
...

def add_symbol spec
  case spec
  when :x11 then ...
  when :autotools then ...
  when :foo then ...
  ...
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. The part I most dislike is that there is some duplication of code between the Hash case and the non-Hash case. I've refactored it accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I disliked that part also.

@camillol
Copy link
Contributor Author

@2bits I tried installing ffmpeg and it worked for me, including libass.

ENV.x11 is no longer needed in the latest patch, as long as you have depends_on :x11 instead.

@jacknagel
Copy link
Contributor

FYI, I'm keeping a rebased version of this in my tree: https://github.com/jacknagel/homebrew/tree/xquartz

In case anyone wants to apply this on top of master without resolving conflicts (or at least fewer conflicts). There are also a couple of additional patches.

@MikeMcQuaid
Copy link
Member

This seems relevant:

X11 Install on demand

X11 on Mountain Lion now uses install on demand. When you first launch an app that requires X11 libraries, you are directed to a download location for the most up-to-date version of X11 for Mac."

http://www.apple.com/osx/whats-new/features.html

I wonder what it installs and where?

Signed-off-by: Jack Nagel <[email protected]>
 - pkg-config no longer defaults to checking /usr/X11/lib/pkgconfig;
   instead this path is added via ENV.x11 or depends_on :x11. Formulae
   that expect X11 libs should be explicitly marked as depends_on :x11.
 - Remove warning about /usr/X11 as a symlink.

Signed-off-by: Jack Nagel <[email protected]>

Conflicts:

	Library/Homebrew/cmd/doctor.rb
@camillol
Copy link
Contributor Author

@MikeMcQuaid reports say it directs you to install XQuartz.

@MikeMcQuaid
Copy link
Member

If it automatically installs like Java does it might be cool to try and trigger that process.

@camillol
Copy link
Contributor Author

Good idea! It's probably enough to attempt connecting to the $DISPLAY. But we can do that after ML comes out.

@jacknagel
Copy link
Contributor

@camillol Now that #10510 has landed we can move forward with this; please rebase it when you get a chance.

@samueljohn
Copy link
Contributor

Nobody running (or knowing somebody running) ML beta?

@lumaxis
Copy link

lumaxis commented Jun 27, 2012

Any progress on this?

@2bits
Copy link
Contributor

2bits commented Jun 27, 2012

A major change to Homebrew was just pulled a couple of days ago, and it is getting adjusted in #13037. After that has settled, XQuartz is next on the list, and the time-scale for that is probably 2 weeks or less.

@MikeMcQuaid
Copy link
Member

We should probably get this in ASAPish as Mountain Lion is due in July some time.

@lumaxis
Copy link

lumaxis commented Jun 27, 2012

Do you consider these commits to be in a state which can be tested?

@2bits
Copy link
Contributor

2bits commented Jun 27, 2012

Yes but they might need to be rebased for them to apply cleanly. We are waiting for @camillol to do that.

@jacknagel
Copy link
Contributor

If @camillol is busy I can do the rebase tomorrow. I agree that we need to get it in soon.

@samueljohn
Copy link
Contributor

Yes please. This is a heroic work. And it fits very well to my recent
addition, since for Xcode-only I need the redirection of where X11
includes and libs are located, too. I added the switching in
ENV.x11.

Two questions:

  • Will there be a formula for Xquartz, eventually?
  • X11 from Lion is no more supported, right?

@jacknagel
Copy link
Contributor

Will there be a formula for Xquartz, eventually?

Building it from source is a lot of work (there are a ton of individual pieces to it), but a formula that installs the binaries from the dmg (I have no idea if that is possible) would be cool.

X11 from Lion is no more supported, right?

It will be utilized if XQuartz is not installed, but the presence of XQuartz will mean "use XQuartz instead", and XQuartz will be recommended from here on out.

@samueljohn
Copy link
Contributor

Thanks, sounds good. Pointing to the downloaded seems reasonable for now.

Now I understand why @adamv doesn't want me to add missing ENV.x11
to formulae :)

@jacknagel
Copy link
Contributor

I have a rebased version of this locally; planning to start merging it in the morning.

@jacknagel
Copy link
Contributor

@samueljohn Are there any known setups where MacOS.sdk_path is nil?

I'm trying to clean up ENV.x11 and I want to know if it's safe to assume that it will be a Pathname.

@samueljohn
Copy link
Contributor

If you only have the CLT, sdk_path is nil. Using to_s returns an
empty string. And appending the usual /usr/include works in both
cases then.

@jacknagel
Copy link
Contributor

Thanks.

@jacknagel
Copy link
Contributor

Ran into a few issues so I decided to wait a day, I'll try to merge it tomorrow morning though.

@jacknagel
Copy link
Contributor

Merged.

@jacknagel jacknagel closed this Jul 1, 2012
@2bits
Copy link
Contributor

2bits commented Jul 1, 2012

Congrats @camillol! Thanks for making this happen.

@camillol
Copy link
Contributor Author

Hey guys, sorry for not following Homebrew development lately, but I've been very busy. Many thanks to @jacknagel for handling the rebase and merge in my stead, and to everyone who supported this work!

@Homebrew Homebrew locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants