ORC-1810: [C++] Add environment variable ORC_FORMAT_URL #2094
Conversation
| ExternalProject_Add (orc-format_ep | ||
| URL "https://www.apache.org/dyn/closer.lua/orc/orc-format-${ORC_FORMAT_VERSION}/orc-format-${ORC_FORMAT_VERSION}.tar.gz?action=download" | ||
| URL ${ORC_FORMAT_URL} | ||
| URL_HASH SHA256=739fae5ff94b1f812b413077280361045bf92e510ef04b34a610e23a945d8cd5 |
There was a problem hiding this comment.
User provided URL may not match the URL_HASH below. We should probably avoid passing URL_HASH to ExternalProject_Add
| @@ -33,7 +33,8 @@ option(ORC_PREFER_STATIC_LZ4 "Prefer static lz4 library, if available" | |||
| option(ORC_PREFER_STATIC_ZSTD "Prefer static zstd library, if available" ON) | |||
| option(ORC_PREFER_STATIC_ZLIB "Prefer static zlib library, if available" ON) | |||
| option(ORC_PREFER_STATIC_GMOCK "Prefer static gmock library, if available" ON) | |||
|
|
|||
| set(DEFAULT_ORC_FORMAT_URL "https://www.apache.org/dyn/closer.lua/orc/orc-format-${ORC_FORMAT_VERSION}/orc-format-${ORC_FORMAT_VERSION}.tar.gz?action=download") | |||
There was a problem hiding this comment.
What about moving it to below line 139?
| @@ -33,7 +33,8 @@ option(ORC_PREFER_STATIC_LZ4 "Prefer static lz4 library, if available" | |||
| option(ORC_PREFER_STATIC_ZSTD "Prefer static zstd library, if available" ON) | |||
| option(ORC_PREFER_STATIC_ZLIB "Prefer static zlib library, if available" ON) | |||
| option(ORC_PREFER_STATIC_GMOCK "Prefer static gmock library, if available" ON) | |||
|
|
|||
| set(DEFAULT_ORC_FORMAT_URL "https://www.apache.org/dyn/closer.lua/orc/orc-format-${ORC_FORMAT_VERSION}/orc-format-${ORC_FORMAT_VERSION}.tar.gz?action=download") | |||
| set(ORC_FORMAT_URL ${DEFAULT_ORC_FORMAT_URL} CACHE STRING "URL from which to download ORC format library" FORCE) | |||
There was a problem hiding this comment.
What about using environment variable?
if(DEFINED ENV{ORC_FORMAT_URL})
set(ORC_FORMAT_SOURCE_URL "$ENV{ORC_FORMAT_URL}")
else()
set(ORC_FORMAT_SOURCE_URL
"https://www.apache.org/dyn/closer.lua/orc/orc-format-${ORC_FORMAT_VERSION}/orc-format-${ORC_FORMAT_VERSION}.tar.gz?action=download"
)
endif()
luffy-zh
changed the title
There was a problem hiding this comment.
+1
Thanks!
FYI: Apache Arrow accepts not only source URL but also checksum. It's for accepting newer/older versions than pre-defined versions at https://github.com/apache/arrow/blob/main/cpp/thirdparty/versions.txt . Apache Arrow uses ExternalProject_Add()/FetchContent_Declare() for optional external libraries. But Apache ORC uses ExternalProject_Add() for mandatory orc-format dependency. So situations in Apache Arrow and Apache ORC may be different.
| @@ -33,7 +33,6 @@ option(ORC_PREFER_STATIC_LZ4 "Prefer static lz4 library, if available" | |||
| option(ORC_PREFER_STATIC_ZSTD "Prefer static zstd library, if available" ON) | |||
| option(ORC_PREFER_STATIC_ZLIB "Prefer static zlib library, if available" ON) | |||
| option(ORC_PREFER_STATIC_GMOCK "Prefer static gmock library, if available" ON) | |||
|
|
|||
There was a problem hiding this comment.
We may want to revert this needless change.
What changes were proposed in this pull request?
Add an CMake option ORC_FORMAT_URL to indicate where to download the orc-format_ep
Why are the changes needed?
Handle the issue discussed apache/arrow#43417
How was this patch tested?
Test it locally
Was this patch authored or co-authored using generative AI tooling?
NO