Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-43746: [C++] Add support for Boost 1.86 #43766

Merged
merged 49 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
bfb3558
GH-43746: [C++] Add support for Boost 1.86
kou Aug 20, 2024
4b9a97a
Use BoostConfig.cmake
kou Aug 20, 2024
7f15c65
Find missing Boost::process
kou Aug 20, 2024
5d29648
Move include
kou Aug 20, 2024
eb4285f
Reuse workaround
kou Aug 20, 2024
1c47c9d
Remove garbage
kou Aug 22, 2024
c1f4b06
Restore needed comment
kou Aug 22, 2024
6bb8baa
Use void for methods that never be failed
kou Aug 22, 2024
22b0508
Use Result
kou Aug 22, 2024
364e105
Add a missing comment
kou Aug 22, 2024
710b908
Make boost/process/v2/ optional
kou Aug 22, 2024
5d46dc0
Add ABORT_NOT_OK()
kou Aug 22, 2024
8426a20
Use boost::filesystem with old Boost
kou Aug 22, 2024
c2d9fcd
Don't use "stderr"
kou Aug 22, 2024
1f041ba
Include sys/sysctl.h explicitly
kou Aug 22, 2024
95c0169
Use graceful shutdown instead of process group
kou Aug 22, 2024
ba84ec1
Include missing unordered_map
kou Aug 23, 2024
2e19f38
Use process::environment::value()
kou Aug 23, 2024
c8d9566
Add missing v2 check
kou Aug 23, 2024
d754328
Remove needless Boost_NO_BOOST_CMAKE
kou Aug 23, 2024
4e68903
Add missing ntdll link
kou Aug 23, 2024
014a07e
Link to bcrypt
kou Aug 23, 2024
e03eebe
Fold a long line
kou Aug 23, 2024
5ca1ede
Install boost-cmake
kou Aug 23, 2024
dd86197
Can't use boost-cmake for now
kou Aug 23, 2024
d2f7f48
Add debug log
kou Aug 23, 2024
2c0b142
Reduce timeout
kou Aug 23, 2024
d578c6e
Fix inverted condition
kou Aug 23, 2024
d42114b
Keep PATHEXT
kou Aug 23, 2024
8945b31
Add more log
kou Aug 23, 2024
3244b19
Call terminate explicitly
kou Aug 23, 2024
7b1d965
Use interrupt
kou Aug 23, 2024
982da7d
Don't capture stderr
kou Aug 23, 2024
ddea25e
Debug
kou Aug 23, 2024
c73b33a
Use v1 on Windows
kou Aug 29, 2024
fc1400d
Force V1
kou Aug 29, 2024
e34715b
Add BOOST_PROCESS_HAVE_V1
kou Aug 29, 2024
66326b9
Boost::XXX may not exist when boost-cmake isn't used
kou Aug 29, 2024
4cf9491
Revert needless change
kou Aug 29, 2024
48cb2a3
Fix order
kou Aug 29, 2024
b8a2963
Disable CMP0167
kou Aug 29, 2024
b2a597e
Use add_gandiva_test()
kou Aug 29, 2024
5127210
Use the default deconstructor
kou Aug 29, 2024
ef022da
Simplify
kou Aug 29, 2024
f2f4858
Simplify
kou Aug 29, 2024
c964c7c
Use pid() not id() for readability
kou Aug 29, 2024
c193c0d
Use private: -> public: order like others
kou Aug 29, 2024
f11af96
Fix a typo
kou Aug 31, 2024
6470664
Simplify
kou Aug 31, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Comment on lines +1344 to +1346
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply use v1 on Boost < 1.86?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It's easy to test on my environment. Debian GNU/Linux sid still use Boost 1.83.
Can we keep this until we have a difficult problem with Boost < 1.86 + v2?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, fair enough.

#
# 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
Loading