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: Export libcurl and curl targets to use by other cmake projects #1879

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@jzakrzewski
Contributor

jzakrzewski commented Sep 10, 2017

With those exports one should be able to use curl in own project like this (just a stupid example):

cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)

project(curluser C)

find_package(curl REQUIRED COMPONENTS libcurl OPTIONAL_COMPONENTS curl)

if(curl_curl_FOUND)
    add_custom_target(ex
        COMMAND curl::curl www.example.com -o example.html
    )
endif()

add_executable(user main.c)
target_link_libraries(user curl::libcurl)

@jzakrzewski jzakrzewski requested review from snikulov and Lekensteyn Sep 10, 2017

@bagder bagder added the cmake label Sep 10, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 10, 2017

Member

The new file is not put into the dist tarball so it fails when built from a release tarball - this is the red travis build:

CMake Error: File /home/travis/build/curl/curl/curl-99.98.97/CMake/curl-config.cmake does not exist.
CMake Error at CMakeLists.txt:1312 (configure_file):
  configure_file Problem configuring file

To fix, add the new file to the CMAKE_DIST variable in the root dir's Makefile.am.

Member

bagder commented Sep 10, 2017

The new file is not put into the dist tarball so it fails when built from a release tarball - this is the red travis build:

CMake Error: File /home/travis/build/curl/curl/curl-99.98.97/CMake/curl-config.cmake does not exist.
CMake Error at CMakeLists.txt:1312 (configure_file):
  configure_file Problem configuring file

To fix, add the new file to the CMAKE_DIST variable in the root dir's Makefile.am.

@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Sep 11, 2017

Contributor

Ah, snap! I tend to forget about that.

Contributor

jzakrzewski commented Sep 11, 2017

Ah, snap! I tend to forget about that.

@snikulov

Why lower letter for prefix curl_ used?
You trying not to clash with FindCURL.cmake module bundled with CMake?

@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Sep 11, 2017

Contributor

@snikulov totally by accident (and because the project write its name that way). You think it should be Curl or even CURL?
Frankly speaking this is the first time I'm doing this and all the information about it is quite scattered across various places.
And this is exactly why I'd like some experienced reviewers ;)

Contributor

jzakrzewski commented Sep 11, 2017

@snikulov totally by accident (and because the project write its name that way). You think it should be Curl or even CURL?
Frankly speaking this is the first time I'm doing this and all the information about it is quite scattered across various places.
And this is exactly why I'd like some experienced reviewers ;)

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Sep 11, 2017

Member

@jzakrzewski Well, If nobody will object, I would like to propose CURL_ prefix for all project's variables and options. What do you think?

Member

snikulov commented Sep 11, 2017

@jzakrzewski Well, If nobody will object, I would like to propose CURL_ prefix for all project's variables and options. What do you think?

@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Sep 11, 2017

Contributor

@snikulov So basically use CURL for module name and namespace? Sure, works form me, I'll update it in the evening.
I understand that if someone simply calls find_package(CURL), he'll get it from FindCURL.cmake bundled with the CMake distribution, but find_package(CURL CONFIG) will pick up the config files, right? Hopefully that's not too confusing.

Contributor

jzakrzewski commented Sep 11, 2017

@snikulov So basically use CURL for module name and namespace? Sure, works form me, I'll update it in the evening.
I understand that if someone simply calls find_package(CURL), he'll get it from FindCURL.cmake bundled with the CMake distribution, but find_package(CURL CONFIG) will pick up the config files, right? Hopefully that's not too confusing.

@Lekensteyn

LGTM

@Lekensteyn

This comment has been minimized.

Show comment
Hide comment
@Lekensteyn

Lekensteyn Sep 17, 2017

Member

I would like to propose CURL_ prefix for all project's variables and options. What do you think?

I would recommend "CURL_" for variable names. If you use find_package(Curl) in CMake before 3.2, it would still set the variable CURL_FOUND. Since 3.2, it sets both CURL_FOUND and Curl_FOUND. So to avoid confusion, juse use uppercase.

Member

Lekensteyn commented Sep 17, 2017

I would like to propose CURL_ prefix for all project's variables and options. What do you think?

I would recommend "CURL_" for variable names. If you use find_package(Curl) in CMake before 3.2, it would still set the variable CURL_FOUND. Since 3.2, it sets both CURL_FOUND and Curl_FOUND. So to avoid confusion, juse use uppercase.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Sep 24, 2017

Member

This looks done but one thing, the min ver is bumped from 2.8 to 2.8.8 and I seem to recall there was some issue or reason we went with 2.8 as the minimum version. The commit is 14aa8f0 but it doesn't have a reference url, does anyone remember why 2.8 was chosen as the minimum? Is there some LTS that uses 2.8.0 ? For reference Ubuntu 14 LTS and CentOS 7 have a packaged cmake 2.8.12.2, Ubuntu 10 LTS (past EOL) has 2.8.2 and I'm not sure about Ubuntu 12.

Member

jay commented Sep 24, 2017

This looks done but one thing, the min ver is bumped from 2.8 to 2.8.8 and I seem to recall there was some issue or reason we went with 2.8 as the minimum version. The commit is 14aa8f0 but it doesn't have a reference url, does anyone remember why 2.8 was chosen as the minimum? Is there some LTS that uses 2.8.0 ? For reference Ubuntu 14 LTS and CentOS 7 have a packaged cmake 2.8.12.2, Ubuntu 10 LTS (past EOL) has 2.8.2 and I'm not sure about Ubuntu 12.

@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Sep 24, 2017

Contributor

I have updated the CMake version only as much as it was required for all the bits and pieces to work. Generally we have agreed (see #1010 ) that 2.8.12 is reasonable. However, I have decided not to jump there without any reason. The dumbest reason would be OK though or we make the update right here, right now and get it over with. What do you think?

Contributor

jzakrzewski commented Sep 24, 2017

I have updated the CMake version only as much as it was required for all the bits and pieces to work. Generally we have agreed (see #1010 ) that 2.8.12 is reasonable. However, I have decided not to jump there without any reason. The dumbest reason would be OK though or we make the update right here, right now and get it over with. What do you think?

@Lekensteyn

This comment has been minimized.

Show comment
Hide comment
@Lekensteyn

Lekensteyn Sep 24, 2017

Member

Since the autotools build system is still present and cmake is still marked experimental, I think it is OK to bump CMake quite a bit. We can declare CMake on Ubuntu 12.04 unsupported (use autotools in that case).

One good reason to set a higher CMake version is to allow for more freedom, rather than working around missing CMake language features in the old version, you can use the newer version instead. And you also do not have to test with older CMake.

CMake 2.8.12 is OK, I just checked and as of November 2016 RHEL 7 has also been updated with this version. Updated #990 (comment)

Member

Lekensteyn commented Sep 24, 2017

Since the autotools build system is still present and cmake is still marked experimental, I think it is OK to bump CMake quite a bit. We can declare CMake on Ubuntu 12.04 unsupported (use autotools in that case).

One good reason to set a higher CMake version is to allow for more freedom, rather than working around missing CMake language features in the old version, you can use the newer version instead. And you also do not have to test with older CMake.

CMake 2.8.12 is OK, I just checked and as of November 2016 RHEL 7 has also been updated with this version. Updated #990 (comment)

@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Sep 25, 2017

Contributor

Great, I'll update this PR one more time to bump the version all the way to 2.8.12 and it'll be ready for the next feature window.
I'd like to add the CMake config files to the autotools build also but I'm still not sure how to do it. It would be nice if people could rely on that files (at least from certain version of curl).

Contributor

jzakrzewski commented Sep 25, 2017

Great, I'll update this PR one more time to bump the version all the way to 2.8.12 and it'll be ready for the next feature window.
I'd like to add the CMake config files to the autotools build also but I'm still not sure how to do it. It would be nice if people could rely on that files (at least from certain version of curl).

jzakrzewski added some commits Sep 10, 2017

include directories not populated
The imported target has been missing include directories
It worked on my machine beacuse of globally-installed curl
@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Oct 7, 2017

Contributor

The new commits are intended to be squashed with the first one.
The config file now provides, next to the imported target, the old-style CURL_INCLUDE_DIRS and CURL_LIBRARIES so it should be painless to migrate

Contributor

jzakrzewski commented Oct 7, 2017

The new commits are intended to be squashed with the first one.
The config file now provides, next to the imported target, the old-style CURL_INCLUDE_DIRS and CURL_LIBRARIES so it should be painless to migrate

@snikulov

LGTM

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