ros-visualization / python_qt_binding

http://wiki.ros.org/python_qt_binding
BSD 3-Clause "New" or "Revised" License
34 stars 54 forks source link

Use PySide2 and Shiboken2 targets for variables #79

Closed stertingen closed 4 years ago

stertingen commented 4 years ago

Hopefully fix https://github.com/ros-visualization/python_qt_binding/issues/78 and continue https://github.com/ros-visualization/python_qt_binding/pull/77.

dirk-thomas commented 4 years ago

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

stertingen commented 4 years ago

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 https://github.com/ros-visualization/qt_gui_core/issues/142#issuecomment-529245251)
  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 commented 4 years ago

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.

dirk-thomas commented 4 years ago

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: 1a760ee65617e4dca58e27b9fe4f5cc4265a57ab.

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

dirk-thomas commented 4 years ago

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

dirk-thomas commented 4 years ago

Cherry-picked to crystal-devel: ff3ef8c76b1051af82aef5a9fc96837d5117f3bf and 3967af6463e8a44b1e4626aec6956ef209b4c7ec.

dirk-thomas commented 4 years ago

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.