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

dartsim: make gz-common geospatial component optional #516

Closed
wants to merge 2 commits into from

Conversation

scpeters
Copy link
Member

🦟 Bug fix

Part of osrf/homebrew-simulation#2274.

Summary

There is currently an issue with gdal on macOS homebrew-core that causes multiple incompatible versions of protobuf to be in the dependency chain of formulae that depend on both gz-msgs9 and gz-common5. This currently has most Garden packages broken on macOS.

A temporary workaround is to disable the gdal dependency from the gz-physics6 homebrew formula until gdal is fixed. Currently, removing gdal from the dependency list of gz-common5 will prevent compilation of all gz-physics plugins because the geospatial component of gz-common5 is listed as required. This pull request changes the geospatial component of gz-common5 into an optional component.

I also noticed a configuration error when gdal could not be found and fixed it in ea8c7cb.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

@scpeters scpeters added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Jun 24, 2023
@scpeters scpeters requested review from azeey and mxgrey as code owners June 24, 2023 05:56
@codecov
Copy link

codecov bot commented Jun 24, 2023

Codecov Report

Merging #516 (c78cdbc) into gz-physics6 (815896d) will decrease coverage by 0.70%.
The diff coverage is n/a.

❗ Current head c78cdbc differs from pull request most recent head 8ba5edc. Consider uploading reports for the commit 8ba5edc to get more accurate results

@@               Coverage Diff               @@
##           gz-physics6     #516      +/-   ##
===============================================
- Coverage        76.18%   75.49%   -0.70%     
===============================================
  Files              142      140       -2     
  Lines             7251     7242       -9     
===============================================
- Hits              5524     5467      -57     
- Misses            1727     1775      +48     
Impacted Files Coverage Δ
dartsim/src/ShapeFeatures.cc 68.88% <ø> (-10.56%) ⬇️

... and 5 files with indirect coverage changes

$<TARGET_FILE:${PROJECT_LIBRARY_TARGET_NAME}-${PHYSICS_ENGINE_NAME}-plugin>
--gtest_output=xml:${CMAKE_BINARY_DIR}/test_results/${target_name}.xml
)
if(NOT SKIP_${PHYSICS_ENGINE_NAME} AND NOT INTERNAL_SKIP_${PHYSICS_ENGINE_NAME})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is INTERNAL_SKIP_* part of the public API of gz-cmake? The prefix INTERNAL_ made me think it wasn't, but I'm not sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

The INTERNAL_SKIP_* variables are set when dependencies that are REQUIRED_BY a particular component are not found (see GzFindPackage.cmake). It's not documented behavior, so it's brittle to depend on, but I couldn't figure out a better way, other than reproducing all the REQUIRED_BY logic within this file, which would be a pain. I initially tried if(TARGET ${PROJECT_LIBRARY_TARGET_NAME}-${PHYSICS_ENGINE_NAME}-plugin), but that didn't work because the test folder is added by GzConfigureBuild.cmake before the component folders are added and the corresponding targets created. Reversing this order in gz-cmake is possible, but it may cause other problems, and would require a gz-cmake release. Using INTERNAL_SKIP_* seems least bad to me, though it's partly moot if we can't apply a similar change to gz-rendering

@scpeters
Copy link
Member Author

This isn't needed since osrf/homebrew-simulation#2274 has been resolved. Let me now if you'd like to make this change or just close the PR

@scpeters
Copy link
Member Author

I'll close this for now; we can reopen if it becomes a priority

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.

2 participants