-
Notifications
You must be signed in to change notification settings - Fork 3.7k
GH-46111: [C++][CI] Fix boost 1.88 on MinGW #46113
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
Conversation
|
|
I don't like this solution and I don't think we are compatible with boost 1.88 at the moment (seeing the other checks and includes for |
cpp/src/arrow/testing/process.cc
Outdated
# endif | ||
# ifdef BOOST_PROCESS_HAVE_V1 | ||
# include <boost/process/v1.hpp> | ||
# ifdef BOOST_PROCESS_HAVE_V2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we force V1 on Windows even if BOOST_PROCESS_HAVE_V2
as per:
if(BOOST_PROCESS_HAVE_V2
AND # 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
(NOT WIN32)
AND # We can't use v2 API with musl libc with Boost Process < 1.86
# because Boost Process < 1.86 doesn't support musl libc[3].
#
# [3] https://github.com/boostorg/process/commit/aea22dbf6be1695ceb42367590b6ca34d9433500
(NOT (ARROW_WITH_MUSL AND (Boost_VERSION VERSION_LESS 1.86))))
target_compile_definitions(Boost::process INTERFACE "BOOST_PROCESS_USE_V2")
endif()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this approach.
But do we need to change the current conditions? Is the following enough?
diff --git a/cpp/src/arrow/testing/process.cc b/cpp/src/arrow/testing/process.cc
index 5f04b31aa1..359e52245c 100644
--- a/cpp/src/arrow/testing/process.cc
+++ b/cpp/src/arrow/testing/process.cc
@@ -57,7 +57,25 @@
# define BOOST_USE_WINDOWS_H = 1
# endif
# ifdef BOOST_PROCESS_HAVE_V1
-# include <boost/process/v1.hpp>
+# include <boost/process/v1/args.hpp>
+# include <boost/process/v1/async.hpp>
+# include <boost/process/v1/async_system.hpp>
+# include <boost/process/v1/group.hpp>
+# include <boost/process/v1/child.hpp>
+# include <boost/process/v1/cmd.hpp>
+# include <boost/process/v1/env.hpp>
+# include <boost/process/v1/environment.hpp>
+# include <boost/process/v1/error.hpp>
+# include <boost/process/v1/exe.hpp>
+# include <boost/process/v1/group.hpp>
+# include <boost/process/v1/handles.hpp>
+# include <boost/process/v1/io.hpp>
+# include <boost/process/v1/pipe.hpp>
+# include <boost/process/v1/shell.hpp>
+# include <boost/process/v1/search_path.hpp>
+# include <boost/process/v1/spawn.hpp>
+# include <boost/process/v1/system.hpp>
+# include <boost/process/v1/start_dir.hpp>
# else
# include <boost/process.hpp>
# endif
Related issue: boostorg/process#480 |
@kou this should be ready. CI failures are unrelated. It seems it fails when the PR for 20.0.0 RC0 is open. But taking a look at the following code I am unsure why it comes as null. I'll open an issue:
Edit, the issue: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as this was already approved by @kou with a minor comment that has been applied I am going to merge to fix CI as I am trying to debug some failures and this is distracting me from the real failures :)
It seems I wrongly checked the CI jobs here, I am unsure where did I saw the CI failures about the merge script (might have been a different/wrong tab). I do think the MinGW failures are unrelated to this PR but I am going to open a new PR to check as this was working without the last commit on this PR (it shouldn't affect the protobuf failure) |
Ok, this did not cause the issue: |
### Rationale for this change Boost has remove their `include/boost/process/v1.hpp` ### What changes are included in this PR? Instead of using their old include file (which was a wrapper for the different includes) add the individual includes. ### Are these changes tested? Via CI. ### Are there any user-facing changes? No * GitHub Issue: #46111 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]>
### Rationale for this change Boost has remove their `include/boost/process/v1.hpp` ### What changes are included in this PR? Instead of using their old include file (which was a wrapper for the different includes) add the individual includes. ### Are these changes tested? Via CI. ### Are there any user-facing changes? No * GitHub Issue: apache#46111 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]>
### Rationale for this change Boost has remove their `include/boost/process/v1.hpp` ### What changes are included in this PR? Instead of using their old include file (which was a wrapper for the different includes) add the individual includes. ### Are these changes tested? Via CI. ### Are there any user-facing changes? No * GitHub Issue: apache#46111 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]>
…on boost 1.88 and use individual includes (#46160) ### Rationale for this change Similar to #46113 but for boost v2. The files `v1.hpp` and `v2.hpp` have been removed from `include/boost/process` on `1.88`: https://github.com/boostorg/process/tree/develop/include/boost/process but were available on `1.87`: https://github.com/boostorg/process/blob/boost-1.87.0/include/boost/process/v1.hpp https://github.com/boostorg/process/blob/boost-1.87.0/include/boost/process/v2.hpp ### What changes are included in this PR? Stop using include for possible missing file and add individual includes ### Are these changes tested? Via CI ### Are there any user-facing changes? No * GitHub Issue: #46159 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Jacob Wujciak-Jens <[email protected]>
…on boost 1.88 and use individual includes (#46160) ### Rationale for this change Similar to #46113 but for boost v2. The files `v1.hpp` and `v2.hpp` have been removed from `include/boost/process` on `1.88`: https://github.com/boostorg/process/tree/develop/include/boost/process but were available on `1.87`: https://github.com/boostorg/process/blob/boost-1.87.0/include/boost/process/v1.hpp https://github.com/boostorg/process/blob/boost-1.87.0/include/boost/process/v2.hpp ### What changes are included in this PR? Stop using include for possible missing file and add individual includes ### Are these changes tested? Via CI ### Are there any user-facing changes? No * GitHub Issue: #46159 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Jacob Wujciak-Jens <[email protected]>
Rationale for this change
Boost has remove their
include/boost/process/v1.hpp
What changes are included in this PR?
Instead of using their old include file (which was a wrapper for the different includes) add the individual includes.
Are these changes tested?
Via CI.
Are there any user-facing changes?
No