Set LINK_LIBRARIES_ONLY_TARGETS=TRUE by default and verify Trilinos is clean with CMake 3.23+

[bartlettroscoe]

As discussed in TriBITSPub/TriBITS#299 (comment), CMake 3.23 will learn the target property LINK_LIBRARIES_ONLY_TARGETS that if set to TRUE, will help catch errors when linking against undefined targets. With the updated TriBITS, raw library names for external packages/TPLs are no longer directly passed to target_link_libraries() and therefore, unless people are directly calling target_link_libraries() against raw lib names in Trilinos CMakeLists.txt files, we should be able to set LINK_LIBRARIES_ONLY_TARGETS to true with Trilinos.

But before I felt confident about that, Trilinos would need to upgrade its minimum CMake version to CMake 3.23+ and all of the Trilinos PR and ATDM Trilinos builds would need to be tested with this option enabled with the upgraded CMake.

[github-actions]

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity.
If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label.
If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.
If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

[bartlettroscoe]

@sebrowne, now that Trilinos is requiring CMake 3.23+, we can pull the trigger on this one. This will make the CMakeLists.txt files more robust and will be critical when refactoring to use raw library linking as listed in:

• EPIC: Features to remove to simplify TriBITS TriBITSPub/TriBITS#569

[sebrowne]

So to clarify, this will turn "nonexistant" targets called with target_link_libaries(myTarget <TYPE> someBadTarget) into configure errors instead of doing lld: error: -lsomeBadTarget not found at link time?

[bartlettroscoe]

So to clarify, this will turn "nonexistant" targets called with target_link_libaries(myTarget <TYPE> someBadTarget) into configure errors instead of doing lld: error: -lsomeBadTarget not found at link time?

@sebrowne, yes. From LINK_LIBRARIES_ONLY_TARGETS:

Enforce that link items that can be target names are actually existing targets.

Otherwise, if someone makes a mistake, you get a build-time error about a missing dependency (which is confusing for some people to figure out). That does not happen much with current TriBITS due to auto-linking between dependent package libraries but if Trilinos moves to manual linking (see TriBITSPub/TriBITS#569) then this will happen all of the time.

[sebrowne]

I've already encountered this in my work on SIERRA's new build system, and like this behavior much better. Fully in favor.

[bartlettroscoe]

FYI: The recent refactoring of the TriBITS legacy TPL system that creates proper CMake IMPORTED targets will allow Trilinos (and TriBITS) to set LINK_LIBRARIES_ONLY_TARGETS to TRUE by default and that will not break Trilinos configure scripts that are passing in raw library names.

[bartlettroscoe]

@sebrowne, @ccober6, and @jwillenbring, this is another one that should be turned on for Trilinos 15.0.

[sebrowne]

We'll get on it immediately (I'll write a small story and have the team take care of it prior to the release). Thanks for circling back!

[bartlettroscoe]

@sebrowne, FYI, I had to revert the merge of the PR:

• Framework: Enable CMAKE_LINK_LIBRARIES_ONLY_TARGETS #12362

with the revert PR:

• Revert "Framework: Enable CMAKE_LINK_LIBRARIES_ONLY_TARGETS" #12372

because it was breaking the installed ZoltanConfig.cmake file, see:

• Zoltan: missing target when building Albany #12371

First, we need to beef up the test TrilinosInstallTests_find_package_Trilinos defined at:

Trilinos/packages/TrilinosInstallTests/CMakeLists.txt

Line 239 in b3965b6

tribits_add_advanced_test(find_package_Trilinos

and:

Trilinos/packages/TrilinosInstallTests/find_package_Trilinos/CMakeLists.txt

Lines 1 to 11 in b3965b6

	cmake_minimum_required(VERSION 3.0)
	# Disable Kokkos warning about not supporting C++ extensions
	set(CMAKE_CXX_EXTENSIONS OFF)
	project(find_package_Trilinos C CXX)
	find_package(Trilinos REQUIRED)
	message("Trilinos_FOUND = '${Trilinos_FOUND}'")
	message("Trilinos_SELECTED_PACAKGES_LIST = '${Trilinos_SELECTED_PACAKGES_LIST}'")
	message("Trilinos_LIBRARIES = '${Trilinos_LIBRARIES}'")

to try to expose this failure. I am surprised that test passed in all of the PR builds shown here before it was merged.

We should try adding a dummy executable as part of that dummy CMake project find_package_Trilinos and linking in the Trilinos libraries and see if it triggers that error reported in #12371.

As for setting CMAKE_LINK_LIBRARIES_ONLY_TARGETS=ON without causing these errors, there are a couple of options we can consider off the top of my head:

Option-1: Set the property LINK_LIBRARIES_ONLY_TARGETS=OFF just for those two targets that are linking in raw m.

Option-2: Create a new Trilinos TriBITS TPL MLib with the trivial file FindTPLMLib.cmake that defines the MLib TPL to just be TPL_MLib_LIBRARIES=m and then make Zoltan and SuiteSparse depend on MLib.

Option-1 involves less code but it seems like kind of a hack. Option-2 involves a little more overhead (in a new file FindTPLMLib.cmake and installing a file MLibConfig.cmake that has to be included in downstream <Package>Config.cmake files) but it very clean consistent with how the rest of TriBITS TPLs work.

NOTE: One option not to try to export the created imported target because CMake does not support that :-(

[sebrowne]

@bartlettroscoe I added that case to the test and tested it, it does work and catch the issue.

[bartlettroscoe]

@sebrowne, now the question is how to deal with the m library. See options above.

But note that the current TriBITS implementation already checks that valid targets are used so that is why this issue with m was the only problem flagged when setting CMAKE_LINK_LIBRARIES_ONLY_TARGETS=TRUE.

[bartlettroscoe]

For now, we can put this back into the backlog since this is not worth fixing for now. If we get Trilinos internal packages using raw CMake, then we may want to try harder to turn this on and address the issue with the m library.