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

Depend on gdal where it's used with rosdep #10

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

Ryanf55
Copy link
Contributor

@Ryanf55 Ryanf55 commented Oct 27, 2023

Rosdep has support for gdal through three packages.

libgdal-dev has the header files, libraries, and cmake files. IT depends on gdal-bin.
gdal-bin is the runtime libraries.
python3-gdal is the pyhon part of gdal.

Since these are all used, they can be distributed by the package.xml.

The purpose of doing this is to remove the need to manually install libgdal-dev, which is only a useful instruction on ubuntu. For all the other platforms that can run ROS1, they need different instructions, which is hard to maintain. Rosdep is the standard and platform-independent way to manage dependencies in ROS 1 and ROS 2.

To find the sources, I searched here:
https://github.com/search?q=repo%3Aros%2Frosdistro%20gdal&type=code

One thing I noticed is that terrain_planner references gdal in the CMakeLists, however it's not actually used there. If the package isn't used, perhaps it doesn't need to be found and linked to. This increases build time, code maintenance, and coupling. Consider removing it from terrain_planner?

Signed-off-by: Ryan Friedman <[email protected]>
@Ryanf55 Ryanf55 changed the title Depend on gdal where it's used Depend on gdal where it's used with rosdep Oct 27, 2023
@Ryanf55 Ryanf55 mentioned this pull request Oct 27, 2023
@Jaeyoung-Lim
Copy link
Member

One thing I noticed is that terrain_planner references gdal in the CMakeLists, however it's not actually used there. If the package isn't used, perhaps it doesn't need to be found and linked to. This increases build time, code maintenance, and coupling. Consider removing it from terrain_planner?

Done in #11 😁

@Jaeyoung-Lim Jaeyoung-Lim merged commit 0e6aa07 into ethz-asl:main Oct 30, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants