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

Add namespace fcl to exported CMake targets #518

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rickertm
Copy link
Contributor

@rickertm rickertm commented Jan 5, 2021

This patch adds the namespace fcl:: to the exported CMake target fcl (resulting in fcl::fcl) as recommended by CMake for imported targets (see CMake documentation A Sample Find Module and CMP0028):

When providing imported targets, these should be namespaced (hence the Foo:: prefix); CMake will recognize that values passed to target_link_libraries() that contain :: in their name are supposed to be imported targets (rather than just library names), and will produce appropriate diagnostic messages if that target does not exist (see policy CMP0028).

The use of double-colons is a common pattern used to namespace IMPORTED targets and ALIAS targets. When computing the link dependencies of a target, the name of each dependency could either be a target, or a file on disk. Previously, if a target was not found with a matching name, the name was considered to refer to a file on disk. This can lead to confusing error messages if there is a typo in what should be a target name.


This change is Reviewable

@traversaro
Copy link
Contributor

traversaro commented Jan 5, 2021

As this is effectively a breaking change for downstream projects that link against the fcl target, could it make sense to add a add_library(fcl ALIAS fcl::fcl) instruction to fcl-config.cmake.in? Eventually by adding a deprecation message if somebody still uses it, something like:

add_library(fcl ALIAS fcl::fcl)
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.17)
  set_property(TARGET fcl PROPERTY DEPRECATION "Deprecated target. Please use fcl::fcl instead.")
endif()

@rickertm
Copy link
Contributor Author

rickertm commented Jan 5, 2021

I didn't want to include an alias without any warning, but thank you for pointing me to the new DEPRECATION property.

It doesn't work on an ALIAS target

CMake Error at CMake/fcl-config.cmake:47 (set_property):
  set_property can not be used on an ALIAS target.

but this would work (or using VERSION_LESS for CMake <3.7):

add_library(fcl INTERFACE IMPORTED)
set_target_properties(fcl PROPERTIES INTERFACE_LINK_LIBRARIES "fcl::fcl")
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.17)
  set_property(TARGET fcl PROPERTY DEPRECATION "Deprecated target. Please use fcl::fcl instead.")
endif()

@traversaro
Copy link
Contributor

Great, thanks! I think fcl requires CMake 3.10, so using VERSION_GREATER_EQUAL is ok.

@rickertm
Copy link
Contributor Author

rickertm commented Jan 5, 2021

Great, I'll update the PR accordingly then.

While fcl requires CMake 3.10, a consumer project of the config file can have a lower version. In this case, this would lead to the following error:

CMake Error at CMake/fcl-config.cmake:46 (if):
  if given arguments:

    "CMAKE_VERSION" "VERSION_GREATER_EQUAL" "3.17"

Changing this to use VERSION_LESS however works fine on a project with CMake 3.0 linking against fcl (first version with CMakeFindDependencyMacro module).

@traversaro
Copy link
Contributor

While fcl requires CMake 3.10, a consumer project of the config file can have a lower version. In this case, this would lead to the following error:

Thanks, indeed I was not thinking of that case.

@traversaro
Copy link
Contributor

traversaro commented Jan 7, 2021

Probably for people that consume fcl via CMake's FetchContent module, it could also make sense to add a fcl::fcl alias target for the build, i.e. add in

:

add_library(${PROJECT_NAME}::${PROJECT_NAME} ALIAS ${PROJECT_NAME})

@rickertm
Copy link
Contributor Author

rickertm commented Jan 7, 2021

Good point, I've just added this to the PR.

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.

2 participants