kou commented on code in PR #43766: URL: https://github.com/apache/arrow/pull/43766#discussion_r1739921453
########## cpp/src/arrow/testing/process.cc: ########## @@ -0,0 +1,300 @@ +// 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. + +#include "arrow/testing/process.h" +#include "arrow/result.h" + +// 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. +#include <boost/asio/io_context.hpp> + +#ifdef BOOST_PROCESS_HAVE_V2 +// We can't use v2 API on Windows because v2 API doesn't support +// process group [1] and GCS testbench uses multiple processes [2]. +// +// [1] https://github.com/boostorg/process/issues/259 +// [2] https://github.com/googleapis/storage-testbench/issues/669 +#ifndef _WIN32 +#define BOOST_PROCESS_USE_V2 +#endif +#endif + +#ifdef BOOST_PROCESS_USE_V2 +#ifdef BOOST_PROCESS_NEED_SOURCE +// Workaround for https://github.com/boostorg/process/issues/312 +#define BOOST_PROCESS_V2_SEPARATE_COMPILATION +#ifdef __APPLE__ +#include <sys/sysctl.h> +#endif +#include <boost/process/v2.hpp> +#include <boost/process/v2/src.hpp> +#else +#include <boost/process/v2.hpp> +#endif +#include <unordered_map> +#else +// We need BOOST_USE_WINDOWS_H definition with MinGW when we use +// boost/process.hpp. 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 +#ifdef __MINGW32__ +#define BOOST_USE_WINDOWS_H = 1 +#endif +#ifdef BOOST_PROCESS_HAVE_V1 +#include <boost/process/v1.hpp> +#else +#include <boost/process.hpp> +#endif +#endif + +#ifdef __APPLE__ +#include <limits.h> +#include <mach-o/dyld.h> +#endif + +#include <chrono> +#include <iostream> +#include <sstream> +#include <thread> + +#ifdef BOOST_PROCESS_USE_V2 +namespace asio = BOOST_PROCESS_V2_ASIO_NAMESPACE; +namespace process = BOOST_PROCESS_V2_NAMESPACE; +namespace filesystem = process::filesystem; +#else +#ifdef BOOST_PROCESS_HAVE_V1 +namespace process = boost::process::v1; +namespace filesystem = boost::process::v1::filesystem; +#else +namespace process = boost::process; +namespace filesystem = boost::filesystem; +#endif Review Comment: ```suggestion #elif defined(BOOST_PROCESS_HAVE_V1) namespace process = boost::process::v1; namespace filesystem = boost::process::v1::filesystem; #else namespace process = boost::process; namespace filesystem = boost::filesystem; ``` ########## cpp/src/arrow/testing/process.cc: ########## @@ -0,0 +1,300 @@ +// 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. + +#include "arrow/testing/process.h" +#include "arrow/result.h" + +// 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. +#include <boost/asio/io_context.hpp> + +#ifdef BOOST_PROCESS_HAVE_V2 +// We can't use v2 API on Windows because v2 API doesn't support +// process group [1] and GCS testbench uses multiple processes [2]. +// +// [1] https://github.com/boostorg/process/issues/259 +// [2] https://github.com/googleapis/storage-testbench/issues/669 +#ifndef _WIN32 +#define BOOST_PROCESS_USE_V2 +#endif +#endif + +#ifdef BOOST_PROCESS_USE_V2 +#ifdef BOOST_PROCESS_NEED_SOURCE +// Workaround for https://github.com/boostorg/process/issues/312 +#define BOOST_PROCESS_V2_SEPARATE_COMPILATION +#ifdef __APPLE__ +#include <sys/sysctl.h> +#endif +#include <boost/process/v2.hpp> +#include <boost/process/v2/src.hpp> +#else +#include <boost/process/v2.hpp> +#endif +#include <unordered_map> +#else +// We need BOOST_USE_WINDOWS_H definition with MinGW when we use +// boost/process.hpp. 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 +#ifdef __MINGW32__ +#define BOOST_USE_WINDOWS_H = 1 +#endif +#ifdef BOOST_PROCESS_HAVE_V1 +#include <boost/process/v1.hpp> +#else +#include <boost/process.hpp> +#endif +#endif + +#ifdef __APPLE__ +#include <limits.h> +#include <mach-o/dyld.h> +#endif + +#include <chrono> +#include <iostream> +#include <sstream> +#include <thread> + +#ifdef BOOST_PROCESS_USE_V2 +namespace asio = BOOST_PROCESS_V2_ASIO_NAMESPACE; +namespace process = BOOST_PROCESS_V2_NAMESPACE; +namespace filesystem = process::filesystem; +#else +#ifdef BOOST_PROCESS_HAVE_V1 +namespace process = boost::process::v1; +namespace filesystem = boost::process::v1::filesystem; +#else +namespace process = boost::process; +namespace filesystem = boost::filesystem; +#endif +#endif + +namespace arrow::util { + +class Process::Impl { + public: + Impl() { + // Get a copy of the current environment. +#ifdef BOOST_PROCESS_USE_V2 + for (const auto& kv : process::environment::current()) { + env_[kv.key()] = process::environment::value(kv.value()); + } +#else + env_ = process::environment(boost::this_process::environment()); +#endif + } + + ~Impl() { +#ifdef BOOST_PROCESS_USE_V2 + // V2 doesn't provide process group support yet: + // https://github.com/boostorg/process/issues/259 + // + // So we try graceful shutdown (SIGTERM + waitpid()) before + // immediate shutdown (SIGTERM). This assumes that the target Review Comment: Oh, I meant `SIGKILL`... ```suggestion // So we try graceful shutdown (SIGTERM + waitpid()) before // immediate shutdown (SIGKILL). This assumes that the target ``` ########## cpp/src/arrow/testing/process.cc: ########## @@ -0,0 +1,300 @@ +// 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. + +#include "arrow/testing/process.h" +#include "arrow/result.h" + +// 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. +#include <boost/asio/io_context.hpp> + +#ifdef BOOST_PROCESS_HAVE_V2 +// We can't use v2 API on Windows because v2 API doesn't support +// process group [1] and GCS testbench uses multiple processes [2]. +// +// [1] https://github.com/boostorg/process/issues/259 +// [2] https://github.com/googleapis/storage-testbench/issues/669 +#ifndef _WIN32 +#define BOOST_PROCESS_USE_V2 +#endif +#endif + +#ifdef BOOST_PROCESS_USE_V2 +#ifdef BOOST_PROCESS_NEED_SOURCE +// Workaround for https://github.com/boostorg/process/issues/312 +#define BOOST_PROCESS_V2_SEPARATE_COMPILATION +#ifdef __APPLE__ +#include <sys/sysctl.h> +#endif +#include <boost/process/v2.hpp> +#include <boost/process/v2/src.hpp> +#else +#include <boost/process/v2.hpp> +#endif +#include <unordered_map> +#else +// We need BOOST_USE_WINDOWS_H definition with MinGW when we use +// boost/process.hpp. 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 +#ifdef __MINGW32__ +#define BOOST_USE_WINDOWS_H = 1 +#endif +#ifdef BOOST_PROCESS_HAVE_V1 +#include <boost/process/v1.hpp> +#else +#include <boost/process.hpp> +#endif +#endif + +#ifdef __APPLE__ +#include <limits.h> +#include <mach-o/dyld.h> +#endif + +#include <chrono> +#include <iostream> +#include <sstream> +#include <thread> + +#ifdef BOOST_PROCESS_USE_V2 +namespace asio = BOOST_PROCESS_V2_ASIO_NAMESPACE; +namespace process = BOOST_PROCESS_V2_NAMESPACE; +namespace filesystem = process::filesystem; +#else +#ifdef BOOST_PROCESS_HAVE_V1 Review Comment: Good catch. Let's simplify this. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
