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

Closed
wants to merge 1 commit into from
Closed

CMake: Improve config installation #2849

wants to merge 1 commit into from

Conversation

ruslo
Copy link
Contributor

@ruslo 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})
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@slodki slodki left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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@")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

bagder commented Sep 26, 2018

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

@snikulov, are you fine with this PR?

@snikulov
Copy link
Member

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

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

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

jay commented Oct 1, 2018

Thanks

@ruslo ruslo deleted the pr.config.install branch October 8, 2018 06:30
@slodki
Copy link
Contributor

slodki commented Nov 24, 2018

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

This breaks all CMake projects that depend on libcurl - only projects without specified components (== curl binary AND libcurl library) are working, projects that require library only must change their sources to be 7.62 compatible.
Is there any way to check what (executable or library) exactly was found if CURL_FOUND is set to TRUE?

@ruslo
Copy link
Contributor Author

ruslo commented Nov 24, 2018

Is there any way to check what (executable or library) exactly was found if CURL_FOUND is set to TRUE?

You can do:

if(TARGET CURL::libcurl)
  # ...
endif()

@slodki
Copy link
Contributor

slodki commented Nov 24, 2018

Could we add

foreach(_comp @EXE_NAME@ @LIB_NAME@)
  if (TARGET @PROJECT_NAME@::${_comp})
    set(@PROJECT_NAME@_${comp}_FOUND TRUE)
  endif()
endforeach()

just before

check_required_components("@PROJECT_NAME@")
?

This should add support for COMPONENTS again IMHO.

@bagder
Copy link
Member

bagder commented Nov 24, 2018

This breaks all CMake projects that depend on libcurl

If this is true, can you please submit this as an issue? Commenting in an old and closed PR may not get the necessary attention...

@lock lock bot locked as resolved and limited conversation to collaborators Feb 22, 2019
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

6 participants