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

Provide CPRConfig.cmake to allow easy find_package() #28

Closed
niklas88 opened this issue Jul 20, 2015 · 17 comments
Closed

Provide CPRConfig.cmake to allow easy find_package() #28

niklas88 opened this issue Jul 20, 2015 · 17 comments

Comments

@niklas88
Copy link

I'm all but a cmake expert but if I interpret this
http://www.cmake.org/cmake/help/git-master/manual/cmake-packages.7.html#creating-packages
link and my cmake output correctly, supplying CPRConfig.cmake
would allow downstream projects to include cpr as a dependency like this:

 set ( CMAKE_PREFIX_PATH ${CMAKE_PREFIX_PATH} vendor/cpr/)
 find_package ( CPR REQUIRED )
 include_directories ( ${CPR_INCLUDE_DIRS} )

and then use ${CPR_LIBRARIES} in target_link_libraries(). Instead of vendoring it could also be installed in the system path getting rid of the first line in the snippet above.

@whoshuu
Copy link
Contributor

whoshuu commented Jul 20, 2015

Great suggestion @niklas88, CMake is definitely the build system of choice for this project, and supporting downstream projects easily through CMake is a high priority.

whoshuu added a commit that referenced this issue Jul 27, 2015
whoshuu added a commit that referenced this issue Aug 7, 2015
@whoshuu whoshuu self-assigned this Aug 7, 2015
@niklas88
Copy link
Author

Hey great seeing you work on this, I just tried using the cpr-config.cmake in my vendoring setting but sadly it failed. I used the following lines to embed it and compiled cpr in it's directory myproject/vendor/cpr:

set ( CMAKE_PREFIX_PATH ${CMAKE_PREFIX_PATH} vendor/cpr/)
find_package ( CPR REQUIRED )
include_directories ( ${CPR_INCLUDE_DIRS} )

Cmake somehow has really horrible error reporting:

cmake --debug-output ..
Running with debug output on.
CMake Error at /usr/share/cmake-3.3/Modules/FindPackageHandleStandardArgs.cmake:148 (message):
  Could NOT find CPR (missing: CPR_LIBRARY)
Call Stack (most recent call first):
  /usr/share/cmake-3.3/Modules/FindPackageHandleStandardArgs.cmake:388 (_FPHSA_FAILURE_MESSAGE)
  vendor/cpr/cpr-config.cmake:21 (find_package_handle_standard_args)
  CMakeLists.txt:19 (find_package)


   Called from: [2]     /home/niklas/scratch/cprtest/vendor/cpr/cpr-config.cmake
            [1]     /home/niklas/scratch/cprtest/CMakeLists.txt
    -- Configuring incomplete, errors occurred!
See also "/home/niklas/scratch/cprtest/build/CMakeFiles/CMakeOutput.log".

I can send you the source tree if you like I tried to build a minimal example before integrating it in my other project.

@whoshuu
Copy link
Contributor

whoshuu commented Aug 23, 2015

Hey @niklas88, thanks for giving it a try. If you can link me a github repo that's a minimal example you think should work, that'd help a lot. I'll take a look at this again in the morning.

@niklas88
Copy link
Author

Ok, I have prepared my test as a github project https://github.com/niklas88/cprtest

@whoshuu
Copy link
Contributor

whoshuu commented Aug 23, 2015

Have you built the project in the submodule? What were the steps you took in the build. I'm most interested in the root build directory, because that will tell me where to find the path to the cpr library.

For instance, if you do an idiomatic build in vendor/cpr/build, then the library path would be vendor/cpr/build/cpr. Since this is a vendored build, you have to set a hint to tell find_library where to find this library. To do that, set CPR_LIBRARY_ROOT to ${CURRENT_CMAKE_LIST_DIR}/vendor/cpr/build/cpr before calling find_package( CPR REQUIRED ) and you should be good.

@whoshuu
Copy link
Contributor

whoshuu commented Aug 23, 2015

My suggestion is that if you have cpr included as a submodule in your project, then it's easier to use add_subdirectory to bring in the includes and symbols. Simple throw in a add_subdirectory(vendor/cpr) in your main CMakeLists.txt and CPR_INCLUDE_DIRS and CPR_LIBRARIES will be correctly populated. An added bonus is that you don't have to manually find and integrate libcurl with your project, that dependency is happened automatically with an add_subdirectory of cpr.

@whoshuu
Copy link
Contributor

whoshuu commented Aug 23, 2015

In other words, using find_package(CPR) with cpr-config.cmake is useful for when the cpr source/build is outside the natural scope of your project. If your project includes cpr as a submodule, it's better to use add_subdirectory.

@niklas88
Copy link
Author

Yeah I realized building in vendor/cpr/build would create things in the wrong path, I tried vendor/cpr als the build directory too. I thought the first line in

set ( CMAKE_PREFIX_PATH ${CMAKE_PREFIX_PATH} vendor/cpr/)
find_package ( CPR REQUIRED )
include_directories ( ${CPR_INCLUDE_DIRS} )

would then set the correct path and at least it finds the cpr-config.cmake because this is what works with the same setup but using netlib-cpp:

set ( CMAKE_PREFIX_PATH ${CMAKE_PREFIX_PATH} vendor/cpp-netlib/)
find_package ( cppnetlib REQUIRED )
include_directories ( ${CPPNETLIB_INCLUDE_DIRS} )

@whoshuu
Copy link
Contributor

whoshuu commented Aug 23, 2015

Gotcha. I think I understand the issue now. I don't set an explicit location for putting libcpr.a. It just shows up in <build>/cpr, when it should show up in <build>/lib, since that's where libraries are searched for when CMAKE_PREFIX_PATH is set. Thanks a bunch! I'll get this resolved soon.

whoshuu added a commit that referenced this issue Aug 23, 2015
@whoshuu
Copy link
Contributor

whoshuu commented Aug 23, 2015

If you rebuild cpr from the latest release/1.1 branch, libcpr.a should be put into the right directory so that CMAKE_PREFIX_PATH can find it. Let me know if it works for you.

@niklas88
Copy link
Author

@whoshuu ok we are definitely making progress. This is definitely an improvement, also sorry I missed adding the rapidjson header only library to the git project. Now it looks like the libcurl symbols aren't available through libcpr.a see this error output:
http://pastebin.com/5aQQarc0

If I link with my system libcurl (I'm on Arch and it may or may not be the same version) the executable works

@whoshuu
Copy link
Contributor

whoshuu commented Aug 23, 2015

Yeah see my note about libcurl in this comment. I don't compile the curl sources into libcpr, so you still have to manually link libcurl to your project. This can be avoided if you use add_subdirectory because CMake figures out the dependencies for you and carries it through when you specify the link libraries of a target.

@whoshuu
Copy link
Contributor

whoshuu commented Aug 23, 2015

It's possible to manually add libcurl to CPR_LIBRARIES in cpr-config.cmake, but I don't believe that's a pattern that's used a lot. It would make it easier to integrate the library in one fell swoop but is far less transparent than having the library client link libcurl themselves.

@niklas88
Copy link
Author

But then one would have to make sure that cpr uses the system library as well and not it's local clone so that non-matching versions are easier to discover. Personally I prefer linking with system libraries when available because that means one gets security updates to dynamic libraries but that might not be common on non Linux systems.

@whoshuu
Copy link
Contributor

whoshuu commented Sep 1, 2015

There's something to be said for having defaults that almost never fail, and allowing customization beyond that for options that may fail. In my experience, libraries which have no external dependencies are highly preferred -- they're the easiest to get up and running, and C++ is notoriously bad at supporting plug and play libraries.

While I sympathize with your concern, the most important thing for me is to encourage using submodules as the first choice for library integration. This method will have first-class support.

If the library is built separately and included later, extra care must be taken by the integrator to make sure the dependencies are compatible, and I am perfectly okay with this. The library is pretty explicit about which dependencies are used from where, though the documentation could use some work with regard to build options. I'll note this and make a separate issue to address that.

For now, I'm going to limit the scope of this issue to simply having a working cpr-config.cmake file that allows an application developer to integrate a cpr library that's built outside the scope of the application. I think you're in a good position to give me the green light on that, so whenever possible, let me know if this issue is resolved by the latest release branch (30bc418).

@niklas88
Copy link
Author

niklas88 commented Sep 3, 2015

Sorry for the delay I'm a bit busy working on my masters thesis. I can confirm it does indeed work wonderfully with my test project now, great work!

@whoshuu whoshuu closed this as completed in df6e585 Sep 3, 2015
@HarrisDePerceptron
Copy link

I do not want a submodule. apparently the proj i am working on has all compiled deps system wide and i do not want just this perticular library as a submodule.

Trying to install system wide:

git clone https://github.com/whoshuu/cpr.git
cd cpr
git submodule update --init opt/curl
cmake -DBUILD_CPR_TESTS=OFF ..
make -j
sudo make install

From the root of the project: cd path/to/project
included this at my root CMakeLists.txt:
find_package(CPR REQUIRED)

mkdir build && cd build 
cmake ..

Gives me an error:

[cmake] CMake Error at /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
[cmake] Could NOT find CPR (missing: CPR_LIBRARY CPR_INCLUDE_DIR)

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

No branches or pull requests

3 participants