GH-20272: [C++] Bump version of bundled AWS SDK #33808

js8544 · 2023-01-20T15:57:02Z

Rationale for this change

Bump AWS SDK version to 1.10.55.

What changes are included in this PR?

Bump AWS SDK version to 1.10.55.

Closes: [C++] Bump version of bundled AWS SDK #20272

github-actions · 2023-01-20T15:57:28Z

Closes: [C++] Bump version of bundled AWS SDK #20272

kou · 2023-01-21T20:07:01Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

    # Workaround for https://github.com/aws/aws-sdk-cpp/issues/1750
    set(AWSSDK_PATCH_COMMAND "sed" "-i.bak" "-e" "s/\"-Werror\"//g"
-                             "<SOURCE_DIR>/cmake/compiler_settings.cmake")
+                             "<SOURCE_DIR>/cmake/compiler_settings.cmake" "&&")


Can we remove this patch?
It seems that 1.10.55 includes the fix for aws/aws-sdk-cpp#1750 .

cpp/cmake_modules/ThirdpartyToolchain.cmake

kou · 2023-01-21T20:15:17Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

@@ -4643,7 +4644,7 @@ macro(build_awssdk)
  set(AWSSDK_CMAKE_ARGS
      ${AWSSDK_COMMON_CMAKE_ARGS}
      -DOPENSSL_ROOT_DIR=${OPENSSL_ROOT_HINT}
-      -DBUILD_DEPS=OFF
+      -DBUILD_DEPS=ON # We need to build aws-crt-cpp from source


Can we build aws-crt-cpp by ourselves instead of using this and prefetch_crt_dependency.sh like other dependencies such as aws-c-common for portability?

I see. I'll try building it manually.

js8544 · 2023-01-22T13:54:41Z

I've changed to building aws-crt-cpp manually. It now works on my ubuntu and mac machine. Let's see if the CI checks pass.

kou · 2023-01-23T00:14:57Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

@@ -4612,6 +4699,7 @@ endif()
 macro(build_awssdk)
  message(STATUS "Building AWS C++ SDK from source")
  set(AWSSDK_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/awssdk_ep-install")
+  set(AWSSDK_SOURCE "${CMAKE_CURRENT_BINARY_DIR}/awssdk_ep-prefix/src/awssdk_ep")


Suggested change

set(AWSSDK_SOURCE "${CMAKE_CURRENT_BINARY_DIR}/awssdk_ep-prefix/src/awssdk_ep")

kou · 2023-01-23T00:17:48Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+if(DEFINED ENV{ARROW_S2N_URL})
+  set(S2N_SOURCE_URL "$ENV{ARROW_S2N_URL}")
+else()
+  set_urls(S2N_SOURCE_URL
+           "https://github.com/awslabs/s2n/archive/${ARROW_S2N_BUILD_VERSION}.tar.gz")
+endif()


s2n -> s2n-tls?
https://github.com/aws/s2n is redirected to https://github.com/aws/s2n-tls .

Interestingly the archive files are stored in s2n not s2n-tls. https://github.com/awslabs/s2n-tls/archive/v1.3.27.tar.gz is not found but https://github.com/awslabs/s2n/archive/v1.3.27.tar.gz is.

awslabs -> aws: https://github.com/aws/s2n-tls/archive/v1.3.27.tar.gz

Could you use S2N_TLS instead of S2N like ARROW_S2N_TLS_URL for all s2n-tls related variables?

BTW, the latest release is 1.3.35: https://github.com/aws/s2n-tls/releases
Can we use the latest version?

@js854 Could you also confirm the above comment?

@kou Oh sorry I missed it. I am looking at it now.

@kou Changed in abfa9774c

kou · 2023-01-24T05:39:21Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+      crypto
+      ssl


Are they artifacts by s2n-tls?
Then we should not create AWS::crypto and AWS::ssl. We should create AWS::s2n-tls and associate libcrypto.a and libssl.a with AWS::s2n-tls.

It turns out they are not needed. I've removed these two.

kou · 2023-01-24T05:41:59Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

                      BUILD_BYPRODUCTS ${AWS_CPP_SDK_COGNITO_IDENTITY_STATIC_LIBRARY}
                                       ${AWS_CPP_SDK_CORE_STATIC_LIBRARY}
                                       ${AWS_CPP_SDK_IDENTITY_MANAGEMENT_STATIC_LIBRARY}
                                       ${AWS_CPP_SDK_S3_STATIC_LIBRARY}
                                       ${AWS_CPP_SDK_STS_STATIC_LIBRARY}
-                      DEPENDS aws_c_event_stream_ep)
+                      DEPENDS aws_c_event_stream_ep aws_crt_cpp_ep)


Can we remove aws_c_event_stream_ep here because aws_crt_cpp_ep depends on aws_c_event_stream_ep?

Suggested change

DEPENDS aws_c_event_stream_ep aws_crt_cpp_ep)

DEPENDS aws_crt_cpp_ep)

kou · 2023-01-24T05:42:33Z

cpp/thirdparty/versions.txt

+  "ARROW_AWS_C_S3_URL aws-c-s3-${ARROW_AWS_C_S3_BUILD_VERSION}.tar.gz https://github.com/awslabs/aws-c-s3/archive/${ARROW_AWS_C_S3_BUILD_VERSION}.tar.gz"
+  "ARROW_AWS_C_SDKUTILS_URL aws-c-sdkutils-${ARROW_AWS_C_SDKUTILS_BUILD_VERSION}.tar.gz https://github.com/awslabs/aws-c-sdkutils/archive/${ARROW_AWS_C_SDKUTILS_BUILD_VERSION}.tar.gz"
+  "ARROW_AWS_LC_URL aws-lc-${ARROW_AWS_LC_BUILD_VERSION}.tar.gz https://github.com/awslabs/aws-lc/archive/${ARROW_AWS_LC_BUILD_VERSION}.tar.gz"
+  "ARROW_S2N_URL s2n-${ARROW_S2N_BUILD_VERSION}.tar.gz https://github.com/awslabs/s2n/archive/${ARROW_S2N_BUILD_VERSION}.tar.gz"


Could you keep alphabetical order?

js8544 · 2023-01-30T09:21:18Z

I am stuck on this problem for some time. Due to this, we need to initialize AWS SDK twice in arrow-s3fs-test because AWS SDK is statically linked. With the new 1.10 version, its initialization involves adding an OpenSSL engine here. The problem is, OpenSSL is dynamically linked, so the second initialization will fail due to a name conflict. The problem is the same as this issue.

In short, AWS SDK is statically linked but OpenSSL is dynamically linked, so we can't init AWS twice as we have been done in arrow-s3fs-test.

What is the best way to resolve this? Any help is appreciated. cc @kou @pitrou

pitrou · 2023-01-30T09:36:52Z

I think we should simply switch to dynamic linking for the AWS SDK. Static linking is fine for leaf dependencies (such as lz4), but it should be unsupported for complex dependencies such as AWS, gRPC, etc.

kou · 2023-01-31T09:07:52Z

I'll take a look at this in this week.

In general, we need additional work to use dynamic linking for bundled libraries. If we use dynamic linking for bundled libraries, we need to install them but it may overwrite existing libraries. So we need to install them to not $PREFIX/lib/ like $PREFIX/lib/arrow/. We may need to use rpath or something to load them installed in $PREFIX/lib/arrow/ at runtime.

pitrou · 2023-01-31T09:26:07Z

If we use dynamic linking for bundled libraries, we need to install them but it may overwrite existing libraries.

Well, yes, but so what? People who are concerned about that should use their distribution's packages.

js8544 · 2023-01-31T14:42:17Z

I found out that S2N provides an option S2N_INTERN_LIBCRYPTO to statically link against and "internalize" libcrypto. With this option on all tests are passing. We can stay with static linkage for now :). cc @kou @pitrou

kou · 2023-02-01T01:35:00Z

If we use dynamic linking for bundled libraries, we need to install them but it may overwrite existing libraries.

Well, yes, but so what? People who are concerned about that should use their distribution's packages.

I think that this is a bad manner. But I just realized that I have never discussed this before. My concern may be overthinking.

kou

Thanks!
I think that we can merge this only with some more small cleanups.

cpp/cmake_modules/ThirdpartyToolchain.cmake

kou · 2023-02-03T07:11:31Z

@github-actions crossbow submit -g cpp -g r

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

kou · 2023-02-08T03:26:19Z

@github-actions crossbow submit -g cpp -g r

kou · 2023-02-08T04:51:35Z

@github-actions crossbow submit -g cpp -g r

kou · 2023-02-08T08:08:13Z

@github-actions crossbow submit -g cpp -g r

github-actions · 2023-02-08T08:14:24Z

Revision: a9b2c3d

Submitted crossbow builds: ursacomputing/crossbow @ actions-31adcf1058

Task	Status
conda-linux-aarch64-cpu-r41
conda-linux-aarch64-cpu-r42
conda-linux-x64-cpu-r41
conda-linux-x64-cpu-r42
conda-osx-arm64-cpu-r41
conda-osx-arm64-cpu-r42
conda-osx-x64-cpu-r41
conda-osx-x64-cpu-r42
conda-win-x64-cpu-r41
homebrew-r-autobrew
homebrew-r-brew
r-binary-packages
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-library-r-base-latest
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-18.04-r-sanitizer
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-20
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp

kou

+1

I've disabled arrow-s3fs-test with bundled aws-sdk-cpp.

ursabot · 2023-02-09T09:45:38Z

Benchmark runs are scheduled for baseline = 6b65c84 and contender = e11ee40. e11ee40 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.37% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.38% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] e11ee40e ec2-t3-xlarge-us-east-2
[Failed] e11ee40e test-mac-arm
[Failed] e11ee40e ursa-i9-9960x
[Finished] e11ee40e ursa-thinkcentre-m75q
[Failed] 6b65c84a ec2-t3-xlarge-us-east-2
[Failed] 6b65c84a test-mac-arm
[Failed] 6b65c84a ursa-i9-9960x
[Finished] 6b65c84a ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

kou · 2023-02-09T21:36:29Z

@github-actions crossbow submit java-jars

github-actions · 2023-02-09T21:38:55Z

Revision: a9b2c3d

Submitted crossbow builds: ursacomputing/crossbow @ actions-d606dbb699

Task	Status
java-jars

### Rationale for this change Bump AWS SDK version to 1.10.55. ### What changes are included in this PR? Bump AWS SDK version to 1.10.55. * Closes: apache#20272 Lead-authored-by: Jin Shang <shangjin1997@gmail.com> Co-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

assignUser · 2023-02-10T19:46:23Z

@pitrou @kou When updating bundled dependencies please make sure to also update our fallback version in the artifactory to avoid something like #34120 (comment) this was missed in the recent round of version bumps (e.g. #13294).

kou · 2023-02-10T20:42:47Z

Oh, sorry.
We need a CI job for fallback cases. We can add an environment variable or a CMake variable to force using fallback cases for the job. Could you open an issue for this?

And we should have a script to mirror upstream archives to Artifactory. Could you open an issue for this too?

assignUser · 2023-02-10T22:17:43Z

I have updated the missing files for now and opened a few issues: #34131 #34132 #34129

…enSSL (#34159) Using OpenSSL causes various issues like #33808 (comment) and #34111. We should try to use aws-lc for libcrypto and libssl. We need to hide them inside s2n-tls to avoid name conflicts with OpenSSL used by other libraries. Some notes: 1. Only linux needs s2n-tls and aws-lc. 2. Because aws-c-http requires curl which requires libcrypto from OpenSSL, we need to hide Aws::libcrypto by S2N_INTERN_LIBCRYPTO=ON. 3. aws-c-cal also uses libcrypto, but it can't hide it. So we need to use OpenSSL instead of aws-lc for it. 4. Mac and windows don't need s2n-tls, but do need some security libs from system, like `-framework Security` and `ncrypt.lib`. 5. arrow-s3fs-test is re-enabled. 6. Force Java Gandiva to use static protobuf lib. * Closes: #34157 Lead-authored-by: Jin Shang <shangjin1997@gmail.com> Co-authored-by: jinshang <jinshang@tencent.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Bump AWS SDK version to 1.10.55. ### What changes are included in this PR? Bump AWS SDK version to 1.10.55. * Closes: apache#20272 Lead-authored-by: Jin Shang <shangjin1997@gmail.com> Co-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

… of OpenSSL (apache#34159) Using OpenSSL causes various issues like apache#33808 (comment) and apache#34111. We should try to use aws-lc for libcrypto and libssl. We need to hide them inside s2n-tls to avoid name conflicts with OpenSSL used by other libraries. Some notes: 1. Only linux needs s2n-tls and aws-lc. 2. Because aws-c-http requires curl which requires libcrypto from OpenSSL, we need to hide Aws::libcrypto by S2N_INTERN_LIBCRYPTO=ON. 3. aws-c-cal also uses libcrypto, but it can't hide it. So we need to use OpenSSL instead of aws-lc for it. 4. Mac and windows don't need s2n-tls, but do need some security libs from system, like `-framework Security` and `ncrypt.lib`. 5. arrow-s3fs-test is re-enabled. 6. Force Java Gandiva to use static protobuf lib. * Closes: apache#34157 Lead-authored-by: Jin Shang <shangjin1997@gmail.com> Co-authored-by: jinshang <jinshang@tencent.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Bump AWS SDK version to 1.10.55. ### What changes are included in this PR? Bump AWS SDK version to 1.10.55. * Closes: apache#20272 Lead-authored-by: Jin Shang <shangjin1997@gmail.com> Co-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

… of OpenSSL (apache#34159) Using OpenSSL causes various issues like apache#33808 (comment) and apache#34111. We should try to use aws-lc for libcrypto and libssl. We need to hide them inside s2n-tls to avoid name conflicts with OpenSSL used by other libraries. Some notes: 1. Only linux needs s2n-tls and aws-lc. 2. Because aws-c-http requires curl which requires libcrypto from OpenSSL, we need to hide Aws::libcrypto by S2N_INTERN_LIBCRYPTO=ON. 3. aws-c-cal also uses libcrypto, but it can't hide it. So we need to use OpenSSL instead of aws-lc for it. 4. Mac and windows don't need s2n-tls, but do need some security libs from system, like `-framework Security` and `ncrypt.lib`. 5. arrow-s3fs-test is re-enabled. 6. Force Java Gandiva to use static protobuf lib. * Closes: apache#34157 Lead-authored-by: Jin Shang <shangjin1997@gmail.com> Co-authored-by: jinshang <jinshang@tencent.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

…enSSL (#34159) Using OpenSSL causes various issues like apache/arrow#33808 (comment) and apache/arrow#34111. We should try to use aws-lc for libcrypto and libssl. We need to hide them inside s2n-tls to avoid name conflicts with OpenSSL used by other libraries. Some notes: 1. Only linux needs s2n-tls and aws-lc. 2. Because aws-c-http requires curl which requires libcrypto from OpenSSL, we need to hide Aws::libcrypto by S2N_INTERN_LIBCRYPTO=ON. 3. aws-c-cal also uses libcrypto, but it can't hide it. So we need to use OpenSSL instead of aws-lc for it. 4. Mac and windows don't need s2n-tls, but do need some security libs from system, like `-framework Security` and `ncrypt.lib`. 5. arrow-s3fs-test is re-enabled. 6. Force Java Gandiva to use static protobuf lib. * Closes: #34157 Lead-authored-by: Jin Shang <shangjin1997@gmail.com> Co-authored-by: jinshang <jinshang@tencent.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actions bot added the Component: C++ label Jan 20, 2023

kou reviewed Jan 21, 2023

View reviewed changes

js8544 force-pushed the jinshang/bump_aws branch from ba9dd47 to ff5f19f Compare January 23, 2023 17:25

kou reviewed Jan 24, 2023

View reviewed changes

h-vetinari mentioned this pull request Jan 27, 2023

[main] Rebuild for aws_sdk_cpp11057 conda-forge/arrow-cpp-feedstock#936

Merged

kou reviewed Feb 1, 2023

View reviewed changes

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

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

js8544 requested a review from kou February 1, 2023 09:34

js8544 force-pushed the jinshang/bump_aws branch from c554f03 to 27650c3 Compare February 3, 2023 06:35

This comment was marked as outdated.

Sign in to view

js8544 and others added 12 commits February 7, 2023 04:05

apacheGH-20272: [C++] Bump version of bundled AWS SDK

8d73fcc

compile each dependency manually

a1f0cf4

fix aws crypto & ssl by product

dacce23

update versions

2cb9e36

review feedback

1145c4c

fix link order

fe07499

clean dependency; lex order

66d7111

s2n internalize libcrypto

e1f32b0

lint cmake

4904021

fix indent

9869c7a

Update cpp/cmake_modules/ThirdpartyToolchain.cmake

5340bda

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Update cpp/cmake_modules/ThirdpartyToolchain.cmake

f3c930d

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Disable s3fs-test with bundled aws-sdk-cpp

8d2eebe

This comment was marked as outdated.

Sign in to view

Fix condition

376264b

This comment was marked as outdated.

Sign in to view

Detect Homebrew's OpenSSL automatically

a9b2c3d

kou mentioned this pull request Feb 9, 2023

[Python] Add support for the SSO credential provider #29912

Closed

kou approved these changes Feb 9, 2023

View reviewed changes

kou merged commit e11ee40 into apache:master Feb 9, 2023

kou mentioned this pull request Feb 10, 2023

[CI][Packaging][Java] java-jars was broken by updating bundled aws-sdk-cpp #34111

Closed

This was referenced Feb 13, 2023

[C++] Configure bundled AWS SDK to use aws-lc instead of OpenSSL #34157

Closed

GH-34157: [C++] Configure bundled AWS SDK to use aws-lc instead of OpenSSL #34158

Closed

GH-34157: [C++] Configure bundled AWS SDK to use aws-lc instead of OpenSSL #34159

Merged

kou mentioned this pull request Sep 15, 2023

GH-37721: [C++] Dependency: bump aws-sdk to 1.11.68 and avoid memory leak #37736

Closed

	DEPENDS aws_c_event_stream_ep aws_crt_cpp_ep)
	DEPENDS aws_crt_cpp_ep)

GH-20272: [C++] Bump version of bundled AWS SDK #33808

GH-20272: [C++] Bump version of bundled AWS SDK #33808

Conversation

js8544 commented Jan 20, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

github-actions bot commented Jan 20, 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

js8544 commented Jan 22, 2023 • edited Loading

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

js8544 Feb 3, 2023 • edited Loading

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

js8544 commented Jan 30, 2023

pitrou commented Jan 30, 2023

kou commented Jan 31, 2023

pitrou commented Jan 31, 2023

js8544 commented Jan 31, 2023

kou commented Feb 1, 2023

kou left a comment

Choose a reason for hiding this comment

kou commented Feb 3, 2023

This comment was marked as outdated.

kou commented Feb 8, 2023

This comment was marked as outdated.

kou commented Feb 8, 2023

This comment was marked as outdated.

kou commented Feb 8, 2023

github-actions bot commented Feb 8, 2023

kou left a comment

Choose a reason for hiding this comment

ursabot commented Feb 9, 2023

kou commented Feb 9, 2023

github-actions bot commented Feb 9, 2023

assignUser commented Feb 10, 2023

kou commented Feb 10, 2023

assignUser commented Feb 10, 2023

js8544 commented Jan 20, 2023 •

edited by github-actions bot

Loading

js8544 commented Jan 22, 2023 •

edited

Loading

js8544 Feb 3, 2023 •

edited

Loading