improve: allow pkgconfig fallback on Linux#1670
Open
grepwood wants to merge 2 commits intoopentibiabr:mainfrom
Open
improve: allow pkgconfig fallback on Linux#1670grepwood wants to merge 2 commits intoopentibiabr:mainfrom
grepwood wants to merge 2 commits intoopentibiabr:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCMakeLists.txt now conditionally resolves dependencies based on VCPKG_TARGET_TRIPLET: when defined it uses find_package(), otherwise it requires PkgConfig and uses pkg_check_modules() to discover inih, Vorbis/Ogg, and GLEW, creating alias/imported targets to expose unified targets. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/CMakeLists.txt (1)
190-207: Consider usingIMPORTED_TARGETfor consistency with the INIReader pattern.The INIReader fallback (line 149) uses
IMPORTED_TARGETand then creates an alias, which is cleaner. The same pattern could be applied here for consistency and to reduce boilerplate.♻️ Suggested refactor using IMPORTED_TARGET
- pkg_check_modules(VORBISFILE REQUIRED vorbisfile) - add_library(Vorbis::vorbisfile INTERFACE IMPORTED) - set_target_properties(Vorbis::vorbisfile PROPERTIES - INTERFACE_INCLUDE_DIRECTORIES "${VORBISFILE_INCLUDE_DIRS}" - INTERFACE_LINK_LIBRARIES "${VORBISFILE_LIBRARIES}" - ) - pkg_check_modules(VORBIS REQUIRED vorbis) - add_library(Vorbis::vorbis INTERFACE IMPORTED) - set_target_properties(Vorbis::vorbis PROPERTIES - INTERFACE_INCLUDE_DIRECTORIES "${VORBIS_INCLUDE_DIRS}" - INTERFACE_LINK_LIBRARIES "${VORBIS_LIBRARIES}" - ) - pkg_check_modules(OGG REQUIRED ogg) - add_library(Ogg::ogg INTERFACE IMPORTED) - set_target_properties(Ogg::ogg PROPERTIES - INTERFACE_INCLUDE_DIRECTORIES "${OGG_INCLUDE_DIRS}" - INTERFACE_LINK_LIBRARIES "${OGG_LIBRARIES}" - ) + pkg_check_modules(VORBISFILE REQUIRED IMPORTED_TARGET vorbisfile) + add_library(Vorbis::vorbisfile ALIAS PkgConfig::VORBISFILE) + pkg_check_modules(VORBIS REQUIRED IMPORTED_TARGET vorbis) + add_library(Vorbis::vorbis ALIAS PkgConfig::VORBIS) + pkg_check_modules(OGG REQUIRED IMPORTED_TARGET ogg) + add_library(Ogg::ogg ALIAS PkgConfig::OGG)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CMakeLists.txt` around lines 190 - 207, Replace the direct add_library(... INTERFACE IMPORTED) entries for Vorbis::vorbisfile, Vorbis::vorbis and Ogg::ogg with the same IMPORTED_TARGET + alias pattern used by INIReader: create a private IMPORTED target (e.g., vorbisfile::imported, vorbis::imported, ogg::imported) via add_library(<name> IMPORTED GLOBAL), set its INTERFACE_INCLUDE_DIRECTORIES and INTERFACE_LINK_LIBRARIES with the corresponding ${VORBISFILE_INCLUDE_DIRS}/${VORBISFILE_LIBRARIES}, ${VORBIS_INCLUDE_DIRS}/${VORBIS_LIBRARIES}, ${OGG_INCLUDE_DIRS}/${OGG_LIBRARIES}, then create the public alias targets Vorbis::vorbisfile, Vorbis::vorbis and Ogg::ogg using add_library(Vorbis::vorbisfile ALIAS vorbisfile::imported) (and likewise for the others) to match the INIReader pattern and reduce boilerplate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CMakeLists.txt`:
- Line 811: Remove the mistaken use of the CMake imported target name
"GLEW::GLEW" from the target_include_directories call;
target_include_directories expects directory paths, not target names, and GLEW's
INTERFACE_INCLUDE_DIRECTORIES are already propagated when you link to it with
target_link_libraries (the existing link to GLEW::GLEW). Locate the call to
target_include_directories that currently lists GLEW::GLEW and delete that entry
so only real directory paths remain (or remove the entire
target_include_directories call if it only contained GLEW::GLEW), leaving the
existing target_link_libraries invocation to provide the include paths.
- Around line 146-151: The CMake logic uses pkg_check_modules when
VCPKG_TARGET_TRIPLET is not defined but never ensures the PkgConfig module is
loaded; add a find_package(PkgConfig REQUIRED) call before invoking
pkg_check_modules (either immediately above the existing conditional or inside
the else branch) so that pkg_check_modules is available; update the block around
VCPKG_TARGET_TRIPLET / pkg_check_modules /
add_library(unofficial::inih::inireader ALIAS PkgConfig::INIReader) accordingly.
---
Nitpick comments:
In `@src/CMakeLists.txt`:
- Around line 190-207: Replace the direct add_library(... INTERFACE IMPORTED)
entries for Vorbis::vorbisfile, Vorbis::vorbis and Ogg::ogg with the same
IMPORTED_TARGET + alias pattern used by INIReader: create a private IMPORTED
target (e.g., vorbisfile::imported, vorbis::imported, ogg::imported) via
add_library(<name> IMPORTED GLOBAL), set its INTERFACE_INCLUDE_DIRECTORIES and
INTERFACE_LINK_LIBRARIES with the corresponding
${VORBISFILE_INCLUDE_DIRS}/${VORBISFILE_LIBRARIES},
${VORBIS_INCLUDE_DIRS}/${VORBIS_LIBRARIES},
${OGG_INCLUDE_DIRS}/${OGG_LIBRARIES}, then create the public alias targets
Vorbis::vorbisfile, Vorbis::vorbis and Ogg::ogg using
add_library(Vorbis::vorbisfile ALIAS vorbisfile::imported) (and likewise for the
others) to match the INIReader pattern and reduce boilerplate.
majestyotbr
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary:
This PR concerns the consumption of the following libraries:
On most Linux distributions these libraries are provided without CMake package configuration files, which means that on Linux, projects normally consume them via pkgconfig. vcpkg is the only package manager I know that provides CMake package configuration files for these libraries and with OTClient's extensive reliance on vcpkg we can now see why OTClient is written with the assumption that these files exist. This assumption falls apart on pretty much every Linux distribution when vcpkg is not used.
This PR introduces detection of vcpkg triplets in places where these libraries are loaded by CMake. The proposed changes accomodate both scenarios: when vcpkg is present, and when it isn't. That way we should not encounter a situation like in #1508 where CICD passes the build, but the build fails on a user's machine.
Issues fixed:
Motivation:
I want to package OTClient for Gentoo in my overlay: https://github.com/grepwood/timberlay
Context:
I can provide ebuilds for dependencies that are missing from Gentoo's main repository. But changes in affected libraries (dev-libs/inih, media-libs/libvorbis, media-libs/libogg, media-libs/glew) that would render this PR unnecessary are not welcome by maintainers of Gentoo Linux. The general argument was that CMake is not used to build them, but upon inspection I found out more substantial reasons to support that decision:
Dependencies:
On systems with vcpkg, nothing changes.
On systems without vcpkg, these libraries need to be installed through the system's package manager. Here is an example:
- media-libs/libvorbis
- media-libs/libogg
- media-libs/glew
- libvorbis-dev
- libogg-dev
- libglew-dev
- libvorbis-devel
- libogg-devel
- glew-devel
Behavior
Actual
OTClient's CMake setup expects that these libraries always ship CMake package configuration files which is not true on Linux distributions in general. This will happen on a system without vcpkg:
The same problem occurs with vorbis, vorbisfile, ogg and glew.
Expected
After applying this patch, source configuration should go down like this:
Fixes
Type of change
Please delete options that are not relevant.
Documentation update - not sure. This change is intended to accomodate package maintainers, which should rely on the distro's package manager instead of vcpkg. These changes do not break the intended way of compiling with vcpkg, but they do offer ways to compile without it as a fallback.
If documentation change is deemed necessary, then the list of packages from the example should be resolved per supported distro and included in the docs. Ubuntu should have the same packages as Debian.
How Has This Been Tested
In grepwood/timberlay@58d26bd I have added OTClient 4.0 with a similar patch grepwood/timberlay@58d26bd#diff-45871ba1da40e367a7028597e8c71485b94db0041b10bc69542667a6286d7239 that just isn't up to date as per the master branch of OTClient, due to OTClient now pulling in FreeType. Building the package passes - but the
src_installphase will differ in the future, because OTClient requires more for running than just the binary.Test Configuration:
Checklist
Summary by CodeRabbit