GNOME Bugzilla – Bug 711544
Rename backend library namespaces to be of the form ‘FolksSomething’
Last modified: 2018-09-21 16:06:20 UTC
Currently, the backend library namespaces are ‘tpf’, ‘swf’, etc. These are unhelpful, and don’t expose the fact that they’re _folks_ libraries. Furthermore, they require weird hacks (*-namespace.vala) to fix the namespaces for GIR files, and these hacks break when subclassing within the library. I just fixed an issue like that for the dummy backend. I suggest that when we next break API, we: • Rename all the backend library namespaces to be of the form ‘FolksSomething’ (e.g. ‘FolksTelepathy’ or ‘FolksEds’). • Add the ability to separately version each backend library so their sonames can be controlled separately.
Created attachment 259186 [details] [review] build: Build backend libraries before the backend modules themselves
Created attachment 259187 [details] [review] build: Use AS_HELP_STRING instead of the deprecated AC_HELP_STRING
Created attachment 259188 [details] [review] build: Update configure.ac as suggested by autoupdate Thanks, autoupdate.
Created attachment 259189 [details] [review] build: Remove unused variables from configure.ac
Created attachment 259190 [details] [review] build: Allow backend libraries to have separate LT versions from libfolks.so With the addition of the dummy backend, backend libraries (such as libfolks-dummy.so) no longer necessarily have the same stability guarantees as the core of libfolks. Consequently, they must have separate LT versions. These versions will be incremented in-line with the core LT version, but may also be incremented separately. They don’t affect whether libfolks will load a given backend module (that’s predicated on the core LT current version); merely whether client code needs to be re-linked against a given backend library due to an ABI-incompatible change in its backend-specific API.
Review of attachment 259186 [details] [review]: ++ It turns out this is actually a no-op - Automake generates depth-first makefiles in postfix order - but it's nice to be clear.
Review of attachment 259187 [details] [review]: ++
Review of attachment 259188 [details] [review]: ::: configure.ac @@ +31,3 @@ m4_define([folks_module_version], folks_lt_current) +AC_INIT([folks],[folks_version], Does removing the underquoting result in folks-folks_version.tar.gz or the correct folks-0.xyz.tar.gz? If it's still fine, ++. @@ +50,3 @@ AC_PROG_CC AM_PROG_CC_C_O +LT_INIT([disable-static]) ++
Review of attachment 259189 [details] [review]: ++ if it still distchecks
Review of attachment 259190 [details] [review]: ::: backends/eds/lib/Makefile.am @@ +89,3 @@ $(AM_LDFLAGS) \ $(CODE_COVERAGE_LDFLAGS) \ + -version-info "$(FOLKS_EDS_LT_VERSION)" \ vim "quickfix" mode is going to think this is the line number of an error in a strangely-named file (which is why it previously had bizarre quoting). If you don't care, ++
commit f4861853833ae9f34f4238cf79ae59c2f3633c49 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Thu Nov 7 13:37:32 2013 +0000 build: Remove unused variables from configure.ac https://bugzilla.gnome.org/show_bug.cgi?id=711544 configure.ac | 3 --- 1 file changed, 3 deletions(-) commit d0d94aeb5af7f5300a0dbe0eb756fd053d9e8f9f Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Thu Nov 7 13:31:34 2013 +0000 build: Update configure.ac as suggested by autoupdate Thanks, autoupdate. https://bugzilla.gnome.org/show_bug.cgi?id=711544 configure.ac | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) commit 8c5d7f90c1f0e872cdf25648bd721aaed90bf3b6 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Thu Nov 7 13:24:32 2013 +0000 build: Use AS_HELP_STRING instead of the deprecated AC_HELP_STRING https://bugzilla.gnome.org/show_bug.cgi?id=711544 configure.ac | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) commit 44dd8bbaadbf2e147d9ea91e64c9d527688cfc47 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Thu Nov 7 13:23:49 2013 +0000 build: Build backend libraries before the backend modules themselves https://bugzilla.gnome.org/show_bug.cgi?id=711544 backends/eds/Makefile.am | 2 +- backends/libsocialweb/Makefile.am | 2 +- backends/telepathy/Makefile.am | 2 +- backends/tracker/Makefile.am | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
Comment on attachment 259188 [details] [review] build: Update configure.ac as suggested by autoupdate (In reply to comment #8) > Does removing the underquoting result in folks-folks_version.tar.gz or the > correct folks-0.xyz.tar.gz? Yup, it’s all fine.
(In reply to comment #10) > Review of attachment 259190 [details] [review]: > > ::: backends/eds/lib/Makefile.am > @@ +89,3 @@ > $(AM_LDFLAGS) \ > $(CODE_COVERAGE_LDFLAGS) \ > + -version-info "$(FOLKS_EDS_LT_VERSION)" \ > > vim "quickfix" mode is going to think this is the line number of an error in a > strangely-named file (which is why it previously had bizarre quoting). If you > don't care, ++ Are you sure? It doesn’t have colons in. What does quickfix mode key off?
Comment on attachment 259190 [details] [review] build: Allow backend libraries to have separate LT versions from libfolks.so Hmm, thinking about it more, the backends’ GIR, Vapi and typelib files all need to use independent API versions from the core of folks as well.
Created attachment 263037 [details] [review] build: Allow backend libraries to have separate LT versions from libfolks.so With the addition of the dummy backend, backend libraries (such as libfolks-dummy.so) no longer necessarily have the same stability guarantees as the core of libfolks. Consequently, they must have separate LT versions. These versions will be incremented in-line with the core LT version, but may also be incremented separately. They don’t affect whether libfolks will load a given backend module (that’s predicated on the core LT current version); merely whether client code needs to be re-linked against a given backend library due to an ABI-incompatible change in its backend-specific API.
Created attachment 263038 [details] [review] build: Minor build simplification No need to create a new directory variable; just use lib_LTLIBRARIES directly.
Created attachment 263039 [details] [review] build: Factor common backends/*/lib makefilery into backend-library.mk This simplifies and standardises the build process for the installed backend libraries (libfolks-eds.so and friends).
Created attachment 263040 [details] [review] build: Allow backend libraries to have individual API versions The API version is included in the GIR file name, and only changes when API is broken or removed. If new backends, such as the dummy backend, are to have unstable APIs, having individual versions for each backend library is necessary to allow backend API breaks without breaking the whole of the folks API. The API version has not been added to the VAPI file name, though it should, because that would be a backwards-incompatible change. It can be added when folks next breaks API compatibility.
Created attachment 263041 [details] [review] build: Update git.mk to fix valac-generated C header file ignores Update to the latest git.mk from master, but also including this patch: https://github.com/behdad/git.mk/pull/13
Created attachment 263042 [details] [review] build: Ensure the TpLowlevel GIR and typelib are cleaned properly
Review of attachment 263037 [details] [review]: +# core folks_lt_* variables are implemented (as all the backend libraries are s/implemented/incremented Otherwise, looks good to apply.
Review of attachment 263038 [details] [review]: Looks good
Review of attachment 263039 [details] [review]: Looks great! If only this were done at a higher level so we didn't have to. I think if someone who hadn't dealt with autotools saw this (even this cleaner, factored form), they'd slowly back up, turn, and run out the room. "8 files changed, 252 insertions(+), 496 deletions(-)" Woohoo! Even more in the build setup than in the code, any time I see such a reduction, I'm happy.
Review of attachment 263040 [details] [review]: "The API version has not been added to the VAPI file name, though it should, because that would be a backwards-incompatible change. It can be added when folks next breaks API compatibility." Added to the roadmap at the time we break compatibility. +# Namespace Vala file +# +# FIXME: This can be removed once the backend namespaces have been sanitised. +# https://bugzilla.gnome.org/show_bug.cgi?id=711544 +# Is there any way to resolve this? Also, is that the right bug number? I don't see an explanation of that issue on this bug (and if it were, we should be solving it with this set of patches before we close it). Otherwise, this looks good.
Review of attachment 263041 [details] [review]: This has actually been merged to git.mk, so no need to note the pull request in the message. Otherwise, looks good.
Review of attachment 263042 [details] [review]: Looks good.
Thanks for the reviews. I fixed the issues mentioned in comment #21 and comment #25 and pushed. Regarding comment #24, that is the correct bug number. The relevant description is in comment #0: the Vala namespaces for the backend libraries don’t match the GIR namespaces, which necessitates weird hacks like the namespace.vala files, and breaks when subclassing within a backend library. The fix is to rename the backend namespaces to be ‘FolksTelepathy’, ‘FolksEds’, etc. This bug will stay open until that’s done. Attachment 263037 [details] pushed as 2599662 - build: Allow backend libraries to have separate LT versions from libfolks.so Attachment 263038 [details] pushed as 133a599 - build: Minor build simplification Attachment 263039 [details] pushed as 9864169 - build: Factor common backends/*/lib makefilery into backend-library.mk Attachment 263040 [details] pushed as 69b04cd - build: Allow backend libraries to have individual API versions Attachment 263041 [details] pushed as 42184a9 - build: Update git.mk to fix valac-generated C header file ignores Attachment 263042 [details] pushed as 120252f - build: Ensure the TpLowlevel GIR and typelib are cleaned properly
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/folks/issues/73.