Add LibAlkimia[5]_INCLUDE_DIR to fix build issues with static libraries
AbandonedPublic

Authored by habacker on Mar 21 2018, 7:56 AM.

Details

Summary

In cases where libraries are defined without library dependencies,
such as when creating static libraries, by default the location of the
alkimia include files is not added for compilation, resulting in compile
errors. This can be fixed by adding LibAlkimia[5]_INCLUDE_DIR to the list
of specified include directories. The camel case style of the the first
part of this variable name has been choosen to match the name given to
find_package() for finding alkimia.

For compatibility with old code the deprectated variable LIBALKIMIA_INCLUDE_DIR
has been reintroduced.

Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>

Diff Detail

Repository
R471 Alkimia Library
Branch
7.0
Lint
No Linters Available
Unit
No Unit Test Coverage
habacker requested review of this revision.Mar 21 2018, 7:56 AM
habacker created this revision.
habacker updated this revision to Diff 30096.Mar 21 2018, 8:03 AM
  • removed deprecated message - it is enough to doc this in the readme file

I followed
mailing list
and got that:

  1. issue is with kmm_storage,
  2. issue is because of alkimia,
  3. issue is when cross compiling for MS Windows.

Trying to compile KMM5 on MS Windows I also encountered issues with kmm_storage (which were not issues on Linux). I didn't relate them to Alkimia but to the fact that kmm_storage is strangely used i.e. it's a convenient static library linked to dynamic kmm_mymoney, but also heavily using kmm_mymoney itself. I identified it as some kind of unwanted circular dependency and restructured KMM build system to make it compile on MS Windows. The patch can be seen here

As far as I don't see any harm in proposed solution, I think we may not fix the root cause of build issue. I would suggest making changes in 4.8 as it has been done in 5.0 and see if it helps.

krop added a subscriber: krop.Mar 28 2018, 5:17 PM

This patch is not fixing the issue correctly.

The correct way to populate the interface is to use target_include_directories in the CMakeLists.txt that creates this library (ie: alkimia/src)

target_include_directories(alkimia INTERFACE "$<INSTALL_INTERFACE:${INCLUDE_INSTALL_DIR}/alkimia>")

if the code using this library is supposed to #include <someheader>

or

target_include_directories(alkimia INTERFACE "$<INSTALL_INTERFACE:${INCLUDE_INSTALL_DIR}>")

if it's supposed to be #include <alkimia/someheader>

There are also quite wrong things done in this file (but they could be fixed in a different patch) :

"NAMESPACE Alkimia::" shall be replaced with an alias.

In D11539#236003, @cgiboudeaux wrote:

This patch is not fixing the issue correctly.
The correct way to populate the interface is to use target_include_directories in the CMakeLists.txt that creates this library (ie: alkimia/src)

There are reasons, why the patch is at is. See D10043 for more details.

There are also quite wrong things done in this file (but they could be fixed in a different patch) :

"NAMESPACE Alkimia::" shall be replaced with an alias.

why ?

krop added a comment.Mar 28 2018, 9:45 PM
In D11539#236003, @cgiboudeaux wrote:

This patch is not fixing the issue correctly.
The correct way to populate the interface is to use target_include_directories in the CMakeLists.txt that creates this library (ie: alkimia/src)

There are reasons, why the patch is at is. See D10043 for more details.

The alkimia build system is such a mess that it should be reworked from scratch. I'm not surprised.

In D11539#236100, @cgiboudeaux wrote:

The alkimia build system is such a mess that it should be reworked from scratch. I'm not surprised.

Volunteers are always welcome. If you have the necessary foo to do it, go for it. This is what Phabricator is for.

In D11539#236003, @cgiboudeaux wrote:

This patch is not fixing the issue correctly.
The correct way to populate the interface is to use target_include_directories in the CMakeLists.txt that creates this library (ie: alkimia/src)

Using target_include_directories setup target alkimia property INTERFACE_INCLUDE_DIRECTORIES, which could be inspected in the generated file LibAlkimia5Targets.cmake e.g.

set_target_properties(Alkimia::alkimia PROPERTIES
  INTERFACE_COMPILE_OPTIONS "\$<\$<CXX_COMPILER_ID:MSVC>:-EHsc>;\$<\$<CXX_COMPILER_ID:Intel>:-fexceptions>;\$<\$<OR:\$<CXX_COMPILER_ID:GNU>,\$<CXX_COMPILER_ID:Clang>,\$<CXX_COMPILER_ID:AppleClang>>:-fexceptions>"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/alkimia/Qt5;${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "Qt5::Core;Qt5::DBus;/usr/lib64/libmpir.so"
)

and this patch does in fact the same in generated file LibAlkimia5Config.cmake - it sets the property INTERFACE_INCLUDE_DIRECTORIES.

set_property(TARGET Alkimia::alkimia PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${LibAlkimia@ALKIMIA_LIB_SUFFIX@_INCLUDE_DIR} ${@MP_CMAKE_MODULE@_INCLUDE_DIR})

In fact target_include_directories sets an additional include ${_IMPORT_PREFIX}/include, probably from the globally defined mp include header location, which is unwanted, because we want to determine the location of the mp include header location at client configuration time, which is added with ${@MP_CMAKE_MODULE@_INCLUDE_DIR}. See D10043 and commit R471:d5c38accc301cebffb95afd7b60b88053d2d96b0 for details.

From my experience target_include_directories cmake lacks support for specifing a scope between PRIVATE and INTERFACE e.g. add to library and all local targets, but not on public interface passed into generated cmake package support files.

Please explain, how to solve this with target_include_directories.

krop added a comment.EditedMar 29 2018, 7:45 PM
In D11539#236100, @cgiboudeaux wrote:

The alkimia build system is such a mess that it should be reworked from scratch. I'm not surprised.

Volunteers are always welcome. If you have the necessary foo to do it, go for it. This is what Phabricator is for.

Challenge accepted :)

Please try https://paste.kde.org/p0ujad7eh This should apply to master.

This diff contains 4 local patches:

  • Modernize the Findxxx modules
  • Move the Findxxx modules into cmake/
  • pkgconfig is not needed.
  • Build system Cleanup

Doing more may be complicated. Do you really need to have a Qt4 and a Qt5 version ? and for both, do you really need to support 2 different mp libraries ?
A couple questions:

  • Why do the findxxx.cmake module specify lib(gmp|mpir) in the NAMES ? This is not needed on linux (with just 'XXX', CMake will look for 'libXXX'). Windows maybe ?
  • a line was commented in src/CMakeLists.txt:

set_target_properties(alkimia PROPERTIES SUFFIX "${ALKIMIA_LIB_SOVERSION}${CMAKE_SHARED_LIBRARY_SUFFIX}")

Why is this needed ? I don't remember seeing that anywhere in our libraries before.

*edit*
diff updated to fix the FindGMP.cmake regex.

Do you really need to have a Qt4 and a Qt5 version ? and for both, do you really need to support 2 different mp libraries ?

Yes, both are still needed (for now). Qt4 is needed to support kmymoney-4.8.x, and gmp is needed as mpir does not support all architectures/platforms that gmp does.

In D11539#236523, @cgiboudeaux wrote:

Challenge accepted :)
Please try https://paste.kde.org/p0ujad7eh This should apply to master.

The patch in this review is to fix a dedicated issue people reported on fedora, opensuse and probably distributions and need to be fixed in 7.0 branch, which will lead to a 7.0.2 release in future. Please file a different review request

habacker abandoned this revision.Mar 31 2018, 8:28 PM

Patch has been superseeded by a patch from Rex Dieter, see R471:2d766e3b13bffed8543affb72404cad0d005f583