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

Fix Homebrew issues on 0.7 and 1.0 #250

Merged
merged 2 commits into from
Aug 22, 2018
Merged

Fix Homebrew issues on 0.7 and 1.0 #250

merged 2 commits into from
Aug 22, 2018

Conversation

tknopp
Copy link
Contributor

@tknopp tknopp commented Aug 22, 2018

@coveralls
Copy link

coveralls commented Aug 22, 2018

Coverage Status

Coverage remained the same at 96.471% when pulling 4e81c9d on tknopp-patch-3 into 2c62d5a on master.

@tknopp
Copy link
Contributor Author

tknopp commented Aug 22, 2018

This makes me so happy. Appveyor failure is some download error. That passed before I modified travis.

@tknopp
Copy link
Contributor Author

tknopp commented Aug 22, 2018

@lobingera: Please have a look and merge. Since this fixes Cairo.jl for Mac users on 0.7 and 1.0 we should make a release 0.5.6 release and then can close all the bug reports. #249 could be included but it changes the structure of CairoSurface and therefore should better be included in its own release.

@affans
Copy link

affans commented Aug 22, 2018

Glad i could help. I needed all of this for vegalite plotting.

edit: wouldn't it be better to submit this to Homebrew.jl also? It wouldn't hurt if homebrew downloaded it when downloading the other libraries.

@lobingera lobingera merged commit 2017ef4 into master Aug 22, 2018
@tkelman
Copy link
Contributor

tkelman commented Aug 22, 2018

This should be fixed at the homebrew recipe level. This is like fixing a missing dependency by manually calling Pkg.add instead of adding it to REQUIRE where it belongs

@tknopp
Copy link
Contributor Author

tknopp commented Aug 22, 2018

Yes that is clear. It just needs someone with homebrew knowledge (i.e. not me) to do this. @staticfloat maybe.

@lobingera
Copy link
Contributor

@tkelman That's why it's called 'Fix', not 'Solution'...

@tkelman
Copy link
Contributor

tkelman commented Aug 23, 2018 via email

@lobingera
Copy link
Contributor

@tkelman To set this straight at the beginning: I agree with you and i do not like the situation to work with fixes like this. I accepted to master, as it's providing a little bit of light at the end of the tunnel of 1.0 blocking items - and yes, to stay in the tunnel-light-metaphor, i'm aware the light could also be a train approaching fast. I'd be very happy if the homebrew and Homebrew.jl and BinDeps.jl people can solve this in configuration of their packages, but i' not inclined to hold my breath to see this happening. I'm trying to put some energy and time on this weekend to educate myself how hb/HB/BinDeps interact and what actually is the basic problem and root cause (... it did work before ...). If you have a shortcut for this, be invited to bring it forward.

@tknopp
Copy link
Contributor Author

tknopp commented Aug 24, 2018

This discussion leads to nowhere. I find it pretty unsatisfying that the people putting energy into this project are blamed for their work. We don't need to justify ourself to someone who does not use Julia anymore.

@timholy: I would really like to get support from you in this matter. We got travis all green with this patch and terms like messy hack workaround are in my opinion not appropriate here (although I have to admit not being a native speaker).

@tkelman
Copy link
Contributor

tkelman commented Aug 24, 2018

The problem is more or less here. https://github.com/Homebrew/homebrew-core/blob/83dda4fe2614ad89c3d9715ce0a17f3f580874bf/Formula/harfbuzz.rb#L25

graphite2 is listed as a "recommended" dependency of harfbuzz by homebrew, but the binaries of harfbuzz they're distributing are built linked to graphite2. If you download homebrew's harfbuzz bottles directly and look at the dylibs with otool you should be able to verify this. Either homebrew should be building bottles without linking against recommended dependencies, or the dependencies that are linked against should be handled as full dependencies when installing bottles. If the brew cli already implements this logic for recommended dependencies, then the bug may be in Homebrew.jl's treatment of them.

@lobingera
Copy link
Contributor

If you download homebrew's harfbuzz bottles directly and look at the dylibs with otool you should be able to verify this.

Can i do this on Linux?

@tkelman
Copy link
Contributor

tkelman commented Aug 24, 2018

Possibly with ObjectFile.jl? Check what the mac build image for BinaryBuilder uses for otool-like functionality when inspecting linkages. Otherwise it's usually violating the mac sdk's eula to use it on non-apple hardware, annoyingly. http://www.newosxbook.com/tools/jtool.html might be a workable alternative

@lobingera
Copy link
Contributor

I see. Still the responsibility to do this correctly is with the homebrew-core formular people? btw: Why are other brew users are not seeing these problems?

@tkelman
Copy link
Contributor

tkelman commented Aug 24, 2018

I'm not sure what brew's behavior would be in a totally clean environment if you do brew install harfbuzz - if it installs graphite2, then Homebrew.jl and the brew cli apparently differ in behavior on recommended dependencies.

@lobingera
Copy link
Contributor

lobingera commented Aug 25, 2018

@tkelman I borrowed a Macbook and otools shows me on the libharfbuzz.dylib (download from bottle) a dependency on graphite. The jtool thing isn't compiling on linux. I've put a question on https://discourse.brew.sh/t/harfbuzz-dependencies/2855

@tkelman
Copy link
Contributor

tkelman commented Aug 25, 2018 via email

@lobingera
Copy link
Contributor

lobingera commented Aug 26, 2018

I couldn't run brew on the other computer. Maybe via travis. Or a volunteer from the julia community.

@lobingera
Copy link
Contributor

@tkelman Can you explain, what 'wrong' effects on a julia installation could happen if graphite2 is installed manually?

@tkelman
Copy link
Contributor

tkelman commented Sep 2, 2018

This should not be released as-is, it's an incorrect fix and masking a bigger problem that should be fixed directly - either Homebrew.jl isn't handling recommended dependencies correctly, in which case this isn't the only package that will see incorrect behavior, or the homebrew bottle itself was built wrong. It is a bit unfortunate that it requires debugging on a mac to identify which it is. Hopefully someone who's hitting the issue can assist with that.

The workaround here incorrectly treats graphite2 as if it was manually requested to be installed by the user, rather than only being installed as a dependency of something else.

@timholy
Copy link
Member

timholy commented Sep 2, 2018

@tkelman, can you just fix it yourself? The key concern is that it appears none of us know how. While I have a very superficial understanding of the issues here, I agree with your sentiments; nevertheless, I assure you we are not going to freeze progress in Cairo.jl indefinitely (see #254) just for a fix that would be "more ideal."

At a minimum, since Cairo isn't the fundamental problem it would be good to report our concerns in the appropriate location. I'm not the right person to do that.

@tkelman
Copy link
Contributor

tkelman commented Sep 2, 2018

Without a mac, no I can't determine the correct fix either. I'm fine releasing 254 without this. Some of the individuals who are hitting the issue that this works around should be able to assist in identifying the actual problem. Really anyone with a mac.

@timholy
Copy link
Member

timholy commented Sep 2, 2018

OK, let's revert this. There are lots of mac users in #230, let's continue the conversation there. @tkelman if you can coach one of them through what they should be trying, that would be a huge help.

timholy added a commit that referenced this pull request Sep 2, 2018
lobingera pushed a commit that referenced this pull request Sep 2, 2018
@tkelman
Copy link
Contributor

tkelman commented Sep 8, 2018

The longer this gets left in place, the more users are going to end up in an incorrect state that will interfere with future debugging and installation, given the wrong treatment of the graphite2 dependency here. The "at a minimum" has not been done, and there's shockingly little indication of anyone caring about fixing this the permanent, correct, non-harmful, upstream way. A little bit of iteration (on a mac) through what's happening on 0.6 and 0.7 to identify whether it's Homebrew.jl or the homebrew packaging of cairo/harfbuzz at fault here is all that's necessary. If this incorrect band-aid gets left here indefinitely, is anyone actually going to look into the real problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants