Conversation
|
Thanks @dirk-thomas for the suggested fix. For context to connect the issues together:
As we didn't hear back from the maintainer, @nuclearsandwich is planning to check with @j-rivero the next steps to get the fix released. |
|
This patch simply let's me build the code in the current state without having to wait for any upstream fixes. Without it I simply can't build it successfully on Bionic since the manifest declares a dependency on the "broken" |
Yeah and that's great I think the workaround should be merged 👍 I pointed out the additional links so that people external to the issue and tagged here knew what issue and fix we were referring to. |
|
Thanks for fixing it up @mikaelarguedas. I agree the workaround is good to have until the upstream issues are fixed. The workaround until now was to uninstall assimp, which sucks if you have ROS 1 installed at the same time. |
|
I wasn't sure if you ran full CI on these pr's yet @mikaelarguedas, so here's CI with
|
I did not, I only ran bionic. Planning on taking a MacOS node offline to try the same patch with assimp 4.1.0 installed from brew (can be done after this is merged) |
| # workaround bug in Assimp 4.1.0 https://github.com/assimp/assimp/issues/1914 | ||
| # Affecting Ubuntu package: libassimp-dev 4.1.0~dfsg-3, Brew: assimp 4.1.0 | ||
| string(REPLACE "/lib/lib/" "/lib/" ASSIMP_LIBRARY_DIRS "${ASSIMP_LIBRARY_DIRS}") | ||
| string(REPLACE "/lib/include" "/include" ASSIMP_INCLUDE_DIRS "${ASSIMP_INCLUDE_DIRS}") |
There was a problem hiding this comment.
The pattern should have a trailing slash to avoid matching in undesired positions.
There was a problem hiding this comment.
Yeah I had the same feeling, unfortunately it cannot have the trailing slash as the path exported by the assimp debian doesnt have one "/usr/lib/include"
CMake Error in CMakeLists.txt:
Imported target "rviz_rendering::rviz_rendering" includes non-existent path
"/usr/lib/include"
Example of job failing with the trailing slash in the pattern : https://ci.ros2.org/job/ci_linux/4579/
There was a problem hiding this comment.
The pattern should test for the end-of-string then?
There was a problem hiding this comment.
Looking at their code, it appears that end of string should work in all cases 👍
|
CI with a77a040
|
The CMake config file provided by the current Debian package sets the variable
ASSIMP_LIBRARY_DIRSto the value/usr/lib/lib/x86_64-linux-gnu. The duplicatelibcomes from the way the value is being parameterized when building the Debian package. Anyway the correct value is/usr/lib/x86_64-linux-gnu.While investigating this problem I noticed that the code trying to find the library (see 826bd0b#diff-5e9bf0e7ab4d201db275acc2fcd12886R28) doesn't check the result of the
find_librarycall. That should happen independent of this workaround. @wjwwood Can you follow up on this when you have a chance?@j-rivero Can you try to get this fixed upstream when you have a chance?