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
ARROW-6782: [C++] Do not require Boost for minimal C++ build #5611
Conversation
set(BOOST_FILESYSTEM_LIBRARY ${Boost_FILESYSTEM_LIBRARY}) | ||
set(BOOST_REGEX_LIBRARY ${Boost_REGEX_LIBRARY}) | ||
if(ARROW_BUILD_INTEGRATION | ||
OR ARROW_BUILD_TESTS |
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.
I'm a little worried about how we're going to maintain this if
statement. Suppose the compute module needs boost for something. How are we going to be sure that OR ARROW_COMPUTE
gets added here? We won't catch it in our C++ tests because ARROW_BUILD_TESTS
will cause boost to be included, and we won't catch it in R or Python because ARROW_PARQUET
is on. But then if someone builds with ARROW_COMPUTE
but not these other flags, it will fail.
I don't have a solution in mind, just wanted to express my fear.
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.
Yes, I would like to have a more declarative way to define dependencies between components and their build toolchain dependencies rather than doing this ad hoc stuff. The only way to really be sure is to test a battery of different build configurations and make sure they all work
OR ARROW_HDFS | ||
OR ARROW_GANDIVA | ||
OR ARROW_PARQUET) | ||
set(ARROW_BOOST_REQUIRED TRUE) |
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.
Do all of these require the same boost libraries? Gandiva requires boost::filesystem?
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.
This flag just determines whether or not we create boost_ep (or use the system / toolchain Boost if it's available), so it's an all-or-nothing thing
Appveyor build in progress... https://ci.appveyor.com/project/wesm/arrow/builds/27991887 |
Weird Appveyor failure that doesn't seem related to this patch
|
The CI is passing here, the Appveyor failure is ARROW-6834 |
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
With these changes, the minimal build now only requires double-conversion_ep. We still have to disable a number of components manually to obtain the minimal build, so there is more work to do