kou commented on code in PR #37821:
URL: https://github.com/apache/arrow/pull/37821#discussion_r1396853470
##########
cpp/cmake_modules/BuildUtils.cmake:
##########
@@ -742,6 +742,14 @@ function(ADD_TEST_CASE REL_TEST_NAME)
--error-exitcode=1 ${TEST_PATH} ${ARG_TEST_ARGUMENTS}")
elseif(WIN32)
add_test(${TEST_NAME} ${TEST_PATH} ${ARG_TEST_ARGUMENTS})
+ elseif(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
+ add_test(${TEST_NAME}
+ ${BUILD_SUPPORT_DIR}/run-test.sh
+ ${CMAKE_BINARY_DIR}
+ test
+ ${CMAKE_CROSSCOMPILING_EMULATOR}
+ ${TEST_PATH}
+ ${ARG_TEST_ARGUMENTS})
Review Comment:
I think that the following is better:
```diff
diff --git a/cpp/cmake_modules/BuildUtils.cmake
b/cpp/cmake_modules/BuildUtils.cmake
index 3930faa81..f8249456e 100644
--- a/cpp/cmake_modules/BuildUtils.cmake
+++ b/cpp/cmake_modules/BuildUtils.cmake
@@ -740,16 +740,8 @@ function(ADD_TEST_CASE REL_TEST_NAME)
valgrind --suppressions=valgrind.supp --tool=memcheck
--gen-suppressions=all \
--num-callers=500 --leak-check=full
--leak-check-heuristics=stdstring \
--error-exitcode=1 ${TEST_PATH} ${ARG_TEST_ARGUMENTS}")
- elseif(WIN32)
- add_test(${TEST_NAME} ${TEST_PATH} ${ARG_TEST_ARGUMENTS})
- elseif(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
- add_test(${TEST_NAME}
- ${BUILD_SUPPORT_DIR}/run-test.sh
- ${CMAKE_BINARY_DIR}
- test
- ${CMAKE_CROSSCOMPILING_EMULATOR}
- ${TEST_PATH}
- ${ARG_TEST_ARGUMENTS})
+ elseif(WIN32 OR CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
+ add_test(NAME ${TEST_NAME} COMMAND ${TEST_NAME} ${ARG_TEST_ARGUMENTS}
else()
add_test(${TEST_NAME}
${BUILD_SUPPORT_DIR}/run-test.sh
```
Because `add_test()` prepends `CMAKE_CROSSCOMPILING_ENUMERATOR`
automatically based on the document:
https://cmake.org/cmake/help/latest/prop_tgt/CROSSCOMPILING_EMULATOR.html
> This command will be added as a prefix to
[add_test()](https://cmake.org/cmake/help/latest/command/add_test.html#command:add_test),
[add_custom_command()](https://cmake.org/cmake/help/latest/command/add_custom_command.html#command:add_custom_command),
and
[add_custom_target()](https://cmake.org/cmake/help/latest/command/add_custom_target.html#command:add_custom_target)
commands for built target system executables.
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -1889,6 +1896,11 @@ TYPED_TEST(TestStringKernels, Strptime) {
}
TYPED_TEST(TestStringKernels, StrptimeZoneOffset) {
+#ifdef __EMSCRIPTEN__
+ GTEST_SKIP()
+ << "Emscripten bug
https://github.com/emscripten-core/emscripten/issues/20467 ";
Review Comment:
```suggestion
<< "Emscripten bug
https://github.com/emscripten-core/emscripten/issues/20467";;
```
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -702,3 +730,38 @@ if(MSVC)
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}
${MSVC_LINKER_FLAGS}")
endif()
endif()
+
+if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
+ # flags are:
+ # 1) We are building library code
+ # 2) We force *everything* to build as position independent
+ # 3) And with support for C++ exceptions
Review Comment:
```suggestion
# 1) We force *everything* to build as position independent
# 2) And with support for C++ exceptions
```
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -702,3 +730,38 @@ if(MSVC)
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}
${MSVC_LINKER_FLAGS}")
endif()
endif()
+
+if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
+ # flags are:
+ # 1) We are building library code
+ # 2) We force *everything* to build as position independent
+ # 3) And with support for C++ exceptions
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fPIC -fexceptions")
+ # size_t is 32 bit in emscripten wasm32 - ignore conversion errors
+ # deprecated-literal-operator error is thrown in datetime (vendored lib in
arrow)
+ set(CMAKE_CXX_FLAGS
+ "${CMAKE_CXX_FLAGS} -fPIC -fexceptions -Wno-error=shorten-64-to-32
-Wno-error=deprecated-literal-operator"
+ )
+
+ # flags for creating shared libraries (only used in pyarrow, because
+ # Emscripten builds libarrow as static)
+ # flags are:
+ # 1) Tell it to use javascript / webassembly 64 bit number support.
Review Comment:
```suggestion
# 1) Tell it to use JavaScript / WebAssembly 64 bit number support.
```
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1406,6 +1431,27 @@ macro(build_brotli)
)
set(BROTLI_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS}
"-DCMAKE_INSTALL_PREFIX=${BROTLI_PREFIX}")
+ set(BROTLI_EP_OPTIONS)
+ if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
+ # "cmake install" is disabled for Brotli on Emscripten, so the
+ # default INSTALL_COMMAND fails. We need to disable the default
+ # INSTALL_COMMAND.
+ list(APPEND
+ BROTLI_EP_OPTIONS
+ INSTALL_COMMAND
+ ${CMAKE_COMMAND}
+ -E
+ true)
+
+ set(BROTLI_BUILD_DIR
${CMAKE_CURRENT_BINARY_DIR}/brotli_ep-prefix/src/brotli_ep-build)
+ set(BROTLI_BUILD_LIBS
+
"${BROTLI_BUILD_DIR}/${CMAKE_STATIC_LIBRARY_PREFIX}brotlienc-static${CMAKE_STATIC_LIBRARY_SUFFIX}"
+
"${BROTLI_BUILD_DIR}/${CMAKE_STATIC_LIBRARY_PREFIX}brotlidec-static${CMAKE_STATIC_LIBRARY_SUFFIX}"
+
"${BROTLI_BUILD_DIR}/${CMAKE_STATIC_LIBRARY_PREFIX}brotlicommon-static${CMAKE_STATIC_LIBRARY_SUFFIX}"
+ )
+
Review Comment:
```suggestion
```
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2405,38 +2510,59 @@ endif()
macro(build_zlib)
message(STATUS "Building ZLIB from source")
- set(ZLIB_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/zlib_ep/src/zlib_ep-install")
- if(MSVC)
- if(${UPPERCASE_BUILD_TYPE} STREQUAL "DEBUG")
- set(ZLIB_STATIC_LIB_NAME zlibstaticd.lib)
- else()
- set(ZLIB_STATIC_LIB_NAME zlibstatic.lib)
+
+ # ensure zlib is built with -fpic
+ # and make sure that the build finds the version in Emscripten ports
+ # - n.b. the actual linking happens because -sUSE_ZLIB=1 is
+ # set in the compiler variables, but cmake expects
+ # it to exist at configuration time if we aren't building it as
+ # bundled. We need to do this for all packages
+ # not just zlib as some depend on zlib, but we don't rebuild
+ # if it exists already
+ if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
+ # build zlib using Emscripten ports
+ if(NOT EXISTS ${EMSCRIPTEN_SYSROOT}/lib/wasm32-emscripten/pic/libz.a)
+ execute_process(COMMAND embuilder --pic --force build zlib)
endif()
+ add_library(ZLIB::ZLIB INTERFACE IMPORTED)
+ # We need -fPIC in target_compile_options() too
+ # to stop it using non-PIC libz.a in linking.
+ target_compile_options(ZLIB::ZLIB INTERFACE -sUSE_ZLIB=1 -fPIC)
+ target_link_options(ZLIB::ZLIB INTERFACE -sUSE_ZLIB=1 -fPIC)
Review Comment:
Could you use `-sRELOCATABLE=1` instead of `-fPIC` if you don't want to use
`-sSIDE_MODULE=1` nor `-sMAIN_MODULE=1`? If we use `-fPIC`, `libz.so` is
searched with Emscripten package on Debian GNU/Linux sid.
```suggestion
# We need -sRELOCATABLE=1 in target_compile_options() too
# to stop it using non-PIC libz.a in linking.
target_compile_options(ZLIB::ZLIB INTERFACE -sUSE_ZLIB=1 -sRELOCATABLE=1)
target_link_options(ZLIB::ZLIB INTERFACE -sUSE_ZLIB=1 -sRELOCATABLE=1)
```
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -702,3 +730,38 @@ if(MSVC)
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}
${MSVC_LINKER_FLAGS}")
endif()
endif()
+
+if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
+ # flags are:
+ # 1) We are building library code
+ # 2) We force *everything* to build as position independent
+ # 3) And with support for C++ exceptions
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fPIC -fexceptions")
+ # size_t is 32 bit in emscripten wasm32 - ignore conversion errors
+ # deprecated-literal-operator error is thrown in datetime (vendored lib in
arrow)
+ set(CMAKE_CXX_FLAGS
+ "${CMAKE_CXX_FLAGS} -fPIC -fexceptions -Wno-error=shorten-64-to-32
-Wno-error=deprecated-literal-operator"
Review Comment:
```suggestion
# deprecated-literal-operator error is thrown in datetime (vendored lib in
arrow)
set(CMAKE_CXX_FLAGS
"${CMAKE_CXX_FLAGS} -fPIC -fexceptions
-Wno-error=deprecated-literal-operator"
```
##########
cpp/cmake_modules/EmscriptenOverrides.cmake:
##########
@@ -0,0 +1,25 @@
+# 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.
+
+# Force some variables for Emscripten
+# to disable things that won't work there
+
+# # override default in Emscripten which is to not use shared libs
+set_property(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS TRUE)
+
+# stripping doesn't work on emscripten
+set(CMAKE_STRIP FALSE)
Review Comment:
I confirmed that we can build without these configurations.
Can we remove this file?
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -702,3 +714,47 @@ if(MSVC)
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}
${MSVC_LINKER_FLAGS}")
endif()
endif()
+
+if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
+ # flags are:
+ # 1) We're using zlib from Emscripten ports
+ # 2) We are building library code
+ # 3) We force *everything* to build as position independent
+ # 4) And with support for C++ exceptions
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -sUSE_ZLIB=1 -fPIC -fexceptions")
+ # size_t is 32 bit in emscripten wasm32 - ignore conversion errors
+ # deprecated-literal-operator error is thrown in datetime (vendored lib in
arrow)
+ set(CMAKE_CXX_FLAGS
+ "${CMAKE_CXX_FLAGS} -sUSE_ZLIB=1 -fPIC -fexceptions
-Wno-error=shorten-64-to-32 -Wno-error=deprecated-literal-operator"
+ )
+
+ # flags for creating shared libraries (only used in pyarrow, because
+ # Emscripten builds libarrow as static)
+ # flags are:
+ # 1) Tell it to use zlib from Emscripten ports
+ # 2) Tell it to use javascript / webassembly 64 bit number support.
+ # 3) Tell it to build with support for C++ exceptions
+ # 4) Skip linker flags error which happens with -soname parameter
+ set(ARROW_EMSCRIPTEN_LINKER_FLAGS "-sWASM_BIGINT=1 -fexceptions
-Wno-error=linkflags")
+ set(CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS
+ "-sSIDE_MODULE=1 ${ARROW_EMSCRIPTEN_LINKER_FLAGS}")
+ set(CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS
+ "-sSIDE_MODULE=1 ${ARROW_EMSCRIPTEN_LINKER_FLAGS}")
+ set(CMAKE_SHARED_LINKER_FLAGS "-sSIDE_MODULE=1
${ARROW_EMSCRIPTEN_LINKER_FLAGS}")
+ if(ARROW_TESTING)
+ # flags for building test executables for use in node
+ set(CMAKE_EXE_LINKER_FLAGS
+ "${ARROW_EMSCRIPTEN_LINKER_FLAGS}
-sERROR_ON_WASM_CHANGES_AFTER_LINK=1 -sALLOW_MEMORY_GROWTH -lnodefs.js
-lnoderawfs.js --pre-js ${BUILD_SUPPORT_DIR}/emscripten-test-init.js"
Review Comment:
I focused on `--pre-js ${BUILD_SUPPORT_DIR}/emscripten-test-init.js"`.
The script just sets `ARROW_TEST_DATA` and `PARQUET_TEST_DATA`.
--
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]
