yuzu icon indicating copy to clipboard operation
yuzu copied to clipboard

build: simplify find modules

Open Tachi107 opened this issue 1 year ago • 10 comments

With this patch I've deleted a few find modules that are now unused since the vcpkg transition, as the CMake code now forces CONFIG mode for Catch2, fmt and nlohmann_json.

I've then simplified the lz4, opus, and zstd modules by exclusively using pkg-config. They were using it already, but were ignoring the result. Also, I believe that manually looking for libraries was required for Conan to work, and it is thus not needed anymore.

Lastly, I believe that there is no platform that ships these system libs without pkg-config/pkgconf, so requiring it should be fine.

Tachi107 avatar Jul 28 '22 21:07 Tachi107

I already did this in https://github.com/yuzu-emu/yuzu/pull/6833.

abouvier avatar Jul 29 '22 12:07 abouvier

Oh ok @abouvier, I did not see it. Clean ups are a bit unrelated to unbundling dependencies though, do you think it makes sense to split these changes in these two different PRs? It'll make reviewing easier for the yuzu team.

Tachi107 avatar Jul 29 '22 16:07 Tachi107

I don't know, whatever is merged first :p

But a few remarks:

  • Alias of non-global imported library is only supported since cmake 3.18, hence the IMPORTED_TARGET GLOBAL in pkg_search_module, because the build environment of yuzu is on cmake 3.16
  • The content of the first variable listed in REQUIRED_VARS will be displayed to the user, so if you use xXx_FOUND the message will be: Found lz4: 1 (found suitable version "1.9.3", minimum required is "1.8") instead of Found lz4: /usr/lib/liblz4.so (found suitable version "1.9.3", minimum required is "1.8") for example with xXx_LINK_LIBRARIES.
  • The prefix used in pkg_search_module and find_package_handle_standard_args cannot be the same or some variables generated by cmake will conflict.

abouvier avatar Jul 29 '22 18:07 abouvier

I had replied to this more than one hour ago via email, but for some reason GitHub dropped my reply...

  • Alias of non-global imported library is only supported since cmake 3.18, hence the IMPORTED_TARGET GLOBAL in pkg_search_module, because the build environment of yuzu is on cmake 3.16

Right, thanks, I forgot about this detail.

  • The content of the first variable listed in REQUIRED_VARS will be displayed to the user, so if you use xXx_FOUND the message will be: Found lz4: 1 (found suitable version "1.9.3", minimum required is "1.8") instead of Found lz4: /usr/lib/liblz4.so (found suitable version "1.9.3", minimum required is "1.8") for example with xXx_LINK_LIBRARIES.

Oh, I missed it, as the documentation says this in the examples section... Thanks for letting me know, I was quite puzzled by that 1 :)

  • The prefix used in pkg_search_module and find_package_handle_standard_args cannot be the same or some variables generated by cmake will conflict.

Could you be more specific?

Tachi107 avatar Jul 29 '22 20:07 Tachi107

  • The prefix used in pkg_search_module and find_package_handle_standard_args cannot be the same or some variables generated by cmake will conflict.

Could you be more specific?

If you use both pkg_search_module(lz4 ...) and find_package_handle_standard_args(lz4 ... (same lowercase prefix), pkg_search_module will create the variable lz4_VERSION even if the package is not found (with empty value in this case). Then find_package_handle_standard_args will produce this message: Could NOT find lz4: Found unsuitable version "", but required is at least "1.8" (found ) instead of: Could NOT find lz4 (missing: DIFFERENT_PREFIX_LINK_LIBRARIES) (Required is at least version "1.8").

abouvier avatar Jul 30 '22 02:07 abouvier

Oh, nice to know, thanks. CMake really is quirky...

Tachi107 avatar Jul 30 '22 10:07 Tachi107

If you use both pkg_search_module(lz4 ...) and find_package_handle_standard_args(lz4 ... (same lowercase prefix), pkg_search_module will create the variable lz4_VERSION even if the package is not found (with empty value in this case). Then find_package_handle_standard_args will produce this message: Could NOT find lz4: Found unsuitable version "", but required is at least "1.8" (found ) instead of: Could NOT find lz4 (missing: DIFFERENT_PREFIX_LINK_LIBRARIES) (Required is at least version "1.8").

Uhm... It seems that changing the pkg_search_module prefix doesn't help with the Opus findmodule.

I have this in FindOpus.cmake (yes, I've renamed it from Findopus.cmake):

find_package(PkgConfig)

if (PKG_CONFIG_FOUND)
    pkg_search_module(opus_pc IMPORTED_TARGET GLOBAL opus)
    if (opus_pc_FOUND)
        add_library(Opus::opus ALIAS PkgConfig::opus_pc)
    endif()
endif()

message("opus_pc_FOUND: ${opus_pc_FOUND}")
message("opus_pc_LINK_LIBRARIES: ${opus_pc_LINK_LIBRARIES}")
message("opus_pc_VERSION: ${opus_pc_VERSION}")

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(Opus
    REQUIRED_VARS
        opus_pc_LINK_LIBRARIES
        opus_pc_FOUND
    VERSION_VAR opus_pc_VERSION
)

And here's the error message I get if opus.pc is not installed on my system:

-- Checking for one of the modules 'opus'
opus_pc_FOUND: 
opus_pc_LINK_LIBRARIES: 
opus_pc_VERSION: 
CMake Error at /usr/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Opus: Found unsuitable version "", but required is at least
  "1.3" (found )
Call Stack (most recent call first):
  /usr/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:592 (_FPHSA_FAILURE_MESSAGE)
  externals/find-modules/FindOpus.cmake:18 (find_package_handle_standard_args)
  externals/CMakeLists.txt:131 (find_package)

Do you have an idea of what is going on?

Tachi107 avatar Jul 30 '22 10:07 Tachi107

I guess that the variable listed in VERSION_VAR must be non existent if the library is not found. Here opus_pc_VERSION is still created with empty value. You should create the variable Opus_VERSION (with value of opus_pc_VERSION) after the alias library and use this one in VERSION_VAR. Every FindXxx.cmake module should create a Xxx_VERSION variable on success anyway.

abouvier avatar Jul 30 '22 14:07 abouvier

I guess that the variable listed in VERSION_VAR must be non existent if the library is not found. Here opus_pc_VERSION is still created with empty value. You should create the variable Opus_VERSION (with value of opus_pc_VERSION) after the alias library and use this one in VERSION_VAR. Every FindXxx.cmake module should create a Xxx_VERSION variable on success anyway.

Uhm, this sounds like working around CMake's quirky behaviour instead of fixing it. I've submitted a bug report upstream, to see what they think about this. I might try to write a patch afterwards.

The bug report is here: https://gitlab.kitware.com/cmake/cmake/-/issues/23807

Tachi107 avatar Jul 31 '22 21:07 Tachi107

As this looks to me like a CMake quirk and since it doesn't create actual issues to the build process, I'll avoid making the code more confusing just to work around it (unless upstream CMake says that this is working as intended).

This is now ready to be merged :)

Tachi107 avatar Aug 01 '22 10:08 Tachi107

The CMake quick has been fixed: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7527

Tachi107 avatar Sep 09 '22 14:09 Tachi107

Alias of non-global imported library is only supported since cmake 3.18, hence the IMPORTED_TARGET GLOBAL in pkg_search_module, because the build environment of yuzu is on cmake 3.16

Right, thanks, I forgot about this detail.

The content of the first variable listed in REQUIRED_VARS will be displayed to the user, so if you use xXx_FOUND the message will be: Found lz4: 1 (found suitable version "1.9.3", minimum required is "1.8") instead of Found lz4: /usr/lib/liblz4.so (found suitable version "1.9.3", minimum required is "1.8") for example with xXx_LINK_LIBRARIES.

Oh, I missed it, as the documentation says this in the examples section... Thanks for letting me know, I was quite puzzled by that 1 :)

The prefix used in pkg_search_module and find_package_handle_standard_args cannot be the same or some variables generated by cmake will conflict.

Could you be more specific?

Tachi107 avatar Oct 11 '22 09:10 Tachi107