aws / aws-sdk-cpp

AWS SDK for C++
Apache License 2.0
1.96k stars 1.05k forks source link

Wrong OpenSSL CMake targets used #2874

Closed gjasny closed 6 months ago

gjasny commented 7 months ago

Describe the bug

Hi,

while working on packaging the 1.11 version for Conan (conan-io/conan-center-index#22905) I noticed that compilation cannot find any OpenSSL headers. The Conan package manager fiddles with some CMake default lookup rules which might reveal the error.

This is for version 1.11.273: In your CMakeLists.txt in line 208 you include external_dependencies https://github.com/aws/aws-sdk-cpp/blob/5929e202e9b1cd84d6234aa01b64f56fd2208350/CMakeLists.txt#L208

which does the OpenSSL crypto lib lookup and as part of that sets CRYPTO_LIBS to the imported target AWS::crypto: https://github.com/aws/aws-sdk-cpp/blob/5929e202e9b1cd84d6234aa01b64f56fd2208350/cmake/external_dependencies.cmake#L40-L54

This is good because linking the CMake aws-cpp-sdk-all against the imported target later will apply both, the link libraries and the include dirs.

But in your CMakeLists.txt lines 250 you overwrite CRYPTO_LIBS with ${OPENSSL_LIBRARIES}. https://github.com/aws/aws-sdk-cpp/blob/5929e202e9b1cd84d6234aa01b64f56fd2208350/CMakeLists.txt#L260-L266

The problem is that OPENSSL_LIBRARIES is empty and I also could not find any invocation of find_package(OpenSSL) which could have set it.

The same problem applies to CLIENT_LIBS.

Could you please take a look what's going on and which of the code blocks is the correct and desired one?

Expected Behavior

OpenSSL is properly used.

Current Behavior

[ 18%] Building CXX object src/aws-cpp-sdk-core/CMakeFiles/aws-cpp-sdk-core.dir/source/http/curl/CurlHttpClient.cpp.o
In file included from /Users/jenkins/w/prod-v2/bsr@2/98097/eacca/p/b/aws-s92c917073c641/b/build/Release/src/aws-cpp-sdk-core/ub_core.cpp:50:
In file included from /Users/jenkins/w/prod-v2/bsr@2/98097/eacca/p/b/aws-s92c917073c641/b/src/src/aws-cpp-sdk-core/source/utils/crypto/openssl/CryptoImpl.cpp:9:
/Users/jenkins/w/prod-v2/bsr@2/98097/eacca/p/b/aws-s92c917073c641/b/src/src/aws-cpp-sdk-core/include/aws/core/utils/crypto/openssl/CryptoImpl.h:12:10: fatal error: 'openssl/ossl_typ.h' file not found
#include <openssl/ossl_typ.h>
         ^~~~~~~~~~~~~~~~~~~~

Reproduction Steps

git clone https://github.com/gjasny/conan-center-index.git
git checkout aws-sdk-cpp/1.11.179
git reset --hard 026b203b4f46d6b637899e2b020cd41e914f9667
# install Conan 2
recipes/aws-sdk-cpp/conan create all/conanfile.py --version 1.11.179 --user local --channel local -pr:b=default -pr:h=default --build=missing

Possible Solution

I commented out the block which overwrites the discovered variables: https://github.com/gjasny/conan-center-index/blob/6d49f275630c48a8f757b2ec0f58fe0ebb607b69/recipes/aws-sdk-cpp/all/patches/1.11.179-0002-fix-openssl-and-curl-lookup.patch#L1-L22

Additional Information/Context

No response

AWS CPP SDK version used

1.11.273

Compiler and Version used

Xcode 15

Operating System and version

macOS 14.3

jmklix commented 6 months ago

Thanks for opening this issue. We are currently in the process of reworking our cmake build process.

I'm not sure that your possible solution will fit all use cases, but I wanted to make sure I understand your request. Are you suggested commenting out this entire block?

    #[[
    if (ENABLE_BCRYPT_ENCRYPTION)
        set(CRYPTO_LIBS Bcrypt)
        set(CRYPTO_LIBS_ABSTRACT_NAME Bcrypt)
    elseif (ENABLE_OPENSSL_ENCRYPTION)
        set(CRYPTO_LIBS ${OPENSSL_LIBRARIES} ${ZLIB_LIBRARIES})
        set(CRYPTO_LIBS_ABSTRACT_NAME crypto ssl z)
    endif ()

    if (ENABLE_CURL_CLIENT)
        set(CLIENT_LIBS ${CURL_LIBRARIES})
        set(CLIENT_LIBS_ABSTRACT_NAME curl)
    elseif (ENABLE_WINDOWS_CLIENT)
        if (USE_IXML_HTTP_REQUEST_2)
            set(CLIENT_LIBS msxml6 runtimeobject)
            set(CLIENT_LIBS_ABSTRACT_NAME msxml6 runtimeobject)
            if (BYPASS_DEFAULT_PROXY)
                list(APPEND CLIENT_LIBS winhttp)
                list(APPEND CLIENT_LIBS_ABSTRACT_NAME winhttp)
            endif ()
        else ()
            set(CLIENT_LIBS Wininet winhttp)
            set(CLIENT_LIBS_ABSTRACT_NAME Wininet winhttp)
        endif ()
    endif ()
    ]]
github-actions[bot] commented 6 months ago

Greetings! It looks like this issue hasn’t been active in longer than a week. We encourage you to check if this is still an issue in the latest release. Because it has been longer than a week since the last update on this, and in the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or add an upvote to prevent automatic closure, or if the issue is already closed, please feel free to open a new one.

MouhK commented 1 month ago

Any update on this issue. I have the same problem

jmklix commented 1 month ago

Yes, this should be fixed with this patch. We updated this sdk to not depend on crypto directly, but rather use the common runtime's(aws-c-common) dependency on crypto. Updating this sdk to the latest version should fix any problems that you might be running into with this. If it doesn't fix it for you please open a new issue/discussion.