azure-sdk-for-cpp: core, identity, storage #23297

jdblischak · 2023-07-07T16:54:54Z

Checklist

Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
License file is packaged (see here for an example).
Source is from official source.
Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
Build number is 0.
A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
When in trouble, please check our knowledge base documentation before pinging a team.

https://github.com/Azure/azure-sdk-for-cpp

This PR contains recipes for the core, identity, and the storage-* C++ libraries

https://github.com/Azure/azure-sdk-for-cpp#vcpkg
I've separated them into separate recipes (as opposed to a single recipe with multiple outputs) because even though it is a mono-repo, the release tags are per library

https://github.com/Azure/azure-sdk-for-cpp/releases
~~I haven't used run_exports because the dependencies in the various vcpkg.json files are specified with only a minimum version bound~~ UPDATE: the recipes now use run_exports to pin the libraries to their major version
I purposefully avoided the alternative core library with AMQP support (azure-core-amqp) because it depends on azure-uamqp-c, and we haven't yet been able to build it and its dependencies for all platforms (azure-uamqp-c -> azure-c-shared-utility -> azure-macro-utils-c & umock-c), eg Add linux osx azure-c-shared-utility-feedstock#4

https://github.com/conda-forge/azure-uamqp-c-feedstock

conda-forge-webservices · 2023-07-07T16:55:03Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/azure-core-cpp, recipes/azure-identity-cpp, recipes/azure-storage-blobs-cpp, recipes/azure-storage-common-cpp, recipes/azure-storage-files-datalake-cpp, recipes/azure-storage-files-shares-cpp, recipes/azure-storage-queues-cpp) and found it was in an excellent condition.

jdblischak · 2023-07-07T16:59:06Z

@xylar I'd appreciate a review of these Azure SDK C++ libraries. Also, please let me know if I can add you as a co-maintainer on these recipes

Shelnutt2 · 2023-07-07T17:07:32Z

I am willing to maintain these packages. Thanks @jdblischak !

xhochy

Comments apply to all recipes but I limited them to the first.

xhochy · 2023-07-09T05:17:46Z

recipes/azure-core-cpp/meta.yaml

+  run:
+    - libcurl
+    - openssl  # [not win]
+    - wil  # [win]


This whole section shouldn't be needed. The first two come via run_exports and the last one is a header only library.

According to conda/conda-build#3796 (comment) adding wil as a run dependency in azure-core-cpp is the best way to propagate the dependency in downstream packages. Currently every package using azure-core-cpp has to depend on wil itself.

@teo-tsirpanis thanks for sharing the discussion in that Issue. @xhochy do you have a strong preference for either option?

List the header only library wil as a runtime requirement for azure-core-cpp, so that wil will be automatically installed in the host environment (and thus be available at build time) for all its downstream dependencies

Include the header only library wil alongside azure-core-cpp in the host requirements of all the dependencies

I don't think 2 is an option because it would make azure-core-cpp unusable by itself.

After discussing with an upstream Azure SDK developer, we decided to apply a patch to remove the exported dependency on wil in the config file for azure-core-cpp

For more context, see Azure/azure-sdk-for-cpp#4784 and Azure/azure-sdk-for-cpp#4785

xhochy · 2023-07-09T05:18:44Z

recipes/azure-core-cpp/meta.yaml

@@ -0,0 +1,58 @@
+{% set name = "azure-core-cpp" %}


To be consistent with other packages, we should name this li azure-core

What are you suggesting the name should be exactly? We can't call it azure-core because that is used by the Python SDK: https://anaconda.org/conda-forge/azure-core

Similarly with the other SDK libraries: https://anaconda.org/conda-forge/azure-storage-common

To be consistent with other packages, we should name this li azure-core

@xhochy Is your suggestion to name it lib-azure-core? If yes, what other packages follow this convention? The AWS SDK for C++ is named aws-sdk-cpp

And in general, I think it would be potentially confusing if the conda-forge recipes all used different names than upstream

I believe there is a typo in xhochy's comment. It should read

we should name this libazure-core

If the package is just headers, config, and libraries (no CLI tools or other binaries), then it should be named libazure-core because the package contains libazure-core.so/dylib.

Examples:

libclang is libraries separate from clang the tool

libcurl is libraries separate from curl the tool

libopenvino just libs from the openvino project

libmad is libs from the mad project

I'm open to adding the prefix lib. My main concerns are consistency and transparency. I think there is an advantage to having the conda recipe names exactly match the upstream names

libclang is libraries separate from clang the tool

But for this example, the upstream maintainers have also named it libclang, right? Same with libcurl. This seems like a slightly different situation than what we are discussing here, where the lib prefix name would only exist in conda-forge

My understanding is that the prefixes -cpp (and -c) communicate that the software contains no CLI tools or other binaries. Looking across the conda-forge ecosystem, the many various AWS libraries either end in -cpp or start with aws-c- (including the very similar aws-sdk-cpp). There are also Azure C libraries that either start with azure-c or end in -c (see the recipe for azure-iot-sdk-c for various iterations on the naming scheme). I see that the lib prefix was added to libgoogle-cloud (conda-forge/google-cloud-cpp-feedstock#83), but the -cpp was also dropped.

Given the existence of the recipes for aws-c-*, aws-sdk-cpp, etc. already in conda-forge, I'd argue that a name like azure-core-cpp already communicates that it is only a C++ library, with the added benefit that it matches the official upstream name.

Is anyone somewhat convinced by my argument above? I don't feel that strongly about it, but I wanted to at least present the counter-argument before we deviate the name from upstream. And if we do decide to add the prefix lib, should we drop the suffix -cpp?

I agree that the lib prefix is excessive. Names like azure-core-cpp unquestionably refer to C++ libraries. Moreover this is the official name of the libraries, and we use the official names in the other cloud SDKs.

It's not immediately apparent that the official name of the component is 'azure-core-cpp' when the components are bundled together under a project called 'azure-sdk-for-cpp'.

The upstream project's docs indicate that both the CMake package and vcpkg names are azure-core-cpp, so let's go with that because it is explicit evidence in the upstream projects docs/source that the upstream maintainers have named these packages in that way. In the future, let's lead with links instead of researching whether or not it's conda-forge convention to name C/CXX libraries with a lib- prefix.

recipes/azure-core-cpp/meta.yaml

xhochy · 2023-07-09T05:21:03Z

@h-vetinari please also have a look as this is third big cloud vendor C++ SDK

h-vetinari

Thanks for the PR!

Aside from some general clean-up requests, I'd expect that we need some sort of azure-sdk metapackage that pulls everything together?

It would also be good to know what kind of ABI-stability (if any) these packages have. Migrating for every version is quite painful in the aws stack, for example.

To summarize the clean-ups:

add run-exports
don't pin (or lower-bound) packages where we will need to rebuild for new versions
remove run-requirements where we can rely on run-exports
don't special-case -DCMAKE_BUILD_TYPE=Release in build scripts
better document & structure the test sections
don't use {{ name }}
[preferable] put CMake metadata in $PREFIX/lib/cmake/<project> (resp. %LIBRARY_LIB%\cmake\<project>)
[optional] use ninja universally

recipes/azure-identity-cpp/build.sh

recipes/azure-identity-cpp/meta.yaml

recipes/azure-storage-blobs-cpp/meta.yaml

recipes/azure-storage-common-cpp/build.sh

recipes/azure-storage-common-cpp/meta.yaml

xylar · 2023-07-10T12:32:41Z

@xylar I'd appreciate a review of these Azure SDK C++ libraries. Also, please let me know if I can add you as a co-maintainer on these recipes

@jdblischak, I'm sorry, I really don't feel qualified to help maintain these. I also have my hands full.

Co-authored-by: h-vetinari <[email protected]>

jdblischak · 2023-07-10T13:42:34Z

@xhochy and @h-vetinari Thanks for the thorough review! I've applied your suggestions to the azure-core-cpp recipe. Once that is in good shape, I'll apply the changes to the remaining recipes. My biggest uncertainty is around what to set for the min_pin and max_pin for the run_exports.

h-vetinari · 2023-07-10T14:05:15Z

My biggest uncertainty is around what to set for the min_pin and max_pin for the run_exports.

Leave out min_pin and set max_pin to 'x.x.x', unless you know that the ABI will not change between 1.2.x and 1.2.y.

teo-tsirpanis · 2023-07-10T14:34:16Z

unless you know that the ABI will not change between 1.2.x and 1.2.y.

They declare compatibility within the same major version. (https://github.com/Azure/azure-sdk-for-cpp/blob/2f3e5de931c3f1c19551f6f58418c85aa410745b/cmake-modules/AzureVcpkg.cmake#L152-L156)

… build scripts

…xports

jdblischak · 2023-07-11T15:30:50Z

@xochy @h-vetinari I removed the runtime requirements, but now I am getting the following errors. This was my initial motivation for adding them to the run requirements:

   INFO (azure-storage-blobs-cpp,lib/libazure-storage-blobs.so): Needed DSO lib/libazure-storage-common.so found in home/conda/staged-recipes/build_artifacts::azure-storage-common-cpp-12.3.2-heddee37_0
  ERROR (azure-storage-blobs-cpp,lib/libazure-storage-blobs.so): Needed DSO lib/libazure-core.so found in ['azure-core-cpp']
  ERROR (azure-storage-blobs-cpp,lib/libazure-storage-blobs.so): .. but ['azure-core-cpp'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)

h-vetinari · 2023-07-11T15:42:00Z

The warning sounds pretty clear to me. azure-storage-blobs-cpp needs to host-depend on azure-core-cpp, so it will inherit the correct run-export.

…need to rebuild for new versions

carterbox · 2023-07-31T23:33:56Z

recipes/azure-core-cpp/meta.yaml

+  run_exports:
+    - {{ pin_subpackage("azure-core-cpp", min_pin="x", max_pin="x") }}


Suggested change

run_exports:

- {{ pin_subpackage("azure-core-cpp", min_pin="x", max_pin="x") }}

run_exports:

- {{ pin_subpackage("azure-core-cpp", max_pin="x") }}

Please update all your exports to unspecify the min_pin. SameMajorVersion means backwards compatability, not forward and backward compatability.

Also, shouldn't we pin the minor version instead?

Suggested change

run_exports:

- {{ pin_subpackage("azure-core-cpp", min_pin="x", max_pin="x") }}

run_exports:

- {{ pin_subpackage("azure-core-cpp", min_pin="x", max_pin="x.x") }}

This suggestion also applies to the other libraries' recipe.

We currently have it set to pin to only the major version because the upstream CMake file declares SameMajorVersion compatibility. See also the previous comments #23297 (comment) and #23297 (comment). @jjerphan what do you think of this choice? Pinning to the minor version would be the more conservative route, but it would also require more maintenance when migrating the pins

I see. I think we better wait for an actual administrator of conda-forge. If you and another maintainer agreed on pinning on the major version already, just do ignore my comment.

carterbox · 2023-07-31T23:37:59Z

recipes/azure-core-cpp/meta.yaml

@@ -0,0 +1,58 @@
+{% set name = "azure-core-cpp" %}


I believe there is a typo in xhochy's comment. It should read

we should name this libazure-core

If the package is just headers, config, and libraries (no CLI tools or other binaries), then it should be named libazure-core because the package contains libazure-core.so/dylib.

Examples:

libclang is libraries separate from clang the tool

libcurl is libraries separate from curl the tool

libopenvino just libs from the openvino project

libmad is libs from the mad project

jjerphan

Thank you for bringing Azure SDK for C++ on conda-forge, @jdblischak!

Here are a few comments, the main important one being about pinning on minor versions rather than on major ones.

I also think we must prefix the names of those libraries with lib.

I volunteer to maintain feedstocks.

recipes/azure-core-cpp/meta.yaml

recipes/azure-identity-cpp/meta.yaml

recipes/azure-core-cpp/meta.yaml

recipes/azure-storage-blobs-cpp/meta.yaml

recipes/azure-storage-common-cpp/meta.yaml

recipes/azure-storage-files-datalake-cpp/meta.yaml

recipes/azure-storage-files-shares-cpp/meta.yaml

recipes/azure-storage-queues-cpp/meta.yaml

recipes/azure-core-cpp/meta.yaml

jjerphan · 2023-08-07T07:37:54Z

recipes/azure-core-cpp/meta.yaml

+  run_exports:
+    - {{ pin_subpackage("azure-core-cpp", min_pin="x", max_pin="x") }}


Also, shouldn't we pin the minor version instead?

Suggested change

run_exports:

- {{ pin_subpackage("azure-core-cpp", min_pin="x", max_pin="x") }}

run_exports:

- {{ pin_subpackage("azure-core-cpp", min_pin="x", max_pin="x.x") }}

This suggestion also applies to the other libraries' recipe.

https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html

jdblischak · 2023-08-07T17:24:54Z

@carterbox Thanks for the review!

Only if you think people would actually want to install all of the components in one go.

I like that advice. I don't need a single meta-package myself. If someone else needs it, they can request it or create it themselves

I prefer that related recipes are clumped together because it's easier to review them for consistency and interdependency, but 10 recipes in a PR is alot. Make a future PR to add more packages in this ecosystem.

Happy to comply with this. Updating 7 recipes at a time is cumbersome enough

As long as you (as the maintainer) trust the #23297 (comment). You're the one who primarily need to clean up the mess if there are ABI breaks.

Ok, let's see if all the maintainers agree. I know that the pinning migrations for the similar aws-* packages are non-trivial, so I'd prefer to minimize this as much as possible

Yes. Ensure that there is a global pinning added before you update to your next version of these packages. If there is only one version available, then the pinning isn't needed. Once there are multiple versions, that's when pinnings become relevant.

Wonderful. Thanks for the concise explanation. I will need to add the pins after the initial feedstock creation but prior to the first updates

jjerphan

Thank you for packaging those libraries from the Azure SDK for C++, @jdblischak! I believe this is useful for a lot of projects.

Regarding what's left on the todo list (#23297 (comment)): I think we can figure moving the CMake metadata to the canonical path in the feedstocks since the tests using CMake projects work.

Regarding packages' names: prefixing C++ and C library with lib is conventional for conda-forge. Yet, the developers of the Azure SDK for C++ chose to suffix them with -c or -cpp (which is also explicit). I am OK with using names that are suffixed with -c and cpp, especially if this prevent discrepancies between packages' names and identifiers to use for CMake.

I think we should also be careful about some packages being renamed. For instance azure-macro-utils-c was renamed macro-utils-c upstream but is published on conda-forge as azure-macro-utils-c. Republishing is an option I think, but better ones might exist for this situation.

jdblischak · 2023-08-09T13:53:42Z

@conda-forge/help-c-cpp, ready for review!

jdblischak · 2023-08-09T13:59:49Z

Higher-level questions that are still being actively discussed:

Do we maintain the official upstream library names, eg azure-core-cpp, or rename them in conda-forge, eg libazure-core-cpp or libazure-core? I am currently leaning towards keeping the official names. However, I'm willing to be convinced if provided examples that this sort of renaming is common in conda-forge. Given that the existing recipes for AWS and Azure C/C++ libraries have not been renamed, I think it would make sense to continue this tradition
Do we pin the run_exports to the major or minor pin? I'm currently leaning towards the major pin since 1) this is what the upstream maintainers say they will follow, and 2) this will reduce the maintenance burden from conda-forge-pinning migrations

Lastly, if anyone else would like to be added to the list of maintainers, please let me know

jdblischak · 2023-08-14T18:26:51Z

Thanks all for your reviews! The recipes are much improved thanks to all of your feedback

teo-tsirpanis · 2023-08-15T12:09:38Z

Great work! Small question: I see the packages are built only for x64 architectures. Is there a way to add support for linux-aarch64, macos-arm64 and linux-ppc64le?

jjerphan · 2023-08-15T12:33:26Z

We need to assess whether the libraries have been designed for the support of those architectures. We can try their support via individual PRs.

azure-sdk-for-cpp: core, identity, storage

03642c5

Manually set CMAKE_BUILD_TYPE=Release for linux builds

a33c5f5

jdblischak mentioned this pull request Jul 7, 2023

Why isn't CMAKE_BUILD_TYPE=Release included in CMAKE_ARGS? conda-forge/ctng-compiler-activation-feedstock#94

Closed

xhochy requested changes Jul 9, 2023

View reviewed changes

h-vetinari reviewed Jul 10, 2023

View reviewed changes

jdblischak and others added 2 commits July 10, 2023 09:32

Apply suggestions from code review

194b6f5

Co-authored-by: h-vetinari <[email protected]>

Apply review feedback to azure-core-cpp

ab839f1

jdblischak added 11 commits July 10, 2023 11:16

Fix package name to fix dependency order for builds

7bc1e2e

Fix path for test on Windows

61eee6d

Fix more file paths for tests

231ada6

azure-identity-cpp also requires wil

b880a54

azure-storage-common-cpp also requires wil

1353503

Add wil as dependency for all SDK libraries

6540d80

azure-sdk-for-cpp: use ninja universally

56181b5

azure-sdk-for-cpp: don't special-case -DCMAKE_BUILD_TYPE=Release in…

1572a20

… build scripts

azure-sdk-for-cpp: add run-exports

d6651d3

azure-sdk-for-cpp: don't use {{ name }}

d0dc6d7

azure-sdk-for-cpp: remove run-requirements where we can rely on run-e…

4538b91

…xports

jdblischak added 2 commits July 11, 2023 11:53

azure-sdk-for-cpp: list recursive dependencies in host reqs

f8cb240

azure-sdk-for-cpp: don't pin (or lower-bound) packages where we will …

c32b753

…need to rebuild for new versions

carterbox requested changes Aug 1, 2023

View reviewed changes

carterbox removed the review-requested label Aug 1, 2023

github-actions bot added the Awaiting author contribution label Aug 1, 2023

jjerphan reviewed Aug 7, 2023

View reviewed changes

jdblischak added 3 commits August 7, 2023 13:00

Remove min_pin for SameMajorVersion compatibility

33ced17

https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html

Add jjerphan as recipe maintainer

7468c05

Use ${SHLIB_EXT}

f9bc538

jdblischak added 4 commits August 7, 2023 16:56

Add example CMake project to test linking

a5b3a22

Fix env vars for test on Windows

1705a18

Test that find_package() can find installed Azure C++ library

6effd14

Add find_package() test for all libraries

97eca8c

jjerphan approved these changes Aug 9, 2023

View reviewed changes

github-actions bot added review-requested and removed Awaiting author contribution labels Aug 9, 2023

jdblischak requested a review from carterbox August 10, 2023 16:59

carterbox merged commit 3251439 into conda-forge:main Aug 13, 2023

jdblischak mentioned this pull request Aug 14, 2023

azure-storage-common-cpp v12.3.3 conda-forge/azure-storage-common-cpp-feedstock#1

Closed

3 tasks

This was referenced Aug 16, 2023

Nightly libtiledb builds have been failing for over a month TileDB-Inc/conda-forge-nightly-controller#7

Closed

Install Azure SDK C++ libraries from conda conda-forge/tiledb-feedstock#205

Merged

jjerphan mentioned this pull request Sep 14, 2023

azure-storage-blobs-cpp v12.9.0 conda-forge/azure-storage-blobs-cpp-feedstock#4

Merged

3 tasks

jdblischak mentioned this pull request Oct 27, 2023

Remove min_pin to return to default run_exports of x.x.x.x conda-forge/tiledb-feedstock#215

Merged

4 tasks

jdblischak mentioned this pull request Jan 15, 2024

Set run_exports max_pin to x.x conda-forge/azure-core-cpp-feedstock#11

Merged

6 tasks

jdblischak mentioned this pull request Feb 27, 2024

Tighten depends max_pin="x.x.x" for all azure-sdk-for-cpp packages conda-forge/conda-forge-repodata-patches-feedstock#674

Merged

5 tasks

jdblischak mentioned this pull request Jun 7, 2024

Add azure-* CPP packages to global pinning conda-forge/conda-forge-pinning-feedstock#6003

Closed

		run_exports:
		- {{ pin_subpackage("azure-core-cpp", min_pin="x", max_pin="x") }}

azure-sdk-for-cpp: core, identity, storage #23297

azure-sdk-for-cpp: core, identity, storage #23297

Conversation

jdblischak commented Jul 7, 2023 • edited Loading

conda-forge-webservices bot commented Jul 7, 2023

jdblischak commented Jul 7, 2023

Shelnutt2 commented Jul 7, 2023

xhochy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xhochy commented Jul 9, 2023

h-vetinari left a comment • edited Loading

Choose a reason for hiding this comment

xylar commented Jul 10, 2023

jdblischak commented Jul 10, 2023

h-vetinari commented Jul 10, 2023

teo-tsirpanis commented Jul 10, 2023

jdblischak commented Jul 11, 2023

h-vetinari commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjerphan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdblischak commented Aug 7, 2023

jjerphan left a comment • edited Loading

Choose a reason for hiding this comment

jdblischak commented Aug 9, 2023

jdblischak commented Aug 9, 2023

jdblischak commented Aug 14, 2023

teo-tsirpanis commented Aug 15, 2023

jjerphan commented Aug 15, 2023

jdblischak commented Jul 7, 2023 •

edited

Loading

h-vetinari left a comment •

edited

Loading

jjerphan left a comment •

edited

Loading

jjerphan left a comment •

edited

Loading