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

Backport gz/sim.hh fix, add redirection header #1983

Merged
merged 2 commits into from
May 2, 2023

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented May 2, 2023

🦟 Bug fix

Backports #1978 and adds a missing header file

Summary

This backports a fix for using the gz/sim.hh header file, but this header file does not yet exist on the ign-gazebo3 branch as a consequence of how gz-cmake derives the auto-generated header name from the cmake project name. For ignition-gazebo*, the auto-generated header name is gz/gazebo.hh. Rather than try to modify gz-cmake, I've added a redirection header file from gz/sim.hh to gz/gazebo.hh for this branch. This should not be merged forward to gz-sim*. This is similar to the sdf/sdf.hh redirection header file used in SDFormat.

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 Rebase-and-merge.

Not all of the needed include paths are exported with
the gz-sim target, so including the gz/sim.hh header
doesn't work easily. This test fails to build and
illustrates the problem.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Needed since the auto-generated header is gz/gazebo.hh

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters requested a review from mjcarroll as a code owner May 2, 2023 20:16
@osrf-triage osrf-triage added this to Inbox in Core development May 2, 2023
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label May 2, 2023
@scpeters scpeters enabled auto-merge (rebase) May 2, 2023 20:20
@scpeters
Copy link
Member Author

scpeters commented May 2, 2023

there's a problem that the gz/gazebo.hh header isn't installed either

never mind I think it's being installed fine

@scpeters
Copy link
Member Author

scpeters commented May 2, 2023

there's a problem that the gz/gazebo.hh header isn't installed either

actually I think it's fine

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #1983 (f2a73cd) into ign-gazebo3 (2673c01) will not change coverage.
The diff coverage is n/a.

❗ Current head f2a73cd differs from pull request most recent head 87bb4c3. Consider uploading reports for the commit 87bb4c3 to get more accurate results

@@             Coverage Diff              @@
##           ign-gazebo3    #1983   +/-   ##
============================================
  Coverage        78.04%   78.04%           
============================================
  Files              255      255           
  Lines            15080    15080           
============================================
  Hits             11769    11769           
  Misses            3311     3311           

Core development automation moved this from Inbox to In review May 2, 2023
@scpeters
Copy link
Member Author

scpeters commented May 2, 2023

I think this will fix the abichecker build after it is merged, but the job currently fails in this PR when trying to compile the ign-gazebo3 branch

@mjcarroll mjcarroll disabled auto-merge May 2, 2023 21:30
@mjcarroll
Copy link
Contributor

We have been in this situation before. I think we can override the ABI checker if we think everything else is okay.

@scpeters
Copy link
Member Author

scpeters commented May 2, 2023

homebrew is also failing, but that should be fixed by #1967. I will merge this now, and we should see clean CI in #1967

@scpeters scpeters merged commit 875de1b into ign-gazebo3 May 2, 2023
Core development automation moved this from In review to Done May 2, 2023
@scpeters scpeters deleted the scpeters/gz_sim_header_3 branch May 2, 2023 21:31
// Since gz-cmake derives the auto-generated header name from the cmake
// project name, for ignition-gazebo*, the auto-generated header name
// is <gz/gazebo.hh>.
// This header file provides a redirection from <gz/sim.hh> to <gz/gazebo.hh>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to have gz/gazebo.hh in gz-sim that forwards to gz/sim.hh? Otherwise, if users start using the header with ign-gazebo3 or ign-gazebo6 and then want to migrate to gz-sim, they will encounter build errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, they would encounter build errors. I suppose we could add a gz/gazebo.hh header that redirects to gz/sim.hh and generates a deprecation warning when used

Copy link
Member Author

Choose a reason for hiding this comment

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

it doesn't seem likely to me but it is possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants