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.

Sign up for GitHub

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

Fix test compilation with USE_INTERNAL_URDF #800

Merged
merged 5 commits into from
Dec 27, 2021
Merged

Fix test compilation with USE_INTERNAL_URDF #800

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3df36ec
Fix test compilation with USE_INTERNAL_URDF
scpeters Dec 23, 2021
5c5894b
remove accidental target_include_directories
scpeters Dec 23, 2021
2ba37d8
Deduplicate cmake logic with interface library
scpeters Dec 27, 2021
de0054c
fix interface linking
scpeters Dec 27, 2021
b50c3ce
move using_parser_urdf out of BUILD_TESTING
scpeters Dec 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 24 additions & 31 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,26 @@ ign_get_libsources_and_unittests(sources gtest_sources)
# Add the source file auto-generated into the build folder from sdf/CMakeLists.txt
list(APPEND sources EmbeddedSdf.cc)

# Use interface library to deduplicate cmake logic for URDF linking
add_library(using_parser_urdf INTERFACE)
if (USE_INTERNAL_URDF)
set(sources ${sources}
urdf/urdf_parser/model.cpp
urdf/urdf_parser/link.cpp
urdf/urdf_parser/joint.cpp
urdf/urdf_parser/pose.cpp
urdf/urdf_parser/twist.cpp
urdf/urdf_parser/urdf_model_state.cpp
urdf/urdf_parser/urdf_sensor.cpp
urdf/urdf_parser/world.cpp)
urdf/urdf_parser/model.cpp
urdf/urdf_parser/link.cpp
urdf/urdf_parser/joint.cpp
urdf/urdf_parser/pose.cpp
urdf/urdf_parser/twist.cpp
urdf/urdf_parser/urdf_model_state.cpp
urdf/urdf_parser/urdf_sensor.cpp
urdf/urdf_parser/world.cpp)
target_include_directories(using_parser_urdf INTERFACE
${CMAKE_CURRENT_SOURCE_DIR}/urdf)
if (WIN32)
target_compile_definitions(using_parser_urdf INTERFACE -D_USE_MATH_DEFINES)
endif()
else()
target_link_libraries(using_parser_urdf INTERFACE
IgnURDFDOM::IgnURDFDOM)
endif()

if (BUILD_TESTING)
Expand Down Expand Up @@ -62,12 +72,9 @@ if (BUILD_TESTING)
endif()

if (TARGET UNIT_ParamPassing_TEST)
Copy link
Contributor

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
Contributor

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
Member Author

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
Member Author

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
Member Author

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

if (NOT USE_INTERNAL_URDF)
target_link_libraries(UNIT_ParamPassing_TEST
IgnURDFDOM::IgnURDFDOM)
endif()
target_link_libraries(UNIT_ParamPassing_TEST
TINYXML2::TINYXML2)
TINYXML2::TINYXML2
using_parser_urdf)
target_sources(UNIT_ParamPassing_TEST PRIVATE
Converter.cc
EmbeddedSdf.cc
Expand All @@ -90,12 +97,9 @@ if (BUILD_TESTING)
endif()

if (TARGET UNIT_parser_urdf_TEST)
if (NOT USE_INTERNAL_URDF)
target_link_libraries(UNIT_parser_urdf_TEST
IgnURDFDOM::IgnURDFDOM)
endif()
target_link_libraries(UNIT_parser_urdf_TEST
TINYXML2::TINYXML2)
TINYXML2::TINYXML2
using_parser_urdf)
target_sources(UNIT_parser_urdf_TEST PRIVATE
SDFExtension.cc
XmlUtils.cc
Expand All @@ -112,7 +116,8 @@ target_link_libraries(${PROJECT_LIBRARY_TARGET_NAME}
PUBLIC
ignition-math${IGN_MATH_VER}::ignition-math${IGN_MATH_VER}
PRIVATE
TINYXML2::TINYXML2)
TINYXML2::TINYXML2
using_parser_urdf)

if (WIN32)
target_compile_definitions(${PROJECT_LIBRARY_TARGET_NAME} PRIVATE URDFDOM_STATIC)
Expand All @@ -126,18 +131,6 @@ target_include_directories(${PROJECT_LIBRARY_TARGET_NAME}
${CMAKE_CURRENT_SOURCE_DIR}
)

if (USE_INTERNAL_URDF)
target_include_directories(${PROJECT_LIBRARY_TARGET_NAME}
PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}/urdf
)
if (WIN32)
target_compile_definitions(${PROJECT_LIBRARY_TARGET_NAME} PRIVATE -D_USE_MATH_DEFINES)
endif()
else()
target_link_libraries(${PROJECT_LIBRARY_TARGET_NAME} PRIVATE IgnURDFDOM::IgnURDFDOM)
endif()

if(NOT WIN32)
add_subdirectory(cmd)
endif()