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: Improve config installation #2849

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@ruslo
Contributor

ruslo commented Aug 8, 2018

Use 'GNUInstallDirs' standard module to set destinations of installed files.

Use uppercase "CURL" names instead of lowercase "curl" to match standard
'FindCURL.cmake' CMake module:

Meaning:

  • Install 'CURLConfig.cmake' instead of 'curl-config.cmake'
  • User should call 'find_package(CURL)' instead of 'find_package(curl)'

Use 'configure_package_config_file' function to generate 'CURLConfig.cmake'
file. This will make 'curl-config.cmake.in' template file smaller and handle
components better. E.g. current configuration report no error if user
specified unknown components (note: new configuration expects no components,
report error if user will try to specify any).

Condition for calling 'find_dependency' should match condition
from 'CMakeLists.txt':

if(CMAKE_USE_OPENSSL)
  find_package(OpenSSL ...)
  ...
endif()

Means in 'curl-config.cmake.in':

if("@CMAKE_USE_OPENSSL@")
  find_dependency(OpenSSL ...)
endif()
endif()
include(GNUInstallDirs)
set(CURL_INSTALL_CMAKE_DIR ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME})

This comment has been minimized.

@donny-dont

donny-dont Aug 8, 2018

Contributor

I've been using vcpkg for building out libraries on Windows. One of its expectations is that lib files just contain object code. With things like cmake files it wants them to go into share/curl. Its pretty good about CMake conventions so I figured I'd mention it.

Other than that this looks good.

This comment has been minimized.

@ruslo

ruslo Aug 9, 2018

Contributor

One of its expectations is that lib files just contain object code. With things like cmake files it wants them to go into share/curl

Can you please clarify is it relate to this patch or it's something you just want to have? Because before it was CMake and wasn't share/curl as you are expecting.

As a note there is no hard requirement for the layout on CMake side, from documentation:

This is merely a convention, so all (W) and (U) directories are still searched on all platforms.

So for simplicity reason it make sense to use same layout on all platforms. ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME} is a generalization of previously used lib/cmake/curl.

@snikulov snikulov added the cmake label Aug 14, 2018

@slodki

Please merge this PR as current 7.61.1 is broken!

User must have OpenSSL installed even if not used by libcurl at all since 7.61.1 release.
Broken at 7867aaa

set(CURL_FIND_REQUIRED_libcurl TRUE)
endif()
endif()
@PACKAGE_INIT@
include(CMakeFindDependencyMacro)

This comment has been minimized.

@slodki

slodki Sep 15, 2018

Contributor

This can be moved into if() scope below - to include only is used

This comment has been minimized.

@ruslo

ruslo Sep 16, 2018

Contributor

There will be more dependencies here, so I think pattern:

include(CMakeFindDependencyMacro)

if(@DEPENDENCY1@)
  # ...
endif()

if(@DEPENDENCY2@)
  # ...
endif()

if(@DEPENDENCY3@)
  # ...
endif()

is better than the pattern:

if(@DEPENDENCY1@)
  include(CMakeFindDependencyMacro)
  # ...
endif()

if(@DEPENDENCY2@)
  include(CMakeFindDependencyMacro)
  # ...
endif()

if(@DEPENDENCY3@)
  include(CMakeFindDependencyMacro)
  # ...
endif()

Unless you have the stronger reason like benchmark.

set(_curl_configurations)
set(_i)
endif()
if("@CMAKE_USE_OPENSSL@")

This comment has been minimized.

@slodki

slodki Sep 15, 2018

Contributor

find_package(OpenSSL REQUIRED) is used currently so CMAKE_USE_OPENSSL and USE_OPENSSL are equal here. But this can change in the future and USE_OPENSSL will be better here, as it means OpenSSL was used (CMAKE_USE_OPENSSL means OpenSSL was requested).

This comment has been minimized.

@ruslo

ruslo Sep 16, 2018

Contributor

But this can change in the future and USE_OPENSSL will be better here, as it means OpenSSL was used (CMAKE_USE_OPENSSL means OpenSSL was requested).

In my opinion this is not a nice approach. For example if I set CMAKE_USE_LIBSSH2=ON I expect error if libssh2 is not found, not silent ignore. But it's a topic for another discussion, not related to this pull request.

CMAKE_USE_OPENSSL variable is symmetrical, hence improve readability:

# CMakeLists.txt

if(CMAKE_USE_OPENSSL)
  find_package(OpenSSL ...)
  ...
endif()
# curl-config.cmake.in

if("@CMAKE_USE_OPENSSL@")
  find_dependency(OpenSSL ...)
endif()
@bagder

This comment has been minimized.

Member

bagder commented Sep 26, 2018

Please rebase this (and force-push) to resolve the conflicts.

@snikulov, are you fine with this PR?

@snikulov

This comment has been minimized.

Member

snikulov commented Sep 26, 2018

@bagder LGTM.
Part of this changes was already merged to curl with #3001 so pull definitely should be rebased.

CMake: Improve config installation
Use 'GNUInstallDirs' standard module to set destinations of installed files.

Use uppercase "CURL" names instead of lowercase "curl" to match standard
'FindCURL.cmake' CMake module:
* https://cmake.org/cmake/help/latest/module/FindCURL.html

Meaning:
* Install 'CURLConfig.cmake' instead of 'curl-config.cmake'
* User should call 'find_package(CURL)' instead of 'find_package(curl)'

Use 'configure_package_config_file' function to generate 'CURLConfig.cmake'
file. This will make 'curl-config.cmake.in' template file smaller and handle
components better.  E.g.  current configuration report no error if user
specified unknown components (note: new configuration expects no components,
report error if user will try to specify any).
@ruslo

This comment has been minimized.

Contributor

ruslo commented Oct 1, 2018

Part of this changes was already merged to curl with #3001 so pull definitely should be rebased

Conflicts with #3001 pull request have already been resolved some time ago, new conflicts appeared after b801b45 commit.

All conflicts resolved now.

@jay jay closed this in 6932849 Oct 1, 2018

@jay

This comment has been minimized.

Member

jay commented Oct 1, 2018

Thanks

@ruslo ruslo deleted the ruslo:pr.config.install branch Oct 8, 2018

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