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

Remove broken rviz_ogre_vendor::RenderSystem_GL target #920

Merged

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Nov 3, 2022

This fixes an issue that happens on the ROS 2 buildfarm, but has gone unnoticed because nothing uses these targets. It was noticed in bazel_ros2_rules: RobotLocomotion/drake-ros#188

The issue is these find library calls are looking for libraries that don't have the lib prefix. The version of CMake on jammy no longer looks for libraries without the lib prefix on Linux by default. This restores that behavior. Edit: Incorrect.

This should be backported to Humble.

@sloretz sloretz added the bug Something isn't working label Nov 3, 2022
@sloretz sloretz self-assigned this Nov 3, 2022
@clalancette
Copy link
Contributor

Your analysis of the problem makes sense to me. But I find two things weird:

  1. That change in behavior in CMake seems like a major change (it looks like it went in in CMake 3.22). I'm surprised it didn't cause more widespread breakage, though maybe the distributions dealt with it.
  2. I'm not super familiar with how OGRE works, but it kind of seems like these libraries don't have lib prefix deliberately. That is, they seem to be plugins that get loaded at runtime into OGRE, not things that external programs should build and link against. That would explain why nothing used the targets before.

With the latter thing said, should we be trying to find_library them at all?

@sloretz
Copy link
Contributor Author

sloretz commented Nov 4, 2022

That change in behavior in CMake seems like a major change (it looks like it went in in CMake 3.22). I'm surprised it didn't cause more widespread breakage, though maybe the distributions dealt with it.

That's a good point, and it looks like my explanation is wrong. The libraries aren't found on Foxy (Focal) either.

-- rviz_ogre_vendor::RenderSystem_GL for IMPORTED_LOCATION_RELEASE: _render_system_gl_static_library_abs-NOTFOUND
-- rviz_ogre_vendor::RenderSystem_GL for IMPORTED_LOCATION_DEBUG: _render_system_gl_static_library_abs-NOTFOUND

With the latter thing said, should we be trying to find_library them at all?

Probably not - and since the target has never worked I think we could remove it and backport it.

@sloretz sloretz changed the title Fix bug where RenderSystem_GL location wasn't found with newer CMake Remove broken rviz_ogre_vendor::RenderSystem_GL target Nov 4, 2022
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the sloretz__rviz_ogre_vendor__find_libs_without_prefix branch from e071244 to ef60930 Compare November 4, 2022 18:35
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

So this seems completely reasonable to me for Rolling. Though I might suggest that once you merge this, you immediately do a release of rviz into Rolling to ensure that there is no downstream breakage.

I'm more nervous about backporting it to Humble. Like you, I think nobody was actually depending on this, but I'm still worried about the side-effects. I'm not totally sure how to alleviate those worries, but I thought I would bring it up.

EricCousineau-TRI pushed a commit to RobotLocomotion/drake-ros that referenced this pull request Nov 7, 2022
@sloretz
Copy link
Contributor Author

sloretz commented Nov 7, 2022

CI (build: --packages-above-and-dependencies rviz_ogre_vendor test: --packages-above rviz_ogre_vendor)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status edit: unrelated networking issue, rerun Build Status

@sloretz sloretz merged commit c239954 into rolling Nov 7, 2022
@delete-merged-branch delete-merged-branch bot deleted the sloretz__rviz_ogre_vendor__find_libs_without_prefix branch November 7, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants