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.

Sign up for GitHub

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

Upgrading to use aws sdk 1.9.291 #3353

Closed
wants to merge 9 commits into from
Closed

Upgrading to use aws sdk 1.9.291 #3353

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
31c17bc
update to aws sdk v1.9.291, patch one out one sdk item observed causi…
dhoke4tdb Jul 15, 2022
d29ef23
explore whether BUILD_DEPS=ON actually needed
dhoke4tdb Jul 18, 2022
76afa25
remove unnecessary BUILD_DEPS=ON
dhoke4tdb Jul 18, 2022
2f8d0a8
reinstate awssdk retrieval of needed libs and augment with a couple t…
dhoke4tdb Jul 27, 2022
4ea5e3a
reinstate working approach, but with additional kruft very crudely sh…
dhoke4tdb Jul 28, 2022
a434181
experimentally identify awssdk libraries that are apparently not curr…
dhoke4tdb Jul 30, 2022
ffdc10f
disable some duplicate items
dhoke4tdb Aug 2, 2022
c42effb
remove kruft
dhoke4tdb Aug 2, 2022
71d5683
remove one unneeded and one dup item
dhoke4tdb Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 60 additions & 16 deletions cmake/Modules/FindAWSSDK_EP.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ endif()

if (NOT AWSSDK_FOUND)
if (TILEDB_SUPERBUILD)
message(STATUS "Could NOT find AWSSDK")
message(STATUS "Adding AWSSDK as an external project")

set(DEPENDS)
Expand Down Expand Up @@ -96,19 +95,18 @@ if (NOT AWSSDK_FOUND)

if (WIN32)
find_package(Git REQUIRED)
set(CONDITIONAL_PATCH cd ${CMAKE_SOURCE_DIR} && ${GIT_EXECUTABLE} apply --ignore-whitespace -p1 --unsafe-paths --verbose --directory=${TILEDB_EP_SOURCE_DIR}/ep_awssdk < ${TILEDB_CMAKE_INPUTS_DIR}/patches/ep_awssdk/awsccommon.patch &&
${GIT_EXECUTABLE} apply --ignore-whitespace -p1 --unsafe-paths --verbose --directory=${TILEDB_EP_SOURCE_DIR}/ep_awssdk < ${TILEDB_CMAKE_INPUTS_DIR}/patches/ep_awssdk/awsconfig_cmake_3.22.patch)
set(CONDITIONAL_PATCH cd ${CMAKE_SOURCE_DIR} && ${GIT_EXECUTABLE} apply --ignore-whitespace -p1 --unsafe-paths --verbose --directory=${TILEDB_EP_SOURCE_DIR}/ep_awssdk < ${TILEDB_CMAKE_INPUTS_DIR}/patches/ep_awssdk/aws_event_loop_windows.v1.9.291.patch)
else()
set(CONDITIONAL_PATCH patch -N -p1 < ${TILEDB_CMAKE_INPUTS_DIR}/patches/ep_awssdk/awsccommon.patch &&
patch -N -p1 < ${TILEDB_CMAKE_INPUTS_DIR}/patches/ep_awssdk/awsconfig_cmake_3.22.patch)
set(CONDITIONAL_PATCH "")
endif()

ExternalProject_Add(ep_awssdk
PREFIX "externals"
# Set download name to avoid collisions with only the version number in the filename
DOWNLOAD_NAME ep_awssdk.zip
URL "https://github.com/aws/aws-sdk-cpp/archive/1.8.84.zip"
URL_HASH SHA1=e32a53a01c75ca7fdfe9feed9c5bbcedd98708e3
GIT_REPOSITORY "https://github.com/aws/aws-sdk-cpp"
GIT_TAG 1.9.291
GIT_SUBMODULES_RECURSE ON #TBD: Seems 'ON' may be default, is this needed? Is this correct?
Copy link
Member

Choose a reason for hiding this comment

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

👍 Yes, needed - this does git clone --recurse-submodules ... which they specify as required in the docs, b/c some of the dependencies are now submodules and won't be populated w/out this option.

PATCH_COMMAND
${CONDITIONAL_PATCH}
CMAKE_ARGS
Expand Down Expand Up @@ -151,15 +149,54 @@ endif ()

if (AWSSDK_FOUND)
set(AWS_SERVICES s3)
AWSSDK_DETERMINE_LIBS_TO_LINK(AWS_SERVICES AWS_LINKED_LIBS)
list(APPEND AWS_LINKED_LIBS aws-c-common
aws-c-event-stream
aws-checksums
aws-cpp-sdk-sts
aws-cpp-sdk-identity-management)
# append these libs in an order manually, by trial and error, determined to support successful
# linking of dependencies on *nix. (windows doesn't seem to care or 'just lucked out'.)
list(APPEND AWS_LINKED_LIBS #aws-c-common
aws-cpp-sdk-s3
aws-cpp-sdk-core
aws-crt-cpp
#AWSSDK::aws-c-auth
Copy link
Member

Choose a reason for hiding this comment

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

Remove cruft, if not necessary?

#AWSSDK::aws-c-cal
#AWSSDK::aws-c-common
#AWSSDK::aws-c-compression
aws-c-event-stream
#xAWSSDK::aws-c-io
aws-c-mqtt
aws-c-s3
aws-c-auth
aws-c-http
aws-c-compression
aws-checksums
aws-c-sdkutils
aws-crt-cpp
#xAWSSDK::aws-c-s3
Copy link
Member

Choose a reason for hiding this comment

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

Remove cruft?

aws-cpp-sdk-s3
aws-c-event-stream
aws-c-mqtt
aws-checksums
aws-c-auth
#AWSSDK::aws-c-http
aws-c-sdkutils
aws-c-io
aws-c-compression
aws-c-cal
aws-cpp-sdk-cognito-identity
aws-cpp-sdk-identity-management
aws-cpp-sdk-sts
aws-c-common
#s2n # There is a libs2n.a generated and in install/lib path... but not an INTERFACE, hmm...
)
if (NOT WIN32)
list(APPEND AWS_LINKED_LIBS #aws-c-common
s2n # There is a libs2n.a generated and in install/lib path... but not an INTERFACE, hmm...
)
endif()

set(TILEDB_AWS_LINK_TARGETS "")
foreach (LIB ${AWS_LINKED_LIBS})
if (NOT ${LIB} MATCHES "aws-*")
if ((NOT ${LIB} MATCHES "aws-*" )
AND (NOT LIB STREQUAL "s2n") )
message(STATUS "AWS_LINKED_LIBS, skipping ${LIB}")
continue()
endif()

Expand All @@ -168,7 +205,12 @@ if (AWSSDK_FOUND)
PATHS ${AWSSDK_LIB_DIR}
${TILEDB_DEPS_NO_DEFAULT_PATH}
)
message(STATUS "Found AWS lib: ${LIB} (${AWS_FOUND_${LIB}})")
if ("${AWS_FOUND_${LIB}}" MATCHES ".*-NOTFOUND")
message(STATUS "not found, ${LIB}")
continue()
else()
message(STATUS "for ${LIB} adding link to ${AWS_FOUND_${LIB}}")
endif()
if (NOT TARGET AWSSDK::${LIB})
add_library(AWSSDK::${LIB} UNKNOWN IMPORTED)
set_target_properties(AWSSDK::${LIB} PROPERTIES
Expand All @@ -177,6 +219,8 @@ if (AWSSDK_FOUND)
)
endif()

list(APPEND TILEDB_AWS_LINK_TARGETS "AWSSDK::${LIB}")

# If we built a static EP, install it if required.
if (TILEDB_AWSSDK_EP_BUILT AND TILEDB_INSTALL_STATIC_DEPS)
install_target_libs(AWSSDK::${LIB})
Expand All @@ -187,6 +231,6 @@ if (AWSSDK_FOUND)
# the AWSSDK does not include links to some transitive dependencies
# ref: github<dot>com/aws<slash>aws-sdk-cpp/issues/1074#issuecomment-466252911
if (WIN32)
list(APPEND AWS_EXTRA_LIBS userenv ws2_32 wininet winhttp bcrypt version)
list(APPEND AWS_EXTRA_LIBS userenv ws2_32 wininet winhttp bcrypt version crypt32 secur32 Ncrypt)
endif()
endif ()
21 changes: 21 additions & 0 deletions cmake/inputs/patches/ep_awssdk/aws_event_loop_windows.v1.9.291.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
diff --git "a/crt/aws-crt-cpp/crt/aws-c-io/source/event_loop.c" "b/crt/aws-crt-cpp/crt/aws-c-io/source/event_loop.c"
index 5eb8a084ac..e46cec1dfd 100644
--- "a/crt/aws-crt-cpp/crt/aws-c-io/source/event_loop.c"
+++ "b/crt/aws-crt-cpp/crt/aws-c-io/source/event_loop.c"
@@ -70,9 +70,16 @@ static void s_aws_event_loop_group_shutdown_async(struct aws_event_loop_group *e
thread_options.cpu_id = -1;
thread_options.join_strategy = AWS_TJS_MANAGED;

+ /*
+ * tiledb encountered issue similar to https://github.com/huggingface/datasets/issues/3310#issuecomment-1155285967.
Copy link
Member

Choose a reason for hiding this comment

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

The underlying issue is here: https://github.com/aws/aws-sdk-cpp/issues/1809
Please link that too, and add a short summary so that we have some context to start from even if the linked comment disappears.

+ */
+#if 1
+ aws_thread_launch(&cleanup_thread, s_event_loop_destroy_async_thread_fn, el_group, &thread_options);
Copy link
Member

Choose a reason for hiding this comment

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

If you are able to easily reproduce the issue mentioned above, could you please check GetLastError here and dump it out. That might be useful to pushing AWS on the issue...

Copy link
Member

Choose a reason for hiding this comment

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

Also a stack trace - I believe the problem is that they are trying to launch a thread after process exit has started, which fails (even on POSIX). See

+#else
AWS_FATAL_ASSERT(
aws_thread_launch(&cleanup_thread, s_event_loop_destroy_async_thread_fn, el_group, &thread_options) ==
AWS_OP_SUCCESS);
+#endif
}

static struct aws_event_loop_group *s_event_loop_group_new(
21 changes: 6 additions & 15 deletions tiledb/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,14 +466,8 @@ if (TILEDB_S3)
message(STATUS "The TileDB library is compiled with S3 support.")
find_package(AWSSDK_EP REQUIRED COMPONENTS s3)
target_link_libraries(TILEDB_CORE_OBJECTS_ILIB
INTERFACE
AWSSDK::aws-cpp-sdk-s3
AWSSDK::aws-cpp-sdk-core
AWSSDK::aws-c-event-stream
AWSSDK::aws-checksums
AWSSDK::aws-c-common
AWSSDK::aws-cpp-sdk-identity-management
AWSSDK::aws-cpp-sdk-sts)
INTERFACE ${TILEDB_AWS_LINK_TARGETS}
)
add_definitions(-DHAVE_S3)
endif()

Expand Down Expand Up @@ -625,6 +619,7 @@ if(APPLE)
if(TILEDB_S3)
# this is a transitive dependency for statically linking S3 SDK
target_link_libraries(TILEDB_CORE_OBJECTS_ILIB INTERFACE "-framework CoreFoundation")
target_link_libraries(TILEDB_CORE_OBJECTS_ILIB INTERFACE "-framework Security")
endif()
endif()

Expand Down Expand Up @@ -846,20 +841,16 @@ if (TILEDB_STATIC)
append_dep_lib(OpenSSL::SSL)
append_dep_lib(OpenSSL::Crypto)
append_dep_lib(CURL::libcurl)
append_dep_lib(AWSSDK::aws-cpp-sdk-s3)
append_dep_lib(AWSSDK::aws-cpp-sdk-core)
append_dep_lib(AWSSDK::aws-c-event-stream)
append_dep_lib(AWSSDK::aws-checksums)
append_dep_lib(AWSSDK::aws-c-common)
append_dep_lib(AWSSDK::aws-cpp-sdk-sts)
append_dep_lib(AWSSDK::aws-cpp-sdk-identity-management)
append_dep_lib(AzureSDK::AzureSDK)
append_dep_lib(GCSSDK::storage_client)
append_dep_lib(GCSSDK::google_cloud_cpp_common)
append_dep_lib(GCSSDK::crc32c)
append_dep_lib(spdlog::spdlog)
append_dep_lib(libmagic)
append_dep_lib(WebP::webp)
foreach(AWSTARG ${TILEDB_AWS_LINK_TARGETS})
append_dep_lib(${AWSTARG})
endforeach()
endif()

############################################################
Expand Down