Naming of targets in third_party is non-standard

[claremacrae]

If the Catch2 project is included via CMake's add_subdirectory() or FetchContent, then the following targets are created, as far as I can tell:

• Catch2
• Catch2::Catch2

Unfortunately I didn't appreciate the significance of this when creating third_party/catch2/CMakeLists.txt - which creates the target catch

The name Catch2::Catch2 is preferred, as if that is missing, a warning is issued when CMake runs, making it easier to track down missing dependencies.

I think it should be possible to create aliases in the CMake files in third_party to retain the old naming, in case any users are already depending on the third_party target-names I created earlier.

[claremacrae]

Evolving list of actions:

Fix this for:

• [x] Catch2
• [x] doctest
• [x] Boost.ut

If possible, add warnings if user references old naming for:

• [x] Decided not to do in this ticket - Catch2
• [x] Decided not to do in this ticket - doctest
• [x] Decided not to do in this ticket - Boost.ut

Other stuff

• [x] Get this working with the starter project - Updating this is now part of the #85

[jwillikers]

@claremacrae, how about a developer warning when someone links against the old targets so that they can eventually be removed?

[claremacrae]

@athrun22 Good idea - do you have any idea where I should look to find out how to do that?

[claremacrae]

(Content moved to comment above)

[jwillikers]

@claremacrae This is an interesting predicament 🧐

CMake support for a library deprecation feature is actively being developed.

Off the top of my head, an interface library could be used to transitively link to the required library and an add_custom_command(TARGET) could be used to issue a warning message POST_BUILD using the cmake -E echo feature. I'll write up an example when I have time.

[jwillikers]

@claremacrae See below for an example for Catch2. Be warned that I have not had a chance to test it yet.

add_library(catch INTERFACE)
target_link_libraries(catch INTERFACE Catch2::Catch2)
add_custom_command(TARGET catch POST_BUILD COMMAND ${CMAKE_EXECUTABLE} ARGS -E echo "ApprovalTests: CMake target 'catch' is deprecated and may be removed in the future. Please link to the target 'Catch2::Catch2' instead.")

[claremacrae]

I thought I had got these all implemented correctly, and just needed to add the warnings...

Then I added these lines for a project that pulled Boost.ut in via FetchContent_Declare()

    if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
        # Turn off some checks for issues that should be fixed in ApprovalTests code
        target_compile_options(boost.ut INTERFACE
                -Wno-newline-eof
                -Wno-shadow-field-in-constructor
                -Wno-weak-vtables
                )
    endif()

And then I went to add the same lines to ApprovalTests.cpp/third_party/ut/CMakeLists.txt and got:

CMake Error at third_party/ut/CMakeLists.txt:12 (target_compile_options):
  target_compile_options can not be used on an ALIAS target.

So I need to check how I added the aliases - to ensure that I use the same combination of target and alias names as the library I am mimicking.

[claremacrae]

@claremacrae See below for an example for Catch2. Be warned that I have not had a chance to test it yet.

add_library(catch INTERFACE)
target_link_libraries(catch INTERFACE Catch2::Catch2)
add_custom_command(TARGET catch POST_BUILD COMMAND ${CMAKE_EXECUTABLE} ARGS -E echo "ApprovalTests: CMake target 'catch' is deprecated and may be removed in the future. Please link to the target 'Catch2::Catch2' instead.")

Thanks @jwillikers

I tried that out and got:

CMake Error at third_party/catch2/CMakeLists.txt:13 (add_custom_command):
  Target "catch" is an INTERFACE library that may not have PRE_BUILD,
  PRE_LINK, or POST_BUILD commands.

[jwillikers]

@claremacrae Oops... that does make sense. My other thought was to compile in a header file with the catch interface target that issued a compiler warning. I should be able to take a closer look at this soon.

[claremacrae]

Thanks!

[claremacrae]

@jwillikers on second thoughts, I'd quite like to separate out any warnings about using old target names to a separate ticket, if it's important to do, and to not include as part of this ticket...

That way, I can press on sooner with releasing recent improvements.

[claremacrae]

Remaining:

• [x] Merge third_party_targets_70 branch to master

[jwillikers]

@claremacrae 💡 Great idea!