Pass MP include header location detected at client configure time to clients
ClosedPublic

Authored by habacker on Jan 23 2018, 12:14 PM.

Details

Summary

Say you have a binary or self compiled MP development package (either gmp
or mpir) and alkimia package and made a binary packages for alkimia. Then
you unpack them into a different location and compiles kmymoney against
these development packages (which is probably the case reported for Mac
OsX).

The MP header will now be searched at the location where the MP
library has been installed on the first location because the location
is written - hard coded - into the installed cmake config file
LibAlkimiaXConfig.cmake. With this fix, cmake passes the MP header
location to the client, which has been detect on configuring the
client.

For compiling alkimia is it now sufficient to use MP_INCLUDE_DIR privatly.

Test Plan

compiled on linux

Diff Detail

Repository
R471 Alkimia Library
Lint
Lint Skipped
Unit
Unit Tests Skipped
conet requested review of this revision.Jan 23 2018, 12:14 PM
conet created this revision.
conet added a subscriber: KMyMoney.Jan 23 2018, 3:01 PM

Seems to work for me on Qt5. Let's see what we hear from Ralf. If he also agrees, then commit and push it to the 7.0 branch

Thomas - was that a typo for the 5.0 branch?

Thomas - was that a typo for the 5.0 branch?

No, Alkimia is on 7.0

Sorry - I was moving too quickly, and was thinking this was a change in KMM to handle libalkimia, not a change to libalkimia itself.

tbaumgart accepted this revision.Feb 3 2018, 3:21 PM
This revision is now accepted and ready to land.Feb 3 2018, 3:21 PM
christiand requested changes to this revision.Feb 3 2018, 3:56 PM
christiand added a subscriber: christiand.

Hi Thomas,

the content is inline.

Best
Chris

src/CMakeLists.txt
39

The ${GMP_INCLUDE_DIR} cannot be used as Public here because it inserts an absolute path. This absolute file path changes with a high chance (e.g. in deb or rpm creating environments vs the system it is installed on). It must be kept in a $<BUILD_INTERFACE:...> and instead added again in the LibAlkimiaConfig after the required use of find_dependency(GMP) (which updates the paths accordingly to the current enviroment).

Alternatively targets have to be added to FindGMP.cmake. Then the hole handling of variables is not required at all.

40

Same here for ${GMP_LIBRARIES}.

This revision now requires changes to proceed.Feb 3 2018, 3:56 PM
habacker added inline comments.Feb 5 2018, 10:45 PM
src/CMakeLists.txt
39

This absolute file path changes with a high chance

and on Windows also

39
instead added again in the LibAlkimiaConfig after the required use of find_dependency(GMP)

see https://cmake.org/cmake/help/v3.0/module/CMakeFindDependencyMacro.html for more details

This patch needs to be rebased after D11212 has been applied.

habacker added inline comments.Mar 12 2018, 9:24 PM
src/LibAlkimiaConfig.cmake.in
14

With R471:0b87a2a5c221f9fd8ff0f11dbea53836544db118 this patch could be reduced to

set_property(TARGET Alkimia::alkimia PROPERTY INTERFACE_INCLUDE_DIRECTORIES "@PACKAGE_INCLUDE_INSTALL_DIR@" ${@MP_CMAKE_MODULE@_INCLUDE_DIR})

Can't we close this patch? I think we have all we need in the repo already. Or am I missing something?

src/LibAlkimiaConfig.cmake.in
14

I just added that to master and 7.0 branches and tested it locally by installing MPIR in some local user's home directory and it worked fine.

The difference is that ${@MP_CMAKE_MODULE@_INCLUDE_DIR}) adds the MP include dir detected at the time the client detects mp library and not the location from configuring alkimia. Say you have compiled alkimia and mades binary package for alkimia and the mp library. Then you move them to another host, unpack them into a different location and compiles kmymoney against these development packages (which is the probably the case reported for mac osx) . The mp header will now be searched at the location, where the mp library has been installed on the first host, because it is written - hard coded - into the installed cmake config files. Therefore this fix uses the mp header location detect on configuring at the second host.

The difference is that ${@MP_CMAKE_MODULE@_INCLUDE_DIR}) adds the MP include dir detected at the time the client detects mp library and not the location from configuring alkimia. ...

Makes sense. And I assume using

${@MP_INCLUDE_DIR@}

does the same thing as your proposed

${@MP_CMAKE_MODULE@_INCLUDE_DIR}

The difference is that ${@MP_CMAKE_MODULE@_INCLUDE_DIR}) adds the MP include dir detected at the time the client detects mp library and not the location from configuring alkimia. ...

Makes sense. And I assume using

${@MP_INCLUDE_DIR@}

does the same thing as your proposed

${@MP_CMAKE_MODULE@_INCLUDE_DIR}

With MP_INCLUDE_DIR = /some-path-on-the-first-host ${@MP_INCLUDE_DIR@} will be expanded in LibAlkimiaConfig.cmake to ${/some-path-on-the-first-host},. which is invalid syntax, while ${@MP_CMAKE_MODULE@_INCLUDE_DIR} will be expanded to ${GMP_INCLUDE_DIR} or ${MPIR_INCLUDE_DIR} depending on the detect mp library. Running

cmake ../kmymoney

on the second host let ${GMP_INCLUDE_DIR} or ${MPIR_INCLUDE_DIR} return the value found by find_dependency(@MP_CMAKE_MODULE@) , which is expanded to find_dependency(MPIR) or find_dependency(GMP), depending on the initial detected mp library.

Inspect the created file <build-root>/src/LibAlkimiaConfig.cmake to see the difference.

Ok, that is true. Looks like I need to play a lot more with this CMake magic to fully understand what's going on.

How much more of this change do we need in LibAlkimia at this point in time since it seems to be ready to me and the CI?

The patch of this review fixes two issues, which may be handled separatly:

  1. The issue reported by Christian Onet on Mac OsX, where I added the related fix to this review
  2. The removal of LIBALKIMIA_INCLUDE_DIR: In the past alkimia clients needed to use LIBALKIMIA_INCLUDE_DIR for adding alkimia include pathes. Upgrading to 7.0 makes this obsolate, because the alkimia package provides an imported cmake target named ' Alkimia::alkimia' which includes already the location of alkimia headers (which is by specified by the call to set_property(TARGET Alkimia::alkimia PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${LIBALKIMIA_INCLUDE_DIR}) in LibAlkimiaConfig.cmake) and simplifies the client part to
add_executable(<target> ...)
target_link_libraries(<target>  Alkimia::alkimia)

Can you then please take care of either

  • updating the patch to current HEAD (7.0) or
  • make the necessary adjustments in git and close the review.

I want to make new release of Alkimia before we create the KMyMoney release on the weekend.

habacker commandeered this revision.Mar 13 2018, 2:09 PM
habacker edited reviewers, added: conet; removed: habacker.
habacker updated this revision to Diff 29404.Mar 13 2018, 2:11 PM
habacker retitled this revision from Fix libalkimia's installed cmake configuration files to Pass MP include header location detected at client configure time to clients.
habacker edited the summary of this revision. (Show Details)
habacker edited the test plan for this revision. (Show Details)
habacker removed reviewers: christiand, conet.
  • rebased to 7.0 branch
  • splitted out removal of LIBALKIMIA_INCLUDE_DIR
This revision is now accepted and ready to land.Mar 13 2018, 2:11 PM
habacker requested review of this revision.Mar 13 2018, 2:11 PM
habacker marked 6 inline comments as done.

reset review state - phabricator seemed to "auto accept" for unkown reasons after clicking all "done" checkboxes

tbaumgart accepted this revision.Mar 13 2018, 3:19 PM

Works for me.

This revision is now accepted and ready to land.Mar 13 2018, 3:19 PM
habacker closed this revision.Mar 15 2018, 5:39 PM

Please see

From https://build.kde.org/job/Extragear%20alkimia%20kf5-qt5%20FreeBSDQt5.9/11/console

17:41:03 -- The following REQUIRED packages have been found:
17:41:03  * GMP

-> GMP has been found

On compiling alkimia the related include dir is added by

target_include_directories(alkimia PRIVATE ${MP_INCLUDE_DIR})

with

set(MP_INCLUDE_DIR ${GMP_INCLUDE_DIR})

I cannot see here anything wrong.

The build failure, at least on FreeBSD, is in a test.
You've changed the include directory target specification to be private, so MP_INCLUDE_DIR won't be made available to tests, hence the failure.

-target_include_directories(alkimia PUBLIC ${MP_INCLUDE_DIR})
+target_include_directories(alkimia PRIVATE ${MP_INCLUDE_DIR})

It works on Linux by chance as MP_INCLUDE_DIR is in a system prefix (/usr namely) while on FreeBSD it's in a separate prefix (/usr/local as is the case with many libraries there)
That is why Windows fails as well.

habacker added a comment.EditedMar 16 2018, 11:43 AM

The build failure, at least on FreeBSD, is in a test.
You've changed the include directory target specification to be private, so MP_INCLUDE_DIR won't be made available to tests, hence the failure.

This is required to not pass the include path into LibAlkimiaConfig.cmake. A solution could be to add MP_INCLUDE_DIR to targets generated in autotests subdir, but this could not be setup with target_include_directories. Any better idea how to support this case ?

Thanks for the fix.