-
Notifications
You must be signed in to change notification settings - Fork 19
Upgrade from messagepack 3.2.0 to 7.0.0 #333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Upgrade from messagepack 3.2.0 to 7.0.0 #333
Conversation
LourensVeen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The number of lines changed in this PR belies the amount of work that went into it. Very nice, and thank you!
| cd msgpack-cxx-$(msgpack_VERSION) && rm -rf build && mkdir -p build && cd build && \ | ||
| export PKG_CONFIG_PATH=$(PKG_CONFIG_PATH):$(PKG_CONFIG_EXTRA_DIRS) && \ | ||
| cmake -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DCMAKE_INSTALL_PREFIX=$(CURDIR)/msgpack -DMSGPACK_CXX11=ON -DMSGPACK_BUILD_EXAMPLES=OFF -DMSGPACK_BUILD_TESTS=OFF .. && \ | ||
| cmake -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DMSGPACK_USE_BOOST=OFF -DCMAKE_INSTALL_PREFIX=$(CURDIR)/msgpack -DMSGPACK_CXX11=ON -DMSGPACK_BUILD_EXAMPLES=OFF -DMSGPACK_BUILD_TESTS=OFF .. && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the CMAKE_POLICY_VERSION_MINIMUM still needed when building 7.0.0.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake does NOT like when I do that, so my recommendation is to keep it for now.
|
I've seen this issue of CMake-built packages insisting that everyone else use CMake too if they want to use the library. It's worrying. On the other hand, if this is just about standardising on CMAKE_PREFIX_PATH then it's easy enough to add support to other build systems I suppose. Well, maybe not relevant here if we'll just always build locally. I've found two more issues while testing this locally:
|
|
I am genuinely surprised that this is passing as many test cases as it is now. I am facing issues with Ubuntu regarding libmuscle's pc files, but the good news is the Mac build works! |
Hey, y'all!
Finally upgraded MessagePack to the latest dependency of
7.0.0. This should solve #317, but it's not quite good to merge yet. I am sure that there are better ways to do this.Problem:
Data/DataConstRefreceives the wrong typeThe error faced by the user stated in #317 is a runtime type error, and after a lot of digging I know why: MessagePack 4.0.0, for the sake of minimizing message sizes, packs floating point types with no fractional component as the smallest integer type it can (e.g. 2.0 becomes 2, while 2.3 stays 2.3). I have tried like hell to debug MUSCLE3 to stop this (there is a way, but it involves more time than it should take), but without success.
Solution: Upgrade to the most recent version
At least this way, we're not locking ourselves in time with a version that doesn't work, but there are changes to how the build system deals with MessagePack.
MessagePack C/C++ has split
One of the breaking changes in 4.0.0 is the split between the C and the C++ libraries, which are
msgpack-candmsgpack-cxxrespectively. Thus, the GitHub URL had to be changed to match this.msgpack-cxxdoes not usepkgconfThere is no such thing as a
.pcfile or a.pc.inwithin themsgpack-cxxlibrary, so there is no way to usepkg-configto get any information about it. Only the C library has it. I was a bit surprised that any compilation was successful under 4.0.0. However,libmsgpack-devlists alibmsgpack-cxx-devas a dependency. I have tried to install this to see if that would be usable withpkg-config, but I was unsuccessful, which tells us that the C++ library cannot be used with it at all. The C++ library is also header-only, which might explain the lack of use of thepkgconftemplate files.Future detection and installation:
There is a way that is described in the msgpack-cxx README which is described as the canonical cmake way, but I'm guessing that's not something we want to do.
To that end, the changes in the build system locally download and extract msgpack regardless if the current system has it or not. 4.0.0 has that problematic type compression behavior mentioned earlier, and it's the same version that's the latest available on apt, so this is probably for the better.
There are a lot of lines here that I commented out rather than deleted out of anticipation that we might need to keep it on hand. However, after all issues have been ironed out in this PR, I want to get rid of these lines before merging.
What needs to be done now:
msgackin the build system HAS to be changed tomsgpack-cxx, if it isn't already.libmuscle.pcandlibmuscle_mpi.pcto fixmake test_examples.