Replace GMP with MPIR
ClosedPublic

Authored by tbaumgart on Mar 10 2018, 3:46 PM.

Details

Summary

MPIR is written in ANSI C and assembly language and runs on many
platforms (including Linux, Mac OS X and Windows on common hardware
configurations). It's a snap-in replacement for GMP.

Test Plan

compile, run tests, make sure libgmp is not referenced anymore, build KMyMoney using it, check that it compiles on KDE CI

Diff Detail

Repository
R471 Alkimia Library
Branch
mpir
Lint
No Linters Available
Unit
No Unit Test Coverage
tbaumgart requested review of this revision.Mar 10 2018, 3:46 PM
tbaumgart created this revision.

This patch breaks building alkimia for kmymoney 4.8 branch on Windows because there is currently no mingwxx-mpir package. To fix that, mpir support should be optional

wojnilowicz requested changes to this revision.Mar 10 2018, 6:08 PM

In my opinion that change is pointless and does not nail the real problem. Alkimia used to compile about a month ago on Windows without change like you suggest. I feel a little bit invisible having to write this again. Maybe I can't pose the problem right, because of English language.

This revision now requires changes to proceed.Mar 10 2018, 6:08 PM
tbaumgart updated this revision to Diff 29178.Mar 10 2018, 6:28 PM

Support both MPIR and GM

Support both MPIR and GM

I see, that you try to get things working, but why do we need to tackle with mpir at all. Can't we just use gmp on every platform? I think it's doable.
If gmp and mpir compiles on Windows and gmp doesn't get linked for Alkimia, then why mpir should? Have you tested it?

As I see, Ralf doesn't even "know" about mpir and compiles 4.8 nevertheless. Shouldn't that tell us something crucial.

Support both MPIR and GM

I see, that you try to get things working, but why do we need to tackle with mpir at all. Can't we just use gmp on every platform? I think it's doable.
If gmp and mpir compiles on Windows and gmp doesn't get linked for Alkimia, then why mpir should? Have you tested it?

As I see, Ralf doesn't even "know" about mpir and compiles 4.8 nevertheless. Shouldn't that tell us something crucial.

See T8064 why supporting MPIR is probably a good idea.

Support both MPIR and GM

I see, that you try to get things working, but why do we need to tackle with mpir at all. Can't we just use gmp on every platform? I think it's doable.
If gmp and mpir compiles on Windows and gmp doesn't get linked for Alkimia, then why mpir should? Have you tested it?

As I see, Ralf doesn't even "know" about mpir and compiles 4.8 nevertheless. Shouldn't that tell us something crucial.

See T8064 why supporting MPIR is probably a good idea.

Sorry, after reading it, I don't see why. I must not understand something.

@wojnilowicz The problem is that GMP uses Autohell as it's build system, meaning it is impossible to build it in a normal Windows environment using MSVC (among other things)
The CI system uses MSVC exclusively for it's builds, as does the Binary Factory.

This change is therefore far from pointless and paves the way for MSVC based builds.

@wojnilowicz The problem is that GMP uses Autohell as it's build system, meaning it is impossible to build it in a normal Windows environment using MSVC (among other things)
The CI system uses MSVC exclusively for it's builds, as does the Binary Factory.

This change is therefore far from pointless and paves the way for MSVC based builds.

Ok, I see. Thanks for explanation.
Maybe that's why we needed following lines in KMM

if(CMAKE_SYSTEM_NAME MATCHES "Windows")
  include_directories(${GMP_INCLUDE_DIR})
endif()

It's still wonder why KMM compiled on MSVC for me if its core requirement Alkimia cannot be compiled.

That line is probably needed on Windows because GMP won't be in the compiler's default search path. Ideally it should probably be there on all platforms, as it is quite possible that GMP is installed somewhere other than /usr (for instance, on FreeBSD many things are installed in /usr/local rather than /usr which causes build issues in many cases - as can be seen with Amarok at the moment for instance)

In regards to why you are able to compile it using MSVC, you'll be using GMP libraries built using MingW. While this works in some cases, it can cause problems in some cases which may not be immediately obvious (byte alignment for instance) and in the case of C++ libraries is impossible due to different symbol mangling used by MSVC and MingW.

wojnilowicz accepted this revision.Mar 10 2018, 7:47 PM

That line is probably needed on Windows because GMP won't be in the compiler's default search path. Ideally it should probably be there on all platforms, as it is quite possible that GMP is installed somewhere other than /usr (for instance, on FreeBSD many things are installed in /usr/local rather than /usr which causes build issues in many cases - as can be seen with Amarok at the moment for instance)

I think you may be right, but I also think that Thomas' change could make that line obsolete for building Windows.

In regards to why you are able to compile it using MSVC, you'll be using GMP libraries built using MingW. While this works in some cases, it can cause problems in some cases which may not be immediately obvious (byte alignment for instance) and in the case of C++ libraries is impossible due to different symbol mangling used by MSVC and MingW.

If Craft didn't build gmp for me then I didn't do that neither. I see that something like that might be built here
https://binary-factory.kde.org/job/MinGW-w64_binaries_for_MSVC/

In general, I guess I was wrong, so sorry guys for being a drag in moving things forward. If that change doesn't break Ralf's build then I'm for it.

This revision is now accepted and ready to land.Mar 10 2018, 7:47 PM
if(CMAKE_SYSTEM_NAME MATCHES "Windows")
  include_directories(${GMP_INCLUDE_DIR})
endif()

This is because alkimia headers require gmp headers, which is currently not added to alkimia target interface (D10043 is intended as fix). Switching to mpir does not change or solve this issue.

src/CMakeLists.txt
91

Not better to specify some mp related variable e.g MP_FIND_PACKAGE_PATTERN set to "MPIR" or "GMP" and to install only the supported file ?

habacker added inline comments.Mar 10 2018, 8:52 PM
CMakeLists.txt
39

This will not been propagated to client packages, which is required. See the comment for alkvalue.h

src/CMakeLists.txt
5

net better to set minor level ? there is a dependency change

src/alkvalue.h
28

ALKIMIA_USE_MPIR needs to be defined also on compiling a client package referencing alkimia headers compiled with mpir support.
cmake command target_compile_definitions should solve this issue (see https://cmake.org/cmake/help/v3.0/command/target_compile_definitions.html)

habacker added inline comments.Mar 10 2018, 8:57 PM
src/alkvalue.cpp
50

Does this need to be defined different for each variant ?

tbaumgart updated this revision to Diff 29194.Mar 10 2018, 10:34 PM
tbaumgart marked an inline comment as done.
  • Only reference the used MP library
tbaumgart marked 3 inline comments as done.Mar 10 2018, 10:36 PM
tbaumgart added inline comments.
src/alkvalue.cpp
50

No.

Maybe that's why we needed following lines in KMM

if(CMAKE_SYSTEM_NAME MATCHES "Windows")
  include_directories(${GMP_INCLUDE_DIR})
endif()

It's still wonder why KMM compiled on MSVC for me if its core requirement Alkimia cannot be compiled.

@habacker @bcooksley
Would adding the GMP/MPIR include directory used during compilation of Alkimia to the .pc file be of help to avoid the above mentioned conditional include_directories statement? I am looking at something like this

diff --git a/src/libalkimia.pc.in b/src/libalkimia.pc.in
index 5b0a8ae..c2087c9 100644
--- a/src/libalkimia.pc.in
+++ b/src/libalkimia.pc.in
@@ -15,5 +15,5 @@ Version: @ALKIMIA_LIB_VERSION@
 
 Libs: -lalkimia@PC_TARGET_SUFFIX@
 Libs.private: -l@PC_LIB@ -l@PC_TARGET_QTPREFIX@Core -l@PC_TARGET_QTPREFIX@DBus 
-Cflags: -I${includedir}
+Cflags: -I${includedir} -I@MP_INCLUDE_DIR@

If memory serves include_directories() alters the search path CMake gives to the compiler for the current project.
That entry should definitely be added to the pkgconfig file to aid projects that use pkgconfig, however for CMake you should be utilising the target_include_directories statements so that the exports generated by CMake have this taken care of for you (this would replace include_directories I believe)

tbaumgart marked an inline comment as done.Mar 11 2018, 10:09 AM
tbaumgart added inline comments.
src/CMakeLists.txt
5

From https://semver.org/

What should I do if I update my own dependencies without changing the public API?

That would be considered compatible since it does not affect the public API. Software that explicitly depends on the same dependencies as your package should have their own dependency specifications and the author will notice any conflicts. Determining whether the change is a patch level or minor level modification depends on whether you updated your dependencies in order to fix a bug or introduce new functionality. I would usually expect additional code for the latter instance, in which case it’s obviously a minor level increment.

Since we did not add any additional code and do not provide additional functionality with this change (other than being able to support building with MSVC) I decided to simply increment the patchlevel since the change is totally backward compatible.

tbaumgart updated this revision to Diff 29214.Mar 11 2018, 10:14 AM
tbaumgart marked an inline comment as done.
  • Remove a leftover from last commit
  • Pass on the includedir of the MP library to the user of Alkimia
habacker added a comment.EditedMar 11 2018, 11:29 AM

I tested this path with a little cmake project

test.cpp

#include <alkimia/alkvalue.h>

int main()
{
    AlkValue a(1.0, 1);
}

CMakeLists

if(COMMAND cmake_policy)
   cmake_policy(SET CMP0003 NEW)
endif(COMMAND cmake_policy)

find_package(LibAlkimia)

add_executable(atest test.cpp)
target_link_libraries(atest Alkimia::alkimia)

and compiled against a qt4 build of alkimia and got no problems. - patch looks good.

This revision was automatically updated to reflect the committed changes.

Looks like some additional work may be required to get the MPIR include path being passed through:
https://build.kde.org/job/Extragear%20kmymoney%20kf5-qt5%20WindowsMSVCQt5.10/3/console

Looks like some additional work may be required to get the MPIR include path being passed through:
https://build.kde.org/job/Extragear%20kmymoney%20kf5-qt5%20WindowsMSVCQt5.10/3/console

If the issue is caused by compiling kmymoney with an mpir development package installed at a different location as on compiling alkimia, see D10043

The MPIR install location should be the same between Alkimia and KMyMoney builds.

There seems to be an issue with non standard install locations. I applied a patch to alkimia git repo which should fix this issue.