search

ros2 / rviz

ROS 3D Robot Visualizer
BSD 3-Clause Clear License
296 stars 212 forks source link

Check if Freetype is installed before building it from source #1212

Open felixf4xu opened 4 months ago

felixf4xu commented 4 months ago

https://github.com/ros2/rviz/blob/526f25105b4f679a4c09128558d94b678affd0fa/rviz_ogre_vendor/CMakeLists.txt#L16-L17

It's better to check if Freetype is already installed before building it from source

My proposed change is like this:

  find_package(Freetype QUIET)
  ament_vendor(freetype_vendor
    SATISFIED ${Freetype_FOUND}
    VCS_URL https://git.savannah.gnu.org/git/freetype/freetype2.git

If it's OK then I will create a PR.

clalancette commented 3 months ago

I agree that we should check whether it is installed first.

However, I think we should go a bit further. What I think we should actually do is to split this out into its own package (freetype_vendor), and then make rviz_ogre_vendor depend on it. That makes it clearer that this is a vendored dependency, which should make it easier to remove in the future. Would you be willing to give that a shot?

felixf4xu commented 3 months ago

I can have a try, but to be honest, I don't know why rviz_ogre needs to compile freetype from source. Maybe a specific source version? some source code patch? or compiler switches?

I also checked https://github.com/ros2/rviz/blob/rolling/rviz_ogre_vendor/FindFreetype.cmake against the cmake's default FindFreetype.cmake, I'm not sure if I need to delete one from this repo.

clalancette commented 3 months ago

I can have a try, but to be honest, I don't know why rviz_ogre needs to compile freetype from source. Maybe a specific source version? some source code patch? or compiler switches?

I'm not entirely sure, though it likely has something to do with Windows. Eventually we want to move away from vendoring these at all, and moving this to freetype_vendor is one step closer to doing that.

I also checked https://github.com/ros2/rviz/blob/rolling/rviz_ogre_vendor/FindFreetype.cmake against the cmake's default FindFreetype.cmake, I'm not sure if I need to delete one from this repo.

It is certainly worth a shot; we'd rather carry around fewer of those if they are built-in to CMake itself.