Use a patch for the revised Shiboken2Config.cmake

[no-preserve-root]

Shiboken has made an incompatible change to its CMake files that requires an upstream change and breaks builds with new shiboken versions. This is not yet released upstream. This change manually applies the merged patch and can be discarded for the next upstream release.

See the upstream issue for details.

[jwhendy]

I think you forgot to add the patch?

Also, I can build this as-is just fine. Can you discuss the reasons we should patch for Shiboken vs. other options? For example, when I ran into things along these lines, note the comment here on an issue linked to yours:

@jwhendy You don't need shiboken2 / pyside2 to build this package and should use sip / pyqt instead. What you posted above is only a warning (CMake Warning ...) and not an error.

The fact that you are not finding any required headers indicates that you haven't sourced the setup file under /opt/ros/melodic before trying to build the workspace.

So... I'm not opposed to this fixing an issue, but is it a universal issue or would we be just fine with some other route? This will result in a lingering issue we have to watch until upstream releases.

[no-preserve-root]

I think you forgot to add the patch?

No, line 35 pulls the relevant commit.

Also, I can build this as-is just fine. Can you discuss the reasons we should patch for Shiboken vs. other options? For example, when I ran into things along these lines, note the comment here on an issue linked to yours:

@jwhendy You don't need shiboken2 / pyside2 to build this package and should use sip / pyqt instead. What you posted above is only a warning (CMake Warning ...) and not an error.

The fact that you are not finding any required headers indicates that you haven't sourced the setup file under /opt/ros/melodic before trying to build the workspace.

So... I'm not opposed to this fixing an issue, but is it a universal issue or would we be just fine with some other route? This will result in a lingering issue we have to watch until upstream releases.

The issue is triggered as follows: if the build script is executed on a machine with a reasonably current shiboken2, it will always try to build with shiboken, run into the bug, and crash the build. If sip is installed, it is executed in addition to shiboken2. The alternative would be to conflict shiboken2, disable the shiboken2 support in ROS, or similar solutions with lots of collateral damage.

For me, the issue prevents me from building. (As far as I can tell, no one with shiboken2 installed can build ATM.) The linked issue is a different problem with the ROS build system that also causes a build error. (I'm looking into fixing that, too.) Are you certain you have an up-to-date shiboken2 (with Shiboken2Config.cmake) installed? Otherwise, the relevant part of the build script will not execute.

[acxz]

Since it fixes a build issue and the change has been committed upstream (which means eventually it will roll out in a release), I see no issue in merging this.

[jwhendy]

No, line 35 pulls the relevant commit

Ah, sorry. I'm used to seeing a local .patch file vs. a source url.

... on a machine with a reasonably current shiboken2

I think this is where I don't really understand which circumstances create/require this to be an issue.

The linked issue is a different problem...

Yes, but note the quote. I'll re-paste it:

dirk-thomas: You don't need shiboken2 / pyside2 to build this package.

Are you certain you have an up-to-date shiboken2 (with Shiboken2Config.cmake) installed? Otherwise, the relevant part of the build script will not execute.

No, I don't, which is why I don't run into the issue. I'm not opposed to this patch and it seems like an issue, it's just not directly clear what it is to me. From your comments, my understanding is that you must have shiboken installed explicitly or as a dep for something else, but because this package behaves differently with it's presence, we have to act accordingly.

I think that's good enough for me, but was confused as I've not encountered this, and my participation in the issue I linked to suggested we don't need it. It didn't cover the situation where another package does need it and thus the issue is inescapable. Thanks for the update/info.

[acxz]

Was able to reproduce the error and fix. LGTM ASAP.
@no-preserve-root can you also update the .SRCINFO file and commit that as well?
Alternatively you can allow maintainers to edit PRs like so: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

[no-preserve-root]

Sorry for the delay, .SRCINFO is updated now.

[acxz]

Nice, LGTM! This should be up when we do our next sync from the github repos to the AUR, hopefully it will be soon. @bionade24 do you know when we can get this patch out to the AUR? It may not be too critical, but it fixes build errors that have come about for multiple people.

[bionade24]

As fast as it builds. I need to fix all python 3.8 problems first.

[acxz]

I believe I have resolved all python 3.8 issues. See the latest pull requests. Ah yes as fast as it builds to confirm they work.