Fix QT4 build for mac by sethrj · Pull Request #10944 · spack/spack

sethrj · 2019-03-20T15:36:58Z

This PR:

adds support for the "latest" QT 4,
fixes an error in the package script that manifests on mac+clang builds,
fixes the build for High Sierra,
allows more components of QT to be disabled to improve chance of success
adds "+static" variant so it can be (in theory) built statically

michaelkuhn

Looks good to me (I touched the system-* stuff recently). However, I cannot say anything about the Mac-specific parts since I do not have one to test them.

Edit: Could you please take care of the flake8 warnings?

var/spack/repos/builtin/packages/qt/package.py:11: [E302] expected 2 blank lines, found 1
var/spack/repos/builtin/packages/qt/package.py:14: [E302] expected 2 blank lines, found 1
var/spack/repos/builtin/packages/qt/package.py:250: [E128] continuation line under-indented for visual indent
var/spack/repos/builtin/packages/qt/package.py:251: [E128] continuation line under-indented for visual indent
var/spack/repos/builtin/packages/qt/package.py:252: [E128] continuation line under-indented for visual indent
var/spack/repos/builtin/packages/qt/package.py:281: [E122] continuation line missing indentation or outdented
var/spack/repos/builtin/packages/qt/package.py:282: [E122] continuation line missing indentation or outdented
var/spack/repos/builtin/packages/qt/package.py:298: [E128] continuation line under-indented for visual indent
var/spack/repos/builtin/packages/qt/package.py:299: [E128] continuation line under-indented for visual indent
var/spack/repos/builtin/packages/qt/package.py:389: [E123] closing bracket does not match indentation of opening bracket's line

var/spack/repos/builtin/packages/qt/package.py

sethrj · 2019-03-26T20:43:28Z

Great! My bad regarding the flake issues (and multiple successive CI-triggering pushes on a PR). I'll get that done ASAP.

sethrj · 2019-03-29T15:43:21Z

@michaelkuhn I had to make several more changes (the initial PR worked on the 10.13 SDK, then I added support for 10.14 SDK with clang-9, now it works with 10.14 SDK with clang-10).

I've also taken care of the flake8 warnings, sorry about that.

scheibelp

I have a few questions. I'll have a chance to look through this more on 4/1

var/spack/repos/builtin/packages/glib/package.py

var/spack/repos/builtin/packages/python/package.py

var/spack/repos/builtin/packages/qt/package.py

sethrj · 2019-04-03T16:12:31Z

@scheibelp After building some downstream code on my mac (which uses Fortran that links against wrapped QT code, ugh) I realized that there are more changes that need to be made to make the Mac QT4 install "ideal". Frankly, it would be ideal to "port" over all the patches and configuration logic used by MacPorts -- what are the licensing implications of doing this?

sethrj · 2019-04-05T17:17:24Z

@scheibelp Never mind about macports; I realized that all I needed to do to make the downstream happier was to install qt not as a framework. So I added the +framework variant: the default now is to install on all systems in lib/ and include/, and to install in Frameworks/ only when the variant is enabled. To restore the original behavior (install by default as a framework on mac) then we'll have to change the default from False to sys.platform == 'darwin'.

var/spack/repos/builtin/packages/qt/package.py

sethrj · 2019-07-28T16:13:43Z

As per the contribution guidelines I've rebased the commits I can and squashed the more complex changes. @scheibelp Would you please take another look? There are no substantial changes since I addressed your last review feedback. Thanks!

adamjstewart

There are a lot of changes in this PR, and I certainly didn't look at all of them in detail, but I'm willing to give this PR a spin and see if it fixes #12139. I think the only open PR I have that modifies Qt is #12138, but I think you already fixed this bug here, right?

var/spack/repos/builtin/packages/python/package.py

var/spack/repos/builtin/packages/qt/package.py

adamjstewart

It worked! I was able to successfully install Qt 4.8.7 on macOS 10.14.5 with Clang 10.0.1. Thanks for digging into such a complicated package!

sethrj · 2019-07-29T11:25:18Z

Excellent! Yes, sorry it ended up being rather large -- it's basically a rewrite of the package definition for QT 4.

scheibelp · 2019-07-30T19:21:36Z

Apologies but when I merged #12138 it generated a conflict here. Do you think you'll have an opportunity to sync it with develop in the near future?

sethrj · 2019-07-30T23:08:47Z

@scheibelp No worries, rebased and fixed.

scheibelp · 2019-07-31T00:20:30Z

I am not seeing the changes we discussed at #10944 (comment) - did something change where you thought that was no longer necessary? To expand on #10944 (comment), I think it would be more straightforward to explicitly disable cocoa by replacing

if ac_fn_c_try_compile "$LINENO"; then :
  glib_have_cocoa=yes
fi

with

glib_have_cocoa=no

(since it appears the test fails even for compilers that support cocoa)

There's no reason not to store the 'minor' mac version number or to use the convenience methods of the `Version` object.

The _localemodule.c file fails to link due to missing symbols from libintl, which is provided by `gettext`. Adding `gettext` as an explicit dependency required an update to specify the names of the libraries it builds.

- Add QT 4.8.7 version - Add option to disable QT build as a framework on mac - Add patches for Clang 10 and OSX SDK up to 10.15 - Add variant to disable QT4 tools - Add variant to disable SSL support - Break QT installation into configure/build/install phases

- Moved dependency declaration in python package - As per a previous discussion about libuuid, removing the 'conflict' for untested versions of MacOS. - Fixed typo and clarified conflict message

In accordance with "innocent until proven guilty" policy

sethrj · 2019-07-31T11:12:22Z

Sorry @scheibelp! I lost track of your previous comment. You're correct, even for mac clang it appears that it's still configuring with

checking for Mac OS X Cocoa support... no

Since that change to glib was because I was playing with building out a macOS GCC8 toolchain, and I no longer have that configuration set up, I'm just reverting the changes to glib. (Having these unrelated changes isn't exactly in line with the newish spack "one PR, one package" policy either.)

scheibelp

I have a few additional requests/questions which I think are minor.

var/spack/repos/builtin/packages/qt/package.py

scheibelp · 2019-08-01T17:22:42Z

Thanks!

sethrj · 2019-08-01T17:50:55Z

Glad to! Thanks for the merge :)

This updates the Spack QT package to enable building qt version 4 on MacOS. This includes the following changes to the qt package: * add version 4.8.7 * add option to build with or without shared libs * add options to disable tools, ssl, sql, and freetype support * add qt4-tools patch when building qt@4+tools * add option to build as a framework (only available on MacOS) * replace qt4-el-capitan patch with qt4-mac patch (which includes the edits from qt4-el-capitan) * apply qt4-pcre-include-conflict.patch only for version 4.8.6 (rather than all 4.x versions) * apply qt4-gcc-and-webkit.patch for 4.x versions before 4.8.7 and create a separate qt4-gcc-and-webkit-487.patch for version 4.8.7 * update patch function for qt@4 on MacOS to update configure variables relevant to Spack (e.g. PREFIX) * add option to build freetype with Spack, as a vendored dependency of QT, or not at all (default is to build with Spack) This includes the following edits outside of the qt package: * Update MacOS version utility function to return all parts of the Mac version (rather than just the first two) * gettext package: implement "libs" * python package: add gettext as a dependency

michaelkuhn reviewed Mar 20, 2019

View reviewed changes

scheibelp reviewed Mar 26, 2019

View reviewed changes

var/spack/repos/builtin/packages/qt/package.py Outdated Show resolved Hide resolved

scheibelp reviewed Mar 30, 2019

View reviewed changes

var/spack/repos/builtin/packages/glib/package.py Outdated Show resolved Hide resolved

var/spack/repos/builtin/packages/python/package.py Outdated Show resolved Hide resolved

var/spack/repos/builtin/packages/qt/package.py Show resolved Hide resolved

sethrj mentioned this pull request Apr 5, 2019

Installation issue: curl #9958

Closed

scheibelp reviewed Apr 17, 2019

View reviewed changes

var/spack/repos/builtin/packages/qt/package.py Outdated Show resolved Hide resolved

sethrj force-pushed the fix-qt4-mac branch from 4da5703 to bcb1c24 Compare July 28, 2019 16:12

sethrj mentioned this pull request Jul 29, 2019

Installation issue: Qt 4 on macOS #12139

Closed

sethrj force-pushed the fix-qt4-mac branch from bcb1c24 to 322da23 Compare July 29, 2019 02:21

adamjstewart reviewed Jul 29, 2019

View reviewed changes

var/spack/repos/builtin/packages/python/package.py Outdated Show resolved Hide resolved

var/spack/repos/builtin/packages/qt/package.py Outdated Show resolved Hide resolved

adamjstewart added macOS patch qt labels Jul 29, 2019

adamjstewart approved these changes Jul 29, 2019

View reviewed changes

sethrj force-pushed the fix-qt4-mac branch from dbb732b to db54727 Compare July 30, 2019 23:07

sethrj added 5 commits July 31, 2019 07:08

Improve mac_os version identifier

60f1949

There's no reason not to store the 'minor' mac version number or to use the convenience methods of the `Version` object.

Add Python dependency on gettext

593841d

The _localemodule.c file fails to link due to missing symbols from libintl, which is provided by `gettext`. Adding `gettext` as an explicit dependency required an update to specify the names of the libraries it builds.

Address reviewer feedback

22485a7

- Moved dependency declaration in python package - As per a previous discussion about libuuid, removing the 'conflict' for untested versions of MacOS. - Fixed typo and clarified conflict message

Remove untested 'conflicts'

38c724e

In accordance with "innocent until proven guilty" policy

sethrj force-pushed the fix-qt4-mac branch from db54727 to 38c724e Compare July 31, 2019 11:09

scheibelp requested changes Aug 1, 2019

View reviewed changes

sethrj added 4 commits August 1, 2019 09:00

Remove stale patch file

21840a6

Re-enable webkit patch for 4.8.5

0742d5d

Update comment

e388c03

Change 'system' variant option to 'spack'

ab91ffb

scheibelp merged commit 46027be into spack:develop Aug 1, 2019

sethrj deleted the fix-qt4-mac branch August 1, 2019 17:50

scheibelp mentioned this pull request Nov 8, 2019

ROOT + Python/libxml2: Maximum Recursion Depth #13637

Closed

adamjstewart mentioned this pull request Dec 23, 2022

Qt: patch additional config file #34682

Merged

Conversation

sethrj commented Mar 20, 2019 • edited by adamjstewart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelkuhn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sethrj commented Mar 26, 2019

Uh oh!

sethrj commented Mar 29, 2019

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sethrj commented Apr 3, 2019

Uh oh!

sethrj commented Apr 5, 2019

Uh oh!

Uh oh!

sethrj commented Jul 28, 2019

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

sethrj commented Jul 29, 2019

Uh oh!

scheibelp commented Jul 30, 2019

Uh oh!

sethrj commented Jul 30, 2019

Uh oh!

scheibelp commented Jul 31, 2019

Uh oh!

sethrj commented Jul 31, 2019

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scheibelp commented Aug 1, 2019

Uh oh!

sethrj commented Aug 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sethrj commented Mar 20, 2019 •

edited by adamjstewart

Loading

michaelkuhn left a comment •

edited

Loading