-
Notifications
You must be signed in to change notification settings - Fork 796
[SYCL] Fix zstd build when static zstd libraries are not found #15856
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
Conversation
| endif() | ||
|
|
||
| # If LLVM_USE_STATIC_ZSTD is specified, make sure we enable zstd only if static | ||
| # libraries are found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create a bug report and PR to upstream this change, if approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For upstreaming:
issue: llvm/llvm-project#113583
PR: llvm/llvm-project#113584
|
|
||
| # If LLVM_USE_STATIC_ZSTD is specified, make sure we enable zstd only if static | ||
| # libraries are found. | ||
| if(LLVM_USE_STATIC_ZSTD AND NOT TARGET zstd::libzstd_static) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry what is zstd::libzstd_static and who sets it? is it set by find_package if it finds the static lib? not a cmake expert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, zstd::libzstd_static is set during find_package if it finds static ZSTD libraries. https://github.com/intel/llvm/blob/sycl/llvm/cmake/modules/Findzstd.cmake#L60
sarnex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense to me, but lets also submit to upstream like you said
|
@intel/llvm-gatekeepers the PR is ready to be merged. |
Problem:
When we use
LLVM_ENABLE_ZSTD=ONalong withLLVM_USE_STATIC_ZSTD=ON, LLVM config fails with the following error when libzstd.a is not found but libzstd.so is found:The bug is in upstream and in the following lines: https://github.com/intel/llvm/blob/sycl/llvm/lib/Support/CMakeLists.txt#L32 - In the
else(), the code just assumes that static libraries are available.Solution:
When user specifies
LLVM_USE_STATIC_ZSTD=ON, we should enable ZSTD only if static zstd library is present. Otherwise, the build should just proceed normally since zstd is optional, even in upstream.