ARROW-8147: [C++] add GCS library to ThirdpartyToolchain #11268

coryan · 2021-09-29T14:29:45Z

This adds the GCS library to ThirdpartyToolchain, and enables the
library in some of the builds. A future PR will add the library to the
package manager configuration files and to any remaining builds where it
needs to go.

github-actions · 2021-09-29T14:30:08Z

https://issues.apache.org/jira/browse/ARROW-8147

github-actions · 2021-09-29T14:30:10Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

coryan · 2021-09-29T16:31:49Z

The "Java JNI" test passes locally with archery docker run debian-java-jni. To me, it seems unlikely that my PR caused the failure, but it is possible I do not understand something here.

The appveyor failure seems to be a race condition in a test. The test has a SleepFor(0.01) which is generally a code smell.

In any case, if I am wrong do let me know and I will investigate / change things as needed.

This adds the GCS library to ThirdpartyToolchain, and enables the library in some of the builds. A future PR will add the library to the package manager configuration files and to any remaining builds where it needs to go.

coryan · 2021-09-30T18:05:28Z

/cc: @kou or @pitrou are you the right folks to look at this PR? If so, please take a look when you have a chance.

kou · 2021-09-30T20:32:02Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+        -DCMAKE_CXX_STANDARD=11
+        "-DCMAKE_CXX_FLAGS=${EP_CXX_FLAGS}"
+        -DCMAKE_INSTALL_LIBDIR=lib
+        "-DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>")


Can we use EP_COMMON_CMAKE_ARGS here?

Done. Also did the same for nlohmann_json (add EP_COMMON_CMAKE_ARGS and sort the arguments), but maybe it has no effect there.

kou · 2021-09-30T20:37:03Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+      -DCMAKE_INSTALL_RPATH=$ORIGIN
+      -DCMAKE_INSTALL_LIBDIR=lib
+      -DGOOGLE_CLOUD_CPP_ENABLE=storage
+      -DBUILD_TESTING=OFF)


Could you sort this list in alphabetical order?

kou · 2021-09-30T20:39:52Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+  if(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "AUTO")
+    find_package(google_cloud_cpp_storage QUIET)
+    if(NOT google_cloud_cpp_storage_FOUND)
+      build_google_cloud_cpp()
+    endif()
+  elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "BUNDLED")
+    build_google_cloud_cpp()
+  elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "SYSTEM")
+    find_package(google_cloud_cpp_storage REQUIRED)
+  endif()


Can we use resolve_dependency() here?

Suggested change

if(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "AUTO")

find_package(google_cloud_cpp_storage QUIET)

if(NOT google_cloud_cpp_storage_FOUND)

build_google_cloud_cpp()

endif()

elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "BUNDLED")

build_google_cloud_cpp()

elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "SYSTEM")

find_package(google_cloud_cpp_storage REQUIRED)

endif()

resolve_dependency(google_cloud_cpp_storage)

Oh, sorry. My suggestion was insufficient.

We need to add google_cloud_cpp_storage entry to build_dependency():

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 36c8c33ab..e12abefc5 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -151,6 +151,8 @@ macro(build_dependency DEPENDENCY_NAME) build_gflags() elseif("${DEPENDENCY_NAME}" STREQUAL "GLOG") build_glog() + elseif("${DEPENDENCY_NAME}" STREQUAL "google_cloud_cpp_storage") + build_google_cloud_cpp_storage() elseif("${DEPENDENCY_NAME}" STREQUAL "gRPC") build_grpc() elseif("${DEPENDENCY_NAME}" STREQUAL "GTest")

And we should use google_cloud_cpp_storage_SOURCE and build_google_cloud_cpp_storage for variable/function names.

Done, I think.

kou · 2021-10-02T19:32:28Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+        -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
+        "-DCMAKE_CXX_FLAGS=${EP_CXX_FLAGS}"


Can we remove them because EP_COMMON_CMAKE_ARGS include them?

Suggested change

-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}

"-DCMAKE_CXX_FLAGS=${EP_CXX_FLAGS}"

kou · 2021-10-02T19:36:00Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+                        PROPERTIES IMPORTED_LOCATION
+                                   "${GOOGLE_CLOUD_CPP_STATIC_LIBRARY_COMMON}"
+                                   INTERFACE_INCLUDE_DIRECTORIES
+                                   "${GOOGLE_CLOUD_CPP_INSTALL_PREFIX}")


Is this a typo?

Suggested change

"${GOOGLE_CLOUD_CPP_INSTALL_PREFIX}")

"${GOOGLE_CLOUD_CPP_INCLUDE_DIR}")

Fixed, thanks.

kou · 2021-10-02T19:37:13Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+                        Threads::Threads
+                        OpenSSL::SSL
+                        OpenSSL::Crypto)
+  add_dependencies(google-cloud-cpp::storage GOOGLE_CLOUD_CPP_ep)


Should we use lowercase here?

Suggested change

add_dependencies(google-cloud-cpp::storage GOOGLE_CLOUD_CPP_ep)

add_dependencies(google-cloud-cpp::storage google_cloud_cpp_ep)

kou · 2021-10-02T19:42:47Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+  if(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "AUTO")
+    find_package(google_cloud_cpp_storage QUIET)
+    if(NOT google_cloud_cpp_storage_FOUND)
+      build_google_cloud_cpp()
+    endif()
+  elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "BUNDLED")
+    build_google_cloud_cpp()
+  elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "SYSTEM")
+    find_package(google_cloud_cpp_storage REQUIRED)
+  endif()


Oh, sorry. My suggestion was insufficient.

We need to add google_cloud_cpp_storage entry to build_dependency():

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 36c8c33ab..e12abefc5 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -151,6 +151,8 @@ macro(build_dependency DEPENDENCY_NAME) build_gflags() elseif("${DEPENDENCY_NAME}" STREQUAL "GLOG") build_glog() + elseif("${DEPENDENCY_NAME}" STREQUAL "google_cloud_cpp_storage") + build_google_cloud_cpp_storage() elseif("${DEPENDENCY_NAME}" STREQUAL "gRPC") build_grpc() elseif("${DEPENDENCY_NAME}" STREQUAL "GTest")

And we should use google_cloud_cpp_storage_SOURCE and build_google_cloud_cpp_storage for variable/function names.

kou · 2021-10-03T19:13:55Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+    set(CRC32C_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/crc32c_ep-install")
+    set(CRC32C_CMAKE_ARGS
+        ${EP_COMMON_CMAKE_ARGS}
+        -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}


Suggested change

-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}

Fixed. FWIW, there are many other uses of this in the file, you may want to clean that up in a separate bug+PR.

cpp/cmake_modules/ThirdpartyToolchain.cmake

kou · 2021-10-03T19:15:07Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+        ${EP_COMMON_CMAKE_ARGS}
+        -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
+        -DCMAKE_CXX_STANDARD=11
+        "-DCMAKE_CXX_FLAGS=${EP_CXX_FLAGS}"


Suggested change

"-DCMAKE_CXX_FLAGS=${EP_CXX_FLAGS}"

kou · 2021-10-03T20:36:15Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+  set(GOOGLE_CLOUD_CPP_CMAKE_ARGS
+      ${EP_COMMON_CMAKE_ARGS}
+      -DBUILD_TESTING=OFF
+      -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}


Suggested change

-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}

kou · 2021-10-03T20:38:19Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+  if(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "AUTO")
+    find_package(google_cloud_cpp_storage QUIET)
+    if(NOT google_cloud_cpp_storage_FOUND)
+      build_google_cloud_cpp_storage()
+    endif()
+  elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "BUNDLED")
+    build_google_cloud_cpp_storage()
+  elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "SYSTEM")
+    find_package(google_cloud_cpp_storage REQUIRED)
+  endif()


resolve_dependency() does this.

Suggested change

if(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "AUTO")

find_package(google_cloud_cpp_storage QUIET)

if(NOT google_cloud_cpp_storage_FOUND)

build_google_cloud_cpp_storage()

endif()

elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "BUNDLED")

build_google_cloud_cpp_storage()

elseif(GOOGLE_CLOUD_CPP_SOURCE STREQUAL "SYSTEM")

find_package(google_cloud_cpp_storage REQUIRED)

endif()

Neat, thanks. Removed.

kou · 2021-10-03T20:38:55Z

ci/docker/ubuntu-20.04-cpp.dockerfile

    ARROW_WITH_ZSTD=ON \
    ASAN_SYMBOLIZER_PATH=/usr/lib/llvm-${llvm}/bin/llvm-symbolizer \
    AWSSDK_SOURCE=BUNDLED \
+    GOOGLE_CLOUD_CPP_SOURCE=BUNDLED \


Suggested change

GOOGLE_CLOUD_CPP_SOURCE=BUNDLED \

google_cloud_cpp_storage_SOURCE=BUNDLED \

Done. Note that this could be a nuisance if you ever want to use Bigtable or Spanner as data sources.

We may need to implement custom resolve_dependency() like feature for them when we also want to use other Google Cloud C++ libraries.

If Google Cloud C++ provides google_cloud_cpp CMake package with storage, bigtable and so on components, we can use google_cloud_cpp_SOURCE and find_package(google_cloud_cpp COMPONENTS storage bigtable ...).

kou · 2021-10-03T20:39:19Z

ci/scripts/cpp_build.sh

      -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX:-${ARROW_HOME}} \
      -DCMAKE_UNITY_BUILD=${CMAKE_UNITY_BUILD:-OFF} \
      -Dgflags_SOURCE=${gflags_SOURCE:-} \
+      -DGOOGLE_CLOUD_CPP_SOURCE=${GOOGLE_CLOUD_CPP_SOURCE:-} \


Suggested change

-DGOOGLE_CLOUD_CPP_SOURCE=${GOOGLE_CLOUD_CPP_SOURCE:-} \

-Dgoogle_cloud_cpp_storage_SOURCE=${google_cloud_cpp_storage_SOURCE:-} \

cpp/CMakeLists.txt

kou · 2021-10-03T20:41:45Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+        -DCRC32C_BUILD_TESTS=OFF
+        -DCRC32C_BUILD_BENCHMARKS=OFF
+        -DCRC32C_USE_GLOG=OFF)
+    set(CRC32C_BUILD_BYPRODUCTS)


Suggested change

set(CRC32C_BUILD_BYPRODUCTS)

Ooops, thanks. Removed.

kou

+1

Thanks.

kou · 2021-10-05T04:33:30Z

Oh, arm64 build on Travis CI is failed:

https://app.travis-ci.com/github/apache/arrow/jobs/541265250#L2124

FAILED: CMakeFiles/crc32c.dir/src/crc32c.cc.o 

/usr/bin/c++   -Iinclude -I/build/cpp/crc32c_ep-prefix/src/crc32c_ep/include -fdiagnostics-color=always -ggdb -O0 -g -fPIC -Wall -Wextra -Werror -fno-exceptions -fno-rtti   -fdiagnostics-color=always -ggdb -O0 -g -fPIC   -std=gnu++11 -MD -MT CMakeFiles/crc32c.dir/src/crc32c.cc.o -MF CMakeFiles/crc32c.dir/src/crc32c.cc.o.d -o CMakeFiles/crc32c.dir/src/crc32c.cc.o -c /build/cpp/crc32c_ep-prefix/src/crc32c_ep/src/crc32c.cc

In file included from /build/cpp/crc32c_ep-prefix/src/crc32c_ep/src/crc32c.cc:11:

/build/cpp/crc32c_ep-prefix/src/crc32c_ep/src/./crc32c_arm64_linux_check.h: In function 'bool crc32c::CanUseArm64Linux()':

/build/cpp/crc32c_ep-prefix/src/crc32c_ep/src/./crc32c_arm64_linux_check.h:36:37: error: the address of 'long unsigned int getauxval(long unsigned int)' will never be NULL [-Werror=address]

   36 |   unsigned long hwcap = (&getauxval != nullptr) ? getauxval(AT_HWCAP) : 0;

      |                          ~~~~~~~~~~~^~~~~~~~~~

cc1plus: all warnings being treated as errors

pitrou · 2021-10-05T08:05:16Z

ci/docker/ubuntu-20.04-cpp.dockerfile

    ARROW_WITH_ZSTD=ON \
    ASAN_SYMBOLIZER_PATH=/usr/lib/llvm-${llvm}/bin/llvm-symbolizer \
    AWSSDK_SOURCE=BUNDLED \
+    google_cloud_cpp_storage_SOURCE=BUNDLED \


Was GCS_SOURCE too ambiguous? This is quite a long parameter name...

We use CMake's package name for XXX_SOURCE.
If we don't follow the convention, we can choose GCS_SOURCE.

Let me know how to proceed, I can do either.

If it's the CMake name then fair enough.

kou · 2021-10-05T08:53:27Z

Forgot to update ARROW_THIRDPARTY_DEPENDENCIES in this pull request.

coryan · 2021-10-05T12:08:02Z

Oh, arm64 build on Travis CI is failed:

https://app.travis-ci.com/github/apache/arrow/jobs/541265250#L2124

That seems to be fixed upstream in crc32c, but the fix is not in any tag or release. Would you be Okay with me pinning to a SHA of this dependency? Can I reproduce these builds myself to verify I have fixed such problems?

kou · 2021-10-05T20:36:09Z

That seems to be fixed upstream in crc32c, but the fix is not in any tag or release. Would you be Okay with me pinning to a SHA of this dependency?

Yes. We sometimes use this approach.

Can I reproduce these builds myself to verify I have fixed such problems?

Yes.

Install Archery: python3 -m pip install -e dev/archery
See also: https://arrow.apache.org/docs/developers/archery.html
Run ubuntu-cpp service with ARCH=arm64v8 environment variable: ARCH=arm64v8 archery docker run ubuntu-cpp
Note that you need qemu-user-static to run this on x86_64.

BTW, it's strange that Travis CI jobs weren't run for this pull request...

In #11268 I neglected to add `google_cloud_cpp_storage` to the third-party dependencies. Closes #11320 from coryan/ARROW-14223-add-google-cloud-cpp-to-thirdparty-dependencies Authored-by: Carlos O'Ryan <coryan@google.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

This adds the GCS library to ThirdpartyToolchain, and enables the library in some of the builds. A future PR will add the library to the package manager configuration files and to any remaining builds where it needs to go. Closes apache#11268 from coryan/ARROW-8147-add-google-cloud-cpp-to-ThirdpartyToolchain Authored-by: Carlos O'Ryan <coryan@google.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

In apache#11268 I neglected to add `google_cloud_cpp_storage` to the third-party dependencies. Closes apache#11320 from coryan/ARROW-14223-add-google-cloud-cpp-to-thirdparty-dependencies Authored-by: Carlos O'Ryan <coryan@google.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actions bot added the Component: C++ label Sep 29, 2021

coryan marked this pull request as ready for review September 29, 2021 16:31

ARROW-8147: [C++] add GCS library to ThirdpartyToolchain

a725c52

This adds the GCS library to ThirdpartyToolchain, and enables the library in some of the builds. A future PR will add the library to the package manager configuration files and to any remaining builds where it needs to go.

kou reviewed Sep 30, 2021

View reviewed changes

Address review comments

da2e245

kou reviewed Oct 2, 2021

View reviewed changes

Address review comments

4e1341e

kou reviewed Oct 3, 2021

View reviewed changes

Address review comments

77d5184

kou approved these changes Oct 5, 2021

View reviewed changes

kou closed this in 2a6f78b Oct 5, 2021

pitrou reviewed Oct 5, 2021

View reviewed changes

coryan mentioned this pull request Oct 5, 2021

ARROW-14223: [C++] add missing third-party dependency #11320

Closed

coryan deleted the ARROW-8147-add-google-cloud-cpp-to-ThirdpartyToolchain branch October 5, 2021 21:24

asfimport mentioned this pull request Oct 5, 2021

[C++] Add google-cloud-cpp to ThirdpartyToolchain #24352

Closed

		-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
		"-DCMAKE_CXX_FLAGS=${EP_CXX_FLAGS}"

	"${GOOGLE_CLOUD_CPP_INSTALL_PREFIX}")
	"${GOOGLE_CLOUD_CPP_INCLUDE_DIR}")

	add_dependencies(google-cloud-cpp::storage GOOGLE_CLOUD_CPP_ep)
	add_dependencies(google-cloud-cpp::storage google_cloud_cpp_ep)

	GOOGLE_CLOUD_CPP_SOURCE=BUNDLED \
	google_cloud_cpp_storage_SOURCE=BUNDLED \

	-DGOOGLE_CLOUD_CPP_SOURCE=${GOOGLE_CLOUD_CPP_SOURCE:-} \
	-Dgoogle_cloud_cpp_storage_SOURCE=${google_cloud_cpp_storage_SOURCE:-} \

ARROW-8147: [C++] add GCS library to ThirdpartyToolchain #11268

ARROW-8147: [C++] add GCS library to ThirdpartyToolchain #11268

Uh oh!

Conversation

coryan commented Sep 29, 2021

Uh oh!

github-actions bot commented Sep 29, 2021

Uh oh!

github-actions bot commented Sep 29, 2021

Uh oh!

coryan commented Sep 29, 2021

Uh oh!

coryan commented Sep 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou left a comment •

edited

Loading

coryan commented Oct 5, 2021 •

edited

Loading