Uh oh!
There was an error while loading. Please reload this page.
ARROW-10541: [C++] Add re2 library to core arrow / ARROW_WITH_RE2 - #8756
ARROW-10541: [C++] Add re2 library to core arrow / ARROW_WITH_RE2#8756xhochy wants to merge 7 commits into
Conversation
xhochy
commented
Nov 24, 2020
@github-actions autotune everything |
| define_option(ARROW_WITH_UTF8PROC | ||
| "Build with support for Unicode properties using the utf8proc library" ON) | ||
| define_option(ARROW_WITH_RE2 | ||
| "Build with support for regular expressions using the re2 library" ON) |
There was a problem hiding this comment.
I find this confusing. I understand how it works--if ARROW_COMPUTE is on, these are on too unless you set them to OFF--but it looks like they're always built by default, unlike the compression libs listed above.
Maybe it's irrelevant that it looks confusing here. Or maybe at least we should add a comment here explaining how the default is really ON iff ARROW_COMPUTE is ON too. Or is it possible to define the default option here to be literally whatever ARROW_COMPUTE is, like define_option(ARROW_WITH_RE2 "..." ARROW_COMPUTE)?
There was a problem hiding this comment.
This is sadly the prettiest solution I can think of at the moment. You can define conditional options in CMake but they only make sense based on attributes that are not set by the user but the environment (e.g. which operating system).
I'll add a comment to this and the above option explaining that they are only used in certain situations.
There was a problem hiding this comment.
Instead of a comment could that explanation be a parenthesis of the description for this option?
xhochy
commented
Nov 25, 2020
Everything failing in #8759 is past the CMake additions I did here, so this should be good to go. |
maartenbreddels
commented
Nov 25, 2020
#8459 builds on Travis, looking good 👍 |
| if(ARROW_GANDIVA) | ||
| set(ARROW_WITH_RE2 ON) | ||
| endif() |
There was a problem hiding this comment.
How about forcing to enable/disable ARROW_WITH_RE2 in ThridpartyToolchain.cmake like ARROW_WITH_THRIFT?
Uh oh!
There was an error while loading. Please reload this page.
| "Build with support for Unicode properties using the utf8proc library" ON) | ||
| "Build with support for Unicode properties using the utf8proc library (only used if ARROW_COMPUTE is ON)" ON) | ||
| define_option(ARROW_WITH_RE2 | ||
| "Build with support for regular expressions using the re2 library (only used if ARROW_COMPUTE or ARROW_GANDIVA is ON)" ON) |
There was a problem hiding this comment.
We can remove ARROW_WITH_UTF8PROC and ARROW_WITH_RE2 if we set them in ThirdpartyToolchain.cmake internally like ARROW_WITH_THRIFT.
There was a problem hiding this comment.
No, the difference to ARROW_WITH_THRIFT is that you can still build ARROW_COMPUTE even if ARROW_WITH_UTF8PROC=OFF. In that case, only some features of compute won't be build but the majority will work.
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
xhochy
commented
Nov 26, 2020
@github-actions autotune |
Because we require RE2 with ARROW_COMPUTE by default. See also: ARROW-10541 and apache#8756
Because we require RE2 with ARROW_COMPUTE by default. See also: ARROW-10541 and #8756Closes#8786 from kou/cpp-example-static Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
No description provided.