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

Be louder when graphics is missing for geospatial #573

Merged
merged 4 commits into from
Jan 29, 2024

Conversation

mjcarroll
Copy link
Contributor

🦟 Bug fix

Summary

Users building from source could get in a situation where dependencies of the graphics component weren't available, so that the component is not created. In this case, the geospatial component still has a hard dependency on the graphics component, but it is still attempting to build.

This adds gating logic to prevent the geospatial component from building when the graphics component isn't build and adds a warning message for the user.

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Jan 20, 2024
Copy link

codecov bot commented Jan 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bc24ece) 83.65% compared to head (660fda4) 83.65%.

Additional details and impacted files
@@             Coverage Diff             @@
##           gz-common5     #573   +/-   ##
===========================================
  Coverage       83.65%   83.65%           
===========================================
  Files              92       92           
  Lines           10285    10285           
===========================================
  Hits             8604     8604           
  Misses           1681     1681           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

The warning is much more obvious and it skips building geospatial if graphics do not exist. Thanks!

image

I'll create another bug for the follow up (assuming it's optional) since it's a different problem.


gz_add_component(geospatial
SOURCES ${sources}
DEPENDS_ON_COMPONENTS graphics
Copy link
Member

Choose a reason for hiding this comment

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

I thought the DEPENDS_ON_COMPONENTS parameter would generate a complaint if the needed component wasn't available, but apparently it doesn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was also my understanding, but does not seem to be the case.

@scpeters
Copy link
Member

scpeters commented Jan 22, 2024

an alternative would be to change every find_package call with REQUIRED_BY graphics to REQUIRED_BY graphics geospatial; not sure that it makes more sense though

EDIT: it's what we do in gz-physics

@mjcarroll
Copy link
Contributor Author

an alternative would be to change every find_package call with REQUIRED_BY graphics to REQUIRED_BY graphics geospatial; not sure that it makes more sense though

It would be the other way around, correct? Anyone who requires geospatial also will require graphics.

@mjcarroll
Copy link
Contributor Author

This is failing, on focal because the folders are being processed geospatial then graphics

@mjcarroll mjcarroll self-assigned this Jan 29, 2024
@mjcarroll mjcarroll merged commit 3b81541 into gz-common5 Jan 29, 2024
15 checks passed
@mjcarroll mjcarroll deleted the mjcarroll/missing_dependency branch January 29, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants