You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by GitBox <gi...@apache.org> on 2020/07/30 07:34:15 UTC
[GitHub] [orc] nehaljwani opened a new pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
nehaljwani opened a new pull request #524:
URL: https://github.com/apache/orc/pull/524
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] nehaljwani commented on pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
nehaljwani commented on pull request #524:
URL: https://github.com/apache/orc/pull/524#issuecomment-670293683
@dongjoon-hyun Apologies for the delay in response.
Here is what I have:
```
/t/n/orc ❯❯❯ ls prefix/lib/libzstd.*
prefix/lib/libzstd.a prefix/lib/libzstd.so prefix/lib/libzstd.so.1 prefix/lib/libzstd.so.1.4.5
```
My attempt at building orc:
```
/t/n/orc ❯❯❯ cmake -S . -Bbuild -GNinja -DBUILD_JAVA=False -DLZ4_HOME=$PWD/prefix -DZLIB_HOME=$PWD/prefix -DZSTD_HOME=$PWD/prefix -DCMAKE_POLICY_DEFAULT_CMP0074=NEW -DSNAPPY_HOME=$PWD/prefix
...
-- ZSTD_HOME: /tmp/nwani_1596768059/orc/prefix
CMake Error at cmake_modules/FindZSTD.cmake:60 (message):
Could not find zstd in system search paths.
Call Stack (most recent call first):
cmake_modules/ThirdpartyToolchain.cmake:139 (find_package)
CMakeLists.txt:120 (INCLUDE)
```
Second attempt with library name specified:
```
/t/n/orc ❯❯❯ cmake -S . -Bbuild -GNinja -DZSTD_STATIC_LIB_NAME=libzstd -DBUILD_JAVA=False -DLZ4_HOME=$PWD/prefix -DZLIB_HOME=$PWD/prefix -DZSTD_HOME=$PWD/prefix -DCMAKE_POLICY_DEFAULT_CMP0074=NEW -DSNAPPY_HOME=$PWD/prefix
...
-- ZSTD_HOME: /tmp/nwani_1596768059/orc/prefix
CMake Error at cmake_modules/FindZSTD.cmake:60 (message):
Could not find zstd in system search paths.
Call Stack (most recent call first):
cmake_modules/ThirdpartyToolchain.cmake:139 (find_package)
CMakeLists.txt:120 (INCLUDE)
```
Third attempt with library named specified with extension (leading into a configure success, but a build failure):
```
/t/n/orc ❯❯❯ cmake -S . -Bbuild -GNinja -DZSTD_STATIC_LIB_NAME=libzstd.a -DBUILD_JAVA=False -DLZ4_HOME=$PWD/prefix -DZLIB_HOME=$PWD/prefix -DZSTD_HOME=$PWD/prefix -DCMAKE_POLICY_DEFAULT_CMP0074=NEW -DSNAPPY_HOME=$PWD/prefix -DZSTD_STATIC_LIB_NAME=libzstd.a
...
-- ZSTD_HOME: /tmp/nwani_1596768059/orc/prefix
-- Found the zstd header: /tmp/nwani_1596768059/orc/prefix/include/zstd.h
-- Found the zstd library: /tmp/nwani_1596768059/orc/prefix/lib/libzstd.a.a <--------------------- !!!!!!
```
I wrote up this patch to unblock myself, however, I am of the opinion that all the Find* modules need a nice refactor to support linking each one of this third party compression libraries dynamically via a CMake option and also switch to using CMake targets instead of CMake variables to consume them (modern CMake approach)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r474422404
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
Thanks, @omalley . Could you try to make a PR, @nehaljwani ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] nehaljwani commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
nehaljwani commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r468267702
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
It doesn't, however, depending on the order of items in the value of `CMAKE_FIND_LIBRARY_SUFFIXES`, it can pick up the shared library here (`libzstd.so`) and then fail later, while linking statically. IMO we should close this PR and open an issue to support linking third party dependencies dynamically and statically, both.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r467259161
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
I understand the use case, but this is used at line 35, `find_library (ZSTD_LIBRARIES NAMES ${ZSTD_STATIC_LIB_NAME} HINTS`. Usually, `find_library` doesn't require suffix, does it?
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
I understand your use case, but this is used at line 35, `find_library (ZSTD_LIBRARIES NAMES ${ZSTD_STATIC_LIB_NAME} HINTS`. Usually, `find_library` doesn't require suffix, does it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] omalley commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
omalley commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r474311146
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
Yeah, we really need to clean up the CMakefiles. At this point, my original goal of using static libraries has become much less relevant. We should probably just default to using dynamic linking everywhere.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] nehaljwani commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
nehaljwani commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r474670420
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
I'll take a jab at it this weekend.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #524:
URL: https://github.com/apache/orc/pull/524#issuecomment-670334953
Thank you for the information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #524:
URL: https://github.com/apache/orc/pull/524#issuecomment-670094903
Ping, @nehaljwani .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #524:
URL: https://github.com/apache/orc/pull/524#issuecomment-669393556
Gentle ping, @nehaljwani .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] nehaljwani commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
nehaljwani commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r479688902
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
@omalley @dongjoon-hyun I've opened another PR: https://github.com/apache/orc/pull/541
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] nehaljwani commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
nehaljwani commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r474670420
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
I'll make an attempt at it this weekend.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r468278539
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
Could you open a PR with that approach? We can close this after that PR is merged.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] nehaljwani commented on a change in pull request #524: ZSTD_STATIC_LIB_NAME: Avoid double .a extensions
Posted by GitBox <gi...@apache.org>.
nehaljwani commented on a change in pull request #524:
URL: https://github.com/apache/orc/pull/524#discussion_r468267702
##########
File path: cmake_modules/FindZSTD.cmake
##########
@@ -24,7 +24,7 @@ endif()
message (STATUS "ZSTD_HOME: ${ZSTD_HOME}")
if (NOT ZSTD_STATIC_LIB_NAME)
- set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd)
+ set (ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX})
Review comment:
It doesn't, however, depending on the order of items in the value of `CMAKE_FIND_LIBRARY_SUFFIXES`, it can pick up the shared library here (`libzstd.so`) and then fail later, while linking statically. IMO we should close this PR and open an issue to support linking third party dependency dynamically and statically, both.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org