Skip to content
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

Add MSGPACK_ENABLE_SHARED option, defaulting to ON #316

Merged
merged 3 commits into from Aug 13, 2015

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Jul 15, 2015

This allows building just static libraries using the CMake build, which is useful if your product is entirely statically linked. By default there is no change to the output - MSGPACK_ENABLE_SHARED must be explicitly set to false to disable building the shared library.

This allows building just static libraries using the CMake build, which
is useful if your product is entirely statically linked. By default
there is no change to the output - MSGPACK_ENABLE_SHARED must be
explicitly set to false to disable building the shared library.
@redboltz
Copy link
Contributor

Thank you for sending the PR. It looks good to me. I don't have much time right now. I will merge it next week.

@redboltz
Copy link
Contributor

I got the following error when I set -DMSGPACK_ENABLE_SHARED=OFF:

[kondo@msboltz] $ cmake -DMSGPACK_ENABLE_SHARED=OFF ..                                                                                                         -- Could NOT find Doxygen (missing:  DOXYGEN_EXECUTABLE) 
-- Configuring done
-- Generating done
-- Build files have been written to: /home/kondo/work/msgpack-c/build
[kondo@msboltz] $ make                                                                                                                                         [  3%] Building C object CMakeFiles/msgpack-static.dir/src/unpack.c.o
[  6%] Building C object CMakeFiles/msgpack-static.dir/src/objectc.c.o
[  9%] Building C object CMakeFiles/msgpack-static.dir/src/version.c.o
[ 12%] Building C object CMakeFiles/msgpack-static.dir/src/vrefbuffer.c.o
[ 16%] Building C object CMakeFiles/msgpack-static.dir/src/zone.c.o
Linking C static library libmsgpack.a
[ 16%] Built target msgpack-static
[ 19%] Building CXX object test/CMakeFiles/buffer.dir/buffer.cpp.o
Linking CXX executable buffer
/usr/bin/ld: cannot find -lmsgpack
collect2: error: ld returned 1 exit status
test/CMakeFiles/buffer.dir/build.make:88: recipe for target 'test/buffer' failed
make[2]: *** [test/buffer] Error 1
CMakeFiles/Makefile2:1026: recipe for target 'test/CMakeFiles/buffer.dir/all' failed
make[1]: *** [test/CMakeFiles/buffer.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

@jen20
Copy link
Contributor Author

jen20 commented Jul 21, 2015

Let me look into this, I wasn't building with GTest available so likely didn't hit this. Interesting that the CI server also didn't.

@redboltz
Copy link
Contributor

When MSGPACK_ENABLE_SHARED is ON, the default setting, no errors are occurred. MSGPACK_ENABLE_SHARED is introduced by this PR. So travis-ci doesn't test with -DMSGPACK_ENABLE_SHARED=OFF.
Of course, you can add -DMSGPACK_ENABLE_SHARED=OFF setting to travis-ci. However currently 32 matrix of tests are running. It's running parallely, but resources are limited. It takes a long time. If -DMSGPACK_ENABLE_SHARED=OFF setting is added to the matrix, 48 tests would run. See https://github.com/msgpack/msgpack-c/blob/master/.travis.yml
The test matrix is not automatically generated, it's not an elegant, so you can add a special test case with -DMSGPACK_ENABLE_SHARED=OFF.

I think that modifying travis-ci settings are a little complicated. I will merge the PR when the error that I pointed out would be fixed. I mean updating travis-ci setting is not mandatory.

This commit causes the tests to be linked against the msgpack-static
target if MSGPACK_ENABLE_SHARED is set to OFF.
@redboltz redboltz merged commit 8401016 into msgpack:master Aug 13, 2015
@redboltz
Copy link
Contributor

In build_cmake.sh, SHARED is not always $5 due to BOOST_INC, so I added BOOST_INC to all .travis.yml environment. Then merged. Thanks.

@jen20
Copy link
Contributor Author

jen20 commented Aug 13, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants