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
Conversation
endif() | ||
include(GNUInstallDirs) | ||
|
||
set(CURL_INSTALL_CMAKE_DIR ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this 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
CMake/curl-config.cmake.in
Outdated
set(CURL_FIND_REQUIRED_libcurl TRUE) | ||
endif() | ||
endif() | ||
@PACKAGE_INIT@ | ||
|
||
include(CMakeFindDependencyMacro) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
CMake/curl-config.cmake.in
Outdated
set(_curl_configurations) | ||
set(_i) | ||
endif() | ||
if("@CMAKE_USE_OPENSSL@") |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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()
Please rebase this (and force-push) to resolve the conflicts. @snikulov, are you fine with this PR? |
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).
Thanks |
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. |
You can do: if(TARGET CURL::libcurl)
# ...
endif() |
Could we add foreach(_comp @EXE_NAME@ @LIB_NAME@)
if (TARGET @PROJECT_NAME@::${_comp})
set(@PROJECT_NAME@_${comp}_FOUND TRUE)
endif()
endforeach() just before curl/CMake/curl-config.cmake.in Line 12 in ae7a09d
This should add support for COMPONENTS again IMHO. |
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... |
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:
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':
Means in 'curl-config.cmake.in':