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

DARTConfig relies on exported targets rather than find_library. #630

Merged
merged 4 commits into from
Mar 16, 2016

Conversation

a-price
Copy link
Contributor

@a-price a-price commented Mar 8, 2016

Referencing the recommendations here: https://cmake.org/Wiki/CMake/Tutorials/How_to_create_a_ProjectConfig.cmake_file and here: https://cmake.org/pipermail/cmake/2011-June/045050.html, I changed the DARTConfig file to refer to the autogenerated targets in DARTTargets.cmake, rather than searching for them with find_library.

This resolves issue #629 on my machine. An updated test project is attached.
CMakeLists.txt
test.txt

@mkoval
Copy link
Collaborator

mkoval commented Mar 8, 2016

I have a two additional suggestions:

  1. Remove "@CMAKE_INSTALL_PREFIX@/lib" from DART_LIBRARY_DIRS; this is not necessary because we are using imported targets.
  2. Set a DART_${comp}_LIBRARY variable for each component in DART_FIND_COMPONENTS, in addition to adding the library to DART_LIBRARIES (e.g. see FindBoost.cmake)

Neither of these are critical, but it would be nice to follow CMake conventions where possible.

@mxgrey
Copy link
Member

mxgrey commented Mar 9, 2016

Just to clarify for my own edification, when we call set(var target), does that give the full path of target to var? What information is actually contained in a cmake target variable?

@a-price
Copy link
Contributor Author

a-price commented Mar 9, 2016

From my reading, the (autogenerated) DARTTargets.cmake file and the file(s) it includes, like DARTTargets-release.cmake, define a set of CMake targets with certain properties. For instance, the Release version of target "dart" is configured with these lines on my machine:

#Import target "dart" for configuration "Release"
set_property(TARGET dart APPEND PROPERTY IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(dart PROPERTIES
IMPORTED_LINK_INTERFACE_LIBRARIES_RELEASE "dart-core;/usr/lib/x86_64-linux-gnu/liburdfdom_sensor.so;/usr/lib/x86_64-linux-gnu/liburdfdom_model_state.so;/usr/lib/x86_64-linux-gnu/liburdfdom_model.so;/usr/lib/x86_64-linux-gnu/liburdfdom_world.so;/usr/lib/x86_64-linux-gnu/libconsole_bridge.so;/usr/lib/x86_64-linux-gnu/libtinyxml.so;/usr/lib/x86_64-linux-gnu/libtinyxml2.so"
IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib/libdart.so.5.1.1"
IMPORTED_SONAME_RELEASE "libdart.so.5.1"
)

list(APPEND _IMPORT_CHECK_TARGETS dart )
list(APPEND _IMPORT_CHECK_FILES_FOR_dart "${_IMPORT_PREFIX}/lib/libdart.so.5.1.1" )

So, CMake knows that there is a library called dart, that its location is ${_IMPORT_PREFIX}/lib/libdart.so.5.1.1, and that it has certain link dependencies listed in IMPORTED_LINK_INTERFACE_LIBRARIES_RELEASE.

When we set the variable DART_LIBRARIES, we're really just returning a list of strings that correspond to targets. It's the target_link_libraries command that actually does the heavy lifting of associating the end-user's target with the one's we've defined in the variable.

@mkoval, feel free to chime in if I've misstated the process.

@mkoval
Copy link
Collaborator

mkoval commented Mar 9, 2016

Yes, @a-price's explanation is completely correct.

DART_LIBRARIES will contain the string dart. It's important to remember that this is the name of a target, not a library. The name of the .so file may not even be dart.so on disk, if you happened to use set_target_properties to change the target's OUTPUT_NAME.

CMake is smart enough to figure this out if you use target_link_libraries to link with the dart target.

@a-price
Copy link
Contributor Author

a-price commented Mar 9, 2016

Per suggestion, I added the DART_<COMP>_LIBRARY variable definition. I'm not sure I understand what the correct value of DART_LIBRARY_DIRS should be, though.

@mkoval
Copy link
Collaborator

mkoval commented Mar 9, 2016

I don't think we need to set DART_LIBRARY_DIRS at all. All of the necessary libraries, including transitive dependencies, are attached to the imported target.

@jslee02 jslee02 added this to the DART 6.0.0 milestone Mar 12, 2016
@jslee02
Copy link
Member

jslee02 commented Mar 15, 2016

I agree with @mkoval . @a-price Could you remove DART_LIBRARY_DIRS? Thanks!

@jslee02
Copy link
Member

jslee02 commented Mar 16, 2016

👍

1 similar comment
@mxgrey
Copy link
Member

mxgrey commented Mar 16, 2016

👍

@mkoval
Copy link
Collaborator

mkoval commented Mar 16, 2016

If there are no objections, can we merge this?

@jslee02
Copy link
Member

jslee02 commented Mar 16, 2016

Sure, merging.

jslee02 added a commit that referenced this pull request Mar 16, 2016
DARTConfig relies on exported targets rather than find_library.
@jslee02 jslee02 merged commit 405e5ac into dartsim:master Mar 16, 2016
jslee02 added a commit that referenced this pull request Apr 24, 2016
jslee02 added a commit that referenced this pull request Apr 25, 2016
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.

4 participants