Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VL] Fix wrong lib suffix for google_cloud_cpp_storage #7933

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented Nov 13, 2024

What changes were proposed in this pull request?

Currently, static google_cloud_cpp_storage lib is used in both static build and dynamic build. The lib suffix was wrongly set for CMake to find the lib.

How was this patch tested?

Local verification.

@github-actions github-actions bot added the VELOX label Nov 13, 2024
Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@PHILO-HE
Copy link
Contributor Author

@zhztheplayer, just verified, we should set ".a" as the suffix for GCS.

@PHILO-HE PHILO-HE merged commit fc549ee into apache:main Nov 14, 2024
47 checks passed
@majetideepak
Copy link
Collaborator

@PHILO-HE, @zhztheplayer This change seems to be breaking our build since it expects a static libcurl.a as well.
Is this expected?

[2024-12-13T11:30:19.159Z] #11 1586.8 CMake Error at /usr/local/lib64/python3.9/site-packages/cmake/data/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
[2024-12-13T11:30:19.159Z] #11 1586.8   Could NOT find CURL (missing: CURL_LIBRARY) (found version "7.76.1")
[2024-12-13T11:30:19.159Z] #11 1586.8 Call Stack (most recent call first):
[2024-12-13T11:30:19.159Z] #11 1586.8   /usr/local/lib64/python3.9/site-packages/cmake/data/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
[2024-12-13T11:30:19.159Z] #11 1586.8   /usr/local/lib64/python3.9/site-packages/cmake/data/share/cmake-3.28/Modules/FindCURL.cmake:194 (find_package_handle_standard_args)
[2024-12-13T11:30:19.159Z] #11 1586.8   /usr/local/lib64/python3.9/site-packages/cmake/data/share/cmake-3.28/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
[2024-12-13T11:30:19.159Z] #11 1586.8   /usr/local/lib64/cmake/google_cloud_cpp_rest_internal/google_cloud_cpp_rest_internal-config.cmake:18 (find_dependency)
[2024-12-13T11:30:19.159Z] #11 1586.8   /usr/local/lib64/python3.9/site-packages/cmake/data/share/cmake-3.28/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
[2024-12-13T11:30:19.159Z] #11 1586.8   /usr/local/lib64/cmake/google_cloud_cpp_storage/google_cloud_cpp_storage-config.cmake:16 (find_dependency)
[2024-12-13T11:30:19.159Z] #11 1586.8   velox/CMakeLists.txt:124 (find_package)
[2024-12-13T11:30:19.159Z] #11 1586.8   velox/CMakeLists.txt:329 (find_gcssdk)

@FelixYBW
Copy link
Contributor

@PHILO-HE, @zhztheplayer This change seems to be breaking our build since it expects a static libcurl.a as well. Is this expected?

Yes, the PR is to link libcurl.a always no matter you use vcpkg on or off.

@majetideepak
Copy link
Collaborator

majetideepak commented Dec 13, 2024

@FelixYBW Why do we need to enforce set(CMAKE_FIND_LIBRARY_SUFFIXES ".a") here?
Can a user not control what library (static vs shared) to build when building the google_cloud_cpp_storage package?
Can we remove this enforcement in the CMakeLists.txt here? It will help users like me who want to use static google_cloud_cpp_storage but dynamic libcurl. Thanks.

@PHILO-HE
Copy link
Contributor Author

Hi @majetideepak, I get your point. Just created one pr to try: #8251.
BTW, we are promoting static build, where vcpkg is leveraged to use static dependencies for gluten native lib. Maybe, you can try to use it.

@majetideepak
Copy link
Collaborator

majetideepak commented Dec 17, 2024

@PHILO-HE Static build is fine to some extent but forcing standard shared libraries such as libcurl, openssl to be statically linked is tricky.
Some library could be expecting libcurl to be available dynamically and if we statically link a different libcurl version in a different library, it can introduce ODR violations which are tricky to fix. We have to track every library used for this.
Openssl has fips issue as reported here #8232

@PHILO-HE
Copy link
Contributor Author

@PHILO-HE Static build is fine to some extent but forcing standard shared libraries such as libcurl, openssl to be statically linked is tricky. Some library could be expecting libcurl to be available dynamically and if we statically link a different libcurl version in a different library, it can introduce ODR violations which are tricky to fix. We have to track every library used for this. Openssl has fips issue as reported here #8232

@majetideepak, I see. We didn't realize this issue before. Considering what you mentioned, some libs should be dynamically linked even if static build is used for Gluten. We should adjust our target of static build. cc @FelixYBW

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants