I just learned that it is possible to add support for python 3.7 by applying this patch from upstream https://code.qt.io/cgit/pyside/pyside-setup.git/commit/?h=5.11&id=4a32f9d00b043b7255b590b95e9b35e9de44c4ed

I already updated and tested (locally) the formula to build against both versions of python (2.7 and 3.7). If you want, I can edit this PR, otherwise 3.7 compatibility can wait until the next upstream release (5.11.1).

After some discussions with the Qt for Python team, I believe that it is probably better to remove the patch (they will not fix it upstream and certainly not in the current way) and use inreplace in the formula to fix the building issue in "brew install", as suggested in the Formula Cookbook.

I just updated the PR accordingly. The automatic check still fails because "llvm" is used during the build (brew audit --new-formula), but that is an explicit requirement of the binding (see here https://wiki.qt.io/Qt_for_Python/GettingStarted for further information, I noted that you also realized that for qt here #29415).

This commit does not include the patch for python 3.7 as I would like to know your opinion about that before.

Unfortunately if this cannot build without an inreplace like that, we'll need to pass on the formula.

I do not understand, may I ask why? The purpose of the inreplace is just adding ${HOMEBREW_PREFIX}/opt to the paths searched by shiboken2 during the build. I have seen a lot of similar uses of inreplace in other formulas.

Anyway, a potential alternative could be to skip the QtUiTools module by passing the optional argument --skip-modules=UiTools to setup.py . This will sacrifice a module and still allow to have a pyside formula.

@albertosottile because in a brand new formula, the build system should be able to find the dependencies on its own, or the formula should be able to specify their locations by passing parameters to configure/setup.py/make/whatever, not by using string replacements. Which dependencies is the build not finding on its own?

It's not about finding dependencies. It's about the fact that brew passes dependency include paths as system paths (-isystem vs -i), specifically for Qt includes as well. And this breaks the pyside2 c++ parser which ignores system paths for two reasons (speed, and circumventing clang issues parsing the standard library on certain platforms).

The patch essentially forces the c++ parser to not ignore the qt brew headers.

Does it build outside of Homebrew against Homebrew's Qt? I still don't understand exactly where the problem lies. Is it a problem with Homebrew's Qt or a problem with the build environment brew provides? If the former, is there a change we can consider making to Homebrew's Qt? If the latter, is there a change we can consider making to https://github.com/Homebrew/brew?

I'd like to see a complete build without patching or string replacements here before we reinstate PySide given its removal in #5725

I can confirm that it does build against homebrew’s Qt, so my guess is that the issue is in the build environment.

It does build outside of homebrew against homebrew's qt.
It's a problem with the latter (the build environment brew provides).

Like I said previously, the c++ parser used by pyside (which uses libclang) skips parsing of system headers for the above stated reasons.

Because brew sets up a build environment that passes qt include headers as -isystem to the parser, it skips the qt include headers, and the bindings generator can't generate qt bindings and the build fails.

The workaround patch above simply re-enables the c++ parser to not consider brew provided include headers as system headers (which includes the qt ones).

So it sounds like upstream could provide a way to specify

--with-qt=#{Formula["qt"].opt_prefix}

that would allow it to build in brew without patching in the formula.

It already does. You can specify --qmake=/path/to/bin/qmake or just add the qt bin folder to path.
The necessary thing here is for brew not to set up its -isystem flag for qt.

If you know that it's the path to the specified Qt, then you could choose not to ignore the -isystem parameter.

I'm talking about the cpp -isystem flags set up by /usr/local/Homebrew/Library/Homebrew/shims/mac/super/clang

pyside's c++ parser calls "clang -E -x c++ - -v" which gives a list of include paths that the parser considers system include paths. And that list contains the qt include paths (as well as the other formula dependencies), because the above shim sets that up.

So you could filter the user-specified Qt out of that list since it's clearly a false positive.

If you know that it's the path to the specified Qt, then you could choose not to ignore the -isystem parameter.

Perhaps. But that seems a bit counter-intuitive and would also be a slight performance loss, because we need to do that check for every header, and there are a lot of headers.
Simply calling clang_Location_isInSystemHeader() is much faster than constantly also checking it against a string prefix.

So you could filter the user-specified Qt out of that list since it's clearly a false positive.

Yes I suppose that would be an acceptable way of doing it.

Also it may work to just invokve clang as xcrun clang -E -x c++ - -v for that part.

That's actually a far less-reaching approach.
Thanks for the hint.

On second thought using xcrun might not be the best solution, because it hardcodes the usage of an xcode-provided clang.

Most people probably don't use a custom clang, but unnecessarily hardcoding it seems wrong.
And it could also hinder those who use something akin to icecream (icecc) / ccache for clang (although i'm not aware if such a thing is supported).

@placinta:

     Not necessarily; Xcode has supported using custom toolchains since version 7.3. See the section on 'Alternative Toolchain Support' in Xcode v7.3's release notes, Apple's documentation on 'Using Alternative Toolchains' here, and this blog post by Erica Sadun for more details. Note that you may have to set the DEVELOPER_DIR environment variable described the section on environment variables in man xcode-select (and possibly other man pages related to Xcode's command-line tooling as well) depending on environment conditions.

Sure, but that still doesn't cover the use-case of simply putting a custom clang in your PATH, which is the first thing that would come to my mind, because I didn't know about custom toolchain support in XCODE, and setting PATH is more UNIX-y anyway.

@placinta:

     I agree that it's somewhat more obscure and complicated than what anyone would like, but, hey, it works. It would be nice (and make a little bit more sense) if Xcode could detect toolchain files installed alongside compilers in your $PATH and prompt to install them into /Library/Developer/Toolchains/ for you automatically, though, even going so far as to activate them directly after it did that, but that'd probably take filing a Radar issue with Apple.

The superenv build issue has been fixed upstream: http://code.qt.io/cgit/pyside/pyside-setup.git/commit/?h=5.11&id=5662706937bd6a1449538539e3a503c6cbc45399. The fix will be included in the next release (5.11.1).

In the meantime, I updated the formula to patch this issue using the upstream patch. The patch needs to be slightly edited to apply it directly on 5.11.0, so at this moment is downloaded from a custom URL. If you thinks this is mergeable, I can prepare a PR on https://github.com/Homebrew/formula-patches for it.

The formula also uses a second patch (taken from upstream) to allow compatibility with Python 3.7. So, the formula now builds pyside for both python and python@2 (both listed as recommended dependencies as in pyqt).

Both those patches will no longer be necessary once 5.11.1 is released. In summary, you can merge the PR as it is or wait for the release of 5.11.1, remove both the patches, and then merge.

@ilovezfs I made some changes according to your suggestions. Please have a look at this when you have time.

@albertosottile I suggest we use one array, two times in the test, instead of copy-pasting the array.

(Look at test block in opencv)

@ilovezfs The test section was reformatted to use only one array and the .pth files were removed since they were unnecessary, as you requested.

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @albertosottile!

Hi guys, just would like to let you know that this seems to cause the following minor (?) issue: when running brew update && brew upgrade the dependencies seem to be taken from the old formula and the upgrade fails:

$ ==> Upgrading 2 outdated packages, with result:
pyside 1.2.2_1 -> 5.11.0, exercism 2.4.1 -> 3.0.4
Error: No available formula with the name "shiboken" (dependency of pyside)

The workaround is to uninstall pyside and install it again:

$ brew uninstall pyside
Uninstalling /usr/local/Cellar/pyside/1.2.2_1... (91 files, 23.2MB)
$ brew install pyside
==> Downloading https://homebrew.bintray.com/bottles/pyside-5.11.0.high_sierra.bottle.tar.gz
==> Downloading from https://akamai.bintray.com/8b/8b9e5689c3a267f36d88cf2933a56cb9f93d4537edc3daafac89774846abf570?__gda__=exp=1531726400~hmac=69c4c42cf649d002468558cbf26188dedd4dedd813e3c927801f666872089e71&response-content-dispositio
######################################################################## 100.0%
==> Pouring pyside-5.11.0.high_sierra.bottle.tar.gz
🍺  /usr/local/Cellar/pyside/5.11.0: 453 files, 78MB

@d1vanov thanks for the heads up. I've reported this as a brew issue here: Homebrew/brew#4482

@ilovezfs PySide2 5.11.1 has just been released:
https://download.qt.io/official_releases/QtForPython/pyside2/PySide2-5.11.1-src/pyside-setup-everywhere-src-5.11.1.tar.xz

You could just bump this formula and remove the two included patches but, it would probably be better to consider reviewing this PR for libclang first. If merged, the bumped formula for PySide2 could be linked to libclang instead of llvm.

As I mentioned before, the PR for libclang would eliminate the need of downloading the full llvm to run PySide2 (280 MB vs 1.3 GB) and it would also fix a linking conflict between system and llvm libraries.