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

Export targets for CPR (WIP) #56

Closed
wants to merge 6 commits into from
Closed

Conversation

jgsogo
Copy link

@jgsogo jgsogo commented Oct 14, 2015

I'm working in this pull-request to provide CPR with the CMake sorcery in order to convert it into a package with version semantic and so on... It is a work in progress I'm developing and trying in Windows (MSVC2015) and Ubuntu, but I wanted to share with you these commits in order to know if it is suitable to be merged or I may redirect my efforts into another direction.

Major changes are:

  • CURL is included using ExternalProject_add when it is not used the one from the system. This function is using the sources in opt/curl directory although it can start a download or even a clone-update from a remote repository
  • I've also added CMake sorcery to create cpr-config-* and cpr-version-* files and making CPR itself installable via make install

There are still many issues to address, now I can only think in:

  • Check static/shared build
  • Testing with other compilers in Windows and Unix systems
  • ....

Let's talk about it.

CURL is included in CMake with ExternalProject_Add (if not used a system
installation), this way we are able to export targets from CURL and also
from our library CPR
@whoshuu
Copy link
Contributor

whoshuu commented Oct 15, 2015

Thanks @jgsogo for the PR!

The version number for the next release will be 1.2.0. See issue #43.

There's a lot of CMake here I'm not familiar with. Are there resources (besides the documentation) you can point me to, to help me grok it a bit faster? I'm all for expanding the integration options for this library, so I really appreciate the pull request.

@jgsogo
Copy link
Author

jgsogo commented Oct 15, 2015

The objective is to be able to use CPR in another project just by writing a CMakeLists.txt with these lines:

cmake_minimum_required(VERSION 2.8.7)

find_package(CPR 1.2.0)

add_executable(example_cpr main.cpp)
target_link_libraries(example_cpr ${CPR_LIBRARIES})
target_include_directories(example_cpr PUBLIC ${CPR_INCLUDE_DIRS})

It may be needed to set a path-variable called CPR_DIR in CMake with the path to the cpr-targets.cmake folder (the same as with Boost libraries).

See find_package docs for info about how version string is treated.


I have been following this document for several projects of mine to create the packages (maybe not so trendy now as they link to an updated doc).
About ExternalProject_Add I don't have any special tutorial :/


I'm testing it now in Windows (make and make install works ok) but I'm finding an issue with curl, why do they call the library libcurl_impl.lib, why that _impl suffix?

@jgsogo
Copy link
Author

jgsogo commented Oct 15, 2015

TODO:

  • Test that everything stills works when using CURL from system.
  • Allow the user to compile CURL as static lib and build all variables accordingly
  • Testing

@whoshuu
Copy link
Contributor

whoshuu commented Oct 17, 2015

Hey @jgsogo, thanks for explaining the motivation behind the changes you're proposing. Currently, it's possible to use find_package to get the library and include directory for integrating cpr into another project, with the caveat that curl would have to be pulled in separately.

Just to clarify, your changes intend to allow you to use find_package to pull in curl automatically?

@jgsogo
Copy link
Author

jgsogo commented Oct 18, 2015

These are the files generated by CMake that will be handled and found by find_package command: https://gist.github.com/jgsogo/d409d7573896486c8cec They handle version string and library names (debug version may have a differente name, i.e. a suffix like _d). As you can see these files makes reference to the exact path for the library generated at the same time of those files (yo can have several compiled instances of the same library with different compile options or versions and point in each project to a different one).

Another possibility is to implement a FindCPR.cmake module like any of these which is another option to be used with find_package.

But no, this changes won't pull curl automatically with a call to find_package(cpr). Curl could be downloaded, compiled and installed within a call to make CPR, but now the ExternalProject_Add command is used with the source files from the CURL repository that are attached to the CPR repo as dependency.

@whoshuu
Copy link
Contributor

whoshuu commented Oct 21, 2015

@jgsogo, thanks for the great response! I'll take a closer look at this pull request when I have some spare cycles on the weekend.

@zkSNARK
Copy link

zkSNARK commented Jun 12, 2018

I think the reasoning behind this pull request (perhaps not the exact semantics) is perhaps far more relevant today in 2018 than when the pull request was submitted. Modern cmake enabling use of find_package is really a necessity at this point.

include_directories() is actually described as an anti-pattern in Daniel Pfeifer’s C++Now 2017 talk on modern cmake.

@tastytea
Copy link
Contributor

Please use GNUInstallDirs. It is portable, the variables can be overridden and package maintainers are familiar with it.

set(CPR_MAJOR_VERSION 1)
set(CPR_MINOR_VERSION 2)
set(CPR_PATCH_VERSION 0)
set(CPR_VERSION ${CPR_MAJOR_VERSION}.${CPR_MINOR_VERSION}.${CPR_PATCH_VERSION})
Copy link
Contributor

Choose a reason for hiding this comment

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

The version can be specified in project() without having to use 3 extra variables.

Example:

project(cpr
  VERSION 0.1.2
  LANGUAGES CXX)

This will also automatically generate the 3 variables cpr_VERSION_MAJOR, cpr_VERSION_MINOR and cpr_VERSION_PATCH.

@COM8
Copy link
Member

COM8 commented Jun 10, 2020

With #383 I would say all features proposed here have been merged.
I’m closing this.
Feel free to open a new PR if something is missing.

@COM8 COM8 closed this Jun 10, 2020
@jgsogo jgsogo deleted the export-targets branch June 10, 2020 09:06
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.

5 participants