fix(boost): add compatibility for Boost.Process v1 API in Boost 1.89+ #538
Conversation
📝 WalkthroughWalkthroughThe PR introduces Boost version compatibility handling: it errors out for Boost 1.88, and sets a CMake flag Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @cmake/FairMQDependencies.cmake:
- Around line 26-39: Replace the redundant multi-form Boost checks with
simplified string-version comparisons: remove usages of Boost_VERSION_MAJOR and
Boost_VERSION_MINOR and the duplicate VERSION_EQUAL "1.88" check, and only check
if Boost_VERSION VERSION_EQUAL "1.88.0" to trigger the FATAL_ERROR and if
Boost_VERSION VERSION_GREATER_EQUAL "1.89" to set
FAIRMQ_BOOST_PROCESS_V1_HEADER; if you must support older CMake behavior, add a
short comment noting that component-based checks would be required for older
CMake and keep that documented separately.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmake/FairMQDependencies.cmakefairmq/CMakeLists.txtfairmq/shmem/Manager.cxxfairmq/tools/Process.cxxtest/CMakeLists.txttest/device/_error_state.cxxtest/device/_exceptions.cxxtest/device/_signals.cxx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: latest-gcc-15
- GitHub Check: latest-gcc-13
- GitHub Check: tsan
- GitHub Check: boost187-gcc-15
- GitHub Check: latest-gcc-14
- GitHub Check: latest-gcc-12
- GitHub Check: asan+lsan+ubsan
- GitHub Check: static-analysis
🔇 Additional comments (8)
test/device/_error_state.cxx (1)
12-16: LGTM!The conditional include pattern correctly selects the appropriate Boost.Process header based on the
FAIRMQ_BOOST_PROCESS_V1_HEADERmacro. This aligns with the project-wide compatibility strategy for Boost 1.89+.test/device/_signals.cxx (1)
12-16: LGTM!Consistent conditional include pattern for Boost.Process header compatibility.
fairmq/tools/Process.cxx (1)
13-17: LGTM!The conditional header inclusion and namespace aliasing provide a clean abstraction layer. The
bpalias is consistently used for all Boost.Process operations (async_pipe,child,std_out,std_err,std_in), ensuring portable API access across Boost versions.Also applies to: 27-31
test/device/_exceptions.cxx (1)
12-16: LGTM!Consistent conditional include pattern for Boost.Process header compatibility.
fairmq/CMakeLists.txt (1)
179-181: LGTM!Correctly uses
PRIVATEvisibility for the compile definition since Boost.Process usage is an internal implementation detail of the library. The test build configuration handles propagation separately.fairmq/shmem/Manager.cxx (1)
14-20: The code at lines 27 and 31 is correct as written.boost::this_process::environment()andboost::this_process::path()are part of the top-levelboost::this_processnamespace, not nested withinboost::process::v1. When<boost/process/v1.hpp>is included, these top-level functions remain accessible without modification. Thebpnamespace alias is only needed forboost::processsymbols that moved toboost::process::v1, not forthis_process. No code changes are needed.test/CMakeLists.txt (2)
19-21: LGTM! Correctly propagates the Boost.Process v1 flag to test builds.The conditional logic follows the established pattern and will ensure that test code receives the
FAIRMQ_BOOST_PROCESS_V1_HEADERpreprocessor definition when needed.
141-141: Correctly adds definitions to Device test suite.The Device tests use Boost.Process headers in
_error_state.cxx,_exceptions.cxx, and_signals.cxx, making the${definitions}flag necessary. Verification confirms no other test directories use Boost.Process, so no additional test suites require this flag.
Boost 1.88 replaced Boost.Process with v2, breaking the v1 API. Boost 1.89 restores v1 compatibility via <boost/process/v1.hpp>. - Fail configuration if Boost 1.88 is detected - Define FAIRMQ_BOOST_PROCESS_V1_HEADER for Boost >= 1.89 - Use conditional includes to select v1.hpp or process.hpp - Add namespace aliases (bp, bp_this) for portable API access
dennisklein
force-pushed
the
fix_boost_compat
branch
from
276691f to
ab0336e
Compare
1 participant
Boost 1.88 replaced Boost.Process with v2, breaking the v1 API. Boost 1.89 restores v1 compatibility via <boost/process/v1.hpp>.
Related to #531