GNOME Bugzilla – Bug 712747
Assorted fixes for librest
Last modified: 2021-05-25 12:45:55 UTC
Various clean ups; dependency bumps; annotation fixes; documentation fixes; and removal of deprecation warnings.
Created attachment 260315 [details] [review] build: Replace INCLUDES with AM_CPPFLAGS INCLUDES is deprecated and should not be used in modern autotools.
Created attachment 260316 [details] [review] Bump up dependencies Use modern GLib and GObject-Introspection.
Created attachment 260317 [details] [review] Remove libsoup-gnome dependency We don't need libsoup-gnome for handling proxies: GIO has all the needed extension points, and the corresponding API in libsoup has been deprecated. We still want to be able to disable the dependency bump, though, so the configure switch `--with-gnome` can stay and be used for that purpose.
Created attachment 260318 [details] [review] Fix the expansion of the REST_TYPE_PARAM macro And this, kids, is why patches ought to be reviewed.
Created attachment 260319 [details] [review] Fix annotations Syntax matters.
Created attachment 260320 [details] [review] Turn on introspection scanner warnings So we have a fighting chance to actually fix them, instead of sweeping them under the rug.
Created attachment 260321 [details] [review] Update the ignore file
Created attachment 260322 [details] [review] Further annotation fixes
Created attachment 260323 [details] [review] Annotate RestParams as GHashTable They are the same thing as far as anybody's concerned. In theory we should make RestParams a proper boxed type, though.
Created attachment 260324 [details] [review] Remove deprecated g_type_init() It's unnecessary with modern GLib.
Created attachment 260325 [details] [review] Rename rest-docs from SGML to XML It's an XML file, let's use the right extension.
Created attachment 260326 [details] [review] Add indices and annotation glossary The latter, especially, avoids gtk-doc complaining about introspection annotations in the documentation comments.
Created attachment 260327 [details] [review] docs: Add missing gtk-doc annotations
Created attachment 260328 [details] [review] docs: Add annotations to RestProxyAuth pausing methods Both pause() and unpause() are referenced elsewhere in the API reference, but have no formal documentation annotations.
Review of attachment 260315 [details] [review]: Yep.
Review of attachment 260318 [details] [review]: Whoever reviewed the original patch should be sacked.
Review of attachment 260319 [details] [review]: Good.
Review of attachment 260320 [details] [review]: +1
Review of attachment 260321 [details] [review]: Alternatively use git.mk but sure, this works for now.
Review of attachment 260322 [details] [review]: +1
Review of attachment 260323 [details] [review]: Not convinced that the introspection should be doing this, we didn't use a bare hashtable for a reason.
Review of attachment 260325 [details] [review]: Running out of synonyms for "yes" now.
Review of attachment 260326 [details] [review]: Woot.
Review of attachment 260327 [details] [review]: Cool.
Created attachment 260329 [details] [review] Drop compatibility code The dependency bump to GLib 2.36 allows us to get rid of the compatibility shenanigans.
Review of attachment 260328 [details] [review]: Booyah.
Approved all of the documentation and easy fixes. As discussed on IRC there are vocal/important users who need old GLib. Let's merge these, verify it still works on old Glib, make a release, and then merge the rest.
(In reply to comment #22) > Review of attachment 260323 [details] [review]: > > Not convinced that the introspection should be doing this, we didn't use a bare > hashtable for a reason. the main problem is that, from a bindings purposes, this *is* a hash table — so the marshalling code in the bindings can automatically translate the returned RestParams table into a native data type. if you don't want that to happen, you need to box the GHashTable into another structure (which is kind of what ought to happen anyway, given the TODO at the top of rest-params.c), with its own refcounting semantics, and its own GType.
(In reply to comment #20) > Review of attachment 260321 [details] [review]: > > Alternatively use git.mk but sure, this works for now. git.mk is an abomination and we shall never speak of it ever again.
Attachment 260315 [details] pushed as 5698295 - build: Replace INCLUDES with AM_CPPFLAGS Attachment 260318 [details] pushed as 1c3cba5 - Fix the expansion of the REST_TYPE_PARAM macro Attachment 260319 [details] pushed as 218f44c - Fix annotations Attachment 260320 [details] pushed as 6aa3de7 - Turn on introspection scanner warnings Attachment 260321 [details] pushed as 0293ba3 - Update the ignore file Attachment 260322 [details] pushed as 4796819 - Further annotation fixes Attachment 260325 [details] pushed as b39be0f - Rename rest-docs from SGML to XML Attachment 260326 [details] pushed as 9509fdf - Add indices and annotation glossary Attachment 260327 [details] pushed as 3796b68 - docs: Add missing gtk-doc annotations Attachment 260328 [details] pushed as dfcb399 - docs: Add annotations to RestProxyAuth pausing methods
What about the other patches? They are mostly about dropping deprecated API and bumping dependencies, but they are also a few years old so the increased dependency shouldn't be much of a problem?
(In reply to Timm Bäder from comment #32) > What about the other patches? They are mostly about dropping deprecated API > and bumping dependencies, but they are also a few years old so the increased > dependency shouldn't be much of a problem? Last time, the hold out was RHEL still shipping with ancient versions of everything.
Review of attachment 260316 [details] [review]: This is old enough for RHEL 7, not RHEL 6, but I can live with that.
Review of attachment 260317 [details] [review]: RHEL 7 has a new enough libsoup for this, but not RHEL 6.
Review of attachment 260324 [details] [review]: Again, fine for RHEL 7, but not RHEL 6.
Review of attachment 260329 [details] [review]: Ok for RHEL 7, not RHEL 6.
I don't mind dropping support for RHEL 6, but Christophe might care about it...
I kind of care as I'll probably need to do a bit more work on the el6 package for some convoluted certificate handling issue. However, not much point in holding upstream back too much just because of that. I'd prefer that we have a 0.8.0 release first though before raising the various requirements (and call the version with the new requirements 0.9 or 0.10)
Given that the hmac support in master requires a glib newer than what's in el6, just go ahead with these too, I'll branch for the release.
All the patches that are left do not apply cleanly any more; I'll have to rebase them on top of current master. I was also wondering if we shouldn't bump up the API to 0.8, and see if we can change some of it while we're at it.
(In reply to Emmanuele Bassi (:ebassi) from comment #41) > All the patches that are left do not apply cleanly any more; I'll have to > rebase them on top of current master. > > I was also wondering if we shouldn't bump up the API to 0.8, and see if we > can change some of it while we're at it. I'd even bump API to 1.0, decouple it from the tarball version, and try to keep the new API stable in the long run. I've pushed a patch to master setting the version number to 0.9 (I probably misunderstood all this API stuff when releasing something numbered 0.7.9x with the intent of the stable release being 0.8.0 :). I haven't broken API as there is no reason to do it yet, so it's still 0.7 I have also made a 0.8.0 release sticking to an older glib release in the corresponding branch where stable bugfix releases can be maade, and master (0.9) can be broken as we want.
FYI I have locally ported librest to GTask somewhere I think, another break that would be good is remove that one function that existed with a typo. If you're into removing things, I'm not sure why there's an xml parser inside librest, but I have no problem with it if there's a good reason of course. Actually, I'm not sure how popular all the stuff in rest-extras is. Basically, if you're planning to break API, I'd just fix everything we can find :)
The XML parser is there from the days of libmojito — sorry, libsocialweb; dealing with the XML crap that some social networks returned was a ton of work, and JSON was not the lingua franca for web services at the time.
(In reply to Timm Bäder from comment #43) > FYI I have locally ported librest to GTask somewhere I think, another break > that would be good is remove that one function that existed with a typo. If > you're into removing things, I'm not sure why there's an xml parser inside > librest, but I have no problem with it if there's a good reason of course. > Actually, I'm not sure how popular all the stuff in rest-extras is. > > Basically, if you're planning to break API, I'd just fix everything we can > find :) With 0.8.0 released and branched, I think we can do whatever we want with master. Porting to GTask to have an API more consistent with the rest of the gio stack makes a lot of sense now. Fixing that typo (did not notice it) makes sense too once things are broken. Fwiw, I'm using the XML parser in libgovirt (but maybe I'm the only user of it, I guess I could live without it). I could not find a user of the -extras stuff. But maybe it's a matter of pushing it a bit more rather than having each app reinvent it (I did not check how realistic it would be to use that in existing apps).
Review of attachment 260317 [details] [review]: See https://bugzilla.gnome.org/show_bug.cgi?id=758166 for a bug about this. Since we are using SoupSessionSync/SoupSessionAsync (but we should change this..), we need to explicitly use it as otherwise it's not going to be used.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/librest/-/issues/ Thank you for your understanding and your help.