Cleanup in mcap_vendor package
#62
Conversation
MichaelOrlov
force-pushed
the
morlov/cleanup-in-mcap_vendor
branch
from
ad7d88b to
70a96c8
Compare
MichaelOrlov
requested review from
james-rms,
jhurliman,
emersonknapp and
clalancette
mcap_vendor/CMakeLists.txt
Outdated
| DESTINATION | ||
| ${CMAKE_INSTALL_PREFIX}/include | ||
| DIRECTORY ${_mcap_include_dir}/mcap | ||
| DESTINATION ${CMAKE_INSTALL_PREFIX}/include |
There was a problem hiding this comment.
While we are in here, we may as well make this follow our new recommendations to put this in a sub-sub-directory. You can change this to:
DESTINATION include/${PROJECT_NAME}
(I believe the CMAKE_INSTALL_PREFIX is implicit, and we should add ${PROJECT_NAME} to the end)
There was a problem hiding this comment.
@clalancette I am not sure that I am correctly understand how do you want it to be with suggestion to replace it with DESTINATION include/${PROJECT_NAME}.
Do you want it to be the path as install/mcap_vendor/include/mcap_vendor/ and then header files from mcap ?
If so, this is actually not what we want for this package.
Currently we have destination path as install/mcap_vendor/include/mcap/ and then header files from mcap. The rational for that is that include files not from mcap_vendor package itself but rather from external mcap C++ project. And we want it to be clear and consistent.
There was a problem hiding this comment.
Or may be you want the path to be as install/mcap_vendor/include/mcap_vendor/mcap/ and then header files from mcap. This is possible but looks odd and I don't see a rational for this.
There was a problem hiding this comment.
Or may be you want the path to be as install/mcap_vendor/include/mcap_vendor/mcap/ and then header files from mcap. This is possible but looks odd and I don't see a rational for this.
This is what I am going for.
There is a good rationale for it; see ros2/ros2#1150 for the gory details. In short, if you look in /opt/ros/rolling/include, you'll see that every single one of the includes in the core packages lives in a sub-sub-directory so that we don't accidentally pull in the wrong header file during underlay/overlay processing. I think we should do the same here.
There was a problem hiding this comment.
@clalancette Ok I changed it t o be under the ${PROJECT_NAME} i.e. install/mcap_vendor/include/mcap_vendor/mcap/
There was a problem hiding this comment.
@clalancette Apparently it doesn't work this way. Build fails with error message that mcap/mcap.hpp not found.
I guess will need to change include to point to the mcap_vendor/mcap/mcap.hpp.
I will try to debug it locally.
There was a problem hiding this comment.
Yep. changing include to the mcap_vendor/mcap/mcap.hpp helped with compilation error.
There was a problem hiding this comment.
Fixed it as amend commit
|
Gist: https://gist.githubusercontent.com/MichaelOrlov/8dd65556eb06031d211447a0b0193f65/raw/86c621126e24e948807e3c86fd2a43fd1f5d1b35/ros2.repos
|
MichaelOrlov
force-pushed
the
morlov/cleanup-in-mcap_vendor
branch
from
a88c9e4 to
c67f27a
Compare
|
re-run CI after fixing compilation issue.
|
|
@clalancette I've addressed your comments. Could you please reiterate on review? |
- Use `${PROJECT_NAME}` instead of confusing `mcap` for library name and
exporting target, to be consistent with all other packages in ROS2.
- Add export for path to the `mcap` includes and export for mcap_vendor
library. It was an issue in downstream packages that `mcap/mcap.hpp` was
not visible since we were not exporting path to it explicitly.
- Removed test section with linters for `mcap_vendor` package. Since
package itself doesn't contain any tests and own source code and we
shouldn't run linters on any third party code. Prevent CI failures.
Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov
force-pushed
the
morlov/cleanup-in-mcap_vendor
branch
from
c67f27a to
f3606f9
Compare
|
re-run Windows build after rebasing on top of
|
mcap_vendor/CMakeLists.txt
Outdated
| @@ -35,48 +34,37 @@ macro(build_mcap_vendor) | |||
| file(GLOB _lz4_srcs | |||
| ${lz4_SOURCE_DIR}/lib/*.c) | |||
|
|
|||
| add_library( | |||
| mcap SHARED | |||
| add_library(${PROJECT_NAME} SHARED | |||
There was a problem hiding this comment.
So I really don't think you want to change the name of the library like this.
That is, the idea behind vendor packages is that they are temporary (for some definition of temporary) stand-ins for the package in question. In some day in the future, we should be able to remove the vendor package and just get the library from the underlying OS.
Thus, all of our vendor packages actually build and export the actual library name, not a "vendored" version of it. Someday if libmcap is available in Ubuntu, then we can make this package just skip building on Ubuntu.
For those reasons, I do think you should switch back to the mcap library and target name.
There was a problem hiding this comment.
@clalancette Ok, fair point about libmcap. I have reverted renames.
Signed-off-by: Michael Orlov <[email protected]>
|
re-run Windows build after renames and fixing include issue.
|
Signed-off-by: Michael Orlov <[email protected]>
|
@clalancette I see.
BTW could be that calling before ?? |
I don't think so; that seems to be the typical way we do things. As a random example that I know works on Windows, see https://github.com/ros2/geometry2/blob/rolling/tf2/CMakeLists.txt . |
|
@clalancette It seems the problem is that Although @clalancette Do you have any ideas what's wrong in CMake file? |
…s(mcap)` Signed-off-by: Michael Orlov <[email protected]>
|
Trying re-run Windows build after removing
|
|
@clalancette I have a suspicious that Can we somehow verify that |
|
Trying re-run Windows build after suppressing warnings with
|
|
In short, I don't know. Windows is always a pain, and we don't have good ways to go into the CI containers and see what is in there. To move forward here, someone with a Windows machine is going to have to debug it. I do have a Windows machine locally, but I probably don't have time to look into it this week. Ideas:
|
|
@clalancette I would suggest to move forward with merging this PR and I will create a follow up issue about Windows build failure. Wouldn't you mind to approve this PR? |
MichaelOrlov
force-pushed
the
morlov/cleanup-in-mcap_vendor
branch
from
9aa40e0 to
fc5f8aa
Compare
|
Re-run CI
|
Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>
MichaelOrlov
force-pushed
the
morlov/cleanup-in-mcap_vendor
branch
from
b898f19 to
2bcff24
Compare
* Cleanup in `mcap_vendor` package
- Use `${PROJECT_NAME}` instead of confusing `mcap` for library name and
exporting target, to be consistent with all other packages in ROS2.
- Add export for path to the `mcap` includes and export for mcap_vendor
library. It was an issue in downstream packages that `mcap/mcap.hpp` was
not visible since we were not exporting path to it explicitly.
- Removed test section with linters for `mcap_vendor` package. Since
package itself doesn't contain any tests and own source code and we
shouldn't run linters on any third party code. Prevent CI failures.
Signed-off-by: Michael Orlov <[email protected]>
* Install `mcap` headers in `include/${PROJECT_NAME}/mcap` and export them
Signed-off-by: Michael Orlov <[email protected]>
* Revert renaming `mcap` library to `mcap_vendor` and fixed include issue
Signed-off-by: Michael Orlov <[email protected]>
* Add `LIBRARY DESTINATION lib` for install targets
Signed-off-by: Michael Orlov <[email protected]>
* Attempt to fix Windows build error by removing `ament_export_libraries(mcap)`
Signed-off-by: Michael Orlov <[email protected]>
* Nitpick. remove extra space before `HAS_LIBRARY_TARGET`
Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: James Smith <[email protected]>
Use
${PROJECT_NAME}instead of confusingmcapfor library name and exporting target, to be consistent with all other packages in ROS2.Add export for path to the
mcapincludes and export for mcap_vendor library. It was an issue in downstream packages thatmcap/mcap.hppwas not visible since we were not exporting path to it explicitly.Removed test section with linters for
mcap_vendorpackage. Since package itself doesn't contain any tests and its own code and we shouldn't run linters on any third party code. Prevent CI failures.Signed-off-by: Michael Orlov [email protected]