Skip to content

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