search

Azure / azure-sdk-for-cpp

This repository is for active development of the Azure SDK for C++. For consumers of the SDK we recommend visiting our versioned developer docs at https://azure.github.io/azure-sdk-for-cpp.
MIT License
173 stars 122 forks source link

Adding Azure SDK as an external CMake project causes compile errors when including blobs.hpp #1038

Closed lordgamez closed 3 years ago

lordgamez commented 3 years ago

Describe the bug When adding azure-sdk-for-cpp as an external library to a client application and including blobs.hpp compilation fails. Including blobs.hpp transitively includes policy.hpp as well which uses CurlTransport included from curl.hpp. CurlTransport is not defined as the client library using Azure SDK does not contain the BUILD_CURL_HTTP_TRANSPORT_ADAPTER definition which is required for the CurlTransport compilation.

Workaround: add add_definitions("-DBUILD_CURL_HTTP_TRANSPORT_ADAPTER") to the cmake files of the client library. Preliminary analysis: Seems to be present since commit cd2a8a3

Exception or Stack Trace

In file included from /home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/core/azure-core/inc/azure/core/credentials.hpp:12,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp:11,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/append_blob_client.hpp:6,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs.hpp:6,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/extensions/azure/storage/AzureBlobStorage.h:27,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/extensions/azure/storage/AzureBlobStorage.cpp:21:
/home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/core/azure-core/inc/azure/core/http/policy.hpp:122:84: error: ‘CurlTransport’ is not a member of ‘Azure::Core::Http’; did you mean ‘HttpTransport’?
  122 |     std::shared_ptr<HttpTransport> Transport = std::make_shared<Azure::Core::Http::CurlTransport>();
      |                                                                                    ^~~~~~~~~~~~~
      |                                                                                    HttpTransport
/home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/core/azure-core/inc/azure/core/http/policy.hpp:122:84: error: ‘CurlTransport’ is not a member of ‘Azure::Core::Http’; did you mean ‘HttpTransport’?
  122 |     std::shared_ptr<HttpTransport> Transport = std::make_shared<Azure::Core::Http::CurlTransport>();
      |                                                                                    ^~~~~~~~~~~~~
      |                                                                                    HttpTransport
/home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/core/azure-core/inc/azure/core/http/policy.hpp:122:99: error: no matching function for call to ‘make_shared<<expression error> >()’
  122 |     std::shared_ptr<HttpTransport> Transport = std::make_shared<Azure::Core::Http::CurlTransport>();
      |                                                                                                   ^
In file included from /usr/include/c++/9/memory:81,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/extensions/azure/storage/AzureBlobStorage.h:24,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/extensions/azure/storage/AzureBlobStorage.cpp:21:
/usr/include/c++/9/bits/shared_ptr.h:714:5: note: candidate: ‘template<class _Tp, class ... _Args> std::shared_ptr<_Tp> std::make_shared(_Args&& ...)’
  714 |     make_shared(_Args&&... __args)
      |     ^~~~~~~~~~~
/usr/include/c++/9/bits/shared_ptr.h:714:5: note:   template argument deduction/substitution failed:
In file included from /home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/core/azure-core/inc/azure/core/credentials.hpp:12,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp:11,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/append_blob_client.hpp:6,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs.hpp:6,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/extensions/azure/storage/AzureBlobStorage.h:27,
                 from /home/ggyimesi/projects/nifi-minifi-cpp-fork/extensions/azure/storage/AzureBlobStorage.cpp:21:
/home/ggyimesi/projects/nifi-minifi-cpp-fork/build/thirdparty/azure-sdk-cpp-src/sdk/core/azure-core/inc/azure/core/http/policy.hpp:122:99: error: template argument 1 is invalid
  122 |     std::shared_ptr<HttpTransport> Transport = std::make_shared<Azure::Core::Http::CurlTransport>();
      |                                                                                                   ^
make[2]: *** [extensions/azure/CMakeFiles/minifi-azure.dir/build.make:102: extensions/azure/CMakeFiles/minifi-azure.dir/storage/AzureBlobStorage.cpp.o] Error 1

To Reproduce Steps to reproduce the behavior:

  1. Create a CMake project using Azure SDK as an external project. 
    ExternalProject_Add(
    azure-sdk-cpp-external
    GIT_REPOSITORY "https://github.com/Azure/azure-sdk-for-cpp.git"
    GIT_TAG "85fb3e35306ace5218e93ceeed8fda59828fa9bc"
    BUILD_IN_SOURCE true
    SOURCE_DIR "${BINARY_DIR}/thirdparty/azure-sdk-cpp-src"
    BUILD_BYPRODUCTS "${AZURESDK_LIBRARIES_LIST}"
   EXCLUDE_FROM_ALL TRUE
   STEP_TARGETS build
)
  2. Include azure/storage/blobs.hpp in your project and compile.

Code Snippet

#include "azure/storage/blobs.hpp"
#include <memory>

int main() {
  auto container_client = Azure::Storage::Blobs::BlobContainerClient::CreateFromConnectionString("connectionstring", "containername");
  return 0;
}

Expected behavior It should be possible to add Azure SDK as an external project and include its headers without the need to define BUILD_CURL_HTTP_TRANSPORT_ADAPTER in the client library.

Screenshots N/A

Setup (please complete the following information):

Additional context Add any other context about the problem here.

Information Checklist Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

Jinming-Hu commented 3 years ago

I think this is a bug of core SDK. We should have a default transport layer if not explicitly specified.

RickWinter commented 3 years ago

@vhvb1989 Matching the transport to the OS by default would make on-boarding easier. On Windows they get WinHttp and everywhere else they would get CURL.

vhvb1989 commented 3 years ago

the symbol BUILD_CURL_HTTP_TRANSPORT_ADAPTER is added by out CMake module DefineTransportAdapter. It makes sure to set a default transport adapter depending on the OS when no transport was explicitly mentioned at generate time.

@RickWinter , before fixing this, can you define if we will support using ExternalProject_Add from CMake?

My understanding is that @antkmsft is finalizing the vcpkg support to be our main release vehicle. vcpkg and CMake ExternalProject_Add behaves differently. Vcpkg would get any dependency chain (for blobs, it would take storage-common, azure core, libcurl, json, xml, etc..). While using CMake, we would still need to document how to make it work (how to get dependencies, and what dependencies).

At any case, I would personally prefer to recommend using fetchContent vs ExternalProject_Add when using CMake as a package manager. See: https://www.scivision.dev/cmake-fetchcontent-vs-external-project/ When using fetchContent, the top CMake list works well to define all the default symbols we need.

ahsonkhan commented 3 years ago

@antkmsft can you please validate that this scenario is fixed with your change in #1346

vhvb1989 commented 3 years ago

I am sure this is fixed on latest preview release. @RickWinter can we close and ask @lordgamez to re-open if he continue to see this issue?

lordgamez commented 3 years ago

I am sure this is fixed on latest preview release. @RickWinter can we close and ask @lordgamez to re-open if he continue to see this issue?

I am okay with this approach, thank you!