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

add azure-storage-cpp/7.5.0 #5605

Merged
merged 16 commits into from
Jun 26, 2021

Conversation

dvirtz
Copy link
Contributor

@dvirtz dvirtz commented May 23, 2021

Specify library name and version: azure-storage-cpp/7.5.0


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@CLAassistant
Copy link

CLAassistant commented May 23, 2021

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@dvirtz dvirtz force-pushed the dvirtz/azure-storage-sdk branch from e44cac9 to c271eaf Compare May 23, 2021 12:05
@conan-center-bot

This comment has been minimized.

@dvirtz
Copy link
Contributor Author

dvirtz commented May 23, 2021

same issue as in #5570 (comment)

@dvirtz dvirtz closed this May 25, 2021
@dvirtz dvirtz reopened this May 25, 2021
@conan-center-bot

This comment has been minimized.

@jgsogo
Copy link
Contributor

jgsogo commented May 25, 2021

same issue as in #5570 (comment)

Ping us when this happens... we can trigger a build to generate these missing packages behind the scenes... if we realize about it. It should be better now that the CI will send us a message: #5614

@dvirtz
Copy link
Contributor Author

dvirtz commented May 25, 2021

Thanks @jgsogo . Is cpprestsdk being regenerated now?

@conan-center-bot

This comment has been minimized.

@dvirtz dvirtz force-pushed the dvirtz/azure-storage-sdk branch from c271eaf to 4f1227b Compare May 30, 2021 08:25
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@dvirtz
Copy link
Contributor Author

dvirtz commented May 30, 2021

all errors now are Missing prebuilt package

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Some minor suggestions

- find_package(LibXML2 REQUIRED)
+ find_package(libuuid REQUIRED)
+ find_package(cpprestsdk REQUIRED)
+ find_package(LibXml2 REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the one exposed by conan wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

set(AZURESTORAGE_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/includes)
-set(AZURESTORAGE_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/includes ${CASABLANCA_INCLUDE_DIR} ${Boost_INCLUDE_DIRS} ${OPENSSL_INCLUDE_DIRS} ${UUID_INCLUDE_DIRS} ${LibXML2_INCLUDE_DIR})
+set(AZURESTORAGE_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/includes ${cpprestsdk_INCLUDE_DIR} ${Boost_INCLUDE_DIRS} ${OpenSSL_INCLUDE_DIRS} ${libuuid_INCLUDE_DIRS} ${LibXml2_INCLUDE_DIR})

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not use cmake targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could but it would require patching more files and this one seems to work.
I prefer to keep the patch minimal if that's ok.

def build(self):
cmake = CMake(self)
if not self.settings.compiler.cppstd:
cmake.definitions["CMAKE_CXX_STANDARD"] = 11
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be move to the cmake since it the test package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@conan-center-bot

This comment has been minimized.

@dvirtz dvirtz force-pushed the dvirtz/azure-storage-sdk branch from 3d69517 to cd64d3e Compare June 23, 2021 11:44
@conan-center-bot

This comment has been minimized.

prince-chrismc
prince-chrismc previously approved these changes Jun 23, 2021
Comment on lines 117 to 118
self.cpp_info.names["cmake_find_package"] = "AzureStorage"
self.cpp_info.names["cmake_find_package_multi"] = "AzureStorage"
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does it come from? I don't see official Find/config files upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy-paste leftovers, will remove


self.cpp_info.libs = tools.collect_libs(self)
if self.settings.os == "Windows":
self.cpp_info.system_libs = ["Ws2_32", "rpcrt4", "xmllite", "bcrypt"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.cpp_info.system_libs = ["Ws2_32", "rpcrt4", "xmllite", "bcrypt"]
self.cpp_info.system_libs = ["ws2_32", "rpcrt4", "xmllite", "bcrypt"]

lowercase windows system libs for mingw@linux

Comment on lines 51 to 54
self.requires("cpprestsdk/2.10.18")
if self.settings.os != "Windows":
self.requires("libxml2/2.9.10")
self.requires("libuuid/1.0.3")
Copy link
Contributor

@SpaceIm SpaceIm Jun 24, 2021

Choose a reason for hiding this comment

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

Suggested change
self.requires("cpprestsdk/2.10.18")
if self.settings.os != "Windows":
self.requires("libxml2/2.9.10")
self.requires("libuuid/1.0.3")
self.requires("cpprestsdk/2.10.18")
if self.settings.os != "Windows":
self.requires("boost/1.76.0")
self.requires("libxml2/2.9.10")
self.requires("libuuid/1.0.3")

Boost is a direct dependency for non-Windows: https://github.com/Azure/azure-storage-cpp/blob/5b1c1597e6a637bc205d91342054814c8d901777/Microsoft.WindowsAzure.Storage/CMakeLists.txt#L21-L22

Don't rely on transitive dependencies coming from cpprestsdk.

@conan-center-bot
Copy link
Collaborator

All green in build 26 (c9eb4525929d754e3cac4bf4411676a932b20b1d):

  • azure-storage-cpp/7.5.0@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 31ad410 into conan-io:master Jun 26, 2021
@dvirtz dvirtz deleted the dvirtz/azure-storage-sdk branch July 20, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants