Fix zstd detection when zstd_FOUND is true but target is missing #2334
Conversation
|
@sum01 @jimmy-park @Tachi107 does the pr look good for you? |
|
This doesn't look right to me. What exactly is GitHub actions doing
that's causing this issue?
|
|
This isn’t caused by the build environment itself, but by differences in how zstd is discovered across systems. On some systems, zstd provides an imported target named zstd::zstd rather than zstd::libzstd (for example, the Findzstd.cmake used by RocksDB: https://github.com/facebook/rocksdb/blob/main/cmake/modules/Findzstd.cmake In those cases, find_package(zstd) sets zstd_FOUND to true, but zstd::libzstd is not defined. For this reason, checking whether zstd::libzstd exists is more reliable than relying on zstd_FOUND alone. |
|
On Wed Jan 21, 2026 at 10:17 PM CET, TH wrote:
On some systems, zstd provides an imported target named zstd::zstd
rather than zstd::libzstd (for example, the Findzstd.cmake used by
RocksDB:
https://github.com/facebook/rocksdb/blob/main/cmake/modules/Findzstd.cmake
).
Here RocksDB's Findzstd module is *creating* a target named zstd::zstd.
If I recall correctly, libzstd itself never creates such a target.
In those cases, find_package(zstd) sets zstd_FOUND to true, but
zstd::libzstd is not defined. For this reason, checking whether
zstd::libzstd exists is more reliable than relying on zstd_FOUND
alone.
find_package(zstd) will look for a CMake Config file created by libzstd.
These config files only exist if libzstd has been built with CMake
instead of GNU Make, the latter being the official build system.
When built with GNU Make, libzstd only ships a pkg-config file, which is
the official way of consuming the project (hence the fallback to using
pkg_check_modules).
All this to say: zstd::libzstd should always be present if zstd_FOUND is
defined. If it isn't, there's probably a bug somewhere else. For
example, you might be defining a Findzstd.cmake module which gets used
instead of the upstream Config file. So, you'd end up finding libzstd
with that Find module, only to discard the results because the target
name is not the expected one.
Really, all this is just a consequence of CMake's crappy design (why
can't I just do myzstd_dep = find_package(zstd) and be done with it?).
Still, a more appropriate solution would be to make sure that *only* the
upstream Config file is run, instead of any custom Find module you may
have in your CMake module path. To do so, you can just add the `CONFIG`
keyword to the `find_package()` call.
What do you think about it?
|
|
Using find_package(zstd CONFIG) still fails to compile in my case. zstd::libzstd_static zstd::libzstd_shared The zstd::libzstd alias target was only added in zstd v1.5.6, as seen here: Therefore, even when zstd_FOUND is defined and the upstream Config file is used, zstd::libzstd may legitimately not exist, depending on the zstd version provided by the system. |
|
On Thu Jan 22, 2026 at 2:25 AM CET, TH wrote:
Using find_package(zstd CONFIG) still fails to compile in my case.
After inspecting zstdTargets.cmake, I found that in the default zstd
package shipped with Ubuntu 24.04 (zstd v1.5.5), there is no
zstd::libzstd target. Only the following targets are defined:
zstd::libzstd_static
zstd::libzstd_shared
The zstd::libzstd alias target was only added in zstd v1.5.6
Got it. I forgot about this issue in particular, but now that you
mention it I remember again. Still, the `<name>_FOUND` variable is
pretty standard, and letting it defined while considering the package
not found may cause other issues down the line, due to mismatched
expectations.
What about using `find_packge(zstd CONFIG VERSION 1.5.6)`? This, to me,
has some advantages. By specifying `CONFIG`,
- The lookup is marginally faster
- Error messages in case the library is not found are clearer
And, with `VERSION`, the library is only found if the expected target
name is guaranteed to exist.
Also, adding `REQUIRED` to the `find_package()` call should not be done,
as that will prevent the official pkg-config file to be used.
Does this solve your issue?
|
|
Agreed — this makes sense to me. |
|
The refreshed patch looks good to me :)
|
|
@Theresa-0328 @Tachi107 thanks for taking care of this pull request. Is it ready to merge? |
|
On Thu Jan 22, 2026 at 11:41 PM CET, yhirose wrote:
Is it ready to merge?
Yep
|
|
Merged. Thanks for the fine contribution! |
3 participants
Some zstd installations (e.g. GitHub Actions ubuntu-24.04) set zstd_FOUNDwithout providing a zstd::libzstd target, causing invalid target_link_libraries().Use imported target presence to detect zstd usage instead.
Fixes #2313