-
-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update for SIP5 build #26
Conversation
There are a number of problems with this pull request:
For these reasons I'll reject this pull request, sorry. |
Ok, leave for the future. |
@Ghostkeeper - I think there is some confusion between the PyQt5 module name (which is just a namespace) and PyQt5 itself. Using 'PyQt5.sip' as namespace does not add any build or runtime dependency. Having an non-namespaced How about adding a CMake option to specify the full module name, defaulting to "sip"? Note, most downstreams already default to 'PyQt5.sip' today, patching libArcus, libSavitar and python-libnest2d. For reference, see Ultimaker/libArcus#76 |
OUTPUT_STRIP_TRAILING_WHITESPACE | ||
) | ||
if(NOT ${_process_status} EQUAL 0) | ||
message(FATAL_ERROR "Failed to get sip.h. Error: ${_process_output}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "Failed to generate sip.h"
@@ -68,16 +68,26 @@ endif() | |||
|
|||
get_filename_component(_python_binary_path ${Python3_EXECUTABLE} DIRECTORY) | |||
|
|||
find_program(SIP_EXECUTABLE sip | |||
find_program(SIP_EXECUTABLE sip5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be
find_program(SIP_EXECUTABLE
NAMES sip sip4 sip5
...
HINTS ${CMAKE_PREFIX_PATH}/bin ${CMAKE_INSTALL_PATH}/bin ${_python_binary_path} ${Python3_SITELIB}/PyQt5 | ||
) | ||
|
||
execute_process( | ||
COMMAND sip-module --sip-h --target-dir ${CMAKE_CURRENT_BINARY_DIR} PyQt5.sip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module name should be an CMAKE_OPTION, defaulting to 'sip' for legacy compat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, you should not rebuild the existing PyQt5.sip module and conflict with an already installed one. If you really want to build your own (why?), use a unique name.
@@ -94,7 +94,7 @@ | |||
for (int i = 0; i < (int)sipCpp->size(); ++i) | |||
{ | |||
SceneNode *cpp = new SceneNode(*sipCpp->at(i)); | |||
PyObject *pobj = sipConvertFromInstance(cpp, sipClass_SceneNode, sipTransferObj); | |||
PyObject *pobj = sipConvertFromType(cpp, sipType_SceneNode, sipTransferObj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ghostkeeper
This is just an update from Sip ABI 11 (Sip <= 4.18.x) to Sip ABI 12 (Sip >= 4.19.0). As Sip 4.19.2 is the minimum required version, this should be correct anyway (and removes deprecated code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its even much older, sipConvertFromInstance is deprecated since 4.8.0!
) | ||
|
||
execute_process( | ||
COMMAND ${Python3_EXECUTABLE} -c "import sip; print(sip.SIP_VERSION_STR)" | ||
COMMAND ${Python3_EXECUTABLE} -c "import sipbuild; print(sipbuild.builder.SIP_VERSION_STR)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage over sip-build -V
here?
This is not correct. If you don't build it inline, you will always have a runtime dependency on the sip module. Build patched to use -n PyQt5.sip, but without PyQt5.sip installed at runtime:
With PyQt5.sip available:
This is true for libArcus, libSavitar and pynest2d. Note that PyQt5 itself is not installed, neither is Qt. The only file in site-packages/PyQt5 is the sip module. If you want to use those modules with cura (the primary use case, isn't it), which depends on PyQt5, you are bound to use the same module as cura pulls in through PyQt5. That is the reason why most distros patch the modules to use the PyQt5.sip namespace, as already mentioned: Ultimaker/libArcus#76 |
A few things you need to understand, as expectation management here. We develop Cura for work, meaning that we each get a limited number of hours and a sprint to complete. There is some leeway in this, but there's a lot to do every day. We can (and do) also work on Cura-related things in our spare time. But if that is part of our main code base (not a plug-in) then it needs to be reviewed and tested in many cases. Changing a dependency of 3 of our libraries which is also central to the most difficult part of building Cura for Windows is certainly not something I'd do on my own in my spare time. All other developers will be impacted, our build system needs to be updated on 4 servers, and we'd need to do an integration test. So that leaves Ultimaker's time to implement this. Ultimaker has its own distribution through the .AppImage file for Linux (~0.7% of Cura's users atm). If it can be made easy for people to create a package "native" for certain distributions, then we'd be happy to oblige, but if it impacts our planning then Ultimaker will not make this change unless there is a direct benefit for our own distribution method. As a result:
|
# : PyQT update to 5.15.6 and "simplify" it. SIP update to 6.5.0 and reintroduce SIP4 for cura and its dependencies. Summary: This PR proposes the update of the latest versions of PyQT/SIP and what goes with it. ## Sip As mentioned in the update from sip to sip5, this is a transitional version to remove what is deprecated in sip4. Sip6 completely removes the deprecated parts. We have already made the effort to support updates to sip >= 5. Just like the different software have integrated these changes to support the current versions of sip or to offer an option to choose one or the other version. Unfortunately, cura and its dependencies don't seem to want to update [1], and I reintroduced sip4 just for them; this is what is done in the others having cura :/ ## PyQt At the same time, I took the opportunity to simplify PyQT. We are the only ones to offer such a separate installation (except sub-package). I used the some the logic as some other linux distributions like Arch to propose only one PyQT package (what we do today for devel/pyside2) now named devel/py-qt5-pyqt. This allows us to be in adequacy with the packages that the author of these libraries proposes, namely: PyQt - devel/py-qt5-pyqt PyQt-3D – not in ports PyQt-Charts - x11-toolkits/py-qt5-chart PyQt-DataVisualization – not in ports PyQt-NetworkAuth – net/py-qt5-networkauth PyQt-Purchasing – not in ports PyQt-WebEngine – www/py-qt5-webengine SIP – devel/py-sip py-sip - devel/py-qt5-sip PyQt-builder - devel/py-qtbuilder pyqtdeploy – not in ports Qscintilla - devel/py-qt5-qscintilla2 The disadvantage of this method is that if you are a user of a tool that requires only a single dependency like py-qt5-core or py-qt5-widgets, you will pull all the dependencies from Qt5. However, this has to be put into perspective because only two are concerned (devel/py-qstylizer and dns/knock [2]). The others already have substantial dependencies. ## Detailled actions - rm of all */py-qt5-* but a few ports and create devel/py-qt5-pyqt - Add a dedicated MASTER_SITES for py-qt5-webengine - Update SIP to 6.3.1, qtbuilder to 1.12.0 - Simplify PyQt, There is only 4 values : pyqt5 chart networkauth and webengine - ${SIP} is now bin/sip-build-${PYTHON_VER} - adapt all ports to USE_PYQT= pyqt5 instead of USE_PYQT= core gui svg etc. - reintroduce sip4 only for Ultimaker ports (libsavitar, libArcus, pynest2d and Cura) - devel/git-cola : fix pkg-plist issue. Don’t know if it’s related to sip or the ports is broken. - science/py-veusz : update to 3.4 and remove SIP patches (fixed upstream) ### PyQt - use new pyproject and new sip to build the ports - ex- pyqt5-core remove useless PyQt5,Qt patch. - ex- pyqt5-core remove configure patch : not needed since now PyQt5 is not separated. BTW use the option --protected-is-public instead of patch the port. - net/py-qt5-networkauth, www/py-qt5-webengine and x11-toolkits/py-qt5-chart use “PYQT_DIST build system” - remove Api option (installed by default) - www/qutebrowser : update options - Install dist-info (maybe required by some ports) ### sip4 / cura - devel/libSavitar update to 4.11.0 and add BUILD_DEPENDS to sip4 - devel/py-pynest2d update to 4.12.1, remove patches included upstream and add BUILD_DEPENDS to sip4 - net/libarcus update to 4.12.1, remove patches included upstream and add BUILD_DEPENDS to sip4 [1] Ultimaker/libSavitar#26 (comment) [2] and maybe remove pyqt option since there is a mention that “pyqt:5 isn't used” Test Plan: [x] Poudriere 13 amd64 [ ] exp-run [ ] Test ports at runtime [X] Test conflicts for sip4/sip6. I installed graphics/qgis and cad/cura with devel/py-sip and devel/py-sip4 [ ] Test flavors [ ] Test regression for pyqt (there are patches for webengine, still needed?) Reviewers: #portmgr! Subscribers: mat, #contributor_reviewers_ports Differential Revision: https://reviews.freebsd.org/D33237 # category/port: Subject goes here, max 50 cols -| # <then a blank line> # 72 columns --| # # Do not add a Submitted by line. If someone besides the committer sent in the # change, the commit author should be set using `git commit --author`. # # Uncomment and complete these metadata fields, as appropriate: # # PR: <If and which Problem Report is related.> # Reported by: <If someone else reported the issue.> # Reviewed by: <If someone else reviewed your modification.> # Tested by: <If someone else tested the change.> # Approved by: <If you needed approval for this commit.> # Obtained from: <If the change is from a third party.> # Fixes: <Short hash and title line of commit fixed by this change> # MFH: <Ports tree branch name you plan to merge to.> # Relnotes: <Set to 'yes' for mention in release notes.> # Security: <Vulnerability reference (one per line) or description.> # Sponsored by: <If the change was sponsored by an organization.> # Pull Request: <https://github.com/freebsd/freebsd-ports/pull/###> # Differential Revision: <https://reviews.freebsd.org/D###> # # "Pull Request" and "Differential Revision" require the *full* GitHub or # Phabricator URL.
# MOVED: # Uncomment and add a short description of why things changed. # : PyQT update to 5.15.6 and "simplify" it. SIP update to 6.5.0 and reintroduce SIP4 for cura and its dependencies. Summary: This PR proposes the update of the latest versions of PyQT/SIP and what goes with it. ## Sip As mentioned in the update from sip to sip5, this is a transitional version to remove what is deprecated in sip4. Sip6 completely removes the deprecated parts. We have already made the effort to support updates to sip >= 5. Just like the different software have integrated these changes to support the current versions of sip or to offer an option to choose one or the other version. Unfortunately, cura and its dependencies don't seem to want to update [1], and I reintroduced sip4 just for them; this is what is done in the others having cura :/ ## PyQt At the same time, I took the opportunity to simplify PyQT. We are the only ones to offer such a separate installation (except sub-package). I used the some the logic as some other linux distributions like Arch to propose only one PyQT package (what we do today for devel/pyside2) now named devel/py-qt5-pyqt. This allows us to be in adequacy with the packages that the author of these libraries proposes, namely: PyQt - devel/py-qt5-pyqt PyQt-3D – not in ports PyQt-Charts - x11-toolkits/py-qt5-chart PyQt-DataVisualization – not in ports PyQt-NetworkAuth – net/py-qt5-networkauth PyQt-Purchasing – not in ports PyQt-WebEngine – www/py-qt5-webengine SIP – devel/py-sip py-sip - devel/py-qt5-sip PyQt-builder - devel/py-qtbuilder pyqtdeploy – not in ports Qscintilla - devel/py-qt5-qscintilla2 The disadvantage of this method is that if you are a user of a tool that requires only a single dependency like py-qt5-core or py-qt5-widgets, you will pull all the dependencies from Qt5. However, this has to be put into perspective because only two are concerned (devel/py-qstylizer and dns/knock [2]). The others already have substantial dependencies. ## Detailled actions - rm of all */py-qt5-* but a few ports and create devel/py-qt5-pyqt - Add a dedicated MASTER_SITES for py-qt5-webengine - Update SIP to 6.3.1, qtbuilder to 1.12.0 - Simplify PyQt, There is only 4 values : pyqt5 chart networkauth and webengine - ${SIP} is now bin/sip-build-${PYTHON_VER} - adapt all ports to USE_PYQT= pyqt5 instead of USE_PYQT= core gui svg etc. - reintroduce sip4 only for Ultimaker ports (libsavitar, libArcus, pynest2d and Cura) - devel/git-cola : fix pkg-plist issue. Don’t know if it’s related to sip or the ports is broken. - science/py-veusz : update to 3.4 and remove SIP patches (fixed upstream) ### PyQt - use new pyproject and new sip to build the ports - ex- pyqt5-core remove useless PyQt5,Qt patch. - ex- pyqt5-core remove configure patch : not needed since now PyQt5 is not separated. BTW use the option --protected-is-public instead of patch the port. - net/py-qt5-networkauth, www/py-qt5-webengine and x11-toolkits/py-qt5-chart use “PYQT_DIST build system” - remove Api option (installed by default) - www/qutebrowser : update options - Install dist-info (maybe required by some ports) ### sip4 / cura - devel/libSavitar update to 4.11.0 and add BUILD_DEPENDS to sip4 - devel/py-pynest2d update to 4.12.1, remove patches included upstream and add BUILD_DEPENDS to sip4 - net/libarcus update to 4.12.1, remove patches included upstream and add BUILD_DEPENDS to sip4 [1] Ultimaker/libSavitar#26 (comment) [2] and maybe remove pyqt option since there is a mention that “pyqt:5 isn't used” Test Plan: [x] Poudriere 13 amd64 [ ] exp-run [ ] Test ports at runtime [X] Test conflicts for sip4/sip6. I installed graphics/qgis and cad/cura with devel/py-sip and devel/py-sip4 [ ] Test flavors [ ] Test regression for pyqt (there are patches for webengine, still needed?) Reviewers: #portmgr! Subscribers: mat, #contributor_reviewers_ports Differential Revision: https://reviews.freebsd.org/D33237 # category/port: Subject goes here, max 50 cols -| # <then a blank line> # 72 columns --| # # category/port: Subject goes here, max 50 cols -| # <then a blank line> # 72 columns --| # # Do not add a Submitted by line. If someone besides the committer sent in the # change, the commit author should be set using `git commit --author`. # # Uncomment and complete these metadata fields, as appropriate: # # PR: <If and which Problem Report is related.> # Reported by: <If someone else reported the issue.> # Reviewed by: <If someone else reviewed your modification.> # Tested by: <If someone else tested the change.> # Approved by: <If you needed approval for this commit.> # Obtained from: <If the change is from a third party.> # Fixes: <Short hash and title line of commit fixed by this change> # MFH: <Ports tree branch name you plan to merge to.> # Relnotes: <Set to 'yes' for mention in release notes.> # Security: <Vulnerability reference (one per line) or description.> # Sponsored by: <If the change was sponsored by an organization.> # Pull Request: <https://github.com/freebsd/freebsd-ports/pull/###> # Differential Revision: <https://reviews.freebsd.org/D###> # # "Pull Request" and "Differential Revision" require the *full* GitHub or # Phabricator URL. # # Do not add a Submitted by line. If someone besides the committer sent in the # change, the commit author should be set using `git commit --author`. # # Uncomment and complete these metadata fields, as appropriate: # # PR: <If and which Problem Report is related.> # Reported by: <If someone else reported the issue.> # Reviewed by: <If someone else reviewed your modification.> # Tested by: <If someone else tested the change.> # Approved by: <If you needed approval for this commit.> # Obtained from: <If the change is from a third party.> # Fixes: <Short hash and title line of commit fixed by this change> # MFH: <Ports tree branch name you plan to merge to.> # Relnotes: <Set to 'yes' for mention in release notes.> # Security: <Vulnerability reference (one per line) or description.> # Sponsored by: <If the change was sponsored by an organization.> # Pull Request: <https://github.com/freebsd/freebsd-ports/pull/###> # Differential Revision: <https://reviews.freebsd.org/D###> # # "Pull Request" and "Differential Revision" require the *full* GitHub or # Phabricator URL.
# MOVED: # Uncomment and add a short description of why things changed. # MOVED: # Uncomment and add a short description of why things changed. # : PyQT update to 5.15.6 and "simplify" it. SIP update to 6.5.0 and reintroduce SIP4 for cura and its dependencies. Summary: This PR proposes the update of the latest versions of PyQT/SIP and what goes with it. ## Sip As mentioned in the update from sip to sip5, this is a transitional version to remove what is deprecated in sip4. Sip6 completely removes the deprecated parts. We have already made the effort to support updates to sip >= 5. Just like the different software have integrated these changes to support the current versions of sip or to offer an option to choose one or the other version. Unfortunately, cura and its dependencies don't seem to want to update [1], and I reintroduced sip4 just for them; this is what is done in the others having cura :/ ## PyQt At the same time, I took the opportunity to simplify PyQT. We are the only ones to offer such a separate installation (except sub-package). I used the some the logic as some other linux distributions like Arch to propose only one PyQT package (what we do today for devel/pyside2) now named devel/py-qt5-pyqt. This allows us to be in adequacy with the packages that the author of these libraries proposes, namely: PyQt - devel/py-qt5-pyqt PyQt-3D – not in ports PyQt-Charts - x11-toolkits/py-qt5-chart PyQt-DataVisualization – not in ports PyQt-NetworkAuth – net/py-qt5-networkauth PyQt-Purchasing – not in ports PyQt-WebEngine – www/py-qt5-webengine SIP – devel/py-sip py-sip - devel/py-qt5-sip PyQt-builder - devel/py-qtbuilder pyqtdeploy – not in ports Qscintilla - devel/py-qt5-qscintilla2 The disadvantage of this method is that if you are a user of a tool that requires only a single dependency like py-qt5-core or py-qt5-widgets, you will pull all the dependencies from Qt5. However, this has to be put into perspective because only two are concerned (devel/py-qstylizer and dns/knock [2]). The others already have substantial dependencies. ## Detailled actions - rm of all */py-qt5-* but a few ports and create devel/py-qt5-pyqt - Add a dedicated MASTER_SITES for py-qt5-webengine - Update SIP to 6.3.1, qtbuilder to 1.12.0 - Simplify PyQt, There is only 4 values : pyqt5 chart networkauth and webengine - ${SIP} is now bin/sip-build-${PYTHON_VER} - adapt all ports to USE_PYQT= pyqt5 instead of USE_PYQT= core gui svg etc. - reintroduce sip4 only for Ultimaker ports (libsavitar, libArcus, pynest2d and Cura) - devel/git-cola : fix pkg-plist issue. Don’t know if it’s related to sip or the ports is broken. - science/py-veusz : update to 3.4 and remove SIP patches (fixed upstream) ### PyQt - use new pyproject and new sip to build the ports - ex- pyqt5-core remove useless PyQt5,Qt patch. - ex- pyqt5-core remove configure patch : not needed since now PyQt5 is not separated. BTW use the option --protected-is-public instead of patch the port. - net/py-qt5-networkauth, www/py-qt5-webengine and x11-toolkits/py-qt5-chart use “PYQT_DIST build system” - remove Api option (installed by default) - www/qutebrowser : update options - Install dist-info (maybe required by some ports) ### sip4 / cura - devel/libSavitar update to 4.11.0 and add BUILD_DEPENDS to sip4 - devel/py-pynest2d update to 4.12.1, remove patches included upstream and add BUILD_DEPENDS to sip4 - net/libarcus update to 4.12.1, remove patches included upstream and add BUILD_DEPENDS to sip4 [1] Ultimaker/libSavitar#26 (comment) [2] and maybe remove pyqt option since there is a mention that “pyqt:5 isn't used” Test Plan: [x] Poudriere 13 amd64 [ ] exp-run [ ] Test ports at runtime [X] Test conflicts for sip4/sip6. I installed graphics/qgis and cad/cura with devel/py-sip and devel/py-sip4 [ ] Test flavors [ ] Test regression for pyqt (there are patches for webengine, still needed?) Reviewers: #portmgr! Subscribers: mat, #contributor_reviewers_ports Differential Revision: https://reviews.freebsd.org/D33237 # category/port: Subject goes here, max 50 cols -| # <then a blank line> # 72 columns --| # # category/port: Subject goes here, max 50 cols -| # <then a blank line> # 72 columns --| # # Do not add a Submitted by line. If someone besides the committer sent in the # change, the commit author should be set using `git commit --author`. # # Uncomment and complete these metadata fields, as appropriate: # # PR: <If and which Problem Report is related.> # Reported by: <If someone else reported the issue.> # Reviewed by: <If someone else reviewed your modification.> # Tested by: <If someone else tested the change.> # Approved by: <If you needed approval for this commit.> # Obtained from: <If the change is from a third party.> # Fixes: <Short hash and title line of commit fixed by this change> # MFH: <Ports tree branch name you plan to merge to.> # Relnotes: <Set to 'yes' for mention in release notes.> # Security: <Vulnerability reference (one per line) or description.> # Sponsored by: <If the change was sponsored by an organization.> # Pull Request: <https://github.com/freebsd/freebsd-ports/pull/###> # Differential Revision: <https://reviews.freebsd.org/D###> # # "Pull Request" and "Differential Revision" require the *full* GitHub or # Phabricator URL. # # category/port: Subject goes here, max 50 cols -| # <then a blank line> # 72 columns --| # # Do not add a Submitted by line. If someone besides the committer sent in the # change, the commit author should be set using `git commit --author`. # # Uncomment and complete these metadata fields, as appropriate: # # PR: <If and which Problem Report is related.> # Reported by: <If someone else reported the issue.> # Reviewed by: <If someone else reviewed your modification.> # Tested by: <If someone else tested the change.> # Approved by: <If you needed approval for this commit.> # Obtained from: <If the change is from a third party.> # Fixes: <Short hash and title line of commit fixed by this change> # MFH: <Ports tree branch name you plan to merge to.> # Relnotes: <Set to 'yes' for mention in release notes.> # Security: <Vulnerability reference (one per line) or description.> # Sponsored by: <If the change was sponsored by an organization.> # Pull Request: <https://github.com/freebsd/freebsd-ports/pull/###> # Differential Revision: <https://reviews.freebsd.org/D###> # # "Pull Request" and "Differential Revision" require the *full* GitHub or # Phabricator URL. # # Do not add a Submitted by line. If someone besides the committer sent in the # change, the commit author should be set using `git commit --author`. # # Uncomment and complete these metadata fields, as appropriate: # # PR: <If and which Problem Report is related.> # Reported by: <If someone else reported the issue.> # Reviewed by: <If someone else reviewed your modification.> # Tested by: <If someone else tested the change.> # Approved by: <If you needed approval for this commit.> # Obtained from: <If the change is from a third party.> # Fixes: <Short hash and title line of commit fixed by this change> # MFH: <Ports tree branch name you plan to merge to.> # Relnotes: <Set to 'yes' for mention in release notes.> # Security: <Vulnerability reference (one per line) or description.> # Sponsored by: <If the change was sponsored by an organization.> # Pull Request: <https://github.com/freebsd/freebsd-ports/pull/###> # Differential Revision: <https://reviews.freebsd.org/D###> # # "Pull Request" and "Differential Revision" require the *full* GitHub or # Phabricator URL.
Update for SIP5 build: