devel/py-pyface: Update to 6.1.2 - Introduce new default option QT5 [1] to let the port make use of the Qt5 toolkit. (Support for the wxPython toolkit is also available but was left out intentionally because it isn't available for Python 3, yet.) - Also add a "do-test" target to make future QA easier while I'm here. Changelog since 6.1.2: https://github.com/enthought/pyface/blob/6.1.2/CHANGES.txt PR: 239576 Submitted by: vladimir.chukharev@gmail.com (maintainer) Approved by: vladimir.chukharev@gmail.com (maintainer) [1]
Details
Details
- Reviewers
Vladimir.Chukharev_gmail.com koobs - Group Reviewers
Python - Commits
- rP538305: devel/py-pyface: Update to 6.1.2
- poudriere -> OK (11.2-, 11.3-, 12.0-RELEASE, 13.0-CURRENT@r352110 amd64 + i386) for each py27 + py36 flavor
- portlint -> OK (one false positive about " When USES=pyqt:5 is defined, you must also define USE_PYQT=xxxx)
- make test -> OK (for py27 + py36, requires graphical display)
Everything is fine so far except the special case with the py-qt5-* ports that are used as TEST_DEPENDS. The question is here if the usage of the TEST_DEPENDS is acceptable?
There seems to be no other way to use some of the py-qt5-* ports only for testing purposes, e.g. with USE_PYQT=package_test.
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| devel/py-pyface/Makefile | ||
|---|---|---|
| 41–53 ↗ | (On Diff #62561) | Should all of this be scoped to only particular PYTHON_VER's ? |
Comment Actions
The patch looks good to me. And I hope this was clear in the the discussion in PR 239576, though that was not stated explicitly there.
IMHO, the comment by @koobs can be marked 'done', as is the reply marked.
I filed PR 240445 for the portlint false positive, with a proposed patch.
Regarding the questions about py-qt5-*, USE_PYQT and TEST_DEPENDS, hardly it can be solved within this review, IMHO.
I accept the revision, in hope that I do not violate some rules by doing so.
Comment Actions
Uses/pyqt.mk should grow either standard support for :[build,run,test] args, or if it cant in its current state (it currently supports a <component>[_build|_run] format), then it should support <component>[_test] too, allowing those dependencies to be set only as test dependencies
Given the change to Uses/pyqt.mk to add this support is probably quite small, I'd look to add support with this diff, as a good candidate/use-case/warrant/example for its use
Add kde@ to blocking reviewers for that change
Comment Actions
Personally I'd go with changing the pyqt format to the more consistently used in the tree <component>:<target> format but that's a more invasive change requiring a substantial amount of existing port churn. It would be good as a second step evolution of pyqt.mk to bring it in line with the rest of the framework
Comment Actions
There was never a need so far for this, but I don't really see a reason why this could not be added. I'll look into adding support for ${component}_test later today.
| devel/py-pyface/Makefile | ||
|---|---|---|
| 41–53 ↗ | (On Diff #62561) | For reference, I'm working on wxPython4: https://reviews.freebsd.org/D21915 |
Comment Actions
@tcberner: Thank you for D23241 !
I changed this diff slightly to make use of the updated Mk/Uses/pyqt.mk once it lands:
- py-qt5-* TEST_DEPENDS were converted into *_test for USE_PYQT
- Updated comment about the current situation regarding wxPython 4 as pointed out by @lbartoletti
The test results by using make test for the py37 flavor were fine:
Ran 571 tests in 45.598s OK (SKIP=61)
Using make -V TEST_DEPENDS also yields the expected results:
py37-mock>0:devel/py-mock@py37 py37-nose>0:devel/py-nose@py37 py37-numpy>0:math/py-numpy@py37 py37-pygments>0:textproc/py-pygments@py37 /usr/local/bin/python3.7:lang/python37 py37-qt5-network>=5.13.1:net/py-qt5-network@py37 py37-qt5-opengl>=5.13.1:x11/py-qt5-opengl@py37 py37-qt5-test>=5.13.1:devel/py-qt5-test@py37 py37-qt5-webkit>=5.13.1:www/py-qt5-webkit@py37 py37-qt5-webkitwidgets>=5.13.1:www/py-qt5-webkitwidgets@py37
Comment Actions
Looks OK from my limited understanding of the context / semantics of the change. Over to @kai