wgtmac commented on code in PR #2094: URL: https://github.com/apache/orc/pull/2094#discussion_r1897140156
########## cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -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";) Review Comment: What about moving it to below line 139? ########## cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -136,8 +137,9 @@ endfunction() # ---------------------------------------------------------------------- # ORC Format +message(STATUS "Using ORC_FORMAT_URL: ${ORC_FORMAT_URL}") 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 Review Comment: User provided URL may not match the `URL_HASH` below. We should probably avoid passing URL_HASH to ExternalProject_Add ########## cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -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) Review 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() ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
