-
-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
qt5 5.6.2 #5843
qt5 5.6.2 #5843
Conversation
c976381
to
34ee6ff
Compare
Fails to build for me. Logs here. However, note that while the build "fails" there's nothing obvious in the logs--it appears to have built to completion; maybe something in the formula after that point? [edit] Ah, found something buried halfway through:
|
Looks like this is #5391 (comment) |
@rleigh-codelibre already fixed that a while ago, if you re-pull :) |
@@ -27,28 +26,6 @@ class Qt5 < Formula | |||
sha256 "73d33dd2563c39542844c276a7bd43463f2974fde141e7afeb3057168adbe606" => :mavericks | |||
end | |||
|
|||
# Restore `.pc` files for framework-based build of Qt 5 on OS X. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think getting rid of this would still be a mistake, FWIW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DomT4 Can you elaborate a bit more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it's still going to be very breaking, especially as Homebrew moves stuff from qt
to qt5
. The Qt ecosystem isn't exactly known for its rapid response to upstream changes. Up to y'all though, I'm no longer around really, just noticed this and thought I'd add the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DomT4 thanks for chiming in. For the formulae mentioned in the comment, the situation currently is
- wireshark: upstream fixed it
- mkvtoolnix: upstream provides the configure option
--without-qt-pkg-config
and then checks the environment variablesQT_CFLAGS
andQT_LIBS
- poppler: upstream checks the environment variables
POPPLER_QT5_CFLAGS
,POPPLER_QT5_LIBS
,POPPLER_QT5_TEST_CFLAGS
, andPOPPLER_QT5_TEST_LIBS
as an alternative to pkg-config.
So if the patch is removed, these are the required diffs:
diff --git a/Formula/mkvtoolnix.rb b/Formula/mkvtoolnix.rb
index cd2e296..17ed39d 100644
--- a/Formula/mkvtoolnix.rb
+++ b/Formula/mkvtoolnix.rb
@@ -64,6 +64,10 @@ class Mkvtoolnix < Formula
if build.with?("qt5")
qt5 = Formula["qt5"]
+ ENV["QT_CFLAGS"] = "-I#{qt5.opt_include} -I#{qt5.opt_include}/QtNetwork -I#{qt5.opt_include}/QtWidgets -I#{qt5.opt_include}/QtGui -I#{qt5.opt_include}/QtCore"
+ ENV["QT_LIBS"] = "-F#{qt5.opt_lib} -framework QtNetwork -framework QtWidgets -framework QtGui -framework QtCore"
+
+ args << "--without-qt-pkg-config"
args << "--with-moc=#{qt5.opt_bin}/moc"
args << "--with-uic=#{qt5.opt_bin}/uic"
args << "--with-rcc=#{qt5.opt_bin}/rcc"
diff --git a/Formula/poppler.rb b/Formula/poppler.rb
index a405768..f860c15 100644
--- a/Formula/poppler.rb
+++ b/Formula/poppler.rb
@@ -53,6 +53,13 @@ class Poppler < Formula
]
if build.with? "qt5"
+ qt5 = Formula["qt5"]
+
+ ENV["POPPLER_QT5_CFLAGS"] = "-I#{qt5.opt_include} -I#{qt5.opt_include}/QtXml -I#{qt5.opt_include}/QtWidgets -I#{qt5.opt_include}/QtGui -I#{qt5.opt_include}/QtCore"
+ ENV["POPPLER_QT5_LIBS"] = "-F#{qt5.opt_lib} -framework QtXml -framework QtWidgets -framework QtGui -framework QtCore"
+ ENV["POPPLER_QT5_TEST_CFLAGS"] = "-I#{qt5.opt_include} -I#{qt5.opt_include}/QtTest -I#{qt5.opt_include}/QtCore"
+ ENV["POPPLER_QT5_TEST_LIBS"] = "-F#{qt5.opt_lib} -framework QtTest -framework QtCore"
+
args << "--enable-poppler-qt5"
else
args << "--disable-poppler-qt5"
There are probably similar changes that would be needed in other taps too. If upstream is not going to reverse its decision to remove the pkg-config files, it seems at some point we need to bite the bullet and make the requisite changes (and do the corresponding upstream reporting) in formulae that are affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems at some point we need to bite the bullet and make the requisite changes (and do the corresponding upstream reporting) in formulae that are affected.
Agreed. We should stick to the upstream version wherever possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ilovezfs Thanks. Tested again on 10.12 and it builds and links successfully, and my code builds and runs against it without problems. |
qca problem is fixed by a revision bump. qjackctl problem is caused by opportunistic linking to Both are unrelated to this PR and can be fixed before, during, or after it without consequence. |
Created with
brew bump-formula-pr
.