help needed: Updating pyqt to 4.11.1 #30881

NikolausDemmel · 2014-07-15T15:14:15Z

Unfortunately this fails to compile (on Mavericks) with the following linker error: http://pastebin.com/rWmepPhu

Possibly this is related to Homebrew building with the custom libc++ makespec.

Any ideas?

NikolausDemmel · 2014-07-15T15:15:51Z

Library/Formula/pyqt.rb

-         fname = os.environ["QMAKESPEC"]
-
-        if not os.path.dirname(fname):
-+        if not os.path.dirname(fname) or fname.startswith('unsupported'):


At least this line is already included upstream in 4.11.1

NikolausDemmel · 2014-07-18T12:16:47Z

Where this came up downstream: http://answers.ros.org/question/186630/indigo-rqt_image_view-depends-on-build_isolated/?answer=186648#post-id-186648

NikolausDemmel · 2014-07-31T12:47:45Z

The problem seems to stem from first running system python, "configure.py", *args and then system python, "./configure-ng.py", *args, which results in the duplicate symbol link issues. Not running configure.py before configure-ng.py seems to result in a successful build. The formula has the following comment on why configure.py is being run first:

  # We need to run "configure.py" so that pyqtconfig.py is generated, which
  # is needed by PyQWT (and many other PyQt interoperable implementations such
  # as the ROS GUI libs). This file is currently needed for generating build
  # files appropriate for the qmake spec that was used to build Qt. This method
  # is deprecated and will be removed with SIP v5, so we do the actual compile
  # using the newer configure-ng.py as recommended.

Does anyone have insight in whether this still applies?

- the custom QMAKESPEC hacks for Mavericks seem to not be needed any longer - creating the `pyconfig.py` file by running `python configure.py` in a different folder seems to work, but I am not sure if this is actually still needed, so for now I commented this out. I have sucessfully built and used PyQWT even without this generated `pyqtconfig.py`. It's use is deprecated anyway it seems. I don't know if not having it breaks other stuff, which is why I left the code in the formula for now, albeit commented out.

NikolausDemmel · 2014-07-31T14:19:37Z

I have a working formula. Can someone test if it builds on lion and mountain lion?

I have these notes on the changes to the formula:

the custom QMAKESPEC hacks for Mavericks seem to not be needed any longer
creating the pyconfig.py file by running python configure.py in a
different folder seems to work, but I am not sure if this is actually still
needed, so for now I commented this out. I have sucessfully built and used
PyQWT even without this generated pyqtconfig.py. It's use is deprecated
anyway it seems. I don't know if not having it breaks other stuff, which is
why I left the code in the formula for now, albeit commented out.

adamv · 2014-07-31T14:53:02Z

Error Message

failed: brew audit pyqt
Stacktrace

        pyqt:
 * Don't need 'FileUtils.' before remove_entry_secure.

NikolausDemmel · 2014-07-31T14:56:20Z

Fixed. I need to remember to run brew audit locally. That was in commented code anyway...

dakcarto · 2014-07-31T20:19:21Z

@NikolausDemmel wrote:

creating the pyconfig.py file by running python configure.py in a
different folder seems to work, but I am not sure if this is actually still
needed, so for now I commented this out. I have sucessfully built and used
PyQWT even without this generated pyqtconfig.py. It's use is deprecated
anyway it seems. I don't know if not having it breaks other stuff, which is
why I left the code in the formula for now, albeit commented out.

Please do not remove or comment-out the generation of pyqtconfig.py via configure.py until it is actually removed from the source distribution, or fully broken. There are very large projects, like QGIS, that rely upon it for generating complex Python bindings because there is currently no way to gather certain information from the new system that pyqtconfig.py readily provided.

The solution here is to work upstream with Phil Thompson to get the missing bits into configure-ng.py's resultant PYQT_CONFIGURATION, which currently only offers PYQT_CONFIGURATION["sip_flags"]. (This has been brought up before, but I couldn't find a reference.) Then, there is no reason to generate pyqtconfig.py anymore.

Good thinking to just configure to a separate directory with configure.py, since only the pure pyqtconfig.pyis needed.

NikolausDemmel · 2014-07-31T23:32:38Z

Thanks for explaining the need for pyqtconfig.py. I will uncomment it again.

NikolausDemmel · 2014-07-31T23:44:41Z

Library/Formula/pyqt.rb

-    if ENV.compiler == :clang and MacOS.version >= :mavericks
-      ENV.append "QMAKESPEC", "unsupported/macx-clang-libc++"
-    end
-


I removed the custom qt makespec since on my system it seems to compile fine with libc++; this however is with configure-ng.py; @dakcarto, do you believe the custom makespec is still needed such that the generated pyqtconfig.py is generated correctly? Do you have a way of testing it, maybe with QGIS?

jacknagel · 2014-08-01T02:46:18Z

Library/Formula/pyqt.rb

+          (lib/"python#{version}/site-packages/PyQt4").install "pyqtconfig.py"
+        end
+      ensure
+        remove_entry_secure dir
      end


Can we just make a subdirectory in the existing build tree rather than create a separate tmpdir?

I guess yes. Do you have a ruby snippet to copy everything to subdirectory (excluding the subdirectory)?

NikolausDemmel · 2014-08-01T09:37:18Z

@jensenb: It seems you originally introduced the patch concerning the macro parsing. Do you have any insight if the custom makespec is still needed (it seems qt now defaults to clang+libc++ on mavericks), and without it, is this patch still needed?

NikolausDemmel · 2014-08-01T10:04:57Z

Maybe setting the QMAKESPEC environment variable still makes a difference. On my machine the diff between the resulting pyqtconfig.py:

$ diff pyqtconfig-nomkspec.py pyqtconfig-mkspec.py -u
--- pyqtconfig-nomkspec.py  2014-08-01 11:41:22.000000000 +0200
+++ pyqtconfig-mkspec.py    2014-08-01 11:47:15.000000000 +0200
@@ -52,8 +52,8 @@
 _default_macros = {
     'AIX_SHLIB':                '',
     'AR':                       'ar cq',
-    'CC':                       'gcc',
-    'CFLAGS':                   '-pipe',
+    'CC':                       'clang',
+    'CFLAGS':                   '-pipe -mmacosx-version-min=10.7',
     'CFLAGS_APP':               '',
     'CFLAGS_CONSOLE':           '',
     'CFLAGS_DEBUG':             '-g',
@@ -73,10 +73,10 @@
     'CFLAGS_WARN_OFF':          '-w',
     'CFLAGS_WARN_ON':           '-Wall -W',
     'CHK_DIR_EXISTS':           'test -d',
-    'CONFIG':                   'qt warn_on release app_bundle incremental global_init_link_order lib_version_first plugin_no_soname link_prl',
+    'CONFIG':                   'qt warn_on release app_bundle incremental global_init_link_order lib_version_first plugin_no_soname link_prl clang_pch_style',
     'COPY':                     'cp -f',
-    'CXX':                      'g++',
-    'CXXFLAGS':                 '-pipe',
+    'CXX':                      'clang++',
+    'CXXFLAGS':                 '-pipe -stdlib=libc++ -mmacosx-version-min=10.7',
     'CXXFLAGS_APP':             '',
     'CXXFLAGS_CONSOLE':         '',
     'CXXFLAGS_DEBUG':           '-g',
@@ -103,7 +103,7 @@
     'INCDIR_OPENGL':            '/System/Library/Frameworks/OpenGL.framework/Headers   /System/Library/Frameworks/AGL.framework/Headers/',
     'INCDIR_QT':                '/usr/local/Cellar/qt/4.8.6/include',
     'INCDIR_X11':               '',
-    'LFLAGS':                   ' -headerpad_max_install_names',
+    'LFLAGS':                   ' -headerpad_max_install_names -stdlib=libc++ -mmacosx-version-min=10.7',
     'LFLAGS_CONSOLE':           '',
     'LFLAGS_CONSOLE_DLL':       '',
     'LFLAGS_DEBUG':             '',
@@ -134,8 +134,8 @@
     'LIBS_WEBKIT':              '',
     'LIBS_WINDOWS':             '',
     'LIBS_X11':                 '',
-    'LINK':                     'g++',
-    'LINK_SHLIB':               'g++',
+    'LINK':                     'clang++',
+    'LINK_SHLIB':               'clang++',
     'LINK_SHLIB_CMD':           '',
     'MAKEFILE_GENERATOR':       'UNIX',
     'MKDIR':                    'mkdir -p',

MikeMcQuaid · 2014-08-01T10:13:03Z

Whatever we decide to do here can we please add comments to every patch explaining why it's needed and where it has been submitted upstream? @dakcarto can you confirm that for the patch you need too?

jensenb · 2014-08-01T11:34:28Z

The main rational behind the macro parsing was discussed #25225 . I also submitted the issue upstream http://www.riverbankcomputing.com/pipermail/pyqt/2013-December/033537.html , but it seems only the first part of the patch has since been adopted upstream. The second part involving the stripping of the comments from the variables (for example here: https://github.com/mirror/qt/blob/4.8/mkspecs/unsupported/macx-clang-libc%2B%2B/qmake.conf) isn't necessary for building pyqt per se. It does however affect downstream projects that rely on the pyqtconfig.py for setting compiler flags, where typically contents of each variable gets passed straight away to the compiler, resulting in the compiler bailing out due to incorrect arguments.

I am not sure if you still need to set the QMAKESPEC by hand, if I recall correctly pyqt defaults to gcc if not configured otherwise, but I need to reverify this.

MikeMcQuaid · 2014-08-01T11:41:24Z

@jensenb Any bits that weren't accepted upstream should be refactored into using inreplace if they are essential for the software to work or, ideally, removed.

NikolausDemmel · 2014-08-01T11:44:47Z

@MikeMcQuaid: is the patch :DATA as used currently in that formula less acceptable than inreplace?

MikeMcQuaid · 2014-08-01T12:23:58Z

@NikolausDemmel the patch DSL should be used only for patches that have been submitted upstream pending their acceptance. If they aren't accepted upstream the patches should be removed. If it's needed for it to compile/work at all then we should use inreplace and other configuration.

jensenb · 2014-08-01T12:24:17Z

yup both configure.py and configure-ng.py default to macx-g++ on my system if the QMAKESPEC is not set, the relevant code sections:

configure.py: 1823

    if "QMAKESPEC" in list(os.environ.keys()):
        fname = os.environ["QMAKESPEC"]

        if not os.path.dirname(fname) or fname.startswith("unsupported"):
            qt_macx_spec = fname
            fname = os.path.join(qt_archdatadir, "mkspecs", fname)
    elif sys.platform == "darwin":
        # The Qt Mac binary installer defaults to xcode which we don't want.
        # Use Qt5's macx-clang if it is available, otherwise fall back to
        # macx-g++.
        fname = os.path.join(qt_archdatadir, "mkspecs", "macx-clang")
        if os.path.isdir(fname):
            qt_macx_spec = "macx-clang"
        else:
            fname = os.path.join(qt_archdatadir, "mkspecs", "macx-g++")
            qt_macx_spec = "macx-g++"

configure-ng.py:2319

    if spec == '':
        # We only bother about Linux and OS/X.
        if target_config.py_platform.startswith('linux'):
            spec = 'linux-g++'
        elif target_config.py_platform == 'darwin':
            # It doesn't matter if the default was actually clang.
            spec = 'macx-g++'

Set explicitly setting the mkspec is still necessary.

I am not sure what to do about the missing patch section upstream. I could write him again, maybe he didn't understand what it was for, and like I mentioned above, the errors only manifest themselves in further downstream projects that rely on pyqtconfig.py. But for the time being the patch is still necessary.

MikeMcQuaid · 2014-08-01T12:29:34Z

@jensenb If it wasn't accepted upstream then we should remove it. It's not Homebrew's job to fix things up that upstream won't, I'm afraid.

NikolausDemmel · 2014-08-01T12:36:14Z

@MikeMcQuaid: I have to disagree here. IMHO it's homebrew's job to do its best to provide working packages on OSX. If the maintainers of some package fails to understand the necessity for a patch or it gets lost upstream in some issue tracker for poorly maintained software, that does not sound like a good reason to intentionally break things for our users by removing an already included and working patch. There is only so much time people have to poke maintainers upstream.

jensenb · 2014-08-01T12:40:25Z

I will contact Phil and at least see what the reason was for not accepting the patch in its entirety.

MikeMcQuaid · 2014-08-01T12:42:49Z

@jensenb Thanks.
@NikolausDemmel Read this: http://en.wikipedia.org/wiki/OpenSSL#Predictable_keys_.28Debian-specific.29. Also, consider that we have 3028 packages and 5 maintainers who are busy full-time with their e.g. jobs. It's not at all sustainable for Homebrew to try and maintain our own versions of all of these. If PyQt doesn't work without patches that are rejected by upstream: stop using PyQt and use other software that works correctly.

jensenb · 2014-08-01T13:00:23Z

I stand corrected, the remaining patch hunk is no longer needed either. It seems the functionality has been included after all, just not in PyQt directly, instead it has been added to SIP: http://www.riverbankcomputing.co.uk/hg/sip/rev/044852da62d4 and officially released in SIP 4.16. So just make sure that QMAKESPEC is set appropriately and it should work fine without patches.

…re.py / configure-ng.py

NikolausDemmel · 2014-08-01T13:10:11Z

Thanks @jensenb, I removed the patch.

If someone can point out to me how to best copy the contents of a directory to a subdirectory (without that subdirectory itself) in ideomatic ruby then I can do the change that @jacknagel suggested.

If it looks good after that I can squash and rebase.

NikolausDemmel · 2014-08-04T19:58:39Z

Any further comments on the current state of this PR?

It passes tests and seems to work for me. Is this ready to be merged if I squash and rebase?

adamv · 2014-08-04T20:08:26Z

Library/Formula/pyqt.rb

+      # temporary directory and only retain the pyqtconfig.py from that.
+
+      require "tmpdir"
+      dir = Dir.mktmpdir


Can this be done in a subdirectory of the current buildpath instead of a new temporary directory?

Can someone point out to me how to best copy the contents of a directory to a subdirectory (without that subdirectory itself) in ideomatic ruby?

Ah, nevermind then; I didn't read what the code was doing very carefully.

MikeMcQuaid · 2014-08-05T11:00:43Z

Looks good to me. @jensenb any objections?

jensenb · 2014-08-05T11:42:38Z

Looks good to me.

NikolausDemmel · 2014-08-05T12:19:41Z

Thanks Mike.

dakcarto · 2014-08-05T15:55:19Z

@NikolausDemmel Thanks for taking the time to work through this update.

NikolausDemmel reviewed Jul 15, 2014
View reviewed changes

NikolausDemmel changed the title ~~Updating pyqt to 4.11.1~~ help needed: Updating pyqt to 4.11.1 Jul 18, 2014

This was referenced Jul 18, 2014

qt gui fails on recent OS X homebrew due to outdated pyqt ros-visualization/qt_gui_core#47

Closed

[meta] OS X support in indigo ros/rosdistro#4597

Closed

jacknagel added the help wanted label Jul 24, 2014

TonyLianLong mentioned this pull request Jul 30, 2014

Error while installing PyQt #31245

Closed

Updating pyqt to 4.11.1

08afb5a

pyqt: make brew audit happy

9842130

NikolausDemmel reviewed Jul 31, 2014
View reviewed changes

NikolausDemmel added 2 commits August 1, 2014 01:47

pyqt: removed commented out generation of pyqtconfig.py

04e4a49

pyqt: fix style; use " instead of '

bd02faa

jacknagel reviewed Aug 1, 2014
View reviewed changes

pyqt: reintroduce the qmake macro parsing patch

bfa28c7

pyqt: tiny cleanup

3b58ae0

NikolausDemmel added 2 commits August 1, 2014 12:05

pyqt: custom qmakespec still needed

eb01519

pyqt: fix comment

4a504bf

pyqt: removed patch (no longer needed); updated comment about configu…

c07fcad

…re.py / configure-ng.py

adamv reviewed Aug 4, 2014
View reviewed changes

pyqt: small tweaks.

548fe24

MikeMcQuaid closed this in 91d3c76 Aug 5, 2014

NikolausDemmel deleted the pyqt-4.11.1 branch August 5, 2014 12:17

Homebrew locked and limited conversation to collaborators Feb 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

help needed: Updating pyqt to 4.11.1 #30881

help needed: Updating pyqt to 4.11.1 #30881

NikolausDemmel commented Jul 15, 2014

NikolausDemmel Jul 15, 2014

NikolausDemmel commented Jul 18, 2014

NikolausDemmel commented Jul 31, 2014

NikolausDemmel commented Jul 31, 2014

adamv commented Jul 31, 2014

NikolausDemmel commented Jul 31, 2014

dakcarto commented Jul 31, 2014

NikolausDemmel commented Jul 31, 2014

NikolausDemmel Jul 31, 2014

jacknagel Aug 1, 2014

NikolausDemmel Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

jensenb commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

jensenb commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

jensenb commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

jensenb commented Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

NikolausDemmel commented Aug 4, 2014

adamv Aug 4, 2014

NikolausDemmel Aug 4, 2014

adamv Aug 4, 2014

MikeMcQuaid commented Aug 5, 2014

jensenb commented Aug 5, 2014

NikolausDemmel commented Aug 5, 2014

dakcarto commented Aug 5, 2014

help needed: Updating pyqt to 4.11.1 #30881

help needed: Updating pyqt to 4.11.1 #30881

Conversation

NikolausDemmel commented Jul 15, 2014

NikolausDemmel Jul 15, 2014

Choose a reason for hiding this comment

NikolausDemmel commented Jul 18, 2014

NikolausDemmel commented Jul 31, 2014

NikolausDemmel commented Jul 31, 2014

adamv commented Jul 31, 2014

NikolausDemmel commented Jul 31, 2014

dakcarto commented Jul 31, 2014

NikolausDemmel commented Jul 31, 2014

NikolausDemmel Jul 31, 2014

Choose a reason for hiding this comment

jacknagel Aug 1, 2014

Choose a reason for hiding this comment

NikolausDemmel Aug 1, 2014

Choose a reason for hiding this comment

NikolausDemmel commented Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

jensenb commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

jensenb commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

jensenb commented Aug 1, 2014

MikeMcQuaid commented Aug 1, 2014

jensenb commented Aug 1, 2014

NikolausDemmel commented Aug 1, 2014

NikolausDemmel commented Aug 4, 2014

adamv Aug 4, 2014

Choose a reason for hiding this comment

NikolausDemmel Aug 4, 2014

Choose a reason for hiding this comment

adamv Aug 4, 2014

Choose a reason for hiding this comment

MikeMcQuaid commented Aug 5, 2014

jensenb commented Aug 5, 2014

NikolausDemmel commented Aug 5, 2014

dakcarto commented Aug 5, 2014