Skip to content

Commit

Permalink
apacheGH-43746: [C++] Add support for Boost 1.86 (apache#43766)
Browse files Browse the repository at this point in the history
### Rationale for this change

`boost/process/*.hpp` are deprecated since Boost 1.86. And it seems that it also adds backward incompatible change. We need to use `boost/process/v2/*.hpp` instead.

### What changes are included in this PR?

This introduces `arrow::util::Process` for testing. It wraps boost/process/ API. So we don't need to use boost/process/ API directly in our tests.

We still use the v1 API on Windows because the v2 API doesn't process group and we don't have a workaround for it on Windows. If GCS's testbench doesn't use multiple processes, we can use the v2 API on Windows because we don't need to use process group in our use case.

See also:
* The v2 API and process group: boostorg/process#259
* GCS's testbench and multiple processes: googleapis/storage-testbench#669

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#43746

Lead-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
  • Loading branch information
3 people authored and mapleFU committed Sep 3, 2024
1 parent c4845d0 commit 354c69d
Show file tree
Hide file tree
Showing 17 changed files with 500 additions and 333 deletions.
4 changes: 1 addition & 3 deletions .github/workflows/cpp.yml
Original file line number Diff line number Diff line change
Expand Up @@ -409,12 +409,10 @@ jobs:
ARROW_WITH_SNAPPY: ON
ARROW_WITH_ZLIB: ON
ARROW_WITH_ZSTD: ON
# Don't use preinstalled Boost by empty BOOST_ROOT and
# -DBoost_NO_BOOST_CMAKE=ON
# Don't use preinstalled Boost by empty BOOST_ROOT
BOOST_ROOT: ""
ARROW_CMAKE_ARGS: >-
-DARROW_PACKAGE_PREFIX=/${{ matrix.msystem_lower}}
-DBoost_NO_BOOST_CMAKE=ON
-DCMAKE_FIND_PACKAGE_PREFER_CONFIG=ON
# We can't use unity build because we don't have enough memory on
# GitHub Actions.
Expand Down
5 changes: 4 additions & 1 deletion .github/workflows/ruby.yml
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,10 @@ jobs:
-source "https://nuget.pkg.github.com/$GITHUB_REPOSITORY_OWNER/index.json"
- name: Build C++ vcpkg dependencies
run: |
vcpkg\vcpkg.exe install --triplet $env:VCPKG_TRIPLET --x-manifest-root cpp --x-install-root build\cpp\vcpkg_installed
vcpkg\vcpkg.exe install `
--triplet $env:VCPKG_TRIPLET `
--x-manifest-root cpp `
--x-install-root build\cpp\vcpkg_installed
- name: Build C++
shell: cmd
run: |
Expand Down
48 changes: 38 additions & 10 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ macro(resolve_dependency DEPENDENCY_NAME)
IS_RUNTIME_DEPENDENCY
REQUIRED_VERSION
USE_CONFIG)
set(multi_value_args COMPONENTS PC_PACKAGE_NAMES)
set(multi_value_args COMPONENTS OPTIONAL_COMPONENTS PC_PACKAGE_NAMES)
cmake_parse_arguments(ARG
"${options}"
"${one_value_args}"
Expand Down Expand Up @@ -287,6 +287,9 @@ macro(resolve_dependency DEPENDENCY_NAME)
if(ARG_COMPONENTS)
list(APPEND FIND_PACKAGE_ARGUMENTS COMPONENTS ${ARG_COMPONENTS})
endif()
if(ARG_OPTIONAL_COMPONENTS)
list(APPEND FIND_PACKAGE_ARGUMENTS OPTIONAL_COMPONENTS ${ARG_OPTIONAL_COMPONENTS})
endif()
if(${DEPENDENCY_NAME}_SOURCE STREQUAL "AUTO")
find_package(${FIND_PACKAGE_ARGUMENTS})
set(COMPATIBLE ${${PACKAGE_NAME}_FOUND})
Expand Down Expand Up @@ -1289,15 +1292,19 @@ if(ARROW_USE_BOOST)
set(Boost_USE_STATIC_LIBS ON)
endif()
if(ARROW_BOOST_REQUIRE_LIBRARY)
set(ARROW_BOOST_COMPONENTS system filesystem)
set(ARROW_BOOST_COMPONENTS filesystem system)
set(ARROW_BOOST_OPTIONAL_COMPONENTS process)
else()
set(ARROW_BOOST_COMPONENTS)
set(ARROW_BOOST_OPTIONAL_COMPONENTS)
endif()
resolve_dependency(Boost
REQUIRED_VERSION
${ARROW_BOOST_REQUIRED_VERSION}
COMPONENTS
${ARROW_BOOST_COMPONENTS}
OPTIONAL_COMPONENTS
${ARROW_BOOST_OPTIONAL_COMPONENTS}
IS_RUNTIME_DEPENDENCY
# libarrow.so doesn't depend on libboost*.
FALSE)
Expand All @@ -1316,14 +1323,35 @@ if(ARROW_USE_BOOST)
endif()
endforeach()

if(WIN32 AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
# boost/process/detail/windows/handle_workaround.hpp doesn't work
# without BOOST_USE_WINDOWS_H with MinGW because MinGW doesn't
# provide __kernel_entry without winternl.h.
#
# See also:
# https://github.com/boostorg/process/blob/develop/include/boost/process/detail/windows/handle_workaround.hpp
target_compile_definitions(Boost::headers INTERFACE "BOOST_USE_WINDOWS_H=1")
if(TARGET Boost::process)
# Boost >= 1.86
target_compile_definitions(Boost::process INTERFACE "BOOST_PROCESS_HAVE_V1")
target_compile_definitions(Boost::process INTERFACE "BOOST_PROCESS_HAVE_V2")
else()
# Boost < 1.86
add_library(Boost::process INTERFACE IMPORTED)
if(TARGET Boost::filesystem)
target_link_libraries(Boost::process INTERFACE Boost::filesystem)
endif()
if(TARGET Boost::system)
target_link_libraries(Boost::process INTERFACE Boost::system)
endif()
if(TARGET Boost::headers)
target_link_libraries(Boost::process INTERFACE Boost::headers)
endif()
if(Boost_VERSION VERSION_GREATER_EQUAL 1.80)
target_compile_definitions(Boost::process INTERFACE "BOOST_PROCESS_HAVE_V2")
# Boost < 1.86 has a bug that
# boost::process::v2::process_environment::on_setup() isn't
# defined. We need to build Boost Process source to define it.
#
# See also:
# https://github.com/boostorg/process/issues/312
target_compile_definitions(Boost::process INTERFACE "BOOST_PROCESS_NEED_SOURCE")
if(WIN32)
target_link_libraries(Boost::process INTERFACE bcrypt ntdll)
endif()
endif()
endif()

message(STATUS "Boost include dir: ${Boost_INCLUDE_DIRS}")
Expand Down
13 changes: 9 additions & 4 deletions cpp/src/arrow/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -644,9 +644,13 @@ else()
endif()

set(ARROW_TESTING_SHARED_LINK_LIBS arrow_shared ${ARROW_GTEST_GTEST})
set(ARROW_TESTING_SHARED_PRIVATE_LINK_LIBS arrow::flatbuffers RapidJSON)
set(ARROW_TESTING_STATIC_LINK_LIBS arrow::flatbuffers RapidJSON arrow_static
${ARROW_GTEST_GTEST})
set(ARROW_TESTING_SHARED_PRIVATE_LINK_LIBS arrow::flatbuffers RapidJSON Boost::process)
set(ARROW_TESTING_STATIC_LINK_LIBS
arrow::flatbuffers
RapidJSON
Boost::process
arrow_static
${ARROW_GTEST_GTEST})
set(ARROW_TESTING_SHARED_INSTALL_INTERFACE_LIBS Arrow::arrow_shared)
set(ARROW_TESTING_STATIC_INSTALL_INTERFACE_LIBS Arrow::arrow_static)
# that depend on gtest
Expand All @@ -667,9 +671,10 @@ set(ARROW_TESTING_SRCS
io/test_common.cc
ipc/test_common.cc
testing/fixed_width_test_util.cc
testing/generator.cc
testing/gtest_util.cc
testing/process.cc
testing/random.cc
testing/generator.cc
testing/util.cc)

#
Expand Down
18 changes: 4 additions & 14 deletions cpp/src/arrow/filesystem/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,15 @@ if(ARROW_GCS)
EXTRA_LABELS
filesystem
EXTRA_LINK_LIBS
google-cloud-cpp::storage
Boost::filesystem
Boost::system)
google-cloud-cpp::storage)
endif()

if(ARROW_AZURE)
add_arrow_test(azurefs_test
EXTRA_LABELS
filesystem
EXTRA_LINK_LIBS
${AZURE_SDK_LINK_LIBRARIES}
Boost::filesystem
Boost::system)
${AZURE_SDK_LINK_LIBRARIES})
endif()

if(ARROW_S3)
Expand All @@ -75,11 +71,7 @@ if(ARROW_S3)
else()
list(APPEND ARROW_S3_TEST_EXTRA_LINK_LIBS arrow_static)
endif()
list(APPEND
ARROW_S3_TEST_EXTRA_LINK_LIBS
${AWSSDK_LINK_LIBRARIES}
Boost::filesystem
Boost::system)
list(APPEND ARROW_S3_TEST_EXTRA_LINK_LIBS ${AWSSDK_LINK_LIBRARIES})
add_arrow_test(s3fs_test
SOURCES
s3fs_test.cc
Expand Down Expand Up @@ -122,9 +114,7 @@ if(ARROW_S3)
s3_test_util.cc
STATIC_LINK_LIBS
${AWSSDK_LINK_LIBRARIES}
${ARROW_BENCHMARK_LINK_LIBS}
Boost::filesystem
Boost::system)
${ARROW_BENCHMARK_LINK_LIBS})
if(ARROW_TEST_LINKAGE STREQUAL "static")
target_link_libraries(arrow-filesystem-s3fs-benchmark PRIVATE parquet_static)
else()
Expand Down
52 changes: 12 additions & 40 deletions cpp/src/arrow/filesystem/azurefs_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,6 @@
// specific language governing permissions and limitations
// under the License.

#include <algorithm> // Missing include in boost/process

// This boost/asio/io_context.hpp include is needless for no MinGW
// build.
//
// This is for including boost/asio/detail/socket_types.hpp before any
// "#include <windows.h>". boost/asio/detail/socket_types.hpp doesn't
// work if windows.h is already included. boost/process.h ->
// boost/process/args.hpp -> boost/process/detail/basic_cmd.hpp
// includes windows.h. boost/process/args.hpp is included before
// boost/process/async.h that includes
// boost/asio/detail/socket_types.hpp implicitly is included.
#include <boost/asio/io_context.hpp>
// We need BOOST_USE_WINDOWS_H definition with MinGW when we use
// boost/process.hpp. See BOOST_USE_WINDOWS_H=1 in
// cpp/cmake_modules/ThirdpartyToolchain.cmake for details.
#include <boost/process.hpp>

#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

Expand All @@ -53,6 +35,7 @@
#include "arrow/result.h"
#include "arrow/status.h"
#include "arrow/testing/gtest_util.h"
#include "arrow/testing/process.h"
#include "arrow/testing/util.h"
#include "arrow/util/future.h"
#include "arrow/util/io_util.h"
Expand All @@ -67,7 +50,6 @@ namespace arrow {
using internal::TemporaryDir;
namespace fs {
using internal::ConcatAbstractPath;
namespace bp = boost::process;

using ::testing::IsEmpty;
using ::testing::Not;
Expand Down Expand Up @@ -174,42 +156,32 @@ class AzuriteEnv : public AzureEnvImpl<AzuriteEnv> {
private:
std::unique_ptr<TemporaryDir> temp_dir_;
arrow::internal::PlatformFilename debug_log_path_;
bp::child server_process_;
std::unique_ptr<util::Process> server_process_;

using AzureEnvImpl::AzureEnvImpl;

public:
static const AzureBackend kBackend = AzureBackend::kAzurite;

~AzuriteEnv() override {
server_process_.terminate();
server_process_.wait();
}
~AzuriteEnv() = default;

static Result<std::unique_ptr<AzureEnvImpl>> Make() {
auto self = std::unique_ptr<AzuriteEnv>(
new AzuriteEnv("devstoreaccount1",
"Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/"
"K1SZFPTOtr/KBHBeksoGMGw=="));
auto exe_path = bp::search_path("azurite");
if (exe_path.empty()) {
return Status::Invalid("Could not find Azurite emulator.");
}
self->server_process_ = std::make_unique<util::Process>();
ARROW_RETURN_NOT_OK(self->server_process_->SetExecutable("azurite"));
ARROW_ASSIGN_OR_RAISE(self->temp_dir_, TemporaryDir::Make("azurefs-test-"));
ARROW_ASSIGN_OR_RAISE(self->debug_log_path_,
self->temp_dir_->path().Join("debug.log"));
auto server_process = bp::child(
boost::this_process::environment(), exe_path, "--silent", "--location",
self->temp_dir_->path().ToString(), "--debug", self->debug_log_path_.ToString(),
// For old Azurite. We can't install the latest Azurite with
// old Node.js on old Ubuntu.
"--skipApiVersionCheck");
if (!server_process.valid() || !server_process.running()) {
server_process.terminate();
server_process.wait();
return Status::Invalid("Could not start Azurite emulator.");
}
self->server_process_ = std::move(server_process);
self->server_process_->SetArgs({"--silent", "--location",
self->temp_dir_->path().ToString(), "--debug",
self->debug_log_path_.ToString(),
// For old Azurite. We can't install the latest
// Azurite with old Node.js on old Ubuntu.
"--skipApiVersionCheck"});
ARROW_RETURN_NOT_OK(self->server_process_->Execute());
return self;
}

Expand Down
Loading

0 comments on commit 354c69d

Please sign in to comment.