Fix support for assimp 5.1.0 #817

traversaro · 2022-01-07T08:49:42Z

assimp 5.1.0 has a relativly working CMake support (after the upstream work in assimp/assimp#3455) and assimp::assimp imported target, so if assimp > 5.0.1 we can avoid to used the custom logic necessary for old assimp versions.

Fix ros2/ros2#1222 .

traversaro · 2022-01-07T08:55:06Z

Note that Ubuntu Jammy uses assimp 5.1.4 (see https://launchpad.net/ubuntu/jammy/+source/assimp), so this PR could be useful for ros2/ros2#1221 .

clalancette

Nice, thanks for this! Confirmed that I can build on macOS Monterey after this PR. I'm going to run CI on our normal stuff, plus Jammy just to ensure that this works there as well.

clalancette · 2022-01-11T14:50:26Z

CI:

Linux
Linux-aarch64
Windows
Linux Jammy

traversaro · 2022-01-18T13:16:58Z

Sorry @clalancette, I had missed your reply.

The Jammy CI is failing with the error:

15:24:35 CMake Error at /usr/lib/x86_64-linux-gnu/cmake/draco/draco-config.cmake:41 (add_library):
15:24:35   add_library cannot create imported target "draco::draco" because another
15:24:35   target with the same name already exists.
15:24:35 Call Stack (most recent call first):
15:24:35   /usr/lib/x86_64-linux-gnu/cmake/assimp-5.1/assimpConfig.cmake:37 (find_package)
15:24:35   /home/jenkins-agent/workspace/ci_linux/ws/install/rviz_assimp_vendor/share/rviz_assimp_vendor/cmake/rviz_assimp_vendor-extras.cmake:15 (find_package)
15:24:35   /home/jenkins-agent/workspace/ci_linux/ws/install/rviz_assimp_vendor/share/rviz_assimp_vendor/cmake/rviz_assimp_vendorConfig.cmake:41 (include)
15:24:35   CMakeLists.txt:33 (find_package)

I had a similar error (i.e. related to draco) on a Debian Testing CI (that I set up exactly to early catch problem that will arise in future Ubuntu distro) in robotology/robotology-superbuild#974 . I think there are several problem related to the fact that the Debian packaging of assimp is tryng to patch assimp to make sure that the Debian's draco instead of the vendored one. However, the situation was in rapid evolution, so probably it could make sense to try to re-run Jammy CI.

clalancette · 2022-01-19T16:30:04Z

so probably it could make sense to try to re-run Jammy CI.

I did that (without this patch) here: https://ci.ros2.org/job/ci_linux/15962/console . Looks like it is still broken, so we may have to dig into the Debian/Ubuntu Jammy situation a bit more.

traversaro · 2022-01-19T16:41:35Z

I have another build on latest Jammy that works fine, so I guess it is some kind of error related to repeated CMake calls, i.e.:

find_package(assimp)
find_package(assimp)

I will quickly check on a Jammy instance.

traversaro · 2022-01-19T16:48:30Z

Bingo:

cmake_minimum_required(VERSION 3.15)

project(assimptest)

find_package(assimp REQUIRED)
find_package(assimp REQUIRED)

fails with:

CMake Error at /usr/lib/x86_64-linux-gnu/cmake/draco/draco-config.cmake:41 (add_library):
  add_library cannot create imported target "draco::draco" because another
  target with the same name already exists.
Call Stack (most recent call first):
  /usr/lib/x86_64-linux-gnu/cmake/assimp-5.1/assimpConfig.cmake:37 (find_package)
  CMakeLists.txt:6 (find_package)

while:

cmake_minimum_required(VERSION 3.15)

project(assimptest)

find_package(assimp REQUIRED)
# find_package(assimp REQUIRED)

works fine.

traversaro · 2022-01-19T16:49:36Z

An even simpler example to reproduce the problem is:

cmake_minimum_required(VERSION 3.16)
project(assimptest)
find_package(draco REQUIRED)
find_package(draco REQUIRED)

(i.e. the issue is in libdraco-dev, not in libassimp-dev)

traversaro · 2022-01-19T17:34:07Z

I opened a upstream (Ubuntu) bug in https://bugs.launchpad.net/ubuntu/+source/draco/+bug/1958432 . As the problem originates in Debian, it would have been more appropriate to report in Debian, but to be completly honest I am not an expert use of reportbug and all the machinery required to report bugs in Debian. On the other hand, the upstream mantainer is @roehling, and he seems to be interested in ROS, so perhaps it could be interested in the problem and the suggested solution even if not reported via Debian's bug tracker.

roehling · 2022-01-19T19:16:39Z

I just uploaded a fixed version of draco to Debian unstable, and it should find its way to Ubuntu within a couple of days, since Ubuntu 22.04 is still syncing with Debian.

@traversaro: I am not notified about new bugs which are filed in Launchpad, which is the main reason why it is better to report Debian related bugs in Debian. Mentioning me on Github does help, though. ;)

traversaro · 2022-01-19T20:46:25Z

I just uploaded a fixed version of draco to Debian unstable, and it should find its way to Ubuntu within a couple of days, since Ubuntu 22.04 is still syncing with Debian.

Great, thanks!

@traversaro: I am not notified about new bugs which are filed in Launchpad, which is the main reason why it is better to report Debian related bugs in Debian. Mentioning me on Github does help, though. ;)

Yes, I suspected that. On the other hand, I am a bit afraid afraid of doing something wrong when submitting Debian bugs as I need to manually compose mails, without the possibility of validating (as far as I know) the message before I send it.

clalancette · 2022-01-20T21:52:03Z

So it looks like the new libdraco-dev has propagated through Debian into Jammy (I see 1.5.0+dfsg-5 available in Jammy now). Thanks @roehling !

Unfortunately, I think the changes in this PR aren't sufficient. With the newest libdraco-dev installed, I can now successfully build rviz_rendering and rviz_assimp_vendor, but trying to link downstream packages (like rviz_rendering_tests) fails:

CMake Error at ros2_ws/install/ament_cmake_gmock/share/ament_cmake_gmock/cmake/ament_add_gmock.cmake:69 (add_executable):
  Target "mesh_loader_test_target" links to target "assimp::assimp" but the
  target was not found.  Perhaps a find_package() call is missing for an
  IMPORTED target, or an ALIAS target is missing?
Call Stack (most recent call first):
  ros2_ws/install/ament_cmake_gmock/share/ament_cmake_gmock/cmake/ament_add_gmock.cmake:52 (_ament_add_gmock)
  CMakeLists.txt:57 (ament_add_gmock)

@traversaro Would you mind taking a look?

traversaro · 2022-01-21T09:46:27Z

@traversaro Would you mind taking a look?

I added 2e47d07 that should fix the issue.

clalancette · 2022-01-21T14:08:50Z

Thanks for giving it a try again. Unfortunately, I still have exactly the same issue on Jammy, even with the updated code. Any other thoughts on what it might be?

traversaro · 2022-01-21T14:26:40Z

Thanks for giving it a try again. Unfortunately, I still have exactly the same issue on Jammy, even with the updated code. Any other thoughts on what it might be?

Interesting. Do you have a log to check? Otherwise I will try to setup an instance on which to debug the problem, but it may take longer on my side.

clalancette · 2022-01-21T14:43:53Z

Ah, I may have found it. I think rviz_rendering is missing an export of rviz_assimp_vendor. Will do a bit more testing locally and push a patch shortly if it works.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This reverts commit 2f86475.

clalancette · 2022-01-21T16:07:19Z

All right, at least locally what I just pushed seems to fix the problem for me. I'll run another round of CI to check on it:

Linux
Linux-aarch64
Windows
Linux Jammy

Ace314159 · 2022-01-21T20:00:24Z

Ah, I may have found it. I think rviz_rendering is missing an export of rviz_assimp_vendor. Will do a bit more testing locally and push a patch shortly if it works.

I'm not sure if the issue was fixed by this, but I had to export rviz_assimp_vendor in rviz_common. I didn't need to export it in rviz_rendering, but that could just be due to environment differences.

clalancette · 2022-01-21T20:05:01Z

I'm not sure if the issue was fixed by this, but I had to export rviz_assimp_vendor in rviz_common. I didn't need to export it in rviz_rendering, but that could just be due to environment differences.

Hm, interesting. It actually looks like rviz_common doesn't directly depend on assimp at all, so it actually should not have a dependency there at all. It should just inherit it from rviz_rendering. I'll try that out locally here and see what happens.

Separately, the Linux Jammy job is still failing with the draco::draco problem. Presumably the underlying Jammy Dockerfile is out-of-date, since with local packages here it works fine. I'm still somewhat reluctant to merge without this, but if it continues for a lot longer we can consider it.

traversaro · 2022-01-21T20:23:11Z

I'm still somewhat reluctant to merge without this, but if it continues for a lot longer we can consider it.

No problem for me to just wait for the Dockerfile update to merge this.

It doesn't directly depend on it, so it should not include it. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2022-01-21T20:51:28Z

Hm, interesting. It actually looks like rviz_common doesn't directly depend on assimp at all, so it actually should not have a dependency there at all. It should just inherit it from rviz_rendering. I'll try that out locally here and see what happens.

Yeah, that worked fine for me locally, so 0d26798 removes it.

clalancette · 2022-01-24T14:39:09Z

I re-ran the job on Jammy, and it looks like the Docker images were updated over the weekend. So the build passed (there are a bunch of warnings on Jammy, but those are completely unrelated to this PR). So I'm going to go ahead and merge this; thanks for the fix @traversaro !

traversaro · 2022-01-24T14:59:43Z

Thanks for merging @clalancette !

Ace314159 · 2022-01-24T17:28:30Z

@clalancette Can this be backported to the galactic and foxy branches? Is there an automated tool to do this? If not, I can make the PR.

ros-discourse · 2022-01-25T00:53:33Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-january-20th-2022/23986/1

clalancette · 2022-01-25T13:48:58Z

@clalancette Can this be backported to the galactic and foxy branches?

I'm somewhat hesitant to do it, just because this changes enough of how things are done that it could cause problems in Foxy and Galactic.

Are you more interested in the support for assimp 5.1.0, or in the part of this PR where we did the export of rviz_assimp_vendor properly?

Ace314159 · 2022-01-25T17:08:22Z

@clalancette I'm interested in the support for assimp 5.1.0, which I don't think will cause any issues in Foxy and Galactic.

traversaro mentioned this pull request Jan 7, 2022

library_abs set to Not FOUND on Mac Big Sur intel ros2/ros2#1222

Closed

clalancette approved these changes Jan 11, 2022

View reviewed changes

traversaro and others added 4 commits January 21, 2022 15:25

Fix support for assimp 5.1.0

b9b2445

Add ament_export_dependencies on assimp for rviz_assimp_vendor

2f86475

Make sure to export rviz_assimp_vendor dep from rviz_rendering.

0d1ea9f

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Revert "Add ament_export_dependencies on assimp for rviz_assimp_vendor"

b3a2786

This reverts commit 2f86475.

clalancette force-pushed the patch-1 branch from 2e47d07 to b3a2786 Compare January 21, 2022 16:05

traversaro mentioned this pull request Jan 21, 2022

Upstream patches RoboStack/ros-galactic#31

Open

Remove assimp_vendor dependency from rviz_common.

0d26798

It doesn't directly depend on it, so it should not include it. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette merged commit 3e12c99 into ros2:ros2 Jan 24, 2022

Ace314159 mentioned this pull request Feb 1, 2022

[foxy] Fix support for assimp 5.1.0 (#817) #826

Merged

clalancette mentioned this pull request Feb 3, 2022

Assimp vendor package doesn’t forward assimp::assimp target properly #724

Closed

Ace314159 mentioned this pull request Feb 7, 2022

[galactic] Fix support for assimp 5.1.0 (#817) #827

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix support for assimp 5.1.0 #817

Fix support for assimp 5.1.0 #817

traversaro commented Jan 7, 2022 •

edited

Loading

traversaro commented Jan 7, 2022

clalancette left a comment

clalancette commented Jan 11, 2022

traversaro commented Jan 18, 2022

clalancette commented Jan 19, 2022

traversaro commented Jan 19, 2022

traversaro commented Jan 19, 2022

traversaro commented Jan 19, 2022 •

edited

Loading

traversaro commented Jan 19, 2022 •

edited

Loading

roehling commented Jan 19, 2022

traversaro commented Jan 19, 2022

clalancette commented Jan 20, 2022 •

edited

Loading

traversaro commented Jan 21, 2022

clalancette commented Jan 21, 2022

traversaro commented Jan 21, 2022

clalancette commented Jan 21, 2022 •

edited

Loading

clalancette commented Jan 21, 2022 •

edited

Loading

Ace314159 commented Jan 21, 2022

clalancette commented Jan 21, 2022

traversaro commented Jan 21, 2022

clalancette commented Jan 21, 2022

clalancette commented Jan 24, 2022

traversaro commented Jan 24, 2022

Ace314159 commented Jan 24, 2022

ros-discourse commented Jan 25, 2022

clalancette commented Jan 25, 2022

Ace314159 commented Jan 25, 2022

Fix support for assimp 5.1.0 #817

Fix support for assimp 5.1.0 #817

Conversation

traversaro commented Jan 7, 2022 • edited Loading

traversaro commented Jan 7, 2022

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Jan 11, 2022

traversaro commented Jan 18, 2022

clalancette commented Jan 19, 2022

traversaro commented Jan 19, 2022

traversaro commented Jan 19, 2022

traversaro commented Jan 19, 2022 • edited Loading

traversaro commented Jan 19, 2022 • edited Loading

roehling commented Jan 19, 2022

traversaro commented Jan 19, 2022

clalancette commented Jan 20, 2022 • edited Loading

traversaro commented Jan 21, 2022

clalancette commented Jan 21, 2022

traversaro commented Jan 21, 2022

clalancette commented Jan 21, 2022 • edited Loading

clalancette commented Jan 21, 2022 • edited Loading

Ace314159 commented Jan 21, 2022

clalancette commented Jan 21, 2022

traversaro commented Jan 21, 2022

clalancette commented Jan 21, 2022

clalancette commented Jan 24, 2022

traversaro commented Jan 24, 2022

Ace314159 commented Jan 24, 2022

ros-discourse commented Jan 25, 2022

clalancette commented Jan 25, 2022

Ace314159 commented Jan 25, 2022

traversaro commented Jan 7, 2022 •

edited

Loading

traversaro commented Jan 19, 2022 •

edited

Loading

traversaro commented Jan 19, 2022 •

edited

Loading

clalancette commented Jan 20, 2022 •

edited

Loading

clalancette commented Jan 21, 2022 •

edited

Loading

clalancette commented Jan 21, 2022 •

edited

Loading