lopsided98 / nix-ros-overlay

ROS overlay for the Nix package manager
Apache License 2.0
174 stars 68 forks source link

workaround included: rqt won't load cpp plugins due to SIP not loading even though Shiboken/PySide should be used #404

Open Pleune opened 2 months ago

Pleune commented 2 months ago

It seem like qt_qui_cpp is keeping rqt_image_view from loading into the rqt plugins list. It looks like after d3c90ec, Pyside/Shiboken should be used instead of PyQt/SIP, but python_qt_binding is still preferring SIP. Interestingly, when running plugins directly, they seem to work. For example ros2 run rqt_image_view rqt_image_view, instead of trying to add it to rqt.

Error when running rqt

Could not import "pyqt" bindings of qt_gui_cpp library - so C++ plugins will not be available:
Traceback (most recent call last):
  File "/nix/store/hyf3pk2c0cglgfzb064nlxg76ldn6zwx-ros-env/lib/python3.11/site-packages/qt_gui_cpp/cpp_binding_helper.py", line 43, in <module>
    from . import libqt_gui_cpp_sip
ImportError: cannot import name 'libqt_gui_cpp_sip' from 'qt_gui_cpp' (/nix/store/hyf3pk2c0cglgfzb064nlxg76ldn6zwx-ros-env/lib/python3.11/site-packages/qt_gui_cpp/__init__.py)

Which shows that QT_BINDING is being chosen as pyqt. But as libqt_gui_cpp_sip is not in the qt_qui_cpp folder, due to the linked commit. Reverting that commit does not help, because SIP still is broken and does not compile. I have not investigated trying to fix that compile issue.

/nix/store/hyf3pk2c0cglgfzb064nlxg76ldn6zwx-ros-env/lib/python3.11/site-packages/qt_gui_cpp/cpp_binding_helper.py:

  33   │ from python_qt_binding import QT_BINDING
  34   │ from python_qt_binding.QtCore import qWarning
  35   │
  36   │
  37   │ try:
  38   │     if QT_BINDING == 'pyside':
  39   │         from . import libqt_gui_cpp_shiboken
  40   │         qt_gui_cpp = libqt_gui_cpp_shiboken.qt_gui_cpp
  41   │
  42   │     elif QT_BINDING == 'pyqt':
  43   │         from . import libqt_gui_cpp_sip
  44   │         qt_gui_cpp = libqt_gui_cpp_sip.qt_gui_cpp
  45   │
  46   │     else:
  47   │         raise ImportError('Qt binding name "%s" is unknown.' % QT_BINDING)

I also expect that this is not an issue on darwin, because on darwin SIP is not preferred by python_qt_bindings

/nix/store/hyf3pk2c0cglgfzb064nlxg76ldn6zwx-ros-env/lib/python3.11/site-packages/python_qt_binding/binding_helper.py:

  50   │ def _select_qt_binding(binding_name=None, binding_order=None):
  51   │     global QT_BINDING, QT_BINDING_VERSION
  52   │
  53   │     # order of default bindings can be changed here
  54   │     if platform.system() == 'Darwin':
  55   │         DEFAULT_BINDING_ORDER = ['pyside']
  56   │     else:
  57   │         DEFAULT_BINDING_ORDER = ['pyqt', 'pyside']
  58   │
  59   │     binding_order = binding_order or DEFAULT_BINDING_ORDER

The way python_qt_binding is written, pyqt will always be chosen on non-Darwin, unless the SELECT_QT_BINDING or SELECT_QT_BINDING_ORDER attrs are set on sys to pyside.

Now, what I have done, is just patched python-qt-bindings to never check for pyqt:

Overlay:

python-qt-binding = rosSuper.python-qt-binding.overrideAttrs (
  {
    patches ? [ ],
    propagatedBuildInputs ? [ ],
    ...
  }:
  {
    patches = patches ++ [ ./python_qt_binding.patch ];
    propagatedBuildInputs =
      propagatedBuildInputs
      ++ (with rosSelf.pythonPackages; [
        shiboken2
        pyside2
      ]);
  }
);

with

./python_qt_binding.patch

diff -rBNu a/src/python_qt_binding/binding_helper.py b/src/python_qt_binding/binding_helper.py
--- a/src/python_qt_binding/binding_helper.py   2024-05-02 11:03:48.047406945 -0400
+++ b/src/python_qt_binding/binding_helper.py   2024-05-02 11:04:19.445777970 -0400
@@ -51,10 +51,7 @@
     global QT_BINDING, QT_BINDING_VERSION

     # order of default bindings can be changed here
-    if platform.system() == 'Darwin':
-        DEFAULT_BINDING_ORDER = ['pyside']
-    else:
-        DEFAULT_BINDING_ORDER = ['pyqt', 'pyside']
+    DEFAULT_BINDING_ORDER = ['pyside']

     binding_order = binding_order or DEFAULT_BINDING_ORDER

And this works! The image_view plugin loads (and other cpp plugins also fixed)

image

I would recommend adding this patch as a supplement to d3c90ec, using PySide in all situations. But, admittedly, I am not 100% confident that this does not mess anything else up.

Now, I don't know why the icons are broken... but they still work when you click them.

Pleune commented 2 months ago

The patch only worked on iron and not humble (I have not tested others). Here is a version that works on both:

diff -rBNu a/src/python_qt_binding/binding_helper.py b/src/python_qt_binding/binding_helper.py
--- a/src/python_qt_binding/binding_helper.py   2024-05-02 14:19:45.418404395 -0400
+++ b/src/python_qt_binding/binding_helper.py   2024-05-02 14:25:15.680231007 -0400
@@ -49,6 +49,8 @@
 def _select_qt_binding(binding_name=None, binding_order=None):
     global QT_BINDING, QT_BINDING_VERSION

+    binding_name = 'pyside'
+
     # order of default bindings can be changed here
     DEFAULT_BINDING_ORDER = ['pyqt', 'pyside']
     binding_order = binding_order or DEFAULT_BINDING_ORDER
sorki commented 2 months ago

I've had the same problem recently and found out that rviz is patched to work with pyqt5 for ROS1 so I've created a similar overlay for rqt which does help for noetic (but does the exact opposite of yours, using pyqt instead of pyside). This allows to use ImageView and other native plugins (icons do work as well).

Related to https://github.com/lopsided98/nix-ros-overlay/issues/301

  # use the same python-qt-bindings fix as rviz does
  # otherwise rqt fails to import any bindings
  # https://github.com/lopsided98/nix-ros-overlay/issues/301
  fixROSpyqtbindings = (final: prev: {
    rosPackages.noetic = prev.rosPackages.noetic.overrideScope (rosSelf: rosSuper: {
      python-qt-binding = (rosSuper.python-qt-binding.override {
        python3Packages = rosSelf.python3Packages.overrideScope (pyFinal: pyPrev: {
          pyqt5 = pyPrev.pyqt5.overrideAttrs ({
            patches ? [], ...
          }: {
            patches = patches ++ [ (final.fetchpatch {
              url = "https://aur.archlinux.org/cgit/aur.git/plain/restore-sip4-support.patch?h=python-pyqt5-sip4&id=6e712e6c588d550a1a6f83c1b37c2c9135aae6ba";
              hash = "sha256-NfMe/EK1Uj88S82xZSm+A6js3PK9mlgsaci/kinlsy8=";
            }) ];
          });
        });
      }).overrideAttrs({
        propagatedNativeBuildInputs ? [],
        postPatch ? "", ...
      }: {
        propagatedNativeBuildInputs = with rosSelf.pythonPackages;
          (rosSelf.lib.subtractLists [ shiboken2 pyside2 ] propagatedNativeBuildInputs)
          ++ [ sip_4 ];
        postPatch = ''
          sed -e "1 i\\import PyQt5" \
              -e "s#sipconfig\._pkg_config\['default_mod_dir'\], 'PyQt5'#PyQt5.__path__[0]#" \
              -i cmake/sip_configure.py
        '' + postPatch;
      });
    });
  });
Pleune commented 2 months ago

This seems like a more proper fix. but I do think it would be good to keep the pyside build inputs included as well, because in theory consumers of python-qt-bindings could ask for one or the other implementation. Does including them cause any issues? Additionally, this would also not work with ros humble on darwin, since python-qt-bindings' release for humble removes pyqt as an option (see my snippet above).

I would love to submit a PR with these changes if nobody else can, but I will be a few weeks before I have time to do a quick test on each distro.

Pleune commented 4 weeks ago

I've had the same problem recently and found out that rviz is patched to work with pyqt5 for ROS1 so I've created a similar overlay for rqt which does help for noetic (but does the exact opposite of yours, using pyqt instead of pyside). This allows to use ImageView and other native plugins (icons do work as well).

Related to #301

  # use the same python-qt-bindings fix as rviz does
  # otherwise rqt fails to import any bindings
  # https://github.com/lopsided98/nix-ros-overlay/issues/301
  fixROSpyqtbindings = (final: prev: {
    rosPackages.noetic = prev.rosPackages.noetic.overrideScope (rosSelf: rosSuper: {
      python-qt-binding = (rosSuper.python-qt-binding.override {
        python3Packages = rosSelf.python3Packages.overrideScope (pyFinal: pyPrev: {
          pyqt5 = pyPrev.pyqt5.overrideAttrs ({
            patches ? [], ...
          }: {
            patches = patches ++ [ (final.fetchpatch {
              url = "https://aur.archlinux.org/cgit/aur.git/plain/restore-sip4-support.patch?h=python-pyqt5-sip4&id=6e712e6c588d550a1a6f83c1b37c2c9135aae6ba";
              hash = "sha256-NfMe/EK1Uj88S82xZSm+A6js3PK9mlgsaci/kinlsy8=";
            }) ];
          });
        });
      }).overrideAttrs({
        propagatedNativeBuildInputs ? [],
        postPatch ? "", ...
      }: {
        propagatedNativeBuildInputs = with rosSelf.pythonPackages;
          (rosSelf.lib.subtractLists [ shiboken2 pyside2 ] propagatedNativeBuildInputs)
          ++ [ sip_4 ];
        postPatch = ''
          sed -e "1 i\\import PyQt5" \
              -e "s#sipconfig\._pkg_config\['default_mod_dir'\], 'PyQt5'#PyQt5.__path__[0]#" \
              -i cmake/sip_configure.py
        '' + postPatch;
      });
    });
  });

This fix does not with with the new ros2 Jade or Rolling distros, where the python-qt-bindings includes an updated sip_configure.py file that actually applies the same patch above. This needs to be patched out on only the recent repos in order for python-qt-bindings to with with sip on new ROS2 releases

See here: https://github.com/ros-visualization/python_qt_binding/blob/1fbfb441b86dfc9df252fb7e4a7c0d736a5d1cbe/cmake/sip_configure.py#L143