View Issue Details

IDProjectCategoryView StatusLast Update
0031667CommunityOCCT:Configurationpublic2022-02-26 21:31
Reportercraffael Assigned Tobugmaster  
PrioritynormalSeverityfeature 
Status newResolutionopen 
PlatformLinuxOSUbuntu 
Target Version7.7.0 
Summary0031667: Configuration, CMake - Dependencies between targets are not specified for static builds and not public
DescriptionIn 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.
TagsNo tags attached.
Test case number

Relationships

related to 0030116 newapn Open CASCADE Documentation - provide hints for use of OCCT in CMake-based projects 

Activities

git

2020-07-16 12:11

administrator   ~0093149

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

craffael

2020-10-26 22:52

reporter   ~0096285

@utverdov may I ask what is the reason that this has been postponed to 7.6.0?

kgv

2021-08-23 23:35

developer   ~0103380

-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?

craffael

2021-08-24 00:19

reporter   ~0103383

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 .

kgv

2021-08-24 00:48

developer   ~0103389

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?

craffael

2021-08-25 18:42

reporter   ~0103436

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).

kgv

2021-08-30 15:55

developer   ~0103548

Last edited: 2021-08-30 15:57

> 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.

git

2022-02-26 21:31

administrator   ~0107087

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.

Issue History

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 utverdov 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