Skip to content

Fix test compilation with USE_INTERNAL_URDF#800

Merged
scpeters merged 5 commits intogazebosim:sdf10from
scpeters:fix_build_with_internal_urdf
Dec 27, 2021
Merged

Fix test compilation with USE_INTERNAL_URDF#800
scpeters merged 5 commits intogazebosim:sdf10from
scpeters:fix_build_with_internal_urdf

Conversation

@scpeters
Copy link
Copy Markdown
Member

@scpeters scpeters commented Dec 23, 2021 •
edited
Loading

🦟 Bug fix

Fixes compilation with USE_INTERNAL_URDF == True

Summary

I noticed that the 10.7.0~pre1 debbuilds failed to compile some tests with complaints about finding urdf headers:

src/parser_urdf.cc:30:10: fatal error: urdf_model/model.h: No such file or directory
 #include <urdf_model/model.h>
          ^~~~~~~~~~~~~~~~~~~~
compilation terminated.
src/CMakeFiles/UNIT_ParamPassing_TEST.dir/build.make:257: recipe for target
 'src/CMakeFiles/UNIT_ParamPassing_TEST.dir/parser_urdf.cc.o' failed

It turns out the debbuilds are using USE_INTERNAL_URDF == TRUE since pkg-config is not listed as a build-depend in the debian metadata. I will resolve that shortly. In the meantime, this ensures that the USE_INTERNAL_URDF logic for include and windows compiler definitions used for libsdformat10 is repeated for tests that add parser_urdf.cc via target_sources.

Update: I've simplified the cmake logic using an interface library called using_parser_urdf.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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
Fix test compilation with USE_INTERNAL_URDF
3df36ec 
The USE_INTERNAL_URDF logic for include and
windows compiler definitions needs to be repeated
for tests that add parser_urdf.cc via target_sources.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters requested a review from j-rivero December 23, 2021 06:55
@scpeters scpeters requested a review from azeey as a code owner December 23, 2021 06:55
@scpeters scpeters mentioned this pull request Dec 23, 2021
Closed
@scpeters
remove accidental target_include_directories
5c5894b 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 23, 2021 •
edited
Loading

Codecov Report

Merging #800 (b50c3ce) into sdf10 (1a2c406) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf10     #800   +/-   ##
=======================================
  Coverage   88.30%   88.30%           
=======================================
  Files          66       66           
  Lines       10571    10571           
=======================================
  Hits         9335     9335           
  Misses       1236     1236

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 1a2c406...b50c3ce. Read the comment docs.

j-rivero
j-rivero reviewed Dec 24, 2021
View reviewed changes
src/CMakeLists.txt
@@ -62,7 +62,13 @@ if (BUILD_TESTING)
endif()

if (TARGET UNIT_ParamPassing_TEST)
Copy link
Copy Markdown
Contributor

@j-rivero j-rivero Dec 24, 2021

Choose a reason for hiding this comment

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

Looking at the nested ifs and the target_ commands, is this check now superfluous and can be removed?

Copy link
Copy Markdown
Contributor

@j-rivero j-rivero Dec 24, 2021

Choose a reason for hiding this comment

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

if that is the case we can probably join both instructions related to UNIT_parser_urdf_TEST and UNIT_ParamPassing_TEST after the same if (USE_INTERNAL_URDF) clause.

Copy link
Copy Markdown
Member Author

@scpeters scpeters Dec 27, 2021

Choose a reason for hiding this comment

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

this block is inside if(BUILD_TESTING), but this test does not run on windows, so I think it is important to check that the test target exists. I think there is a more elegant way to express this; let me think about it...

Copy link
Copy Markdown
Member Author

@scpeters scpeters Dec 27, 2021

Choose a reason for hiding this comment

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

ok I've simplified it quite a bit in 2ba37d8 by using a cmake interface library

Copy link
Copy Markdown
Member Author

@scpeters scpeters Dec 27, 2021

Choose a reason for hiding this comment

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

I believe I've addressed this comment and will merge so I can prepare another prerelease pull request

j-rivero
j-rivero approved these changes Dec 24, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Minor comment, looks good.

@scpeters
Deduplicate cmake logic with interface library
2ba37d8 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
fix interface linking
de0054c 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
move using_parser_urdf out of BUILD_TESTING
b50c3ce 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Dec 27, 2021

@osrf-jenkins run tests please

This was referenced Dec 27, 2021
Merged
Merged
@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Dec 27, 2021

@osrf-jenkins run tests please

@scpeters scpeters merged commit cf81994 into gazebosim:sdf10 Dec 27, 2021
@scpeters scpeters deleted the fix_build_with_internal_urdf branch December 27, 2021 08:30
@osrf-triage
Copy link
Copy Markdown

osrf-triage commented Jan 10, 2022

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

@osrf-triage
Copy link
Copy Markdown

osrf-triage commented Mar 25, 2022

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

@osrf-triage
Copy link
Copy Markdown

osrf-triage commented Apr 13, 2022

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@j-rivero j-rivero j-rivero approved these changes

@azeey azeey Awaiting requested review from azeey azeey is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@scpeters @codecov-commenter @osrf-triage @j-rivero @chapulina