Skip to content
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

Hide GDAL from public interface, fix export #59

Merged
merged 1 commit into from
Jan 28, 2024

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Jan 20, 2024

Purpose

  • Enforce export in CI
  • Hide implementation details of GDAL in implementation
  • Install headers per latest ament recommendations to allow package: https://docs.ros.org/en/humble/How-To-Guides/Ament-CMake-Documentation.html#adding-targets
  • This is crucial before realeasing as a library
  • Remove unused yaml dependency
  • Add all missing libraries in ament_export_dependencies
  • Fix a few typos in comments
  • Add badges for all jobs but point them to the default ros2 branch rather than any branch
  • Fix style check not running on ros2 branch

Ticket

Relates to #50, we should fix the export, hide unused things, and then test consuming the library so consumers don't report bugs against the export.

Risk

Low, the CI checks should catch anything. I did remove the implementation of transform to a translation unit, so this may impact performance a tad, but it's necessary if we have any hope to hide GDAL.

@Ryanf55 Ryanf55 added bug Something isn't working enhancement New feature or request labels Jan 20, 2024
@Ryanf55 Ryanf55 force-pushed the fix-exposing-transform-header branch 2 times, most recently from ea2fa55 to 54d8652 Compare January 20, 2024 18:26
* Enforce export in CI
* Hide implementation details of GDAL in implementation
* Switch to build-scope includes
* Install headers per latest ament recommendations to allow package: https://docs.ros.org/en/humble/How-To-Guides/Ament-CMake-Documentation.html#adding-targets
* This is crucial before realeasing as a library
* Remove unused yaml dependency
* Add all missing libraries in ament_export_dependencies
* Fix a few typos in comments
* Add badges for all jobs but point them to the default ros2 branch
  rather than any branch
* Fix style check not running on ros2 branch

Signed-off-by: Ryan Friedman <[email protected]>
@Ryanf55 Ryanf55 force-pushed the fix-exposing-transform-header branch from 54d8652 to 3528778 Compare January 20, 2024 19:46
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jan 25, 2024

Ready for final review.

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM!

@Jaeyoung-Lim Jaeyoung-Lim merged commit 3fd9206 into ethz-asl:ros2 Jan 28, 2024
3 checks passed
@Ryanf55 Ryanf55 deleted the fix-exposing-transform-header branch January 28, 2024 20:44
@@ -25,8 +25,6 @@ find_package(rclcpp REQUIRED)
find_package(Eigen3 REQUIRED)
find_package(GDAL 3 REQUIRED)

find_package(yaml_cpp_vendor REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to revert this as yaml-cpp does not resolve correctly on macOS. Suggest:

# Fix for yaml_cpp not resolving correctly on macOS
if (APPLE)
  find_package(yaml_cpp_vendor REQUIRED)
  link_directories(${yaml_cpp_vendor_LIBRARY_DIRS})
endif()

to make it clear why it's included.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srmainwaring Thanks for pointing this out. Probably means we need a macOS CI for this package

Copy link
Contributor

@srmainwaring srmainwaring Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though the problem is that the pre-requisite is a functioning humble build for macOS CI and that's expensive as needs to be built from source. Ideally there would be a macOS runner with ROS 2 support we could pull down, put I don't think this exists.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see yaml being used which is the reason I took it out; can you share the compile or runtime error you have if this is absent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You won't see in on Ubuntu. The compiler error is an unresolved library (yaml-cpp) on macOS. This is because the yaml_cpp_vendored library (transitive) dependencies do not seem to work correctly on macOS. It's a pain, should be fixed upstream, but macOS is Tier 3 support so is unlikely to happen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the info. I contributed improvements to yamlcpp (not the vendored version, but upstream).

Related: Ryanf55#1 (comment)

Copy link
Collaborator Author

@Ryanf55 Ryanf55 Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create an upstream issue just to track?
https://github.com/ros2/yaml_cpp_vendor/issues

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch: #60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants