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

Closed
wants to merge 3 commits into from

Conversation

jzakrzewski
Copy link
Contributor

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)

@bagder
Copy link
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
Copy link
Contributor Author

Ah, snap! I tend to forget about that.

Copy link
Member

@snikulov snikulov left a comment

Choose a reason for hiding this comment

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

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

@jzakrzewski
Copy link
Contributor Author

@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
Copy link
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?

@jzakrzewski
Copy link
Contributor Author

@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.

Copy link
Contributor

@Lekensteyn Lekensteyn left a comment

Choose a reason for hiding this comment

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

LGTM

@Lekensteyn
Copy link
Contributor

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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).

@jay jay added the feature-window A merge of this requires an open feature window label Oct 3, 2017
The imported target has been missing include directories
It worked on my machine beacuse of globally-installed curl
@jzakrzewski
Copy link
Contributor Author

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

Copy link
Member

@snikulov snikulov left a comment

Choose a reason for hiding this comment

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

LGTM

@bagder bagder removed the feature-window A merge of this requires an open feature window label Sep 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants