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.

Sign up for GitHub

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

Jump to bottom

Add rviz_rendering dependency to rviz_common #727

Merged
merged 2 commits into from
Jul 15, 2021
Merged

Add rviz_rendering dependency to rviz_common #727

merged 2 commits into from
Jul 15, 2021

Conversation

rebecca-butler
Copy link
Contributor

I was unable to build rviz_common due to the following error in visualizer_app.cpp:

error: ‘get’ is not a member of ‘rviz_rendering::OgreLogging’
  280 |     rviz_rendering::OgreLogging::get()->useLogFileAndStandardOut();

Adding the missing dependency on rviz_rendering seems to fix the issue.

Signed-off-by: Rebecca Butler rebecca@openrobotics.org

@rebecca-butler
Add rviz_rendering dependency to rviz_common
0652a9f 
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
clalancette
clalancette reviewed Jul 14, 2021
View reviewed changes
rviz_common/CMakeLists.txt
@@ -246,6 +246,7 @@ ament_target_dependencies(rviz_common
rclcpp
resource_retriever
rviz_assimp_vendor
rviz_rendering
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I'm not opposed to doing this, but theoretically this shouldn't be needed; we have rviz_rendering::rviz_rendering as part of target_link_libraries above. Any ideas on why that is not sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

It seems more "ament-like" to do this and remove the target from target_link_libraries above. But I also do wonder why the existing logic is not working...

Copy link
Member

Choose a reason for hiding this comment

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

ah, so the version of rviz_rendering released into Rolling does not have the API rviz_rendering::OgreLogging::get(). So it must be that the rviz_common is not picking up the version in the workspace. FWIW, I also experience this build error.

Copy link
Member

Choose a reason for hiding this comment

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

I guess using ament_target_dependencies causes a re-ordering of compile flags that happens to make it work.

jacobperron
jacobperron reviewed Jul 14, 2021
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I would also remove rviz_rendering::rviz_rendering from target_link_libraries above since it is redundant.

@rebecca-butler
Remove rviz_rendering from target_link_libraries
da48000 
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
@rebecca-butler rebecca-butler merged commit 4a4da3e into ros2 Jul 15, 2021
@delete-merged-branch delete-merged-branch bot deleted the bug-ogre-logging branch July 15, 2021 19:09
@clalancette
Copy link
Contributor

Did full CI get run for this?

@rebecca-butler rebecca-butler restored the bug-ogre-logging branch July 15, 2021 19:42
rebecca-butler added a commit that referenced this pull request Jul 15, 2021
@rebecca-butler
Revert "Add rviz_rendering dependency to rviz_common (#727)"
2159aef 
This reverts commit 4a4da3e.
@rebecca-butler
Copy link
Contributor Author

@clalancette my mistake, I forgot to run CI. Should I revert this PR?

@clalancette
Copy link
Contributor

@clalancette my mistake, I forgot to run CI. Should I revert this PR?

Well, let's run CI now. If it passes green, great, we can just leave it. If it isn't green, then we should revert.

@rebecca-butler
Copy link
Contributor Author

build: --packages-up-to rviz_common

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@rebecca-butler
Copy link
Contributor Author

Looks like CI is green, so no need to revert. Thanks for pointing that out @clalancette!

@rebecca-butler rebecca-butler deleted the bug-ogre-logging branch July 15, 2021 20:57
@doisyg
Copy link

doisyg commented Aug 18, 2021

Could this be merged in galactic too ? I have the same issue on the galactic branch

rebecca-butler added a commit that referenced this pull request Aug 18, 2021
@rebecca-butler
Add rviz_rendering dependency to rviz_common (#727)
7588274 
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
@rebecca-butler rebecca-butler mentioned this pull request Aug 18, 2021
Merged
rebecca-butler added a commit that referenced this pull request Aug 19, 2021
@rebecca-butler
Add rviz_rendering dependency to rviz_common (#727) (#748)
ba05260 
Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
@jacobperron jacobperron mentioned this pull request Oct 12, 2021
Closed
jacobperron pushed a commit that referenced this pull request Oct 12, 2021
@rebecca-butler @jacobperron
Add rviz_rendering dependency to rviz_common (#727)
b1c2897 
* Add rviz_rendering dependency to rviz_common

Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>

* Remove rviz_rendering from target_link_libraries

Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>
@jacobperron jacobperron mentioned this pull request Oct 12, 2021
Merged
jacobperron added a commit that referenced this pull request Oct 18, 2021
@jacobperron @rebecca-butler
Add rviz_rendering dependency to rviz_common (#727) (#785)
6f23363 
* Add rviz_rendering dependency to rviz_common

Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>

* Remove rviz_rendering from target_link_libraries

Signed-off-by: Rebecca Butler <rebecca@openrobotics.org>

Co-authored-by: Rebecca Butler <rebecca@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette left review comments

@jacobperron jacobperron jacobperron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rebecca-butler @clalancette @doisyg @jacobperron