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

cmake refactor #90

Merged
merged 22 commits into from
Feb 13, 2015
Merged

cmake refactor #90

merged 22 commits into from
Feb 13, 2015

Conversation

debris
Copy link
Contributor

@debris debris commented Feb 12, 2015

this pull request is enhancement of #87

if fixes the following
  • building unit_testsuite in msvc
  • building in Debug mode in msvc
  • building dynamic libraries in Xcode generated project on macos
  • fixes MACOSX_RPATH policy warning on macos
  • include_directories are not included globally any more
  • unifies CMake files style
    • use 4 spaces width tabs
    • variables use upper case
    • macros/functions use lower case
  • separated parts of cmake logic
    • separated CMakeCompilerSettings.cmake
    • separated CMakeDependencies.cmake
  • refactored FindXXX.cmake files
    • as far as i know, we do now have to add paths usr/include && usr/local/include, they should be searched by default
    • ${CMAKE_SOURCE_DIR}/win32-deps/ moved to CMAKE_PREFIX_PATH, so new cmake search paths are backwards compatible
    • FindJsoncpp.cmake don't generate any files. Instead it's using PATH_SUFFIXES to solve jsoncpp versions incompatibility.
changes:
  • src/jsonrpccpp/CMakeLists.txt now installs all header files, if it is unwanted, we can remove them from the list by using list REMOVE_ITEM. I think it's much cleaner solution, than previous one.

Architecture of the library itself remained unchanged, i didn't want to create any incomatibilities with previous version

@debris
Copy link
Contributor Author

debris commented Feb 12, 2015

do we need these two lines in .travis.yml?

- g++ ../src/examples/simpleclient.cpp -ljsonrpccpp-client -ljsoncpp -ljsonrpccpp-common -lcurl -o sampleclient
- g++ ../src/examples/simpleserver.cpp -ljsonrpccpp-server -ljsoncpp -ljsonrpccpp-common -lmicrohttpd -o sampleserver

@cinemast
Copy link
Owner

Yes. It should check if linking against the library without cmake is possible, after the library has been installed.

@debris
Copy link
Contributor Author

debris commented Feb 12, 2015

are these changes ok for you?

@debris
Copy link
Contributor Author

debris commented Feb 12, 2015

Great! Yes, I'm done :)

@cinemast cinemast mentioned this pull request Feb 12, 2015

# TODO not sure why some .h were not included
# if we don't want to include it, remove it using list REMOVE_ITEM
file(GLOB jsonrpc_header_common common/*.h)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files were not included beacuse they are not part of the public API. Which makes it more easy to change internal functionality. Please revert that change, or use remove for all the others.

@debris
Copy link
Contributor Author

debris commented Feb 12, 2015

changes :

  • jsonrpc_header_server && jsonrpc_client_server are used by cmake add_library command. This allows all header files to be included into cmake generated project (like Xcode).
  • jsonrpc_install_header_server && jsonrpc_install_header_client are used by cmake install command and are part of public API

update, i will fix travis build in a minute

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling eeb1cb5 on debris:jsonrpc4debug into 7677dc7 on cinemast:master.

@debris
Copy link
Contributor Author

debris commented Feb 13, 2015

I fixed issues. Can you review pull request again?

@cinemast
Copy link
Owner

After a quick review, it looks good. I will try to build on other platforms. If everything is fine. I will merge it. Thanks again for your contribution 👍

@cinemast
Copy link
Owner

Aside that, everything looks fine. Don't forget to add yourself to the AUTHORS.md ;)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8ef64fb on debris:jsonrpc4debug into 7677dc7 on cinemast:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8ef64fb on debris:jsonrpc4debug into 7677dc7 on cinemast:master.

@debris
Copy link
Contributor Author

debris commented Feb 13, 2015

done!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7a596a7 on debris:jsonrpc4debug into 7677dc7 on cinemast:master.

@cinemast
Copy link
Owner

Great! I guess a lot of people will appreciate MSVC support. Thanks again.

cinemast pushed a commit that referenced this pull request Feb 13, 2015
cmake refactor + msvc support
@cinemast cinemast merged commit 5dce039 into cinemast:master Feb 13, 2015
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

3 participants