Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow linking to a system copy of cppgir #282

Closed
mid-kid opened this issue Jun 10, 2023 · 13 comments · May be fixed by #305
Closed

Allow linking to a system copy of cppgir #282

mid-kid opened this issue Jun 10, 2023 · 13 comments · May be fixed by #305

Comments

@mid-kid
Copy link
Contributor

mid-kid commented Jun 10, 2023

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
Copy link
Contributor

ilya-fedin commented Jun 11, 2023

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
Copy link
Contributor

ilya-fedin commented Jun 11, 2023

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
Copy link
Contributor Author

mid-kid commented Jun 12, 2023

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
Copy link
Contributor

ilya-fedin commented Jun 12, 2023

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
Copy link
Contributor Author

mid-kid commented Jun 12, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

mid-kid commented Aug 2, 2023

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

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Aug 6, 2023

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
Copy link
Contributor Author

mid-kid commented Aug 7, 2023

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

@mymedia2
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

ilya-fedin commented Oct 12, 2023

@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.

@john-preston john-preston closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2023
mymedia2 added a commit to mymedia2/telegram-cmake_helpers that referenced this issue Jan 8, 2024
mymedia2 added a commit to mymedia2/telegram-cmake_helpers that referenced this issue Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants