-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
work around libassimp-dev bug #288
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern should have a trailing slash to avoid matching in undesired positions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern should test for the end-of-string then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at their code, it appears that end of string should work in all cases 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
CI with a77a040 |
The CMake config file provided by the current Debian package sets the variable
ASSIMP_LIBRARY_DIRS
to the value/usr/lib/lib/x86_64-linux-gnu
. The duplicatelib
comes 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_library
call. 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?