Update the FindTBB cmake module

While everything is found in a local setup, TBB is not found in any of the CI docker images. Needs to be investigated.

There is a "small" issue with the pkg-config support in debian:10. It seems that the FindPkgConfig.cmake module in cmake 3.13 is buggy. It does not find the installed tbb version correctly. More precisely: tbb requires libatomic, a gcc library that needs to be linked by -latomic without an explicit path. This is not detected correctly by pkg_check_modules in cmake 3.13

added 1 commit

579a56b1 - Add backport of FindPkgConfig cmake module

Compare with previous version

changed the description

requested review from @gruenich

@gruenich Could you have a look? You were involved in the old discussion in the cmake playground.

added 1 commit

96c009de - Add Changelog entry reporting about the backported FindPkgConfig

Compare with previous version

added 1 commit

9b7b4907 - correct typo in Changelog

Compare with previous version

We agreed on bumping the TBB version. Or is the pkg-providing TBB the compromise?

I read the changes, and they look good to me. Do you mind squashing the last three (or at least last two) commits?
I tested the branch and it works for me. Tested with openSuse 15.2, TBB installed from distribution packages, version TBB 2019.9, with CMake config file as indicated by output.

One thing that is missing (but is missing for, e.g., suitesparse and maybe others too) is a test within dune-common that tbb works. We can just conclude that it is found by cmake, but not if all include paths and necessary link libraries are set. This would only be caught if someone uses tbb.

Another question that bothers me: why do we search for tbb in dune-common at all? If it is not used there, it should not be search for. We can provide the FindTBB module if it should be used by other downstream modules, but currently I do not see a need to configure it for dune-common at all.

Maybe these two thought are independent of a merge and could be addressed later. I will just run this branch in the system test and then make a merge.

resolved all threads

added 1 commit

98bd7629 - Add backport of FindPkgConfig cmake module

Compare with previous version

enabled an automatic merge when the pipeline for 98bd7629 succeeds

mentioned in commit 7c5c1028

merged

mentioned in issue #199 (closed)

Just to summarize and have this documented:

Old FindPkgConfig.cmake failed if there where libraries in the *_LINK_LIBRARIES variable that were not located in the default CMake library search path, but in a directory where the compiler would search for the library anyway. In that case *_LINK_LIBRARIES would contain somthing like pkgcfg_lib_PkgConfigTBB_atomic-NOTFOUND after the trying to find the full path of the library. As result the *__LINK_LIBRARIES variable would be converted to OFF during the whether the library was detected.

A minimal invasive workaround might have been:

...
include(FindPackageHandleStandardArgs)
if(NOT ${_tbb}_LINK_LIBRARIES AND COMMAND _pkg_create_imp_target)
  list(TRANSFORM ${_tbb}_LINK_LIBRARIES REPLACE ".*_([-a-zA-Z]*)-NOTFOUND" "\\1")
  _pkg_create_imp_target(${_tbb} 1)
endif()
find_package_handle_standard_args("TBB"
...

The newer versions basically also just reuse the unresolved library name if something fails.

Maybe we should simply implement a dune_pkg_check_modules that calls pkg_check_modules and then does the workaround.

Thanks for documenting one of the problems with the old FindPkgConfig module.

The question is, what is a more stable solution for our find modules. My intention to include a full backport of the pkg-config module was to stay consistent with cmake: we have exactly the same behavior as the FindPkgConfig module in 3.19 and switch to newer versions provided by cmake if version >= 3.19.4. There are other things fixed in FindPkgConfig module since version 3.16 that are automatically included in the full backport.

But, on the other hand, a full backport is more difficult to port to other modules, e.g. if you want to include the FindTBB file in older implementations. Then, you could either require cmake >= 3.16 (I think with this version the relevant bug was fixed) or backport the file as well. I think, it should be documented more properly which cmake version is required for the FindTBB file or which backport needs to be included otherwise.

You minimal invasive fix might work. But it needs to be tested again on all platforms whether this fix introduces any behavior different from the default pkg-config behavior of newer cmake versions.

Can you elaborate on why you don't want to have a clean backport of the full module file?

Btw. the backport not only helps for the TBB module, but also for other upcoming changes like a proper export of pkg-config files for dune modules. Once we increase the minimal cmake version, we might simply drop the backport without any change necessary. Dropping a workaround function needs a deprecation period and updates in all modules that use that function.

The relevant change in FindPkgConfig in cmake 3.16 is:

Changed in version 3.16: If a full path to the found library can't be determined, but it's still 
visible to the linker, pass it through as -l<name>. Previous versions of CMake failed in this case.

This also applies to to the FindPython3 stuff.

It violates packaging policies on some Linux distributions. (Not that anybody would care)
In general there might might be dependencies on other CMake modules, which might lead to really hard to fix bugs.
As it is now I cannot reuse FindTBB.cmake without using the full DUNE chain as the CMake_MODULE_PATH is only modified in DuneMacros.cmake and never exported otherwise. Read: I cannot set(CMAKE_MODULE_PATH ${dune_common_CMAKE_MODULE_PATH} ${CMAKE_MODULE_PATH}) and then call find_package(TBB). This was quite surprising and took quite some time to realize. (Ideally other projects would be able to reuse our FindTBB.cmake just by addding the path with any knowledge of DUNE).
What if a downstream module previously decided that it also needs its own FindPkgConfig.cmake but in an insufficient version and that is now in the search path before ours?

To the very least 3) should be fixed, e.g. by adjusting the dune_common_MODULE_PATH in dune-common-config.cmake accordingly.

... also for other upcoming changes like a proper export of pkg-config files for dune modules

That might be. But maybe that can wait until there is proper support in currently used versions if it is not top priority.

If it means that people will be forced to use the full DUNE way if they use CMake, then I am not so sure whether that is a good decision.

These are good points that need be be addressed.

I also want to put all the necessary information needed by dune-common into its dune-common-config.cmake file. This is the cleaner way than using DuneMacros.cmake. You are right about this. This does not only hold true for this inclusion of CMAKE_MODULE_PATH directories, but also other information.

We backport c++ functions from more recent standard libraries all the time, isn't this similar? It is not a full software that is backported and included but just one function/module and should not (this needs to be fixed) influence any other software or module with its dependencies.
backporting a module must always guarantee that the backported code works with the required cmake versions and other provided code. There should not be any hidden dependencies and language requirements. If so, this would be a bug in the backport.
Maybe can be fixed by your suggestion to put the MODULE_PATH appendix to the dune-common-config.cmake file
This is a bit harder to fix. Need to think about this.

The FindTBB.cmake file simply has a requirement. It works directly with cmake >= 3.16 or if additionally the backported FindPkgConfig.cmake is included. This needs to be documented, if we stay with this solution. So, you can use it without any other DUNE cmake file and include it directly if these requirements are met. Maybe we add a proper test inside the FindTBB file that all version requirements are fulfilled.

I have made an attempt to remove the global modification of CMAKE_MODULE_PATH and add a search directory only where needed, see !1000 (merged). Not yet sure whether this already solves the problem.

Simon, thanks for your fix! I basically agree with you: Back-porting code should be a last resort, but it will fix over itself over time and is better compared to write wrappers that will bit-rot and make our code less standard and harder to understand.

Update the FindTBB cmake module

Summary

Details

See Also

Activity

Update the FindTBB cmake module

Summary

Details

See Also

Merge request reports

Activity