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.
Details
- Reviewers
wojnilowicz bcooksley habacker - Commits
- R471:6e08cda0cb01: Replace GMP with MPIR
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
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
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.
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.
@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.
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 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 | ||
---|---|---|
92 | 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 ? |
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. |
src/alkvalue.cpp | ||
---|---|---|
50 | Does this need to be defined different for each variant ? |
src/alkvalue.cpp | ||
---|---|---|
50 | No. |
@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)
src/CMakeLists.txt | ||
---|---|---|
5 | From https://semver.org/
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. |
- Remove a leftover from last commit
- Pass on the includedir of the MP library to the user of Alkimia
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.
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
There seems to be an issue with non standard install locations. I applied a patch to alkimia git repo which should fix this issue.
Thanks for the fix, it now compiles successfully!
https://build.kde.org/job/Extragear%20kmymoney%20kf5-qt5%20WindowsMSVCQt5.10/