GH-29847: [C++] Build with Azure SDK for C++ by Tom-Newton · Pull Request #36835 · apache/arrow

Tom-Newton · 2023-07-24T07:24:15Z

Rationale for this change

We want to use the Azure SDK for C++ to read/write to Azure blob storage. Obviously this is pretty important for building an AzureFileSystem.

What changes are included in this PR?

Builds the the relevant parts of the azure SDK as a cmake external project. Adds a couple of simple tests that just assert that the Azure SDK is working and a couple of lines in AzureFileSystem to initialise the blob storage client to ensure the build is working correctly in all environments.

I started with the build setup from #12914 but I did make few changes.

Although its atypical for this project we chose to switch from cmake's ExternalProject to FetchContent. FetchContent is recomended by the Azure docs https://github.com/Azure/azure-sdk-for-cpp#cmake-project--fetch-content. It also solves a few problems including: automatically linking system curl and ssl instead of bootstrapping vcpkg and installing curl and ssl from there.
Only build one version of the Azure SDK for C++ because it contains all the components. Previously we were unnecessarily building 5 different versions of the whole thing on top of each other. This created race conditions for which version each component came from.
We are using azure-core_1.10.2 which is a very recent version. There are a couple of important reasons for this 1. an important managed identity fix, 2. fixed support for curl versions < 7.71.0.

There will be follow up PRs to enable Azure in the manylinux builds. We need to update vcpkg first so we can get a version of the Azure SDK which contains an important managed identity fix.

Are these changes tested?

Yes. There is a simple test that just runs the Azure client against azurite. Additionally just initialising the client in AzureFileSystem goes a long way towards ensuring the build is working.

Are there any user-facing changes?

No

Closes: [C++] Add azure-sdk-for-cpp to ThirdpartyToolchain #29847

github-actions · 2023-07-24T07:24:44Z

⚠️ GitHub issue #29847 has been automatically assigned in GitHub to PR creator.

…C++ filesystem (#36988) ### Rationale for this change We need to write tests for #18014. azurite is like a fake Azure blob storage so it can be used to write integration tests ### What changes are included in this PR? Extract the `azurite` related changes from #12914 to create a smaller PR that's easier to review. I have made very minimal changes compared to that PR. Currently `azurite` is configured for all the environments where `ARROW_AZURE` was enabled by #35701. I assume its deliberate that its not enabled yet for windows, alpine, conda, debian or fedora builds. ### Are these changes tested? Its tested by there aren't really any good tests in this PR. I used this `azurite` config in #36835 to make an integration test that uses the Azure C++ SDK. On its own we can't really write tests for this `azurite` setup PR. ### Are there any user-facing changes? No * Closes: #36886 Lead-authored-by: Thomas Newton <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

ci/docker/ubuntu-20.04-cpp.dockerfile

cpp/cmake_modules/ThirdpartyToolchain.cmake

cpp/thirdparty/versions.txt

ci/scripts/python_wheel_manylinux_build.sh

ci/vcpkg/vcpkg.json

ci/docker/ubuntu-20.04-cpp.dockerfile

cpp/cmake_modules/ThirdpartyToolchain.cmake

cpp/src/arrow/filesystem/azurefs_test.cc

cpp/thirdparty/versions.txt

cpp/vcpkg.json

Tom-Newton · 2023-08-25T09:15:38Z

I agree dependencies should not be build with -Werror etc. This is an issue with the use of the flag variables directly. As we can now use cmake 3.16 we should move to target based properties vs global flags but that is of course a major refactor...

So what do we think is the best option today? Is disabling -Werror at a global level an acceptable solution? Being able to disable -Werror would also be helpful for supporting Ubuntu 20 #36835 (comment)

kou · 2023-08-26T03:13:39Z

I'll provide a patch for -Werror later. Please wait for a few days...

kou · 2023-08-26T07:32:56Z

This is an ad-hoc patch but this will work. We need a real improvement later.

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 1dfaf71b4..9ff91f978 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -5082,6 +5082,13 @@ function(build_azure_sdk)
   set(CMAKE_EXPORT_NO_PACKAGE_REGISTRY TRUE)
   set(DISABLE_AZURE_CORE_OPENTELEMETRY TRUE)
   set(ENV{AZURE_SDK_DISABLE_AUTO_VCPKG} TRUE)
+  if(MSVC)
+    string(REPLACE "/WX" "" CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG}")
+    string(REPLACE "/WX" "" CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG}")
+  else()
+    string(REPLACE "-Werror" "" CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG}")
+    string(REPLACE "-Werror" "" CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG}")
+  endif()
   fetchcontent_makeavailable(azure_sdk)
   set(AZURE_SDK_VENDORED
       TRUE

cpp/cmake_modules/ThirdpartyToolchain.cmake

kou · 2023-08-28T00:46:51Z

@github-actions crossbow submit -g cpp

github-actions · 2023-08-28T00:49:53Z

Revision: e3fb84c

Submitted crossbow builds: ursacomputing/crossbow @ actions-ea66ccd89b

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

kou

+1

I'll merge this tomorrow if nobody objects it.

Tom-Newton · 2023-08-30T08:57:34Z

Thanks for your help on this @kou. I feel kind of bad about how much work I created for you during review, with my lack of C++ experience. Hopefully the native Azure support is worth it 🙂.

felipecrv · 2023-08-30T11:58:09Z

Thank you for this PR @Tom-Newton!

kou · 2023-08-31T01:15:47Z

Don't worry. :-)
I'm happy that we have more contributors like you!

conbench-apache-arrow · 2023-08-31T20:17:12Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 3cb4481.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…Azure C++ filesystem (apache#36988) ### Rationale for this change We need to write tests for apache#18014. azurite is like a fake Azure blob storage so it can be used to write integration tests ### What changes are included in this PR? Extract the `azurite` related changes from apache#12914 to create a smaller PR that's easier to review. I have made very minimal changes compared to that PR. Currently `azurite` is configured for all the environments where `ARROW_AZURE` was enabled by apache#35701. I assume its deliberate that its not enabled yet for windows, alpine, conda, debian or fedora builds. ### Are these changes tested? Its tested by there aren't really any good tests in this PR. I used this `azurite` config in apache#36835 to make an integration test that uses the Azure C++ SDK. On its own we can't really write tests for this `azurite` setup PR. ### Are there any user-facing changes? No * Closes: apache#36886 Lead-authored-by: Thomas Newton <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

### Rationale for this change We want to use the Azure SDK for C++ to read/write to Azure blob storage. Obviously this is pretty important for building an `AzureFileSystem`. ### What changes are included in this PR? Builds the the relevant parts of the azure SDK as a cmake external project. Adds a couple of simple tests that just assert that the Azure SDK is working and a couple of lines in `AzureFileSystem` to initialise the blob storage client to ensure the build is working correctly in all environments. I started with the build setup from apache#12914 but I did make few changes. 1. Although its atypical for this project we chose to switch from cmake's `ExternalProject` to `FetchContent`. `FetchContent` is recomended by the Azure docs https://github.com/Azure/azure-sdk-for-cpp#cmake-project--fetch-content. It also solves a few problems including: automatically linking system curl and ssl instead of bootstrapping vcpkg and installing curl and ssl from there. 2. Only build one version of the Azure SDK for C++ because it contains all the components. Previously we were unnecessarily building 5 different versions of the whole thing on top of each other. This created race conditions for which version each component came from. 3. We are using `azure-core_1.10.2` which is a very recent version. There are a couple of important reasons for this 1. [an important managed identity fix](Azure/azure-sdk-for-cpp#4723), 2. [fixed support for curl versions < 7.71.0](Azure/azure-sdk-for-cpp#4792). There will be follow up PRs to enable Azure in the manylinux builds. We need to update `vcpkg` first so we can get a version of the Azure SDK which contains [an important managed identity fix](Azure/azure-sdk-for-cpp#4723). ### Are these changes tested? Yes. There is a simple test that just runs the Azure client against azurite. Additionally just initialising the client in `AzureFileSystem` goes a long way towards ensuring the build is working. ### Are there any user-facing changes? No * Closes: apache#29847 Lead-authored-by: Thomas Newton <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: shefali singh <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

github-actions bot added Component: C++ awaiting review Awaiting review labels Jul 24, 2023

Tom-Newton mentioned this pull request Jul 24, 2023

[C++] Add azure-sdk-for-cpp to ThirdpartyToolchain #29847

Closed

Tom-Newton force-pushed the tomnewton/build_azure_sdk branch from 4e3e7a5 to d0f5b65 Compare July 24, 2023 07:43

Tom-Newton mentioned this pull request Aug 1, 2023

GH-36886: [C++] Configure azurite in preparation for testing Azure C++ filesystem Tom-Newton/arrow#3

Closed

Tom-Newton force-pushed the tomnewton/build_azure_sdk branch from d0f5b65 to 8915352 Compare August 1, 2023 22:36

Tom-Newton mentioned this pull request Aug 1, 2023

GH-36886: [C++] Configure azurite in preparation for testing Azure C++ filesystem #36988

Merged

Tom-Newton force-pushed the tomnewton/build_azure_sdk branch from 8915352 to 9d14edd Compare August 7, 2023 08:08

Tom-Newton force-pushed the tomnewton/build_azure_sdk branch from 9d14edd to 6ec7910 Compare August 8, 2023 07:39

Tom-Newton commented Aug 12, 2023

View reviewed changes

ci/docker/ubuntu-20.04-cpp.dockerfile Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 12, 2023

Tom-Newton commented Aug 12, 2023

View reviewed changes

cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated Show resolved Hide resolved

Tom-Newton commented Aug 12, 2023

View reviewed changes

cpp/thirdparty/versions.txt Outdated Show resolved Hide resolved

Tom-Newton changed the title ~~WIP GH-29847: [C++] Build with azure C++ sdk~~ GH-29847: [C++] Build with azure C++ sdk Aug 12, 2023

github-actions bot added the Component: Python label Aug 13, 2023

Tom-Newton force-pushed the tomnewton/build_azure_sdk branch from 8aa551a to 2560caf Compare August 13, 2023 12:06

Tom-Newton marked this pull request as ready for review August 13, 2023 12:07

Tom-Newton requested review from assignUser, kou and raulcd as code owners August 13, 2023 12:07

github-actions bot removed the Component: Python label Aug 13, 2023

kou changed the title ~~GH-29847: [C++] Build with azure C++ sdk~~ GH-29847: [C++] Build with Azure SDK for C++ Aug 14, 2023

kou reviewed Aug 14, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Aug 14, 2023

Tom-Newton added 5 commits August 26, 2023 13:31

Patch for disabling Werror in Azure build

a205b87

Add comment

19e6a56

Use newer Azure SDK with support for older curl

f5ebc08

Re-enable Azure build on ubuntu 20

1ef88c8

Require azure-core >= 1.10.2 in FindAzure.cmake

6398a65

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 26, 2023

kou reviewed Aug 26, 2023

View reviewed changes

cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated Show resolved Hide resolved

Fix a lint failure

e3fb84c

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 26, 2023

kou approved these changes Aug 29, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Aug 29, 2023

kou merged commit 3cb4481 into apache:main Aug 30, 2023

kou removed the awaiting merge Awaiting merge label Aug 30, 2023

Tom-Newton mentioned this pull request Oct 15, 2023

ARROW-2034: [C++] Filesystem implementation for Azure Blob Storage #12914

Closed

Tom-Newton mentioned this pull request Jan 15, 2024

[CI][Packaging] Upgrade vcpkg version used for release builds #39621

Closed

Conversation

Tom-Newton commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jul 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tom-Newton commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Aug 26, 2023

Uh oh!

kou commented Aug 26, 2023

Uh oh!

Uh oh!

kou commented Aug 28, 2023

Uh oh!

github-actions bot commented Aug 28, 2023

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Tom-Newton commented Aug 30, 2023

Uh oh!

felipecrv commented Aug 30, 2023

Uh oh!

kou commented Aug 31, 2023

Uh oh!

conbench-apache-arrow bot commented Aug 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Tom-Newton commented Jul 24, 2023 •

edited

Loading

Tom-Newton commented Aug 25, 2023 •

edited

Loading