Patch to add PyQt5 to ports collection

[madpilot]

Summary

The following patch adds PyQt5 ports.

There are rough edges and some polish is needed. To get PyQt5 correctly install modularized I had to hack the configure script a little, these patches could be too aggressive, if anyone has a better solution I'll be listening.

I made this patch with the intent to get the deskutils/calibre port updated to 2.x. I have it running correctly on my machine with patchset. If someone wants the calibre port patch to test it I'll be happy to put it online.

I have ported most modules but left a few behind. I can port those too, just need a little more time to do that.

In the patches I have made the API option non default, because I have done no work with qscintilla. The present port is qt4 only and does not play well with PyQt5.

Since PyQt5 ports are based on the existing PyQt4 ones I left kde@ as maintainer. So I'm looking for comments help and approval(once the patch is more polished) from the kde team.

Test Plan

exp-run: successfully completed: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=204672

[madpilot]

Updated patch after applying the fixes discovered via the exp run.

Thanks to antoine for performing the test!

[rakuco]

Phew, I can't believe I've managed to look at the entire diff :-)

First of all, big thanks for this contribution and all the hard work you've put into it!

I've added several inline comments to the files, but there are some general remarks that fit better here:

• PYQT_DIST=yes with QT_NONSTANDARD=yes is redundant. The former sets the latter.
• Ditto for PYQT_DIST=yes and setting PORTVERSION in the port.
• I think PYQT_DIST=yes should imply USES=python, as was the case in bsd.pyqt.mk.
• The PyQt5 ports all have USES=qmake instead of USE_QT5=qmake_build. This is the opposite of what the PyQt4 ports do. Is that intentional?
• qscintilla2-qt{4,5}-designerplugin should probably be called qscintilla2-designerplugin-qt{4,5}, as most ports (except for the PyQt ones) that need the -qt suffix put it at the end.
• All PyQt4 ports need PORTREVISION bumps because the sip files are installed into a different directory.
• I think Qt4's libqscintilla2.so should not be renamed. It would avoid several changes and PORTREVISION bumps in this patch. We could just rename Qt5's version to libqscintilla2-qt5.so as there's nothing depending on it yet.

With all the above said, there are multiple big changes in this patch, so I'd like to land them separately to minimize the impact and make sure things do not break. What do you think of:

1. Landing Uses/pyqt.mk without PyQt5 support and without the change to SIPDIR. This should cause no changes to end users whatsoever.
2. Landing the PyQt4/SIP/QScintilla version updates (which usually require a PORTREVISION bump in things that depend on qscintilla) together with the SIPDIR path changes, as the packages/ports will be rebuilt anyway.
3. Landing the PyQt5 ports and changes to Uses/pyqt.mk.

Since you'll be away until the middle of December, I (or kde@ as a whole) can take care of these if you agree with the proposals and comments.

Mk/Uses/pyqt.mk

3 ↗

(On Diff #10429)

PyQt is preferred over py-qt.

4 ↗

(On Diff #10429)

Style nitpick: wrong indentation.

8–10 ↗

(On Diff #10429)

Style nitpick: these should be aligned.

12 ↗

(On Diff #10429)

Style nitpick: the "FreeBSD" in email addresses are usually spelled "FreeBSD", not "freebsd".

14 ↗

(On Diff #10429)

PyQt is preferred over py-qt.

16–17 ↗

(On Diff #10429)

Style nitpick: indentation.

23 ↗

(On Diff #10429)

Style nitpicks:

• Please start the sentence with a capital letter and end it with a period.
• Please call it "PyQt" instead of "py-qt".

24 ↗

(On Diff #10429)

If "dummy" is only supposed to be used by sip, I think it makes more sense to just call it "sip" and make it explicit that other ports should not use this value.

27 ↗

(On Diff #10429)

How about "USES=pyqt needs a version number (valid values: ${_PYQT_SUPPORTED})"?

30 ↗

(On Diff #10429)

Same thing about capitalization and PyQt/py-qt.

42 ↗

(On Diff #10429)

Is the comment there an oversight?

43 ↗

(On Diff #10429)

Is .error really necessary besides IGNORE?

72 ↗

(On Diff #10429)

Capitalization.

76 ↗

(On Diff #10429)

Capitalization.

80 ↗

(On Diff #10429)

Capitalization.

84 ↗

(On Diff #10429)

Capitalization.

87 ↗

(On Diff #10429)

"prel" sounds confusing. How about PYQT_PY_RELNAME?

188–189 ↗

(On Diff #10429)

If you export these outside PYQT_DIST, it's a good idea to document them in the comments in the beginning.

201 ↗

(On Diff #10429)

This is actually called OPTIONS_FILE. In any case, are you sure this is necessary? Without this patch, make -C devel/py-qt4-core -V OPTIONS_FILE already returns "/var/db/ports/devel_py-qt4-core/options", which looks sensible.

209 ↗

(On Diff #10429)

Extra empty line.

210 ↗

(On Diff #10429)

You can say "PyQt" instead of "PyQt4" here. PyQt5's configure.py does the same thing. It also accepts a new option --no-python-dbus, but it's easier to just keep one workaround for both versions.

222 ↗

(On Diff #10429)

Wrong variable in the comment.

224 ↗

(On Diff #10429)

Capitalization.

databases/py-qt5-sql/Makefile

1

Not necessary.

devel/py-qt5-core/Makefile

41

Extra empty line.

devel/py-qt5-core/files/patch-configure.py

7–8

PyQt5's configure.py is similar to PyQt4's configure-ng.py, which I tried using before but could not (see http://src.mouf.net/area51/revision/?rev=10132 and http://src.mouf.net/area51/revision/?rev=10185).

At the time, I did not have to change this part and everything worked fine. Are you sure this hunk is necessary? As far as I can see, at least QtCore will always be present when any PyQt port is being built.

31–33

Similarly, are you sure this part is necessary?

41

This doesn't look correct, mname here is just the latest item in the loop above. In PyQt4 we just remove this part of the script, can't it be done for PyQt5 too?

59–62

Instead of checking for "QtCore" again, you can indent this region and piggyback on the check above.

69–71

Ditto.

83–84

Same thing about the weird usage of mname. What I did in that area51 revision I mentioned was this:

if target_config.qsci_api:
  api_list = ' '.join(['%s.api' % m for m in target_config.pyqt_modules])
  out_f.write(... % (api_list, target_config.qsci_api_dir))

92

Not sure if this is necessary.

103–106

What is this part for?

devel/py-qt5-designer/Makefile

1

Not necessary.

devel/py-qt5-qscintilla2/Makefile

28 ↗

(On Diff #10429)

Likely unneeded (and wrong variable name).

devel/py-qt5-test/Makefile

1

Not necessary.

devel/py-sip/Makefile

16 ↗

(On Diff #10429)

Doesn't concurrent break DATADIR and DOCSDIR below?

devel/qscintilla2-qt5/Makefile

5 ↗

(On Diff #10429)

Needs to be removed.

15 ↗

(On Diff #10429)

Qt4Qt5/qscintilla.pro has this:

greaterThan(QT_MAJOR_VERSION, 4) {
	QT += widgets printsupport

So USE_PYQT5 looks wrong. The designer part is only needed by the designer plugin port.

31–45 ↗

(On Diff #10429)

I think these changes are quite intrusive and should be put in an actual patch file for visibility. By the way, Fedora's patch looks smaller and achieves the same result: http://pkgs.fedoraproject.org/cgit/qscintilla.git/tree/QScintilla-gpl-2.9.1-qt5.patch

devel/qscintilla2-qt5/files/patch-Qt4Qt5_qscintilla.pro

7–8 ↗

(On Diff #10429)

What does this do?

graphics/py-qt5-svg/Makefile

1

Not necessary.

5

Needs to be removed.

lang/py-qt5-qml/Makefile

22

Wrong description.

misc/py-qt4-demo/Makefile

21 ↗

(On Diff #10429)

?

misc/py-qt4-doc/pkg-plist

2 ↗

(On Diff #10429)

Wrong sorting.

misc/py-qt5-demo/Makefile

1

Not necessary.

21

?

misc/py-qt5-demo/files/patch-examples__designer__plugins__plugins.py

18–19 ↗

(On Diff #10429)

Qt5's Designer is called just designer, so this patch is not needed.

misc/py-qt5-doc/Makefile

1

Not necessary.

multimedia/py-qt5-multimedia/Makefile

1

Not necessary.

net/py-qt5-network/Makefile

21

Wrong description.

print/py-qt5-printsupport/Makefile

22

Wrong module description.

[madpilot]

Thanks for your review and comments.

I have no objection to the staged commit plan for this patch.

I also have no to the other points you rise.

Regarding the USES=qmake issue, I can't remember anymore if it was an oversight or intentional, but a quick test I just did shows the ports install files to the system directories instead of going into the staging directory. I'm sure this can be fixed to work correctly also with qmake_build, but I have no time to investigate this issue further right now.

Regarding the patch devel/py-qt5-core/files/patch-configure.py, I replied to your comments explaining why I did what I did, I acted to the best of my understanding of how that file works(I'm no python guru). I could have been influenced by a lack of understanding, so if you know better feel free too modify my work there.

I'm going to add a new patch addressing most inline comments.

I'm sorry I will not be able to help more till mid-December, and am unable to contribute more than the updated patch I'll attach shortly till then, feel free to modify anything you feel should be modified obviously.

Mk/Uses/pyqt.mk

3 ↗

(On Diff #10429)

I reworked this header to be more similar to the ones in the other Uses files.

8–10 ↗

(On Diff #10429)

These look aligned in my editor using 8 space tabs. They are aligned with a single tab, as most other Uses files, should I use spaces instead?

16–17 ↗

(On Diff #10429)

Again a tab vs space issue, this does look ok in vi.

24 ↗

(On Diff #10429)

I reworded the comment and renamed the option, should be ok now.

devel/py-qt5-core/files/patch-configure.py

7–8

Without this all module ports would try to install the core parts, causing conflicts (or stage orphans).

This is the same reason of various of the following changes.

31–33

These are needed to avoid the various modules to all install the same files (pylupdate, pyrcc and pyuic), which should instead be installed by just Core or Xml.

41

Without this change all modules ended up installing their api file as PyQt5.api generating conflicting plists.Removing this part would leave the port without the .api file, I think.

Since our build system builds modules one by one in their own port, the last item in the loop happens to be the right one. I understand this is not really the cleanest way, it does get it working though.

I was unable to find a better solution.

83–84

But this script is putting all definitions in PyQt5.api, and then installing that. I needed to force it to use a different name for each module, so this change works in accord to the one referenced int he comment for line 41 above.

I was unable to find a better solution short of rewriting a big part of this configure script to properly use different names for each module.

My impression is that this new version of PyQt is made only for monolitic builds.

92

Without this change all modules install pyuic5 files during staging, I then end up with orphans or, if I include those in plist, conflicting ports.

103–106

Removing this was needed to make it work modularized.

This part adds -x <module> flags to the sip_flags for each module not explicitly enabled such flags end up in the PYQT_CONFIGURATION module variable. Since we build the modules in isolation -x lines get into that variable ports use to add options to the sip command line causing strange failures due to missing modules even if they are installed.

Only solution I could find was just prevent this code from adding those -x options.

devel/py-sip/Makefile

16 ↗

(On Diff #10429)

Good point.
I guess DATADIR and DOCSDIR definitions below can just be removed, they are being overridden anyway.

devel/qscintilla2-qt5/Makefile

15 ↗

(On Diff #10429)

Adding these causes a circular dependency.

Not sure what's the best way to avoid this.

31–45 ↗

(On Diff #10429)

I integrated the patch you suggest from fedora, some parts need to put into those dynamic changes, depending on prefix, so I left those in the Makefile.

tcberner worked most on this file so he could have better insight.

devel/qscintilla2-qt5/files/patch-Qt4Qt5_qscintilla.pro

7–8 ↗

(On Diff #10429)

This comes from the patches tcberner sent me, so he could have a better explanation.

This gets filled in by the port Makefile via reinplace, I think it's needed to have concurrent qscintilla2-qt4 and qscintilla2-qt5 installations.

misc/py-qt4-demo/Makefile

21 ↗

(On Diff #10429)

This one slipped in by mistake I think. I'm just removing this (and other similar ones)

[madpilot]

Updated patch addressing inline comments from rakuco.

[rakuco]

devel/py-qt5-core/files/patch-configure.py

8–9

I've removed all these checks from my local version of the patch and all PyQt5 ports still built and passed all staging/plist checks.

This function, check_modules(), is only called if one does not pass --enable to the configuration script (--enable works as a white list). Since we always pass that in our ports, this change ends up having no effect.

32–34

I was referring to the pyuic5 part specifically: generate_pyuic5_wrapper just generates a two-line shell script that invokes the pyuic5 Python module.

It is not added to any .pro file, and the hunk below changes the build system so that it is only installed if we are building QtCore.

42

The old area51 solution of just removing this entire check and its block of code also works: the module-specific .api files are not removed, PyQt5.api is never generated and then the hunk below which references mname just needs to do the loop I suggested so that the module-specific .api files get installed.

104–107

This part adds -x <module> flags to the sip_flags for each module not explicitly enabled such flags end up in the PYQT_CONFIGURATION module variable.

I don't think so. pyqt_disabled_features appears to be set in TargetConfiguration.from_introspection() by parsing the output of a Qt program that checks for _Qt_ (not PyQt) features that have been disabled in Qt itself (e.g. whether OpenGL, accessibility or SSL support has been disabled).

The only surprising thing is that devel/qt5-core's qconfig.h defines QT_NO_SSL because we build it with -no-openssl, but that should probably be solved there.

A build without this change worked fine for all PyQt5 ports.

devel/qscintilla2-qt5/Makefile

16 ↗

(On Diff #10612)

Sorry, I meant USE_QT5, not USE_PYQT. I think your current change looks fine (xml is not needed).

32–46 ↗

(On Diff #10612)

some parts need to put into those dynamic changes, depending on prefix, so I left those in the Makefile.

I don't understand why these prefix-dependent changes are needed. Without them the files still get installed into the intended directories. Is that because it does not respect $PREFIX and always installs into Qt's existing directories? I'm not sure fixing a crappy build system like qmake that way is worth the effort.

[rakuco]

devel/qscintilla2-qt5/Makefile

32–46 ↗

(On Diff #10612)

By the way, we'll likely need QScintilla 2.9.1 for the mkspec file to be installed into the right location by default.

[rakuco]

Regarding the USES=qmake issue, I can't remember anymore if it was an oversight or intentional, but a quick test I just did shows the ports install files to the system directories instead of going into the staging directory.

I got what's going on. It's something that also bit me when I tried to make Qt4 use configure-ng.py: both configure-ng.py and Qt5's configure.py generate .pro files and then invoke qmake to generate the Makefiles. qmake uses INSTALL_ROOT instead of DESTDIR in the Makefiles, so we just need to set DESTDIRNAME in Uses/pyqt.mk.

[rakuco]

PyQt5 has finally landed into the ports tree in rP403693.

Guido, thank you very much for working on this! I think I don't have the permissions to close this review request, so please make sure you do that.

[rakuco]

Accept revision for it to be closed.

[rakuco]

Closing now that PyQt5 has landed.