facebook / zstd

Zstandard - Fast real-time compression algorithm
http://www.zstd.net
Other
22.77k stars 2.03k forks source link

Make CMake official? (Makefile build does not provide CMake config file) #3271

Open MaskRay opened 1 year ago

MaskRay commented 1 year ago

README.md says

make is the officially maintained build system of this project.

When using Makefile, CMake config files like zstdConfig.cmake is not installed. This makes projects using CMake awkward to use zstd. E.g. llvm-project has

// https://github.com/llvm/llvm-project/blob/main/llvm/cmake/config-ix.cmake
if(LLVM_ENABLE_ZSTD)
  if(LLVM_ENABLE_ZSTD STREQUAL FORCE_ON)
    find_package(zstd REQUIRED)
    if(NOT zstd_FOUND)
      message(FATAL_ERROR "Failed to configure zstd, but LLVM_ENABLE_ZSTD is FORCE_ON")
    endif()
  elseif(NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
    find_package(zstd QUIET)
  endif()
endif()
set(LLVM_ENABLE_ZSTD ${zstd_FOUND})

// https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/CMakeLists.txt#L28
if(LLVM_ENABLE_ZSTD)
  if(TARGET zstd::libzstd_shared AND NOT LLVM_USE_STATIC_ZSTD)
    set(zstd_target zstd::libzstd_shared)
  else()
    set(zstd_target zstd::libzstd_static)
  endif()
endif()

It could add pkg-config fallback but that is inconvenient, and logic like zstd::libzstd_shared does not have a good replacement.

Related:

The simplest solution is to make CMake official so downstream is motivated to switch to CMake.

Tachi107 commented 1 year ago

The simplest solution is to make CMake official so downstream is motivated to switch to CMake.

I instead think that the simplest solution is to use the pkg-config file, as it gets generated regardless of the build system used to build libzstd, and can be easily used by both CMake and non-CMake users

MaskRay commented 1 year ago

It's unclear how pkg-config works on non-Unix-like systems.

I am not a CMake export, but using a native CMake config file seems to have some advantage that users prefer it instead of CMake's pkg-config support. See https://github.com/PCSX2/pcsx2/blob/master/cmake/SearchForStuff.cmake#L212 , gdal, qt6-base, sprag, etc.

Tachi107 commented 1 year ago

It's unclear how pkg-config works on non-Unix-like systems.

I've never used pkg-config/pkgconf on Windows, but I guess that it works just like it does on Unix-like OSes - it reads a .pc file, it extracts a couple of compiler flags and it passes them to the compiler; it's not Unix or GCC specific.

Regardless of how it works, I doubt it's useful on Windows anyway; it doesn't have a package manager and you'll almost never link to system libraries.

Lastly, you're not forced to use either CMake Config or pkg-config files, you can use both. Here's a permissively licensed find module that uses zstd's CMake Config file if available, and falls back to pkg-config otherwise: https://github.com/cemu-project/Cemu/blob/9b76b0e2d33725d37dc8c6137d8ea2fa9b991542/cmake/Findzstd.cmake

MaskRay commented 1 year ago

If CMake find_package(zstd) doesn't just work, the Cemu solution has to be duplicated by all zstd' CMake users. This is exactly the situation we want to avoid. (For zlib, libxml2, curl, and other nearly-always-enabled-on-Linux depndencies llvm-project has, no pkg-config related CMake code is needed.)

It seems that the zstd upstream is in a good position to solve the problem since a CMake build is available. Alternatively, ship CMake config files in the Makefile build. That'll decrease friction for zstd' CMake users as well.

Perhaps the least zstd can do is to recommend that Linux distributions uses CMake instead of GNU make. That'll solve the problem as well.

mathstuf commented 1 year ago

The make build could generate a FindZSTD that chains off to pkg-config. It should provide the same set of targets and variables as zstdConfig.cmake (though pkg_config will undoubtedly leak some more, keeping the documented interface the same should help a lot).

eli-schwartz commented 1 year ago

Alternatively, ship CMake config files in the Makefile build. That'll decrease friction for zstd' CMake users as well.

This is a nice idea, but it's unclear how to actually do this considering the turing tarpit complexity of the file format of what is otherwise, literally, a key/value *.pc.

I really would be fascinated to hear more about this option though. It would be handy for the meson build as well.

...

Speaking of which, it is incredibly frustrating that e839b31dc4eb0f654b0cc47440df1d9377d1815b added initial cmake config support using a different name than the pkg-config file that predated it by half a decade. If the cmake interface was find_package(libzstd) then Meson would be able to seamlessly find either one, whichever is available.

:thinking: Is there some way to install a "zstd" config that prints a warning, then loads "libzstd" instead?

mathstuf commented 1 year ago

Is there some way to install a "zstd" config that prints a warning, then loads "libzstd" instead?

Yes.

message(AUTHOR_WARNING # or DEPRECATED
  "`find_package(zstd)` is deprecated; please use `find_package(libzstd)`")

find_package(libzstd) # forward args like `QUIET`, `REQUIRED`, `COMPONENTS`, etc.

set(zstd_FOUND "${libzstd_FOUND}")
# Any target compat that is required.
mathstuf commented 1 year ago

it's unclear how to actually do this considering the turing tarpit complexity of the file format of what is otherwise, literally, a key/value *.pc.

It can be more-or-less declarative. It involves creating targets and setting properties on those targets. Prefix detection is imperative, but…minor. It certainly shouldn't involve loops nevermind open-ended loops in the common or even uncommon case.

bsergean commented 1 year ago

fyi moving the cmake files to the top level folder, and just changing one variable (that set the top level) is sufficient to have 'normal' behavior. Make a pr for that.

Cyan4973 commented 1 year ago

Follow up from #3311 : we are opened to the idea of featuring a single cmake file at root that redirects to the full project under build/cmake/.

v-fox commented 1 year ago

Comically, meson build also does not provide cmake-files (and static lib) while cmake build does not provide gzip compatibility. So, with 3 build systems none is feature-complete. I ended up doing double build with cmake and meson for a distro package to get both gzip/lz support and cmake-files & static lib for silly apps that depend on them.

eli-schwartz commented 1 year ago

The cmake files aren't an official interface (per the Makefile), but meson does provide the static library. Use meson configure to see the options, and use the -Ddefault_library=both option to build static and shared libraries at the same time.

MaskRay commented 3 months ago

https://bugs.gentoo.org/872254 mentions some packages that expect zstd cmake: pcsx2, sci-libs/miopen-6.0.2

eli-schwartz commented 3 months ago

It also notes that pcsx2 bundles their own zstd copy via cmake hence why they expect the bundled copy to provide the relevant target.

It's also outdated. Since https://github.com/PCSX2/pcsx2/commit/0784b5930be95fdfffa412d9ea65510e0a28324c that project doesn't look for the cmake config file here, they use their own FindZstd.cmake that does find_library().

(Yes, with a capital Z.)

eli-schwartz commented 3 months ago

Seems miopen does the same: https://github.com/ROCm/MIOpen/commit/839249db9286fd6ec102ae5f9bb93d7f9a7cd0b2

They use an internal Findzstd.cmake, at least...

ionenwks commented 3 months ago

It also notes that pcsx2 bundles their own zstd copy via cmake hence why they expect the bundled copy to provide the relevant target.

It's also outdated. Since PCSX2/pcsx2@0784b59 that project doesn't look for the cmake config file here, they use their own FindZstd.cmake that does find_library().

(Yes, with a capital Z.)

Yes pcsx2 went a bit back & forth with this, formerly they were bundling the entire library and had a generic wrapper to use system's that did find_package() for every libraries thus needed it for zstd too. Later they removed the wrapping and (always) forced all bundled libraries, and then finally that commit switched to system-only (Edit: well, on linux/mac, still bundled for windows) for a few libraries including zstd and added their own FindZstd.cmake.