search

Homebrew / legacy-homebrew

💀 The former home of Homebrew/homebrew (deprecated)
https://brew.sh
26.99k stars 11.36k forks source link

qt5: bug fixes and formula improvements (discussion) #44998

Closed UniqMartin closed 8 years ago

UniqMartin commented 8 years ago

Qt 5.5.1 has landed, the bump commit is ready, and the local test build is currently running. Fortunately, we will be able to drop most of the patches needed for 5.5.0.

The reason this is not yet a PR is that I'd like to use the opportunity to “sneak in” a few improvements to the formula that are unrelated to the version bump, without burdening the test bot. Please have a look at https://github.com/UniqMartin/homebrew/compare/master...qt5-cleanup. Every commit corresponds to one isolated change and the motivation for that change is given in the commit message. I'd appreciate any feedback.

I'd like to create a PR for the version bump (including the improvements) once we have agreed on a subset of the above commits. Is this approach okay with everyone?

CC @DomT4 as you seem to be the primary Homebrew maintainer dealing with this formula.

MikeMcQuaid commented 8 years ago

Looks great and really appreciate the way you've approached this. Made one comment.

UniqMartin commented 8 years ago

Thanks for the quick feedback! So far, my minimal local build has completed successfully. If that's okay, I'll wait with the PR until tomorrow. By then, my full local build will be finished (hopefully without errors) and others will have had a chance to comment here, too.

DomT4 commented 8 years ago

CC @DomT4 as you seem to be the primary Homebrew maintainer dealing with this formula.

I'm happy not to be though :smile_cat: :trollface:.

I'd almost be tempted to turn WebKit off by default as that's a ridiculous chunk of build time and something most people likely don't use, but probably too radical a step to make suddenly. It'd be nice to check if they've resolved any of the outstanding rpath problems upstream and whether we can cut that option but I don't have high hopes.

All the proposed changes look fine to me. If you ever feel a particularly burning passion for Qt it'd be useful to see what we can transition across from using Qt to Qt5, as the former is basically dead upstream now. Appreciate you working on the Qt5 formula & your contributions to Homebrew lately more broadly.

MikeMcQuaid commented 8 years ago

I'd almost be tempted to turn WebKit off by default as that's a ridiculous chunk of build time and something most people likely don't use, but probably too radical a step to make suddenly. It'd be nice to check if they've resolved any of the outstanding rpath problems upstream and whether we can cut that option but I don't have high hopes.

Nah, people definitely use it.

julien-duponchelle commented 8 years ago

I agree people use webkit and Qt team push is usage more and more.

And when distribution build it as a separate package user don't understand why QWebkit is missing when they want to use it.

julien-duponchelle commented 8 years ago

Do you think it's still true? keg_only "Qt 5 conflicts Qt 4 (which is currently much more widely used)."

Qt4 is not supported on the last OSX version.

MikeMcQuaid commented 8 years ago

@noplay Compare brew uses qt to brew uses qt5. We've also hacked Qt4 so it works on 10.11 :wink:

UniqMartin commented 8 years ago

Wow, I didn't expect this comment to grow to such a length. But oh well … :open_mouth:

I'd almost be tempted to turn WebKit off by default as that's a ridiculous chunk of build time and something most people likely don't use, but probably too radical a step to make suddenly.

Not to forget the more recent WebEngine, that is basically a vendored Chromium with all its dependencies and its own build system. Personally, I wouldn't mind switching the default to off, but I guess people expect to get the full package.

It'd be nice to check if they've resolved any of the outstanding rpath problems upstream and whether we can cut that option but I don't have high hopes.

I haven't investigated that thoroughly, but as far as I can tell, it is still broken (or just like that by design). I guess the way Homebrew installs Qt is just not the typical way Qt gets distributed on OS X.

With -no-rpath (our current configure argument):

With -rpath (configure default on OS X):

So both variants have their ups and downs, but I'm pretty sure Homebrew should continue to use -no-rpath (and expect it to work out of the box, though it currently doesn't).

If you ever feel a particularly burning passion for Qt it'd be useful to see what we can transition across from using Qt to Qt5, as the former is basically dead upstream now.

That could happen. Not right now, but probably soon. I guess this is something for another (tracking) issue, but my general thoughts are:

Appreciate you working on the Qt5 formula & your contributions to Homebrew lately more broadly.

Thanks! I guess apart from learning a great deal while doing this, I also very much value the pleasant atmosphere that's created by all Homebrew maintainers I interacted with so far. :heart_eyes_cat:

UniqMartin commented 8 years ago

Compare brew uses qt to brew uses qt5.

That's a sad reality, but I guess it is true. Though I wouldn't be surprised if some of those formulae have gained support for qt5 that went unnoticed during a version bump.

MikeMcQuaid commented 8 years ago

That's a sad reality, but I guess it is true. Though I wouldn't be surprised if some of those formulae have gained support for qt5 that went unnoticed during a version bump.

If that's the case: awesome. Definitely agree to using qt5 over qt when possible. I just suspect it'll take significantly longer than upstream would like...

DomT4 commented 8 years ago

Not to forget the more recent WebEngine, that is basically a vendored Chromium with all its dependencies and its own build system. Personally, I wouldn't mind switching the default to off, but I guess people expect to get the full package.

This is what happens when I write comments at night without caffeine. I meant to reference WebEngine rather than WebKit in my mini-ponder about defaulting to off, but alas I also agree we should match upstream's binary package as much as possible/sensible.

Consequently, make docs (triggered by --with-docs) breaks, because it has to run qdoc et al. before Keg#fix_install_names had a chance to fix them. See #42217.

If we aren't going to be able to get this working without turning on rpath usage and breaking more elements, I'd probably be in favour of removing the option. Otherwise people are going to keep building with docs and get a fair bit into the build before it dies and we have to tell them "at the moment it just isn't possible".

*.app bundles provided by Qt break because we move them out of bin, thus invalidating the embedded relative rpath that points into /lib.

We could handle this by building everything in libexec, removing the in-formula app move and then very carefully symlinking respective bits and pieces into prefix/bin,lib,share,etc etc but it would be a bit of in-formula dancing to get it done and testing to check potential breakage elsewhere. Wary that building Qt5 from source is not fun.

keg_only "Qt 5 conflicts Qt 4 (which is currently much more widely used)."

The only problem with reversing this is that everything that has a hard runtime dependency on qt, and the qt formula itself, would need to be revisioned to enforce the unlink, which means a lot of compiling for non-default-prefix users. It needs to get done one day in the not too distant future, but it's going to be er... not fun :).

UniqMartin commented 8 years ago

Here's how we might end up with a working Qt 5 build with rpath usage (i.e. without -no-rpath):

What do you think? Sounds like an idea that we should give a try?

MikeMcQuaid commented 8 years ago

What do you think? Sounds like an idea that we should give a try?

Sounds great :+1:

UniqMartin commented 8 years ago

keg_only "Qt 5 conflicts Qt 4 (which is currently much more widely used)."

The only problem with reversing this is that everything that has a hard runtime dependency on qt, and the qt formula itself, would need to be revisioned to enforce the unlink, which means a lot of compiling for non-default-prefix users. It needs to get done one day in the not too distant future, but it's going to be er... not fun :).

Indeed, not fun. But assuming #43518 gets merged soon and the move to keg-only Qt 4 happens in a few months, there will be some time for versions to be bumped and bottles to be regenerated. Then, this transition won't be as painful as it would be if done today.

DomT4 commented 8 years ago

What do you think? Sounds like an idea that we should give a try?

If you can make it work and all the brew uses qt5 stuff builds and tests okay, by all means, go for it.

But assuming #43518 gets merged soon and the move to keg-only Qt 4 happens in a few months, there will be some time for versions to be bumped and bottles to be regenerated.

Yes, indeed. I nudged that issue in the team chat earlier and it will likely be merged soon.

UniqMartin commented 8 years ago

Here's finally a status update on Qt 5. The changes I've prepared are in my qt5-cleanup branch again. Once we've settled on the desired subset of changes I'll post this as a PR (and add a revision bump for the formula, too). I apologize in advance for the long read.

Major fix (install names and RPATH)

First a few words about my idea of switching to an RPATH-based build of Qt 5: Adjusting the *.app bundles with install_name_tool was no big deal, but then then all the install names and cross dependencies between the Qt 5 frameworks still use the @rpath/ prefix and this implies that every application that links against Qt 5 will have to embed a suitable RPATH into the binary. This doesn't seem feasible to me and I'd expect a lot of software to fail because of this. So I abandoned this idea.

Commit https://github.com/UniqMartin/homebrew/commit/66e2d5e42b577b07bcd79ba2a5722b25084f7f21: This is what I believe to be a better solution to the problem and it resolves both of our outstanding Qt 5 issues. I submitted that upstream, but whether it will be merged, is not clear yet. There's some heated debate about whether this is an actual issue and if so, whether this is the right approach.

If this fails to be accepted upstream, there would be other ways to fix the outstanding issues for Homebrew, by making some adjustments to the formula itself and by fixing what I believe to be an actual bug in Keg#fixed_name in keg_relocate.rb. (I'll try to address the latter soon, as it should be fixed irrespective of what happens to the Qt 5 formula.)

Other changes/fixes

Commit https://github.com/UniqMartin/homebrew/commit/109d56a9bba3d150993c01d918ef248108ac1df9: Dropped the --with-developer option as suggested by Mike.

Commit https://github.com/UniqMartin/homebrew/commit/a964abd1f62fd692d9de6e0743e6b9a6b3d476a6: I left the *.app bundles in bin, but created symbolic links in libexec so that they'll be picked up by brew linkapps. This is necessary as moving the bundles into a different location actually breaks some functionality. On the other hand, brew audit is not happy if the bundles are left in bin. Any suggestions on how to deal with that? For now I went with making the GUI tools work instead of making brew audit happy.

MikeMcQuaid commented 8 years ago

This all sounds good to me. Not sure how to address the brew audit if those files can't be moved. Can we use install_name_tool to fix them up so they can be moved? Obviously if upstream reject the rpath patch we may need to consider removing it.

DomT4 commented 8 years ago

. On the other hand, brew audit is not happy if the bundles are left in bin. Any suggestions on how to deal with that?

I don't generally think we should change this, since .apps should not live in the bin prefix broadly. I guess I wouldn't be horribly opposed to the check being relaxed exclusively for keg_only formulae. (Although qt5 won't be keg_only forever).

UniqMartin commented 8 years ago

Not sure how to address the brew audit if those files can't be moved. Can we use install_name_tool to fix them up so they can be moved?

No, that's unfortunately hard-coded in the Qt sources. There are several options, but none of them seems particularly attractive:

I guess I wouldn't be horribly opposed to the check being relaxed exclusively for keg_only formulae. (Although qt5 won't be keg_only forever).

IMHO making this check depend on whether a formula is keg_only or not is not a good idea, not to mention that the same issue arises with the qt formula. (I wanted to tackle all of the outstanding Qt 5 issues before messing with Qt 4, but I already have a few pending changes I'm testing locally.)

DomT4 commented 8 years ago

Relax the audit check: *.app bundles could be tolerated. (They are a type of binary after all.)

I really don't want to do this. I don't think they have any business being in bin and pretty firmly believe everything in bin should be executable in the traditional sense.

Relocate all binaries: Pass -bindir=#{libexec} to configure and then create symbolic links in bin.

This seems fine to me, personally. Or at least, the least worst option.

MikeMcQuaid commented 8 years ago

I really don't want to do this. I don't think they have any business being in bin and pretty firmly believe everything in bin should be executable in the traditional sense.

Agreed.

No, that's unfortunately hard-coded in the Qt sources. There are several options, but none of them seems particularly attractive:

So, stupid question: if you just move them: what fails?

Patch Qt sources: Use the location we will be moving the *.app bundles to. This patch obviously wouldn't be accepted upstream, so we would have to keep maintaining it. (That's not horribly complicated, but still a maintenance burden.)

This might be accepted upstream if it was a configure option?

UniqMartin commented 8 years ago

I kinda agree on not wanting to allow *.app bundles in bin. Just didn't want to omit that option.

Relocate all binaries to #{libexec}/bin by passing -bindir to configure.

This seems fine to me, personally. Or at least, the least worst option.

I have tried this one and it seems to work without any loss of functionality or any breakage. Not exactly elegant, but not too bad either given how much symlinking is already going on in the formula. Currently my personal favorite …

So, stupid question: if you just move them: what fails?

The only breakage I'm aware of is that attempting to open the manual from the Help menu of Designer.app and Linguist.app will fail as they won't be able to locate Assistant.app, which is used to show the documentation.

Patch Qt sources: […]

This might be accepted upstream if it was a configure option?

Might be. However, I lack good arguments and (to be honest) also the desire to pursue this upstream.

MikeMcQuaid commented 8 years ago

The only breakage I'm aware of is that attempting to open the manual from the Help menu of Designer.app and Linguist.app will fail as they won't be able to locate Assistant.app, which is used to show the documentation.

I'm OK with accepting that and just documenting the issue in the formula and/or Qt issue tracker.

UniqMartin commented 8 years ago

I'm OK with accepting that and just documenting the issue in the formula and/or Qt issue tracker.

Just as a comment in the part of the formula where those *.app bundles are moved or also more visibly in the formula caveats? So we accept this minor breakage instead of (a) accepting Qt's view on where those *.app bundles should live and instead of (b) working around those views with a bunch of symlinks and relocating the binaries? I'm not entirely happy with this resolution, but am also not passionate enough to put too much more work into this.

@DomT4 Any (final) opinions before I push the updated changes and create a PR?

DomT4 commented 8 years ago

I still prefer overriding the bindir and selectively linking anything not an .app back to prefix/bin as it doesn't cause any breakage and doesn't mandate us having to get some constructive communication flowing upstream at Qt again but if Mike is particularly keen on just moving them I won't fight him over it, heh.

MikeMcQuaid commented 8 years ago

Well, I mean the right approach is fixing this in Qt so it can handle these things being moved around. As you've (reasonably) said you're not bothered about doing much more work on this so "a bit less broken" seems like an improvement to me. I'm not opposed to using libexec I just fear it may break software that has hard-coded paths into the Cellar pretty hard.

UniqMartin commented 8 years ago

Thanks for all your feedback! :heart: I've created two new branches, if you want to have a look:

The only difference between the two is in the final commit that deals with the binaries. Unless there are objections, I intend to create a PR from the former branch late tomorrow or the day after. This will hopefully resolve all remaining Qt 5 issues in Homebrew.

I'm not opposed to using libexec I just fear it may break software that has hard-coded paths into the Cellar pretty hard.

I'm hopeful this approach won't break anything as everything (except for the .app bundles) will be in the same place as before thanks to symlinking. I haven't tested this, though, so that's just an educated guess.

MikeMcQuaid commented 8 years ago

I'm hopeful this approach won't break anything as everything (except for the .app bundles) will be in the same place as before thanks to symlinking. I haven't tested this, though, so that's just an educated guess.

The best thing to check would be using qmake to print out its compiled-in paths.

DomT4 commented 8 years ago

I'm happy with either. If you and Mike prefer the former let's go for that. I think either system should work (mostly) and that's the important thing to achieve here.

Give one of the maintainers a shout before you push your PR and we'll bash the CI timeout in an upwards direction for you.

UniqMartin commented 8 years ago

The best thing to check would be using qmake to print out its compiled-in paths.

$(brew --prefix qt5-modular)/bin/qmake -query | fgrep BIN output:

QT_INSTALL_BINS:/opt/homebrew/Cellar/qt5-modular/5.5.1/libexec/bin
QT_HOST_BINS:/opt/homebrew/Cellar/qt5-modular/5.5.1/libexec/bin

$(brew --prefix qt5-modular)/bin/qtpaths --binaries-dir output:

/opt/homebrew/Cellar/qt5-modular/5.5.1/libexec/bin

For branch qt5-cleanup-bindir this reports the configured binary directory as I expected. I'm not sure which conclusion to draw from this information, though. (Ignore the -modular in the formula name. That's just my minimal version of the formula for faster turnaround times.)

I'm happy with either. If you and Mike prefer the former let's go for that.

If my vote counts, I'm actually in favor of the qt5-cleanup-bindir variant that uses libexec/bin/ and symlinks. But since no one else participates here, I saw a tie between you two and thus proposed to use the slightly simpler and less risky variant.

MikeMcQuaid commented 8 years ago

Maybe it doesn't matter but qmake pointing to libexec feels weird to me.

UniqMartin commented 8 years ago

Maybe it doesn't matter but qmake pointing to libexec feels weird to me.

Why? What I mean is, can you phrase it in a way I might understand it or is this just a gut feeling?

Not sure if it changes your opinion, but I just succeeded in building all core formula that (optionally) depend on Qt 5 against such a Qt 5 installation, with the only exception of open-scene-graph (also fails without --with-qt5, thus an unrelated issue). To be more specific:

UniqMartin commented 8 years ago

Update: open-scene-graph also builds successfully with Qt 5 support after applying a workaround for the SDK-related issue. (I edited the comment above to reflect this.)

MikeMcQuaid commented 8 years ago

Why? What I mean is, can you phrase it in a way I might understand it or is this just a gut feeling?

Partly gut feeling, partly that this is a library where people may rely on the specific locations passed by QMake. For example, we'll likely break all CMakeCache files. I'm concerned there may be other places in other projects that hardcode Homebrew's locations (particularly since we've offered the opt symlink).

MikeMcQuaid commented 8 years ago

Another option that just occurred to me: just delete those application bundles and let whoever wants to add them back work with upstream to make this stuff work.

UniqMartin commented 8 years ago

I agree that changing -bindir to libexec/bin is more likely to break things, but not for the reasons you stated. Everything that uses the opt path will continue to work, because we provide symlinks for all relocated binaries. Everything that directly uses the keg path (qmake, CMakeCache files, etc.) will break anyway, because the keg path is going to change due to the revision bump.

Another option that just occurred to me: just delete those application bundles and let whoever wants to add them back work with upstream to make this stuff work.

This never occurred as an option to me, but I'm not opposed to giving it a try. This would mean Qt 5 will continue to serve as a dependency for other formula just fine, but will be less useful to people who want to use it for actual development and rely on Designer, Linguist, etc.?

DomT4 commented 8 years ago

Another option that just occurred to me: just delete those application bundles and let whoever wants to add them back work with upstream to make this stuff work.

Would prefer to avoid this, personally.

MikeMcQuaid commented 8 years ago

Would prefer to avoid this, personally.

Cool, no worries. Give no users have complained about this I'm not super concerned about it being slightly broken. I think I'd rather leave the paths as-is for now.

UniqMartin commented 8 years ago

Give one of the maintainers a shout before you push your PR and we'll bash the CI timeout in an upwards direction for you.

Alright, let's get this merged! I'll submit qt5-cleanup-appmove today as soon as I hear from one of you that the CI timeout has been adjusted.

DomT4 commented 8 years ago

Timeout bumped. Push when desired.

UniqMartin commented 8 years ago

Thanks! :heart:

DomT4 commented 8 years ago

Closed by #45793 being merged, I'd presume :)

UniqMartin commented 8 years ago

Yes, I think this issue has served its purpose well. Any future developments regarding qt5 are probably worth a separate issue. So glad this is finally done. :smile_cat:

uliano commented 8 years ago

on 10.7 qt5 fails to build

here is the gist

https://gist.github.com/anonymous/561a21c8c22422245f83

DomT4 commented 8 years ago

Looks like a compiler issue:

In file included from ../../../3rdparty/libwebp/src/dec/tree.c:15:
In file included from ../../../3rdparty/libwebp/src/dec/../utils/bit_reader_inl.h:29:
../../../3rdparty/libwebp/src/utils/./endian_inl.h:51:10: error: use of unknown builtin '__builtin_bswap16' [-Wimplicit-function-declaration]
  return __builtin_bswap16(x);
         ^
1 error generated.
make[5]: *** [.obj/private/tmp/qt520151112-5147-waing1/qt-everywhere-opensource-src-5.5.1/qtimageformats/src/3rdparty/libwebp/src/dec/tree.o] Error 1
make[4]: *** [sub-webp-make_first] Error 2
make[4]: *** Waiting for unfinished jobs....

Could try brew install qt5 --cc=gcc-5 - No guarantees though.

UniqMartin commented 8 years ago

@uliano It is a mix between a compiler issue and Qt 5.5.1 not fully supporting OS X 10.7 anymore. Please see #45284 (the last few comments) for more details.

uliano commented 8 years ago

gcc5 doesn't work

g++-5: error: unrecognized command line option '-stdlib=libc++'

for what is worth, here is the new gist

https://gist.github.com/05276192f61f0f8ac843

unfortunately on this machine (MacPro1,1) I Apple wan't let me go beyond 10.7.5.

MikeMcQuaid commented 8 years ago

@uliano It is a mix between a compiler issue and Qt 5.5.1 not fully supporting OS X 10.7 anymore. Please see #45284 (the last few comments) for more details.

Someone should probably add something to the Qt5 formula to just not support 10.7 and below.

UniqMartin commented 8 years ago

@uliano Could you please continue the discussion in the issue (#45284) I pointed out? Please repost your latest comment there. There's no point discussing it here, as this issue is closed and unrelated. If you don't mind editing the qt5 formula, there's still one more thing I can suggest to try.

Someone should probably add something to the Qt5 formula to just not support 10.7 and below.

Yes, maybe we should do this, at least with the latest Xcode/Clang that is supported on 10.7. I'll think about it and prepare something. I'm not sure yet it should be broken when compiling with GCC.

MikeMcQuaid commented 8 years ago

Yes, maybe we should do this, at least with the latest Xcode/Clang that is supported on 10.7. I'll think about it and prepare something. I'm not sure yet it should be broken when compiling with GCC.

The question is if upstream "officially" support it. If not: we shouldn't either (at least not in core). Definitely agreed on the Clang.