GH-43746: [C++] Add support for Boost 1.86 #43766
Conversation
|
@github-actions crossbow submit -g cpp -g r -g linux |
|
|
This comment was marked as outdated.
This comment was marked as outdated.
github-actions
bot
added
awaiting merge
| @@ -65,7 +48,7 @@ namespace arrow { | |||
| using internal::TemporaryDir; | |||
| namespace fs { | |||
| using internal::ConcatAbstractPath; | |||
| namespace bp = boost::process; | |||
| // namespace bp = BOOST_PROCESS_V2_NAMESPACE; | |||
There was a problem hiding this comment.
Yes. I should have removed this intermediated code.
cpp/src/arrow/testing/process.cc
Outdated
| std::unique_ptr<process::process> process_; | ||
| asio::io_context ctx_; | ||
|
|
||
| Status ResolveCurrentExecutable(process::filesystem::path* out) { |
There was a problem hiding this comment.
It's a good idea.
I just moved this from flight/test_util.cc but I should have improved the API.
cpp/src/arrow/testing/process.cc
Outdated
| if (buffered_output.eof()) { | ||
| buffered_output.clear(); | ||
| auto last = buffered_output.str().size(); | ||
| buffered_output.seekg(last); | ||
| buffered_output.seekp(last); | ||
| } |
There was a problem hiding this comment.
What does this do? Can you add an explanatory comment?
There was a problem hiding this comment.
It's for clearing a EOS bit. I'll add a comment.
| ARROW_RETURN_NOT_OK( | ||
| impl_->server_process_->SetEnv("MINIO_ACCESS_KEY", kMinioAccessKey)); |
There was a problem hiding this comment.
Oh, good catch.
I'll remove it.
| impl_->server_process_->SetEnv("MINIO_ACCESS_KEY", kMinioAccessKey)); | ||
| ARROW_RETURN_NOT_OK( | ||
| impl_->server_process_->SetEnv("MINIO_SECRET_KEY", kMinioSecretKey)); | ||
| ARROW_RETURN_NOT_OK(impl_->server_process_->SetEnv("MINIO_BROWSER", "off")); |
There was a problem hiding this comment.
Can you keep the comment from above?
There was a problem hiding this comment.
Ah, I should have not remove the comment. I'll re-add it.
| server_process_.terminate(); | ||
| server_process_.wait(); | ||
| status = server_process->SetArgs({"-m", "testbench", "--port", port_}); | ||
| if (!status.ok()) { |
There was a problem hiding this comment.
Is it useful to add this information, especially as SetArgs cannot actually fail? (same question below)
There was a problem hiding this comment.
No. I added Status return value for consistency but I'll change the return type to void.
| server_process_.wait(); | ||
| } | ||
| } | ||
| ~GcsTestbench() override { server_process_ = nullptr; } |
There was a problem hiding this comment.
Is it possible to keep the "kill process group" behavior, or is that impossible with bp v2?
There was a problem hiding this comment.
It's impossible with v2...
boostorg/process#259
There was a problem hiding this comment.
Ah, too bad. We'll have to do without it, then.
There was a problem hiding this comment.
Hmm. It seems that we can't do with v2. We can't call setpgid() between fork() and exec() with v2...
There was a problem hiding this comment.
If GCS's testbench doesn't use multiple processes, we don't need to use process group: googleapis/storage-testbench#669
There was a problem hiding this comment.
We can terminate all GCS's testbench related processes on Linux and macOS by SIGTERM. Because GCS's testbench's main process terminates all related processes by SIGTERM.
We can't use similar approaches (SendMessageW(WM_CLOSE) or GenerateConsoleCtrlEvent(CTRL_C_EVENT)) on Windows. They didn't work.
So let's still use v1 on Windows until googleapis/storage-testbench#669 is solved.
github-actions
bot
added
awaiting changes
|
Hmm. It seems that some platforms still use Boost < 1.80. |
kou
force-pushed
the
cpp-boost-process-v2
branch
from
4798ea6 to
a5a36ac
Compare
github-actions
bot
added
awaiting change review
kou
force-pushed
the
cpp-boost-process-v2
branch
from
e2de404 to
ff9fd9f
Compare
|
Hmm. |
|
It seems that |
| @@ -1191,57 +1194,6 @@ set(Boost_USE_MULTITHREADED ON) | |||
| if(MSVC AND ARROW_USE_STATIC_CRT) | |||
| set(Boost_USE_STATIC_RUNTIME ON) | |||
| endif() | |||
| set(Boost_ADDITIONAL_VERSIONS | |||
There was a problem hiding this comment.
Do we not need to keep this around for anyone running an older cmake version + boost without the config? (our min is 3.16 after all)
There was a problem hiding this comment.
Let's try our CI.
If we need to keep this, it tells us. :-)
There was a problem hiding this comment.
Hmm.
Our vcpkg doesn't have boost-cmake yet...: #43812
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
|
@github-actions crossbow submit -g cpp -g r -g linux -g wheel java-jars |
|
Revision: c193c0d Submitted crossbow builds: ursacomputing/crossbow @ actions-156765ce39
|
|
If nobody objects this, I'll merge this in the next week. |
cpp/src/arrow/testing/process.cc
Outdated
| #else | ||
| #ifdef BOOST_PROCESS_HAVE_V1 |
There was a problem hiding this comment.
Good catch. Let's simplify this.
cpp/src/arrow/testing/process.cc
Outdated
| #else | ||
| #ifdef BOOST_PROCESS_HAVE_V1 |
There was a problem hiding this comment.
Good catch. Let's simplify this.
github-actions
bot
added
awaiting changes
|
@github-actions crossbow submit verify-rc-source-macos |
|
I am just triggering some tasks to validate this will solve some nightly failures for the verification jobs. |
|
Revision: 6470664 Submitted crossbow builds: ursacomputing/crossbow @ actions-a2a3ccd6a0
|
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 00d3576. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
### 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]>
### 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]>
### 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]>
Rationale for this change
boost/process/*.hppare deprecated since Boost 1.86. And it seems that it also adds backward incompatible change. We need to useboost/process/v2/*.hppinstead.What changes are included in this PR?
This introduces
arrow::util::Processfor 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:
Are these changes tested?
Yes.
Are there any user-facing changes?
No.