wgtmac commented on code in PR #616:
URL: https://github.com/apache/iceberg-cpp/pull/616#discussion_r3279239593
##########
ci/scripts/build_iceberg.sh:
##########
@@ -48,11 +49,20 @@ else
CMAKE_ARGS+=("-DICEBERG_S3=OFF")
fi
+if [[ "${build_enable_sigv4}" == "ON" ]]; then
+ CMAKE_ARGS+=("-DICEBERG_SIGV4=ON")
+else
+ CMAKE_ARGS+=("-DICEBERG_SIGV4=OFF")
+fi
+
if is_windows; then
CMAKE_ARGS+=("-DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake")
CMAKE_ARGS+=("-DCMAKE_BUILD_TYPE=Release")
else
CMAKE_ARGS+=("-DCMAKE_BUILD_TYPE=Debug")
+ if [[ -n "${CMAKE_TOOLCHAIN_FILE:-}" ]]; then
+ CMAKE_ARGS+=("-DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE}")
+ fi
Review Comment:
```suggestion
```
They are not neccessary.
##########
.github/workflows/sigv4_test.yml:
##########
@@ -0,0 +1,71 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# SigV4 build + unit tests (Linux only; aws-cpp-sdk-core via vcpkg).
+name: SigV4 Tests
Review Comment:
It seems `sigv4_test.yml` overlaps with `s3_test.yml`. Could we merge them
into a dedicated `aws_test.yml` workflow?
Currently, `ICEBERG_S3` pulls the AWS SDK via Arrow, while `ICEBERG_SIGV4`
pulls it via `vcpkg`. Testing them separately hides a major risk: if a user
enables both, mismatched AWS SDK versions could cause severe symbol conflicts
or linking errors. By merging them, we can:
1. Enable both `-DICEBERG_S3=ON` and `-DICEBERG_SIGV4=ON` in the same build
to ensure they can safely co-exist.
2. Save CI time by only building and running the related test targets (e.g.,
cmake --build . --target s3_test sigv4_auth_test followed by `ctest -R
"s3|sigv4"`).
##########
src/iceberg/iceberg-config.cmake.in:
##########
@@ -71,7 +71,13 @@ macro(iceberg_find_components components)
endforeach()
endmacro()
-# Find system dependencies
+# AWSSDK's CMake config dispatches sub-package finds via
AWSSDK_FIND_COMPONENTS,
Review Comment:
Can we keep the original `# Find system dependencies` comment?
##########
src/iceberg/iceberg-config.cmake.in:
##########
@@ -71,7 +71,13 @@ macro(iceberg_find_components components)
endforeach()
endmacro()
-# Find system dependencies
+# AWSSDK's CMake config dispatches sub-package finds via
AWSSDK_FIND_COMPONENTS,
+# so a plain find_dependency would not bring in aws-cpp-sdk-core.
+if("AWSSDK" IN_LIST ICEBERG_SYSTEM_DEPENDENCIES)
+ list(REMOVE_ITEM ICEBERG_SYSTEM_DEPENDENCIES AWSSDK)
+ include(CMakeFindDependencyMacro)
Review Comment:
```suggestion
```
This duplicates line 39 above.
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -62,11 +62,13 @@ std::string InferAuthType(
}
AuthManagerRegistry CreateDefaultRegistry() {
- return {
+ AuthManagerRegistry registry = {
Review Comment:
Why adding this indirection?
##########
src/iceberg/iceberg-config.cmake.in:
##########
@@ -71,7 +71,13 @@ macro(iceberg_find_components components)
endforeach()
endmacro()
-# Find system dependencies
+# AWSSDK's CMake config dispatches sub-package finds via
AWSSDK_FIND_COMPONENTS,
+# so a plain find_dependency would not bring in aws-cpp-sdk-core.
+if("AWSSDK" IN_LIST ICEBERG_SYSTEM_DEPENDENCIES)
Review Comment:
This special case does not look that elegant. Perhaps we can do the
following instead:
In this file src/iceberg/iceberg-config.cmake.in:
```
Remove lines 74-80 (the AWSSDK special case and redundant include):
-# AWSSDK's CMake config dispatches sub-package finds via
AWSSDK_FIND_COMPONENTS,
-# so a plain find_dependency would not bring in aws-cpp-sdk-core.
-if("AWSSDK" IN_LIST ICEBERG_SYSTEM_DEPENDENCIES)
- list(REMOVE_ITEM ICEBERG_SYSTEM_DEPENDENCIES AWSSDK)
- include(CMakeFindDependencyMacro)
- find_dependency(AWSSDK COMPONENTS core)
-endif()
iceberg_find_dependencies("${ICEBERG_SYSTEM_DEPENDENCIES}")
Modify the foreach inside iceberg_find_dependencies (line 50):
foreach(dependency ${dependencies})
- find_dependency(${dependency})
+ find_dependency(${dependency}
${ICEBERG_FIND_EXTRA_ARGS_${dependency}})
endforeach()
```
In the cmake_modules/IcebergThirdpartyToolchain.cmake
```
function(resolve_aws_sdk_dependency)
find_package(AWSSDK REQUIRED COMPONENTS core)
list(APPEND ICEBERG_SYSTEM_DEPENDENCIES AWSSDK)
set(ICEBERG_SYSTEM_DEPENDENCIES
${ICEBERG_SYSTEM_DEPENDENCIES}
PARENT_SCOPE)
+ set(ICEBERG_FIND_EXTRA_ARGS_AWSSDK "COMPONENTS;core" PARENT_SCOPE)
endfunction()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
