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 find-package support #645

Merged
merged 3 commits into from
Sep 27, 2021
Merged

CMake find-package support #645

merged 3 commits into from
Sep 27, 2021

Conversation

mirzachi
Copy link
Contributor

@mirzachi mirzachi commented Sep 26, 2021

This PR improves CMake integration of the library.

I was surprised that one cannot simply download, build, install the library on the system and then simply integrate the library within other project via the standard CMake find-package utility. This PR adds this feature. For instance, now one can import the library in other CMake project like this:

CMakeLists.txt

cmake_minimum_required(VERSION 3.20)
project(cpr_client)

set(CMAKE_CXX_STANDARD 17)

find_package(cpr REQUIRED)
if (cpr_FOUND)
    message(STATUS "Found cpr: ${cpr_CONFIG} (found version ${cpr_VERSION})")
endif ()

add_executable(cpr_client main.cpp)

target_link_libraries(cpr_client PRIVATE cpr::cpr)

main.cpp

#include <iostream>
#include <cpr/cpr.h>

int main(int argc, char** argv) {
    cpr::Response r = cpr::Get(cpr::Url{"https://api.github.com/repos/whoshuu/cpr/contributors"},
                               cpr::Authentication{"user", "pass"},
                               cpr::Parameters{{"anon", "true"}, {"key", "value"}});
    std::cout << r.status_code << std::endl;                  // 200
    std::cout << r.header["content-type"] << std::endl;     // application/json; charset=utf-8
    std::cout << r.text << std::endl;                         // JSON text string
}

Note that I handle CURL dependency in cprConfig.cmake.in. This is why one has to only include find_package(cpr REQUIRED).

This feature is feasible only if CPR_FORCE_USE_SYSTEM_CURL is set.
Otherwise, you download libcurl in-source and link against its targets.
This is a problem since you have to not only export and install cpr CMake target, but also all targets that cpr target links against.
In this case, curl_int. Further, its dependecies would have to be handled and exported, etc.
In other words, CMake is not a helpful tool for this use case.

Mirza Avdic added 3 commits September 26, 2021 01:31
Otherwise, we have to export libcurl targets and all deps of libcurl
because it is built in-source. CMake is not very user-friendly when it
comes to exporting and installing targets of in-source built dependencies.
@KingKili
Copy link
Member

First thanks for the PR.
It also looks very good. I am currently thinking if we should also force the usage of CPR_USE_SYSTEM_GTEST.

@mirzachi
Copy link
Contributor Author

Enforcing CPR_USE_SYSTEM_GTEST is not necessary for the provided find-package support. CMake output artifacts for tests are binary executables that you do not install anyways. Since tests are built within the project, all dependencies are resolved within the project already.

USE_SYSTEM_GTEST is the variable that you check against in the later if statement and not CPR_USE_SYSTEM_GTEST.
This might be a bug in CMakeLists.txt!

@KingKili
Copy link
Member

Enforcing CPR_USE_SYSTEM_GTEST is not necessary for the provided find-package support. CMake output artifacts for tests are binary executables that you do not install anyways. Since tests are built within the project, all dependencies are resolved within the project already.

That is true, but I was afraid that gtest itself would be installed by default with cpr. But this option is off by default, therefore it should be fine.

USE_SYSTEM_GTEST is the variable that you check against in the later if statement and not CPR_USE_SYSTEM_GTEST.
This might be a bug in CMakeLists.txt!

Thanks for pointing out, I will have look at it.

@KingKili
Copy link
Member

I just tried it locally, and it worked great. Therefore, I will merge your PR. Thanks for the great work!

@KingKili KingKili merged commit 956dc98 into libcpr:master Sep 27, 2021
@COM8 COM8 added this to the CPR 1.7.0 milestone Sep 28, 2021
@COM8 COM8 linked an issue Sep 28, 2021 that may be closed by this pull request
@jmafc jmafc mentioned this pull request Jul 21, 2022
guylevy84 pushed a commit to guylevy84/cpr that referenced this pull request Aug 12, 2022
guylevy84 pushed a commit to guylevy84/cpr that referenced this pull request Aug 12, 2022
guylevy84 pushed a commit to guylevy84/cpr that referenced this pull request Aug 12, 2022
guylevy84 pushed a commit to guylevy84/cpr that referenced this pull request Aug 12, 2022
guylevy84 pushed a commit to guylevy84/cpr that referenced this pull request Aug 12, 2022
guylevy84 pushed a commit to guylevy84/cpr that referenced this pull request Aug 12, 2022
@ronytigo
Copy link

ronytigo commented May 2, 2023

I get "Could NOT find CURL (missing: HTTP) (found version "7.80.0")"

@sachabest
Copy link

sachabest commented Jan 20, 2024

what's the logic behind requiring the system CURL be used in order for CPR to cmake --install properly? Note that if you fetch CPR with FetchContent and do a variant of cmake --install thereafter, you do actually end up with the "fetched" libcurl installed where you'd expect, including the CMake targets file for CURL itself. The only thing missing from this installation method is the CMake targets file for CPR, which is explicitly flagged behind the SYSTEM_LIBCURL flag in this PR.

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

Successfully merging this pull request may close these issues.

cpr-config.cmake for find_package
5 participants