Skip to content
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

Use PySide2 and Shiboken2 targets for variables #79

Merged

Conversation

stertingen
Copy link
Contributor

Hopefully fix #78 and continue #77.

@dirk-thomas
Copy link
Contributor

Can you clarify what you mean with "hopefully"? How/ what environments and steps have you tested this patch with?

@stertingen
Copy link
Contributor Author

Can you clarify what you mean with "hopefully"? How/ what environments and steps have you tested this patch with?

I have not tested whether this PR breaks the build on Ubuntu/Debian or ROS Kinetic. On Arch Linux using ROS Melodic this seems to work fine.

My tests which lead to the assumtion that this PR fixes some issues with new versions of PySide2 and Shiboken2:

  1. Install ros-melodic-ros-base (AUR)
  2. Create catkin workspace with python_qt_bindings and qt_qui_core
  3. Patch shiboken_helper.cmake line 63 to include --enable-pyside-extensions (might be added to PR, but I don't know when this argument was added to the Shiboken binary)
  4. Patch shiboken_helper_cmake lines 31 and 42 to print every variable set by target properties (might be added to PR if wished, it's just more log output)
  5. Patch qt_gui_cpp/src/qt_gui_cpp_shiboken/CMakeLists.txt to add some missing include directories (See fatal error: 'pyside2_global.h' file not found qt_gui_core#142 (comment))
  6. Run catkin_make_isolated -DPYTHON_EXECUTABLE=/usr/bin/python

CMake output while catkin_make_isolated runs:

-- Using CATKIN_DEVEL_PREFIX: /home/hermann/src/rosws/devel_isolated/qt_gui_cpp
-- Using CMAKE_PREFIX_PATH: /home/hermann/src/rosws/devel_isolated/qt_gui_core;/home/hermann/src/rosws/devel_isolated/qt_gui_app;/home/hermann/src/rosws/devel_isolated/qt_gui;/home/hermann/src/rosws/devel_isolated/qt_dotgraph;/home/hermann/src/rosws/devel_isolated/python_qt_binding;/opt/ros/melodic
-- This workspace overlays: /home/hermann/src/rosws/devel_isolated/qt_gui_core;/home/hermann/src/rosws/devel_isolated/qt_gui_app;/home/hermann/src/rosws/devel_isolated/qt_gui;/home/hermann/src/rosws/devel_isolated/qt_dotgraph;/home/hermann/src/rosws/devel_isolated/python_qt_binding;/opt/ros/melodic
-- Found PythonInterp: /usr/bin/python (found suitable version "3.8", minimum required is "2") 
-- Using PYTHON_EXECUTABLE: /usr/bin/python
-- Using default Python package layout
-- Using empy: /usr/lib/python3.8/site-packages/em.py
-- Using CATKIN_ENABLE_TESTING: ON
-- Call enable_testing()
-- Using CATKIN_TEST_RESULTS_DIR: /home/hermann/src/rosws/build_isolated/qt_gui_cpp/test_results
-- Found gtest: gtests will be built
-- Using Python nosetests: /usr/bin/nosetests-3.8
-- catkin 0.7.19
-- BUILD_SHARED_LIBS is on
-- Shiboken2Config: Using default python: .cpython-38-x86_64-linux-gnu
-- Found PythonInterp: /usr/bin/python (found suitable version "3.8", minimum required is "3") 
-- Found PythonLibs: /usr/lib/libpython3.8.so (found suitable version "3.8.0", minimum required is "3") 
-- SHIBOKEN_PYTHON_INCLUDE_DIRS computed to value: '/usr/include/python3.8'
-- SHIBOKEN_PYTHON_LIBRARIES computed to value: ''
-- libshiboken built for Release
Using SHIBOKEN_LIBRARY: /usr/lib/libshiboken2.cpython-38-x86_64-linux-gnu.so.5.13.2
Using SHIBOKEN_INCLUDE_DIR: /usr/include/shiboken2;/usr/include/python3.8
Using SHIBOKEN_BINARY: Shiboken2::shiboken2
Using PYSIDE_LIBRARY: /usr/lib/libpyside2.cpython-38-x86_64-linux-gnu.so.5.13.2
Using PYSIDE_INCLUDE_DIR: /usr/include/PySide2
-- Found PythonLibs: /usr/lib/libpython3.8.so (found suitable version "3.8.0", minimum required is "3.8") 
-- Shiboken binding generator available.
-- Found PythonInterp: /usr/bin/python (found suitable version "3.8", minimum required is "3.8") 
-- SIP binding generator available at: /usr/bin/sip
-- Python binding generators: shiboken;sip
-- Configuring done
-- Generating done
-- Build files have been written to: /home/hermann/src/rosws/build_isolated/qt_gui_cpp

So SHIBOKEN_LIBRARY, SHIBOKEN_INCLUDE_DIR, SHIBOKEN_BINARY, PYSIDE_LIBRARY and PYSIDE_INCLUDE_DIR are set and printed correctly.

@stertingen
Copy link
Contributor Author

Tested on ROS Kinetic using docker:

docker run --rm -it -v `pwd`:/rosws -w /rosws ros:kinetic-ros-base sh -c "apt-get update && apt-get install -y python-pyside2 libshiboken2-dev && rosdep install --from-paths src -i -y && rm -rf .catkin_workspace build_isolated devel_isolated && catkin_make_isolated && rm -rf .catkin_workspace build_isolated devel_isolated"

Changes made in this PR are okay, but the --enable-pyside-extensions flag as well as the patches to fix the include paths for the Shiboken2 execution (qt_gui_cpp/src/qt_gui_cpp_shiboken/CMakeLists.txt) break the build. (I'll have to take another look at this, but it's not in the scope of this PR.)

Could not test on ROS Melodic because the installation of python-pyside failed due to unmet dependencies.

Could also not test on ROS Melodic with Debian Stretch because I found no PySide2 and Shiboken2 packages for Debian.

@stertingen stertingen changed the title Use PySide2 and Shiboken2 targets for variables WIP: Use PySide2 and Shiboken2 targets for variables Feb 5, 2020
@dirk-thomas dirk-thomas changed the base branch from kinetic-devel to melodic-devel February 28, 2020 22:49
@dirk-thomas
Copy link
Contributor

Thank you for the pull request.

In the mean time I have branched for Melodic (since Kinetic has a much older PySide version). I rebased this patch and applied some minor changes to keep the diff minimal.

The most important fix I added was adding the missing CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES to the Shiboken include paths: 1a760ee.

With this (and ros-visualization/qt_gui_core#201) I was able to build both on Ubuntu Focal for Noetic.

@dirk-thomas dirk-thomas changed the title WIP: Use PySide2 and Shiboken2 targets for variables Use PySide2 and Shiboken2 targets for variables Feb 28, 2020
@dirk-thomas dirk-thomas merged commit c0f2d3d into ros-visualization:melodic-devel Feb 28, 2020
dirk-thomas added a commit that referenced this pull request Feb 28, 2020
@dirk-thomas
Copy link
Contributor

This patch removed the QUIET keyword in the find_package() calls added in #85. I readded them in 8167f68.

dirk-thomas added a commit that referenced this pull request Mar 2, 2020
* Inserted blank lines to improve readability

* Set PYSIDE_LIBRARY and PYSIDE_INCLUDE_DIR for Pyside >= 5.12

* Set SHIBOKEN_LIBRARY and SHIBOKEN_INCLUDE_DIR for newer shiboken version

* Moved location where SHIBOKEN_BINARY is set, added message output

* shiboken_helper.cmake: Print PYSIDE_INCLUDE_DIR and SHIBOKEN_INCLUDE_DIR

* shiboken_helper.cmake: Refactor find_package for Shiboken and PySide

* shiboken_helper.cmake: Include current directory, enable pyside ext.

* shiboken_helper.cmake: Fixed compilation on Kinetic/Xenial

* remove unnecessary whitespace changes

* reorder some lines

* condition can check the variable name

* remove conditionally for very old shiboken versions

* add CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES to shiboken invocation

Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
dirk-thomas added a commit that referenced this pull request Mar 2, 2020
@dirk-thomas
Copy link
Contributor

Cherry-picked to crystal-devel: ff3ef8c and 3967af6.

@dirk-thomas
Copy link
Contributor

The most important fix I added was adding the missing CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES to the Shiboken include paths

While this works on Ubuntu Focal it unfortunately doesn't work on Debian Buster: http://build.ros.org/job/Nbin_db_dB64__qt_gui_cpp__debian_buster_amd64__binary/3/

This seems to be the relevant upstream ticket which has been fixed in CMake 3.14 but Debian Buster contains 3.13.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PySide2 uses CMake target instead of variable
2 participants