kou commented on code in PR #44989:
URL: https://github.com/apache/arrow/pull/44989#discussion_r1906037806
##########
.github/workflows/r.yml:
##########
@@ -282,6 +282,12 @@ jobs:
restore-keys: |
r-${{ matrix.config.rtools }}-ccache-mingw-${{ matrix.config.arch
}}-${{ hashFiles('cpp/src/**/*.cc','cpp/src/**/*.h)') }}-
r-${{ matrix.config.rtools }}-ccache-mingw-${{ matrix.config.arch
}}-
+ - name: Install CMake via MSYS2
+ uses: msys2/setup-msys2@v2
+ with:
+ msystem: UCRT64
+ update: true
+ install: mingw-w64-ucrt-x86_64-cmake
Review Comment:
Hmm. We can't use built-in CMake in `windows-2019`?
##########
.github/workflows/cpp.yml:
##########
@@ -352,6 +357,11 @@ jobs:
with:
fetch-depth: 0
submodules: recursive
+ - name: Install CMake 3.25.0
+ shell: bash
+ run: |
+ ci/scripts/install_cmake.sh 3.25.0 /c/cmake
+ echo "c:\\cmake\\bin" >> $GITHUB_PATH
Review Comment:
Do we need this?
It seems that the built-in CMake is enough new:
https://github.com/actions/runner-images/blob/main/images/windows/Windows2019-Readme.md
> CMake 3.31.2
##########
ci/scripts/install_cmake.sh:
##########
@@ -17,29 +17,49 @@
# specific language governing permissions and limitations
# under the License.
-set -e
+set -ex
+
+if [ "$#" -ne 2 ]; then
+ echo "Usage: $0 <version> <prefix>"
+ exit 1
+fi
declare -A archs
-archs=([amd64]=x86_64
- [arch64]=aarch64
+archs=([x86_64]=x86_64
[arm64]=aarch64
- [arm64v8]=aarch64
- [x86_64]=x86_64)
+ [aarch64]=aarch64)
+
+arch=$(uname -m)
+if [ -z ${archs[$arch]} ]; then
+ echo "Unsupported architecture: ${arch}"
+ exit 0
+fi
+arch=${archs[$arch]}
declare -A platforms
platforms=([linux]=linux
[macos]=macos
[windows]=windows)
Review Comment:
Can we remove this?
##########
.env:
##########
@@ -54,6 +54,7 @@ UBUNTU=22.04
# Default versions for various dependencies
CLANG_TOOLS=14
+CMAKE_VERSION=3.25.0
Review Comment:
Could you remove `_VERSION` suffix like other variables?
```suggestion
CMAKE=3.25.0
```
##########
ci/rtools/aws_c_io_ep.patch:
##########
@@ -0,0 +1,88 @@
+# 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.
+
+diff --git a/source/windows/host_resolver.c b/source/windows/host_resolver.c
+index 59fbb85..ad4a99e 100644
+--- a/source/windows/host_resolver.c
++++ b/source/windows/host_resolver.c
+@@ -4,6 +4,7 @@
+ */
+
+ /* don't move this below the Windows.h include!!!!*/
++#define _WIN32_WINNT 0x0601
+ #include <winsock2.h>
+ #include <ws2tcpip.h>
+
+diff --git a/source/windows/iocp/iocp_event_loop.c
b/source/windows/iocp/iocp_event_loop.c
+index 9ccce30..5cbbef7 100644
+--- a/source/windows/iocp/iocp_event_loop.c
++++ b/source/windows/iocp/iocp_event_loop.c
+@@ -12,6 +12,7 @@
+
+ #include <aws/io/logging.h>
+
++#define _WIN32_WINNT 0x0601
+ #include <Windows.h>
+
+ /* The next set of struct definitions are taken directly from the
+diff --git a/source/windows/secure_channel_tls_handler.c
b/source/windows/secure_channel_tls_handler.c
+index 50caf02..29fe850 100644
+--- a/source/windows/secure_channel_tls_handler.c
++++ b/source/windows/secure_channel_tls_handler.c
+@@ -2,6 +2,7 @@
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0.
+ */
++#define _WIN32_WINNT 0x0601
+ #define SECURITY_WIN32
+
+ #include <aws/io/tls_channel_handler.h>
+@@ -35,6 +36,25 @@
+ # pragma warning(disable : 4306) /* Identifier is type cast to a larger
pointer. */
+ #endif
+
++#ifndef SP_PROT_TLS1_0_SERVER
++#define SP_PROT_TLS1_0_SERVER SP_PROT_TLS1_SERVER
++#endif
++#ifndef SP_PROT_TLS1_0_CLIENT
++#define SP_PROT_TLS1_0_CLIENT SP_PROT_TLS1_CLIENT
++#endif
++#ifndef SP_PROT_TLS1_1_SERVER
++#define SP_PROT_TLS1_1_SERVER 0x00000100
++#endif
++#ifndef SP_PROT_TLS1_1_CLIENT
++#define SP_PROT_TLS1_1_CLIENT 0x00000200
++#endif
++#ifndef SCH_USE_STRONG_CRYPTO
++#define SCH_USE_STRONG_CRYPTO 0x00400000
++#endif
++#ifndef SECBUFFER_ALERT
++#define SECBUFFER_ALERT 0x11
++#endif
++
+ #define KB_1 1024
+ #define READ_OUT_SIZE (16 * KB_1)
+ #define READ_IN_SIZE READ_OUT_SIZE
+@@ -456,7 +476,7 @@ static int s_fillin_alpn_data(
+
+ *extension_length += sizeof(uint32_t) + sizeof(uint16_t);
+
+- *extension_name = SecApplicationProtocolNegotiationExt_ALPN;
++ *extension_name = 2;
Review Comment:
Can we include `sspi.h` explicitly for it?
It seems that it's defined in `sspi.h`:
https://learn.microsoft.com/en-us/windows/win32/api/sspi/ne-sspi-sec_application_protocol_negotiation_ext
##########
.github/workflows/cpp.yml:
##########
@@ -193,6 +193,11 @@ jobs:
with:
fetch-depth: 0
submodules: recursive
+ - name: Install CMake 3.25.0
+ shell: bash
+ run: |
+ ci/scripts/install_cmake.sh 3.25.0 /tmp/local/
+ echo "/tmp/local/bin" >> $GITHUB_PATH
Review Comment:
We can avoid this by using `ubuntu-24.04`. (FYI: `ubuntu-latest` will use
`ubuntu-24.04` soon.)
##########
ci/rtools/BuildAwsCCommon.patch:
##########
Review Comment:
Can we upstream this and other patches?
How about renaming this to `awc_c_common_ep.path` like others?
##########
ci/rtools/BuildAwsCCommon.patch:
##########
@@ -0,0 +1,66 @@
+# 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.
+
+diff --git a/cmake/AwsCFlags.cmake b/cmake/AwsCFlags.cmake
+index b717bca..5aa8ac9 100644
+--- a/cmake/AwsCFlags.cmake
++++ b/cmake/AwsCFlags.cmake
+@@ -120,6 +120,10 @@ function(aws_set_common_properties target)
+ list(APPEND AWS_C_FLAGS -Wno-strict-aliasing)
+ endif()
+
++ if(CMAKE_C_IMPLICIT_LINK_LIBRARIES MATCHES "mingw32")
Review Comment:
```suggestion
+ if (CMAKE_C_IMPLICIT_LINK_LIBRARIES MATCHES "mingw32")
```
##########
.github/workflows/dev.yml:
##########
@@ -53,6 +53,11 @@ jobs:
uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b #
v5.3.0
with:
python-version: 3.12
+ - name: Install CMake 3.25.0
+ shell: bash
+ run: |
+ ci/scripts/install_cmake.sh 3.25.0 /tmp/local/
+ echo "/tmp/local/bin" >> $GITHUB_PATH
Review Comment:
`ubuntu-24.04` will help us.
##########
ci/rtools/BuildAwsCCommon.patch:
##########
@@ -0,0 +1,66 @@
+# 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.
+
+diff --git a/cmake/AwsCFlags.cmake b/cmake/AwsCFlags.cmake
+index b717bca..5aa8ac9 100644
+--- a/cmake/AwsCFlags.cmake
++++ b/cmake/AwsCFlags.cmake
+@@ -120,6 +120,10 @@ function(aws_set_common_properties target)
+ list(APPEND AWS_C_FLAGS -Wno-strict-aliasing)
+ endif()
+
++ if(CMAKE_C_IMPLICIT_LINK_LIBRARIES MATCHES "mingw32")
++ list(APPEND AWS_C_FLAGS -D__USE_MINGW_ANSI_STDIO=1
-Wno-unused-local-typedefs)
++ endif()
++
+ # -moutline-atomics generates code for both older load/store
exclusive atomics and also
+ # Arm's Large System Extensions (LSE) which scale substantially
better on large core count systems.
+ #
+diff --git a/include/aws/common/byte_order.inl
b/include/aws/common/byte_order.inl
+index 1204be0..0abd9cb 100644
+--- a/include/aws/common/byte_order.inl
++++ b/include/aws/common/byte_order.inl
+@@ -13,7 +13,7 @@
+ # include <stdlib.h>
+ #else
+ # include <netinet/in.h>
+-#endif /* _MSC_VER */
++#endif /* _WIN32 */
+
+ AWS_EXTERN_C_BEGIN
+
+@@ -39,7 +39,7 @@ AWS_STATIC_IMPL uint64_t aws_hton64(uint64_t x) {
+ uint64_t v;
+ __asm__("bswap %q0" : "=r"(v) : "0"(x));
+ return v;
+-#elif defined(_MSC_VER)
++#elif defined(_WIN32)
Review Comment:
FYI: MSYS2 also has the same patch:
https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-aws-c-common/001-fix-build-on-mingw-aarch64.patch
If we upstream this, MSYS2 will be happy too.
##########
ci/rtools/awssdk_ep.patch:
##########
@@ -0,0 +1,234 @@
+# 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.
+
+diff --git a/aws-cpp-sdk-core/include/aws/core/utils/Array.h
b/aws-cpp-sdk-core/include/aws/core/utils/Array.h
+index 2b5bbc566..7cb93bdf0 100644
+--- a/aws-cpp-sdk-core/include/aws/core/utils/Array.h
++++ b/aws-cpp-sdk-core/include/aws/core/utils/Array.h
+@@ -54,7 +54,7 @@ namespace Aws
+ {
+ m_data.reset(Aws::NewArray<T>(m_size,
ARRAY_ALLOCATION_TAG));
+
+-#ifdef _WIN32
++#ifdef _MSC_VER
+ std::copy(arrayToCopy, arrayToCopy + arraySize,
stdext::checked_array_iterator< T * >(m_data.get(), m_size));
+ #else
+ std::copy(arrayToCopy, arrayToCopy + arraySize,
m_data.get());
+@@ -82,7 +82,7 @@ namespace Aws
+ if(arr->m_size > 0 && arr->m_data)
+ {
+ size_t arraySize = arr->m_size;
+-#ifdef _WIN32
++#ifdef _MSC_VER
+ std::copy(arr->m_data.get(), arr->m_data.get() +
arraySize, stdext::checked_array_iterator< T * >(m_data.get() + location,
m_size));
+ #else
+ std::copy(arr->m_data.get(), arr->m_data.get() +
arraySize, m_data.get() + location);
+@@ -101,7 +101,7 @@ namespace Aws
+ {
+ m_data.reset(Aws::NewArray<T>(m_size,
ARRAY_ALLOCATION_TAG));
+
+-#ifdef _WIN32
++#ifdef _MSC_VER
+ std::copy(other.m_data.get(), other.m_data.get() +
other.m_size, stdext::checked_array_iterator< T * >(m_data.get(), m_size));
+ #else
+ std::copy(other.m_data.get(), other.m_data.get() +
other.m_size, m_data.get());
+@@ -134,7 +134,7 @@ namespace Aws
+ {
+ m_data.reset(Aws::NewArray<T>(m_size,
ARRAY_ALLOCATION_TAG));
+
+-#ifdef _WIN32
++#ifdef _MSC_VER
+ std::copy(other.m_data.get(), other.m_data.get() +
other.m_size, stdext::checked_array_iterator< T * >(m_data.get(), m_size));
+ #else
+ std::copy(other.m_data.get(), other.m_data.get() +
other.m_size, m_data.get());
+diff --git
a/aws-cpp-sdk-core/include/aws/core/utils/crypto/bcrypt/CryptoImpl.h
b/aws-cpp-sdk-core/include/aws/core/utils/crypto/bcrypt/CryptoImpl.h
+index e26e36b60..3e7189b70 100644
+--- a/aws-cpp-sdk-core/include/aws/core/utils/crypto/bcrypt/CryptoImpl.h
++++ b/aws-cpp-sdk-core/include/aws/core/utils/crypto/bcrypt/CryptoImpl.h
+@@ -29,7 +29,14 @@ namespace Aws
+ {
+ namespace Crypto
+ {
+- static const char* SecureRandom_BCrypt_Tag =
"SecureRandom_BCrypt";
++#ifdef __MINGW32__
++#pragma GCC diagnostic push
++#pragma GCC diagnostic ignored "-Wunused-variable"
++#endif
++ static const char* SecureRandom_BCrypt_Tag = "SecureRandom_BCrypt";
++#ifdef __MINGW32__
++#pragma GCC diagnostic pop
++#endif
+
+ class SecureRandomBytes_BCrypt : public SecureRandomBytes
+ {
+diff --git a/aws-cpp-sdk-core/source/external/cjson/cJSON.cpp
b/aws-cpp-sdk-core/source/external/cjson/cJSON.cpp
+index cdcbf103e..03bcf0258 100644
+--- a/aws-cpp-sdk-core/source/external/cjson/cJSON.cpp
++++ b/aws-cpp-sdk-core/source/external/cjson/cJSON.cpp
+@@ -2483,7 +2483,15 @@ CJSON_AS4CPP_PUBLIC(cJSON *)
cJSON_AS4CPP_CreateInt64(long long num)
+ if (num > INT_MAX || num < INT_MIN)
+ {
+ char buf[21];
+- snprintf(buf, sizeof(buf), "%lld", num);
++#ifdef __MINGW32__
++#pragma GCC diagnostic push
++#pragma GCC diagnostic ignored "-Wformat"
++#pragma GCC diagnostic ignored "-Wformat-extra-args"
++#endif
++ snprintf(buf, sizeof(buf), "%lld", num);
++#ifdef __MINGW32__
++#pragma GCC diagnostic pop
++#endif
Review Comment:
Why do we need this? If it's caused by `-Werror`, let's disable `-Werror`
instead.
##########
ci/rtools/awssdk_ep.patch:
##########
@@ -0,0 +1,234 @@
+# 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.
+
+diff --git a/aws-cpp-sdk-core/include/aws/core/utils/Array.h
b/aws-cpp-sdk-core/include/aws/core/utils/Array.h
+index 2b5bbc566..7cb93bdf0 100644
+--- a/aws-cpp-sdk-core/include/aws/core/utils/Array.h
++++ b/aws-cpp-sdk-core/include/aws/core/utils/Array.h
+@@ -54,7 +54,7 @@ namespace Aws
+ {
+ m_data.reset(Aws::NewArray<T>(m_size,
ARRAY_ALLOCATION_TAG));
+
+-#ifdef _WIN32
++#ifdef _MSC_VER
Review Comment:
It seems that this `#ifdef` doesn't exist in the latest aws-cpp-sdk-core:
https://github.com/aws/aws-sdk-cpp/blob/main/src/aws-cpp-sdk-core/include/aws/core/utils/Array.h#L60
Let's update the bundled aws-cpp-sdk-core instead.
##########
ci/rtools/BuildAwsCCommon.patch:
##########
@@ -0,0 +1,66 @@
+# 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.
+
+diff --git a/cmake/AwsCFlags.cmake b/cmake/AwsCFlags.cmake
+index b717bca..5aa8ac9 100644
+--- a/cmake/AwsCFlags.cmake
++++ b/cmake/AwsCFlags.cmake
+@@ -120,6 +120,10 @@ function(aws_set_common_properties target)
+ list(APPEND AWS_C_FLAGS -Wno-strict-aliasing)
+ endif()
+
++ if(CMAKE_C_IMPLICIT_LINK_LIBRARIES MATCHES "mingw32")
++ list(APPEND AWS_C_FLAGS -D__USE_MINGW_ANSI_STDIO=1
-Wno-unused-local-typedefs)
++ endif()
++
+ # -moutline-atomics generates code for both older load/store
exclusive atomics and also
+ # Arm's Large System Extensions (LSE) which scale substantially
better on large core count systems.
+ #
+diff --git a/include/aws/common/byte_order.inl
b/include/aws/common/byte_order.inl
+index 1204be0..0abd9cb 100644
+--- a/include/aws/common/byte_order.inl
++++ b/include/aws/common/byte_order.inl
+@@ -13,7 +13,7 @@
+ # include <stdlib.h>
+ #else
+ # include <netinet/in.h>
+-#endif /* _MSC_VER */
++#endif /* _WIN32 */
+
+ AWS_EXTERN_C_BEGIN
+
+@@ -39,7 +39,7 @@ AWS_STATIC_IMPL uint64_t aws_hton64(uint64_t x) {
+ uint64_t v;
+ __asm__("bswap %q0" : "=r"(v) : "0"(x));
+ return v;
+-#elif defined(_MSC_VER)
++#elif defined(_WIN32)
+ return _byteswap_uint64(x);
+ #else
+ uint32_t low = x & UINT32_MAX;
+diff --git a/source/windows/thread.c b/source/windows/thread.c
+index 447fcd2..01e643f 100644
+--- a/source/windows/thread.c
++++ b/source/windows/thread.c
+@@ -10,6 +10,7 @@
+ #include <aws/common/private/thread_shared.h>
+ #include <aws/common/string.h>
+
++#define _WIN32_WINNT 0x0601
Review Comment:
We may be able to pass this definition via `ExternalProject_Add()` (or
`FetchContent`) by `CMAKE_C{,XX}_FLAGS` or something.
--
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]
