Allow linking to a system copy of cppgir

[mid-kid]

https://github.com/gentoo/gentoo/blob/master/net-im/telegram-desktop/files/tdesktop-4.8.3-system-cppgir.patch

Currently, this repository bundles cppgir, but this is unnecessary. cppgir provides a cmake config as well as a pkg-config file, and can be queried for its presence in the system.

Unfortunately it doesn't have tagged release versions yet, but it's now packaged by both arch and gentoo.

[ilya-fedin]

cppgir's cmake config sadly lacks a target for its most important part: the codegen utility.
It also lacks target aliases so the targets could be referred the same way in both bundled and installed variants.
pkg-config, IIRC, supports only libraries, so is not applicable at all.

[ilya-fedin]

I also believe it will have source incompatible changes (there's fortunately no binary compatibility as it's a codegen).

I have various problems I reported, the solved ones have already resulted in changes of some generated functions signature and I believe I will encounter more & will report them as well, fastly updating the bundled commit as soon as the issues are fixed with committing new functionality relying on the fixes.

Distros are slow and will periodically create issues as they can't move as fast as we need but we need this functionality as we have to deal with more & more complicated D-Bus specs what requires more & more higher-level abstractions for the code to stay maintainable. Thus I believe it's better to keep the dependency bundled, to save both distro maintainers' and developers' mental health.

[mid-kid]

cppgir's cmake config sadly lacks a target for its most important part: the codegen utility.

You generally don't use targets for executables, because aside from their location, there's really nothing to configure or track about them. Consider using find_program(). I'm not sure what the point about target aliases is about.

The rest is fair, and mostly up to how you see your relationship with the tool. I'm really not aware about its development status beyond its superficial bits, but in general I see it as a necessity to include such libraries and utilities in the distribution, so dependents other than telegram can begin using them as well.

Version compatibility is a hard thing. Of course this is really hard to define when a tool is still in its baby phase, but having to update everything that depends on the tool every time something changes becomes a significant cost after a while, and changes tend to slow down. I don't expect the tool to remain in a constant flow of breakage forever.

[ilya-fedin]

You generally don't use targets for executables

I'd disagree, it's a common practice to have targets for executables in cmake, Qt provides them for all their tools and tdesktop uses e.g. Qt::rcc. Also, the generate_cppgir function uses cppgir target currently.

Consider using find_program().

I'd like to avoid this, targets can abstract the packaged and bundled case requiring zero conditions in desktop-app code (with the only exception of the add_subdirectory/find_package).

[mid-kid]

Nothing wrong with just creating a target here downstream for this purpose, or even proposing one to upstream, I just don't fully see the necessity. The cppgir target (and binary) is only used in cmake_helpers anyway.

[ilya-fedin]

Well, I don't feel comfortable creating it downstream and polluting the logic with packaged/non-packaged conditions even more, especially given that targets allow to not to have such conditions at all outside of the package import place. I don't have any interest to work on that atm though, including committing the necessary changes upstream. Although I will be happy to review if one would like to implement such a change.

[ilya-fedin]

I've made an issue about that for cppgir: https://gitlab.com/mnauw/cppgir/-/issues/42
The aliases should be trivial one-line changes, the export of the tool might be trivial or might require some additional changes, depends on their cmake code.

[mid-kid]

That sounds reasonable, Thanks a lot for looking into this!

[ilya-fedin]

With current master branches of cppgir and cmake_helpers it should be as easy as

diff --git a/external/glib/CMakeLists.txt b/external/glib/CMakeLists.txt
index 2966200..69b36ad 100644
--- a/external/glib/CMakeLists.txt
+++ b/external/glib/CMakeLists.txt
@@ -7,13 +7,23 @@
 add_library(external_glib INTERFACE IMPORTED GLOBAL)
 add_library(desktop-app::external_glib ALIAS external_glib)
 
-function(add_cppgir) # isolate scope
-    set(BUILD_TESTING OFF)
-    set(BUILD_DOC OFF)
-    set(BUILD_EXAMPLES OFF)
-    add_subdirectory(cppgir EXCLUDE_FROM_ALL)
-endfunction()
-add_cppgir()
+if (DESKTOP_APP_USE_PACKAGED)
+    if (DESKTOP_APP_USE_PACKAGED_LAZY)
+        find_package(CppGir QUIET)
+    else()
+        find_package(CppGir)
+    endif()
+endif()
+
+if (NOT CppGir_FOUND)
+    function(add_cppgir) # isolate scope
+        set(BUILD_TESTING OFF)
+        set(BUILD_DOC OFF)
+        set(BUILD_EXAMPLES OFF)
+        add_subdirectory(cppgir EXCLUDE_FROM_ALL)
+    endfunction()
+    add_cppgir()
+endif()
 
 include(generate_cppgir.cmake)
 generate_cppgir(external_glib Gio-2.0)

in theory, although I have no environment to check that.

[mid-kid]

Awesome! I'll make sure to test this once I find time to upgrade telegram.

[mymedia2]

@ilya-fedin
I am afraid your patch from the latest post does not work. Line find_package(CppGir) should be in the generate_cppgir.cmake file so the generate_cppgir function can find the imported targets.
Compare with my solution https://github.com/desktop-app/cmake_helpers/pull/305/files

[ilya-fedin]

I think we're not going to provide the possibility to use system cppgir. Debian is patching tdesktop to use system cppgir and is already providing bad UX to end users so that their users clutter upstream issues (telegramdesktop/tdesktop#26757 (comment)). I definitely don't want more distros to follow that.

[ilya-fedin]

@mymedia2 sorry but I can't review your change (which has problems) and I don't even get notifications for your messages because you've blocked me. You should unblock me to get your PRs reviewed & possibly merged.