GH-42149: [C++] Use FetchContent for bundled ORC #43011
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
github-actions
bot
added
Component: C++
awaiting committer review
kou
commented
Jun 23, 2024
github-actions
bot
added
awaiting changes
|
@github-actions crossbow submit verify-rc-source--macos- |
This comment was marked as outdated.
This comment was marked as outdated.
|
(I'll complete this in the next week...) |
|
Thanks @kou ! |
kou
force-pushed
the
cpp-orc-fetch-content
branch
from
eb9c874 to
5a3817b
Compare
github-actions
bot
added
awaiting change review
kou
force-pushed
the
cpp-orc-fetch-content
branch
from
5a3817b to
28d0e18
Compare
|
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
This comment was marked as outdated.
This comment was marked as outdated.
wgtmac
reviewed
Jul 4, 2024
| set(SNAPPY_HOME | ||
| ${Snappy_ROOT} | ||
| CACHE BOOL "" FORCE) | ||
| set(SNAPPY_LIBRARY |
There was a problem hiding this comment.
Why do we set LZ4_STATIC_LIB but do not set SNAPPY_STATIC_LIB or PROTOBUF_STATIC_LIB?
There was a problem hiding this comment.
Good catch. We don't need to specify *_STATIC_LIB by defining ORC_PREFER_STATIC_*=OFF.
|
|
||
| get_target_property(ORC_ZSTD_ROOT ${ARROW_ZSTD_LIBZSTD} INTERFACE_INCLUDE_DIRECTORIES) | ||
| get_filename_component(ORC_ZSTD_ROOT "${ORC_ZSTD_ROOT}" DIRECTORY) | ||
| # TODO: This should be fixed in upstream. |
There was a problem hiding this comment.
Can we remove this after upgrading to v2.0.2 which should the fix? If yes, perhaps add a comment to we won't forget.
There was a problem hiding this comment.
Yes. I've updated the comment.
| if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "9") | ||
| target_link_libraries(orc::orc INTERFACE stdc++fs) | ||
| endif() | ||
| elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") |
There was a problem hiding this comment.
Does it mean we no longer support these legacy versions?
There was a problem hiding this comment.
Good catch. But it seems that this is not needed with ORC 2.0.1...?
https://github.com/ursacomputing/crossbow/actions/runs/9786486604/job/27021493582 passed
Did you fix this in ORC 2.0.1? #41023 (comment)
There was a problem hiding this comment.
Yes, it is included: https://github.com/apache/orc/commits/v2.0.1/
Commit is apache/orc@0749e4b
kou
force-pushed
the
cpp-orc-fetch-content
branch
from
0ffe906 to
52f14f4
Compare
|
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
github-actions
bot
added
awaiting changes
This comment was marked as outdated.
This comment was marked as outdated.
|
(I'm still fixing CI failures...) |
kou
force-pushed
the
cpp-orc-fetch-content
branch
from
3d88d7c to
cf33494
Compare
|
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
|
Revision: cf33494 Submitted crossbow builds: ursacomputing/crossbow @ actions-f3d88eefb6
|
|
@github-actions crossbow submit debian-* ubuntu-* |
|
Revision: 2e4112d Submitted crossbow builds: ursacomputing/crossbow @ actions-510dfad054
|
|
@raulcd This is ready to merge. |
raulcd
approved these changes
Jul 8, 2024
raulcd
pushed a commit
that referenced
this pull request
Jul 8, 2024
### Rationale for this change This also has a workaround for https://issues.apache.org/jira/browse/ORC-1732 . ### What changes are included in this PR? ORC 2.0.1 has a dependency detection problem. We can't override the detection with ExternalProject but can override the detection with FetchContent. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: #42149 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit e8a795b. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 102 possible false positives for unstable benchmarks that are known to sometimes produce them. |
zanmato1984
pushed a commit
to zanmato1984/arrow
that referenced
this pull request
Jul 9, 2024
### Rationale for this change This also has a workaround for https://issues.apache.org/jira/browse/ORC-1732 . ### What changes are included in this PR? ORC 2.0.1 has a dependency detection problem. We can't override the detection with ExternalProject but can override the detection with FetchContent. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#42149 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
This also has a workaround for https://issues.apache.org/jira/browse/ORC-1732 .
What changes are included in this PR?
ORC 2.0.1 has a dependency detection problem. We can't override the detection with ExternalProject but can override the detection with FetchContent.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.