Separate internal macro into KDevPlatformMacrosInternal.cmake
ClosedPublic

Authored by kossebau on Aug 9 2017, 2:18 PM.

Details

Summary

Also

  • add dox for KDEVPLATFORM_ADD_PLUGIN
  • revert "Do not use $<BUILD_INTERFACE> in installed KDevPlatformMacros.cmake"

Motivation for e32618d64ac4fadbdceb228326df63cfac4b8bc8 was that
kdevplatform_add_library seemed part of public macross, so same like
kdevplatform_add_plugin, due to being in installed & exported macros file.
Though with KDevPlatform_SOURCE_DIR & KDevPlatform_BINARY_DIR not
defined when the macro is imported in other projects, such $<BUILD_INTERFACE>
would be bogus.

Looking again, KDEVPLATFORM_VERSION & KDEVPLATFORM_LIB_SOVERSION would
also not be defined. And actually this macro might not be meant for
public consumption, given that a platform usually is defined by
a given set of libs, so 3rd-party libs should not want to use this macro
anyway. So moving the macro out of the installed macro file makes this
clear and prevents abuse.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Aug 9 2017, 2:18 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptAug 9 2017, 2:18 PM
kossebau added inline comments.Aug 9 2017, 2:42 PM
kdevplatform/cmake/modules/KDevPlatformMacrosPrivate.cmake
32–39 ↗(On Diff #17928)

Hm, on another look I am still wondering what the advantage of doing the $<BUILD_INTERFACE> variant is over just setting those include dirs globally.
It adds the very same non-lib-specific dirs to any lib.

And the ${KDevPlatform_*_DIR}/plugins ones even is rather a hack, as none of the libs these include dirs get set for actually provide those headers. That rule just makes sure that anyone using any of these libs also sees the additional headers provided by any plugins. While actually those wanting those additional headers from plugins should get a separate interface-only library target and link properly against it, no?

kfunk added a subscriber: kfunk.Aug 9 2017, 3:16 PM
kfunk added inline comments.
kdevplatform/cmake/modules/KDevPlatformMacrosPrivate.cmake
1 ↗(On Diff #17928)

Usually these files are called FooInternal.cmake [1] . Please use this pattern

% dpkg -S Private | grep cmake
% dpkg -S Internal | grep cmake                                                                                                                                        
kdelibs5-dev: /usr/share/kde4/apps/cmake/modules/FindKDE4Internal.cmake
libphonon-dev: /usr/share/phonon/buildsystem/FindPhononInternal.cmake
cmake-data: /usr/share/cmake-3.7/Modules/Compiler/IBMCPP-C-DetermineVersionInternal.cmake
cmake-data: /usr/share/cmake-3.7/Modules/Internal/FeatureTesting.cmake
cmake-data: /usr/share/cmake-3.7/Modules/Compiler/Clang-DetermineCompilerInternal.cmake
cmake-data: /usr/share/cmake-3.7/Modules/Compiler/IBMCPP-CXX-DetermineVersionInternal.cmake
libphonon4qt5-dev: /usr/share/phonon4qt5/buildsystem/FindPhononInternal.cmake
cmake-data: /usr/share/cmake-3.7/Modules/Internal
32–39 ↗(On Diff #17928)

Hm, on another look I am still wondering what the advantage of doing the $<BUILD_INTERFACE> variant is over just setting those include dirs globally.

It adds the very same non-lib-specific dirs to any lib.

Still feels cleaner to me. You might have a small test executable which does not link to any of these libs => then without this this test target might see include paths it is not interested in.

kossebau updated this revision to Diff 17931.Aug 9 2017, 3:32 PM
kossebau marked an inline comment as done.

rename to KDevPlatformMacrosInternal.cmake

kossebau retitled this revision from Separate private macro into KDevPlatformMacrosPrivate to Separate internal macro into KDevPlatformMacrosInternal.cmake.Aug 9 2017, 3:35 PM
kfunk accepted this revision.Aug 9 2017, 3:38 PM
This revision is now accepted and ready to land.Aug 9 2017, 3:38 PM
This revision was automatically updated to reflect the committed changes.
apol edited edge metadata.Aug 9 2017, 11:05 PM

The advantage of using BUILD_INTEFACE is that you're setting the paths depending on what you're linking against rather than depending on where they are located which is essentially arbitrary.

and yes, the /plugins part needs to be fixed, I didn't want to start moving things within the repository until we decide what the repository should look like in the new situation.