-
Notifications
You must be signed in to change notification settings - Fork 91
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
Use relative path in an urdf include to avoid confusion between internal and system headers #1259
Conversation
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Codecov Report
@@ Coverage Diff @@
## sdf13 #1259 +/- ##
=======================================
Coverage 87.58% 87.58%
=======================================
Files 128 128
Lines 17090 17090
=======================================
Hits 14968 14968
Misses 2122 2122 |
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.
Not sure how just changing the header in one place is working since there are other files that include that same file. Do we know why FindURDFDom is not finding the vcpkg installed library?
@@ -36,7 +36,7 @@ | |||
#pragma warning(push, 0) | |||
|
|||
#include <vector> | |||
#include "urdf_parser/urdf_parser.h" | |||
#include "../urdf_parser/urdf_parser.h" |
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.
There are places (e.g., link.cpp) that have #include "urdf_parser/urdf_parser.h"
. Do those need to change as well?
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.
Since it is a private copy and we don't expect anyone to use these headers outside of our code, probably we don't need to. The compilation is working since the way for MSVC to include headers is curious: for the quoted form, the second priority after the same directory of the header, is
- In the directories of the currently opened include files, in the reverse order in which they were opened. The search begins in the directory of the parent include file and continues upward through the directories of any grandparent include files.
So if I'm not wrong the first file in the mix when compiling the library is model.cpp and it opens the right directory. That is the reason why we don't need to change more. Might be a good idea anyway to change other occurrences just in case that we (or the compiler) modifies the order of object code.
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.
That is interesting and curious indeed. We should definitely add your comment in the file (maybe just model.cpp) for future readers.
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.
+1 to a comment about why this is necessary
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.
done ad05aa6
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Windows problems seems relative to testing in python:
And Mac problems seems related internally to brew. |
We might have to disable python binding tests for windows. I don't think they were being tested before because the windows machines didn't have |
vcpkg installation from latest changes would bring pybind11 to the environment for most of the builds. Problem is that sdformat is the last piece of software in the buildfarm not using colcon + vcpkg but from-source-deps-with-cmake + tarballs. We need to migrate it. |
@j-rivero should I remove |
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
should be good to go. |
🦟 Bug fix
Summary
I found the following situation when doing some changes in the Windows CI:
With this scenario the following build problem appeared:
The compilation is mixing the headers from the vcpkg installation of urdfdom (tinyxml) with the headers of the internal copy (tinyxml2). The buildsystem defines an include on the internal header directory but the somehow the build system was including the toolchain file configurations before.
The cleaner solution I found was to modify the first include to make MSVC lo look for the headers file in the right directory instead. Since sdformat is not installing this files, we should not be affecting the final user experience at all.
Checklist