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

Don't install deprecated parser_urdf.hh header #276

Merged
merged 3 commits into from
May 15, 2020

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented May 12, 2020

The URDF2SDF class in parser_urdf.hh was deprecated in bitbucket pr #658, and I believe the plan was to remove it from the public API in libsdformat10, so this PR removes it from the list of installed headers and moves it to the src folder.

I included two additional changes:

I did not remove the SDFORMAT_VISIBLE macro for URDF2SDF as it was starting to take a while to figure out how to fix it, so I just left that part out. I removed the SDFORMAT_VISIBLE macro in b763472.

@scpeters scpeters requested a review from azeey May 12, 2020 01:06
@scpeters scpeters force-pushed the privatize_deprecated_header branch from 04f84ec to 5fb487b Compare May 12, 2020 01:35
@scpeters
Copy link
Member Author

the code_check script apparently doesn't like the visibility macro still being there:

@codecov-io
Copy link

codecov-io commented May 12, 2020

Codecov Report

Merging #276 into master will increase coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
+ Coverage   86.44%   86.67%   +0.23%     
==========================================
  Files          59       59              
  Lines        9066     9066              
==========================================
+ Hits         7837     7858      +21     
+ Misses       1229     1208      -21     
Impacted Files Coverage Δ
src/SDFExtension.hh 100.00% <ø> (ø)
src/parser.cc 77.70% <ø> (ø)
src/parser_urdf.cc 80.21% <ø> (+1.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b80e070...9e7e222. Read the comment docs.

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM! For the symbol visibility issue, I think parser_urdf_TEST.cc needs to be compiled with parser_urdf.cc and SDFExtention.cc similar to what we're doing for Convter_TEST.cc. I think it will also need to link against liburdf*.

@scpeters scpeters force-pushed the privatize_deprecated_header branch from 2f840ca to b763472 Compare May 13, 2020 00:11
@scpeters
Copy link
Member Author

For the symbol visibility issue, I think parser_urdf_TEST.cc needs to be compiled with parser_urdf.cc and SDFExtention.cc similar to what we're doing for Convter_TEST.cc. I think it will also need to link against liburdf*.

good call; I was stuck before, but I think it's working now as of b763472

@scpeters
Copy link
Member Author

there were code check complaints about unused private functions, so I made them public (within the private header) and used them in a unit test in 330e6a5

@scpeters
Copy link
Member Author

@osrf-jenkins run tests please

@azeey
Copy link
Collaborator

azeey commented May 15, 2020

Looks good!

* Remove unused includes of parser_urdf.hh in tests
* mv include/sdf/parser_urdf.hh src, since the contents
  of this header were deprecated in 9.2
* parser_urdf.hh: remove deprecation macros and warning suppression
* parser_urdf.hh: remove visibility macro
* codecheck: helper functions public, use in test

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters force-pushed the privatize_deprecated_header branch from 330e6a5 to 9e7e222 Compare May 15, 2020 20:53
@scpeters
Copy link
Member Author

I just squashed and will "rebase and merge" once CI clears

@scpeters scpeters merged commit 0c01884 into gazebosim:master May 15, 2020
@scpeters scpeters deleted the privatize_deprecated_header branch May 15, 2020 21:25
scpeters added a commit to scpeters/sdformat that referenced this pull request Jun 27, 2020
scpeters added a commit that referenced this pull request Jun 30, 2020
* Remove all deprecated *PoseFrame() APIs
* Remove deprecated Pose(), SetPose() APIs
* Migration guide: update per #244, #276
* Changelog for #308.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit to scpeters/sdformat that referenced this pull request Jun 30, 2020
…#308)

* Remove all deprecated *PoseFrame() APIs
* Remove deprecated Pose(), SetPose() APIs
* Migration guide: update per gazebosim#244, gazebosim#276
* Changelog for gazebosim#308.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit to scpeters/sdformat that referenced this pull request Jun 30, 2020
…#308)

* Remove all deprecated *PoseFrame() APIs
* Remove deprecated Pose(), SetPose() APIs
* Migration guide: update per gazebosim#244, gazebosim#276
* Changelog for gazebosim#308.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit that referenced this pull request Jul 2, 2020
* Remove all deprecated *PoseFrame() APIs
* Remove deprecated Pose(), SetPose() APIs
* Migration guide: update per #244, #276
* Changelog for #308.

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters mentioned this pull request Nov 25, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants