View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0031667 | Community | OCCT:Configuration | public | 2020-07-16 12:02 | 2023-09-29 17:43 |
| Reporter | craffael | Assigned To | |||
| Priority | normal | Severity | feature | ||
| Status | new | Resolution | open | ||
| Platform | Linux | OS | Ubuntu | ||
| Target Version | Unscheduled | ||||
| Summary | 0031667: Configuration, CMake - Dependencies between targets are not specified for static builds and not public | ||||
| Description | In the CMake buildsystem, the library dependencies of a toolkit target are only registered if BUILD_SHARD_LIBS=On and they are not added to the "public cmake interface" (i.e. the public keyword is missing in target_link_libraries). I understand that this may be the best option if cmake version 2 is used. But I would say nowadays most users are using Cmake 3 and in Cmake 3 you can declare the dependencies of a cmake library target as public. This has the big advantage, that dependencies are transitively included. Example: Consider a cmake script which finds occt with find_package(...) and then links to the TKBrep target (with target_link_libraries). Until now this meant, that we also needed to link to TKMath, TKernel, TKG2d, TKG3d and TKGeomBase. With my proposed patch, it suffices to link to TKBrep only. The links to TKMath, TKernel, TKG2d, TKG3d and TKGeomBase are then automatically added thanks to the "PUBLIC" keyword in target_link_libraries. The proposed patch only adds the public keyword if cmake 3 or higher is used. Otherwise we fallback to the original behavior. | ||||
| Tags | No tags attached. | ||||
| Test case number | |||||
|
|
Branch CR31667 has been created by craffael. SHA-1: 61f7249b4c822ef0f9c4a2060feb7d895b04590f Detailed log of new commits: Author: craffael Date: Thu Jul 16 11:12:39 2020 +0200 31667: target_link_libraries with public interface for cmake >= 3 |
|
|
|
|
|
-if (BUILD_SHARED_LIBS)
+if(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.0.0")
+ # Declare the dependencies public so that all code that uses this library automatically also links with the dependencies
+ target_link_libraries (${PROJECT_NAME} PUBLIC ${USED_TOOLKITS_BY_CURRENT_PROJECT} ${USED_EXTERNAL_LIBS_BY_CURRENT_PROJECT})
+elseif (BUILD_SHARED_LIBS)
target_link_libraries (${PROJECT_NAME} ${USED_TOOLKITS_BY_CURRENT_PROJECT} ${USED_EXTERNAL_LIBS_BY_CURRENT_PROJECT})
endif()
Shouldn't it be "INTERFACE" instead of "PUBLIC" in this context? |
|
|
I think "PUBLIC" is correct. See the following very detailed explanation: https://cmake.org/pipermail/cmake/2016-May/063400.html And now take for example TKMath which depends on TKernel. Clearly TKMath uses TKernel in its implementation and classes from TKernel appear in the header files of TKMath. E.g. Bnd_Sphere.hxx (and hence Bnd_Sphere.cxx) from TKMath includes Standard.hxx from TKernel. Therefore "PUBLIC" should to be the right keyword, or am I missing something? See also https://stackoverflow.com/questions/26037954/cmake-target-link-libraries-interface-dependencies . |
|
|
According to description, "PUBLIC" means linking + dependency. However, while building a static library, there is no linking happen, so that only "INTERFACE" looks reasonable to me. Where this information finally resides? Within one of generated .cmake files in lib/cmake/opencascade install path? How this information could be used in external project? |
|
|
Well, for what OCCT is concerned with, we could probably just as well use "INTERFACE" instead of "PUBLIC". If you want, I can also make this change to the PR? But I would like to point out, that I believe this to be slightly wrong on a conceptual level. With "conceptual", I mean the following implicit meaning behind PUBLIC and INTERFACE (see e.g. https://stackoverflow.com/a/26038443/2796908): - If your source files and your headers include the headers of another library, then it is a PUBLIC dependency. - If your header files but not your source files include the headers of another library, then it is an INTERFACE dependency. I think that most OCCT dependencies are of type "PUBLIC" (see my previous response). I agree with you, that when we only look at linking, using "PUBLIC" or "INTERFACE" with static libraries has the same effect (it even has the same effect as PRIVATE, see https://cmake.org/pipermail/cmake/2016-May/063400.html). However, when we use `target_link_libraries`, not only linking information will be propagated, but all "Usage Requirements". This includes e.g. also compile definitions or include directories. I think this is not very relevant for OCCT, but if e.g. the TKernel target would add a special compile command, then there would be a difference between target_link_libraries(TKMath PUBLIC TKernel) and target_link_libraries(TKMath INTERFACE TKernel). So to summarize: I think from a conceptual point of view it would be nicer to use "PUBLIC" because it conforms better to the way modern cmake works. But if you prefer "INTERFACE", I can also change the PR, I don't think this will induce any breaking changes. Note also that for shared libraries (BUILD_SHARED_LIBRARIES=On) the existing code seems to use implicitly the "PUBLIC" keyword (https://cmake.org/cmake/help/v3.0/command/target_link_libraries.html). |
|
|
> So to summarize: I think from a conceptual point of view it would be nicer to use "PUBLIC" because it conforms better to the way modern cmake works. The main concern is that I expect that using PUBLIC would actually require libraries being available and configured by CMake. But as far as I recall this has been explicitly disabled for static builds, so that it should be possible building OCCT without 3rd-party libraries at all - only header files are required for that (of course, libraries would be required for building application itself). Don't know if it is for better of worse... Though the main idea of the patch would require these libraries anyway - within PUBLIC or INTERFACE, so it shouldn't matter if we agree with the basic approach. |
|
|
Branch CR31667_1 has been created by kgv. SHA-1: 2fb4b68cfd45de6018408a41831152364270848c Detailed log of new commits: Author: kgv Date: Sat Feb 26 21:02:22 2022 +0300 0031667: Configuration, CMake - Dependencies between targets are not specified for static builds and not public target_link_libraries() is now set with public interface for CMake >= 3. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-07-16 12:02 | craffael | New Issue | |
| 2020-07-16 12:02 | craffael | Assigned To | => craffael |
| 2020-07-16 12:11 | git | Note Added: 0093149 | |
| 2020-07-16 12:15 | craffael | Assigned To | craffael => bugmaster |
| 2020-07-16 13:17 | kgv | Product Version | => 7.4.0 |
| 2020-07-16 13:17 | kgv | Target Version | => 7.5.0 |
| 2020-07-16 13:17 | kgv | Summary | Dependencies between cmake targets are not specified for static builds and not public => Configuration, CMake - Dependencies between targets are not specified for static builds and not public |
| 2020-07-16 13:19 | kgv | Severity | minor => feature |
| 2020-07-16 13:19 | kgv | Product Version | 7.4.0 => |
| 2020-09-11 15:40 |
|
Target Version | 7.5.0 => 7.6.0 |
| 2020-10-26 22:52 | craffael | Note Added: 0096285 | |
| 2021-08-23 23:35 | kgv | Note Added: 0103380 | |
| 2021-08-24 00:19 | craffael | Note Added: 0103383 | |
| 2021-08-24 00:48 | kgv | Note Added: 0103389 | |
| 2021-08-24 00:49 | kgv | Relationship added | related to 0030116 |
| 2021-08-25 18:42 | craffael | Note Added: 0103436 | |
| 2021-08-30 15:55 | kgv | Note Added: 0103548 | |
| 2021-08-30 15:56 | kgv | Note Edited: 0103548 | |
| 2021-08-30 15:57 | kgv | Note Edited: 0103548 | |
| 2021-10-29 17:07 | bugmaster | Target Version | 7.6.0 => 7.7.0 |
| 2022-02-26 21:31 | git | Note Added: 0107087 | |
| 2022-10-24 10:39 |
|
Target Version | 7.7.0 => 7.8.0 |
| 2023-08-01 15:09 | dpasukhi | Target Version | 7.8.0 => Unscheduled |
| 2023-09-29 17:43 | vglukhik | Assigned To | bugmaster => |
