-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[openssl] incorrect CMake config (patch) #40722
Comments
Can you please explain how to reproduce the problem? |
in my use case find_package(OpenSSL CONFIG REQUIRED) failed with something like
You just shouldn't mix CONFIG and MODULE modes for package lookup. I think CMake's own module just notice some inconsistancies (i.e. previously found package), because some variables are already set. |
The call stack shows that you are not using the |
@dg0yt I'm not using it, however this is not the problem. vcpkg.cmake doesn't have any specific handling for OpenSSL (it doesn't set VCPKG_OPENSSL_USE_SINGLE_CONFIG variable in any way). I still think, that the Patch provided by vcpkg is incorrect. If upstream version of package provides CMake package configuration, it should be used and just modified to handle vcpkg installation layout, so that debug/release libraries won't get mixedup. |
Please show the error with the vcpkg toolchain.
Port openssl has a vcpkg cmake wrapper which is invoked via
This upstream uses a unique build system which pessimizes Windows in favor of more exotic systems. Their CMake config doesn't support multi-config. I'm still waiting for other changes to be integrated. That being said, I'm not opposed to a better patch. But we need proper multi-config support. |
@dg0yt
Does Windows Build produces CMake Config? If so I think it could be patched more or less reasonable.
This is what I meant by saying about better solution. Adding "set_target_properties" to imported targets should actually make it multi-config capable, because cmake will handle configurations for you. Here's example from what cmake usually generates with 'proj' library as an example:
and release
So the patch should kinda add similar set_property on corresponding targets and that would be it. |
"Windows Build"? For Windows, OpenSSL's
No added value. I'm familiar with PROJ, and I'm familiar with config generated by CMake. The point is not just to set per-config properties, but to have separate files for general CMake code and for per-config stuff. And note that the build type (as encoded in properties) is fixed at OpenSSL build time (i.e. when generating the config). Your initial post suggests codes which looks up So we also need a mapping of OpenSSL config parameters to the config type to be exported. And ideally forming a patch which is upstream might accept after some years. |
Are you sure, you need separate files per configuration? |
This is an automated message. Per our repo policy, stale issues get closed if there has been no activity in the past 28 days. The issue will be automatically closed in 14 days. If you wish to keep this issue open, please add a new comment. |
The problem still exists There are 2 configs in vcpkg_installed/x64-linux/share/openssl
The contents of OpenSSLConfig.cmake are incorrect: get_filename_component(_ossl_prefix "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(_ossl_prefix "${_ossl_prefix}" PATH)
get_filename_component(_ossl_prefix "${_ossl_prefix}" PATH)
get_filename_component(_ossl_prefix "${_ossl_prefix}" PATH)
get_filename_component(_ossl_prefix "${_ossl_prefix}" PATH) If you call find_package(OpenSSL), it will use the wrong OpenSSLConfig.cmake It affects building of opentelemetry-cpp port with otlp_grpc enabled |
This call does not use CMake config unless you ask for it with extra options or variables. |
If you call find_package(OpenSSL) from another port build, the wrong OpenSSLConfig.cmake will be used |
Virtually all ports use OpenSSL successfully. |
The problem can be reproduced by building the opentelemetry-cpp port with otlp_grpc enabled No special settings in my project. The error appears after updating vcpkg to the latest version. And yes, it can be fixed by patching OpenSSL's portfile.cmake: file(REMOVE "${CURRENT_PACKAGES_DIR}/share/${PORT}/OpenSSLConfig.cmake")
file(REMOVE "${CURRENT_PACKAGES_DIR}/share/${PORT}/OpenSSLConfigVersion.cmake") |
The prefix/include dir patching was probably broken with 3.3.2 in openssl/openssl@1c437b5. But I see the issue was opened for 3.3.1. I added a correction to the 3.4.0 update PR now. It still doesn't give the right libdirs etc. for the debug config. Everybody invited to contribute. |
So this port's |
Fixing in #41742. |
The fix for opentelemetry-cpp was merged. openssl still waits for contributions. |
Is your feature request related to a problem? Please describe.
OpenSSL version 3.3 provides it's own CMake Config.
vcpkg patches this config, one part of the patch corrects paths (vcpkg_cmake_fixup won't work here).
But the other part of the patch does pretty strange thing. It calls CMake's own FindOpenSSL module, which confuses CMake and causes it not be able to find the openssl at all.
Proposed solution
Remove this part:
vcpkg/ports/openssl/cmake-config.patch
Lines 27 to 52 in f0c8848
Instead add something like below, because original OpenSSL config fails to distinguish debug/release modes and that's what hat do be done IMHO
(this was my quick and dirty fix)
Better Solution
Better solution would be to modify OpenSSL Config in the way, that it sets target properties for debug/release builds, so that CMake is able to point to the proper libraries.
The text was updated successfully, but these errors were encountered: