Created attachment 323519 [details] [review]
codegen: Add support for g_autoptr to gdbus-codegen–generated objects

This means that any code generated by gdbus-codegen will now require
GLib 2.44 or newer.

Speaking from experience: various projects still want to use gdbus-codegen but keep their own dependency of GLib to a previous version — usually through the application of the min/max versioning macros. For this reason, enforcing a minimum version in gdbus-codegen is probably not a good plan.

(In reply to Emmanuele Bassi (:ebassi) from comment #2)
> Speaking from experience: various projects still want to use gdbus-codegen
> but keep their own dependency of GLib to a previous version — usually
> through the application of the min/max versioning macros. For this reason,
> enforcing a minimum version in gdbus-codegen is probably not a good plan.

Option #2: add a `#if GLIB_CHECK_VERSION(2, 44, 0)` around the new lines.

(In reply to Philip Withnall from comment #3)
> (In reply to Emmanuele Bassi (:ebassi) from comment #2)
> > Speaking from experience: various projects still want to use gdbus-codegen
> > but keep their own dependency of GLib to a previous version — usually
> > through the application of the min/max versioning macros. For this reason,
> > enforcing a minimum version in gdbus-codegen is probably not a good plan.
> 
> Option #2: add a `#if GLIB_CHECK_VERSION(2, 44, 0)` around the new lines.

And document the API version requirement/stability policy for gdbus-codegen–generated code.

imho: bracket the autoptr support in an #if version check and call it a day.  There is very little benefit to using the other macros in generated code.

Created attachment 323597 [details] [review]
codegen: Add support for g_autoptr to gdbus-codegen–generated objects

This means that any code generated by gdbus-codegen will now require
GLib 2.44 or newer.

Called it a day. Thanks for the reviews!

Attachment 323597 [details] pushed as fd6ca66 - codegen: Add support for g_autoptr to gdbus-codegen–generated objects

*** Bug 755797 has been marked as a duplicate of this bug. ***

The patch in 2.48 covers the WhateverProxy, WhateverSkeleton, WhateverObjectProxy, WhateverObjectSkeleton and WhateverObjectManagerClient, but not the base GInterface that they all share. As a result, you still can't use

g_autoptr (Whatever) = whatever_proxy_new_sync (...);

Patch in a moment, when I've smoke-tested it (but it's exactly what you'd expect from Philip's commit).

Created attachment 327179 [details] [review]
codegen: Add g_autoptr support for the shared GInterface

The rest of the generated classes gained g_autoptr support in fd6ca66,
but this one is still missing. Because whatever_proxy_new_finish() and
whatever_proxy_new_sync() are declared as returning a Whatever *
instead of a WhateverProxy *, and the generated method-call stubs
act on a Whatever *, it's reasonably common to want to declare a
g_autoptr (Whatever).

---

As well as master, I'd like to apply this to 2.48.x if people don't object.

Review of attachment 327179 [details] [review]:

Go directly to commit.  Do not collect $200.

Review of attachment 327179 [details] [review]:

Committed as cbbcaa4 for 2.49.0 and ca189aa for 2.48.1, thanks!

Created attachment 327207 [details] [review]
codegen: Add a symbol for detecting autoptr macros

The gdbus-codegen has grown the ability to define autoptr cleanup
symbols for the types it generates; unfortunately, existing code may
already define those cleanup symbols, leading the build failures, and
requirements to increase the GLib dependency when building the same code
against newer versions of GLib.

Existing code attempts to be forward compatible by using:

  #ifndef glib_autoptr_cleanup_TypeName
  ...
  #endif

to guard the autoptr definitions, but that cannot work as the cleanup
function is defined via an inline function,  not a pre-processor macro.

In order to properly detect autoptr cleanup symbols, gdbus-codegen
should provide a pre-processor symbol that can be used to guard existing
definitions.

This commit just broke existing code that uses gdbus-codegen *and* defines its own autoptr cleanup macros.

(In reply to Emmanuele Bassi (:ebassi) from comment #14)
> This commit just broke existing code that uses gdbus-codegen *and* defines
> its own autoptr cleanup macros.

Ugh, sorry... I'd assumed that nothing would do this, since it clearly isn't forward-compatible for an application to do so.

Review of attachment 327207 [details] [review]:

How about

#define glib_autoptr_cleanup_TypeName glib_autoptr_cleanup_TypeName

if that's how the existing attempts-at-checks work, since it would retroactively make them correct?

(It's a pity G_DEFINE_AUTOPTR_CLEANUP_FUNC can't do this itself, but I don't think a macro can define a macro.)

For reference: http://build.gnome.org/continuous/buildmaster/builds/2016/05/03/24/build/log-xdg-app.txt

(In reply to Emmanuele Bassi (:ebassi) from comment #14)
> This commit just broke existing code that uses gdbus-codegen *and* defines
> its own autoptr cleanup macros.

For modules that want to have g_autoptr support for a type that lacks it, in a way that *won't* break when the originator of the type adds g_autoptr support, C's type-safety is weak enough that this works:

typedef AutoTheirType TheirType;
G_DEFINE_AUTOPTR_CLEANUP_FUNC (AutoTheirType, g_object_unref)

int main (...)
{
  g_autoptr (AutoTheirType) proxy = NULL;

  ...
  /* this returns a TheirType * */
  proxy = their_type_proxy_new ();

  /* this takes a TheirType * as first argument */
  their_type_call_do_something (proxy, ...)
  ...
}

Would you consider a

#if GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_50

check around the new G_DEFINE_AUTOPTR_CLEANUP_FUNC to be a suitable solution?

That would mean API users can't do what I said in Comment #9 until they are willing to depend on GLib 2.50, but that's better than never being able to do that at all.

And it looks like the original commit adding this infd6ca66c16b942d1f7d8c70468a438ad4c887c7b made it into 2.48, so the fact that we do autocleanups for everything but the interface is basically ABI.

I'm a bit confused by the patch - `git grep HAS_AUTO` has zero hits in master for me now, so what would this do?  Oh, I see...you're thinking application should change to inspect this?

That obviously helps but still isn't fully compatible.

A fully compatible option would be requiring a #define to introduce the interface autocleanup.  (Yes, ugly, but it'd be a one liner per app)

(In reply to Simon McVittie from comment #19)
> Would you consider a
> 
> #if GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_50
> 
> check around the new G_DEFINE_AUTOPTR_CLEANUP_FUNC to be a suitable solution?

That seems like a good compromise to me.

(In reply to Simon McVittie from comment #19)
> Would you consider a
> 
> #if GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_50
> 
> check around the new G_DEFINE_AUTOPTR_CLEANUP_FUNC to be a suitable solution?

Yeah, that would be acceptable for a development cycle while we fix application code.

Created attachment 327224 [details] [review]
codegen: make g_autoptr for the GInterface conditional

Some GNOME projects unconditionally work around the generated code's
lack of g_autoptr support by defining the autoptr cleanup function
themselves, which is not forward-compatible; as a result, commit
cbbcaa4 broke them. Do not define the cleanup function unless the
including app "opts in" to newer APIs via GLIB_VERSION_MAX_ALLOWED.

Projects requiring compatibility with GLib < 2.49 can get a
forward-compatible g_autoptr for a generated GInterface type found in
a library, for example ExampleAnimal in the GIO tests, by declaring
and using a typedef with a distinct name outside the library's
namespace:

    typedef AutoExampleAnimal ExampleAnimal;
    G_DEFINE_AUTOPTR_CLEANUP_FUNC (AutoExampleAnimal, g_object_unref)

    ...

    g_autoptr (AutoExampleAnimal) animal = NULL;

    /* returns ExampleAnimal * */
    animal = example_animal_proxy_new_sync (...);
    /* takes ExampleAnimal * first argument */
    example_animal_call_poke_sync (animal, ...);

(In reply to Emmanuele Bassi (:ebassi) from comment #13)
> In order to properly detect autoptr cleanup symbols, gdbus-codegen
> should provide a pre-processor symbol that can be used to guard existing
> definitions.

I don't think this is necessarily a useful change to be making: if the other project is going to have to have source code changes *anyway*, those source code changes might as well be to introduce their own typedef so it doesn't collide.

This could maybe be useful for specific cases like xdg-app, where the same library that is responsible for introducing the type is trying to ensure that it has g_autoptr support even if GLib doesn't. However, if you're going to do that, you might as well #define glib_autoptr_cleanup_TypeName to itself, so that their existing (incorrect) checks retroactively become correct.

Review of attachment 327224 [details] [review]:

Care to squash this patch with the original one, in order to do a single commit for the glib-2-48 branch? This way we avoid the revert-the-revert dance and we keep things building when bisecting GLib's stable branch. If it's too much trouble, I don't particularly mind.

::: gio/gdbus-2.0/codegen/codegen.py
@@ +308,3 @@
             self.h.write('};\n')
             self.h.write('\n')
+            self.h.write('#if GLIB_CHECK_VERSION(2, 44, 0) && defined(GLIB_VERSION_2_50) && GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_50\n')

You could use:

  GLIB_VERSION_MAX_ALLOWED >= G_ENCODE_VERSION (2, 50)

to avoid the check for GLIB_VERSION_2_50 being defined, but it's just a minor quibble.

Looks good to me.

Review of attachment 327224 [details] [review]:

Looks right to me, thanks!

(In reply to Emmanuele Bassi (:ebassi) from comment #25)
> Care to squash this patch with the original one, in order to do a single
> commit for the glib-2-48 branch?

Is this still useful to backport to glib-2-48 if projects have to opt-in to GLib 2.50 APIs to get it?

My justification for wanting to backport it is that I was treating this as essentially a bug in Attachment #323519 [details]... but it broke deployed code (which was, IMO incorrectly, assuming that gdbus-codegen wouldn't fix these omissions) which moves it to being more like "new feature with potential breakage" territory.

Comment on attachment 327224 [details] [review]
codegen: make g_autoptr for the GInterface conditional

Committed for 2.49.0, 1c6cd5f

So sadly this still doesn't work, because if the application itself doesn't define GLIB_VERSION_MAX_ALLOWED, we do it in glib:

#if !defined (GLIB_VERSION_MAX_ALLOWED) || (GLIB_VERSION_MAX_ALLOWED == 0)
# undef GLIB_VERSION_MAX_ALLOWED
# define GLIB_VERSION_MAX_ALLOWED      (GLIB_VERSION_CUR_STABLE)
#endif


So...we could try something like this?

diff --git a/gio/gdbus-2.0/codegen/codegen.py b/gio/gdbus-2.0/codegen/codegen.py
index 6f19772..7f66a2a 100644
--- a/gio/gdbus-2.0/codegen/codegen.py
+++ b/gio/gdbus-2.0/codegen/codegen.py
@@ -307,7 +307,7 @@ class CodeGenerator:
 
             self.h.write('};\n')
             self.h.write('\n')
-            self.h.write('#if GLIB_CHECK_VERSION(2, 44, 0) && defined(GLIB_VERSION_2_50) && GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_50\n')
+            self.h.write('#if GLIB_CHECK_VERSION(2, 44, 0) && defined(GLIB_VERSION_2_50) && !defined(GLIB_VERSION_MAX_ALLOWED_AUTO) && GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_50\n')
             self.h.write('G_DEFINE_AUTOPTR_CLEANUP_FUNC (%s, g_object_unref)\n' % (i.camel_name))
             self.h.write('#endif\n')
             self.h.write('\n')
diff --git a/glib/gversionmacros.h b/glib/gversionmacros.h
index 1611eaf..d31e815 100644
--- a/glib/gversionmacros.h
+++ b/glib/gversionmacros.h
@@ -234,6 +234,7 @@
 #if !defined (GLIB_VERSION_MAX_ALLOWED) || (GLIB_VERSION_MAX_ALLOWED == 0)
 # undef GLIB_VERSION_MAX_ALLOWED
 # define GLIB_VERSION_MAX_ALLOWED      (GLIB_VERSION_CUR_STABLE)
+# define GLIB_VERSION_MAX_ALLOWED_AUTO 1
 #endif
 
 /* sanity checks */

Still failing xdg-app build: http://build.gnome.org/continuous/buildmaster/builds/2016/05/03/33/build/

For now, https://git.gnome.org/browse/gnome-continuous/commit/?id=09e0290fded9a870e92ee7c91584a926def61b56

(In reply to Colin Walters from comment #29)
> So sadly this still doesn't work, because if the application itself doesn't
> define GLIB_VERSION_MAX_ALLOWED, we do it in glib:
 
> #if !defined (GLIB_VERSION_MAX_ALLOWED) || (GLIB_VERSION_MAX_ALLOWED == 0)
> # undef GLIB_VERSION_MAX_ALLOWED
> # define GLIB_VERSION_MAX_ALLOWED      (GLIB_VERSION_CUR_STABLE)
> #endif

Ugh, no, that's even worse, and we're still playing catch up with a bunch of code we don't control by adding new macros in the hope that something will magically work out.

Honestly, at this point we should declare bankruptcy and either say:

 * you *cannot* catch G_DEFINE_AUTOPTR_* and if you try you're on your own so your code will break
 * #define glib_autoptr_cleanup_##TypeName ourselves and hope for the best, as people seem to be fixating on that symbol by the Power of Copy and Paste™.

How is my patch worse?

I obviously disagree with your characterization of my patch as "hope that something will magically work out".

FTR, I just tested it, and it *does* work.

I'm not saying it is necessarily the best solution, but I would say it's better than "bankruptcy", i.e. having older xdg-app fail to compile with a future GLib 2.50.

Regarding your patch in https://bugzilla.gnome.org/show_bug.cgi?id=763379#c13 - I personally would much prefer a fix on the GLib side that doesn't require application changes, which is what Simon's patches combined with mine do.

Long experience tells me people are going to try to compile old software with newer GLib.  As a downstream maintainer for Red Hat Enterprise Linux, I have to make it work.

Created attachment 327239 [details] [review]
Record whether app used _MAX_VERSION, use it in codegen

Define a `GLIB_VERSION_MAX_ALLOWED_AUTO` macro if the application
itself did not set `GLIB_VERSION_MAX_ALLOWED`.  This enables a bugfix
in the GDBus code generator, where we want to detect whether the
application opted-in to `GLIB_VERSION_2_50`.

This fixes building older xdg-app with newer glib.

(In reply to Emmanuele Bassi (:ebassi) from comment #32)
>  * you *cannot* catch G_DEFINE_AUTOPTR_* and if you try you're on your own
> so your code will break

As you can probably infer from the patch I initially attached, I would be inclined to say that anyone doing G_DEFINE_AUTOPTR_ for a typedef that they did not create is already on very thin ice, and they should stop doing that - because it is inevitably going to break as soon as the creator of the typedef adds it (and if we were to treat addition of a G_DEFINE_AUTOPTR_ as an API break, then nobody would add them to existing types, which partially defeats the object of having this very convenient interface available to us).

Here I'm counting gdbus-codegen, not xdg-app, as what really "created" XdgAppSessionHelper.

Having said that, Colin's patch seems pragmatic, and has the side-effect that if someone wants to use this new API, they will have to set a GLIB_VERSION_MAX_ALLOWED - which we probably want them to do *anyway*, in the long term.

(Not a maintainer, etc.)

(In reply to Simon McVittie from comment #36)
> (In reply to Emmanuele Bassi (:ebassi) from comment #32)
> >  * you *cannot* catch G_DEFINE_AUTOPTR_* and if you try you're on your own
> > so your code will break
> 
> As you can probably infer from the patch I initially attached, I would be
> inclined to say that anyone doing G_DEFINE_AUTOPTR_ for a typedef that they
> did not create is already on very thin ice,

I agree with that - we should patch the apps too.

But in practice, the autoptr stuff I think is so new that those sorts of "gotchas" are not necessarily obvious even to very experienced maintainers.

Hopefully everyone knows they shouldn't define a symbol named `g_foo` in their app, but the autoptr stuff I feel is more subtle.

> Having said that, Colin's patch seems pragmatic, and has the side-effect
> that if someone wants to use this new API, they will have to set a
> GLIB_VERSION_MAX_ALLOWED - which we probably want them to do *anyway*, in
> the long term.
> 
> (Not a maintainer, etc.)

I would certainly say you should feel qualified to r+ or r- this patch.  I mean in reality I'm not even aware of an official GLib reviewers list, there's no MAINTAINERS file and the AUTHORS file is radically different from the set of people actively doing things...

Anyways, I don't like my patch much either, it'd be nice to avoid having to touch glibversionmacros.h somehow, but I can't think of a better fix.

(In reply to Colin Walters from comment #37)

> I would certainly say you should feel qualified to r+ or r- this patch.  I
> mean in reality I'm not even aware of an official GLib reviewers list,
> there's no MAINTAINERS file and the AUTHORS file is radically different from
> the set of people actively doing things...

theres glib.doap, which fills the role of MAINTAINERS for gnome modules.

The situation now is that mutter hard-requires GLib with this change, and xdg-app hard-requires GLib without this change.

For now, I've also tagged mutter:
https://git.gnome.org/browse/gnome-continuous/commit/?id=b0a65be42310c3a0691f6d97621c81472c9a2bfa

Options as I see it:
 #1 Back out this change, and the change to mutter, and revisit later
 #2 Apply my patch
 #3 Decide that xdg-app and mutter are unusual in doing this, and patch them to be compatible with both, and hope that there isn't wider breakage

I'm sure mutter would be okay hard requiring 2.50, I doubt xdg-app would.

xdg-app wouldn't, but it can probably live with an ifdef

My suggestion is that we should add a --generate-autocleanup option to gdbus-codegen, and only generate autocleanup macros when it is specified.

(In reply to Matthias Clasen from comment #41)
> My suggestion is that we should add a --generate-autocleanup option to
> gdbus-codegen, and only generate autocleanup macros when it is specified.

That would have been a good solution *before* we shipped https://git.gnome.org/browse/glib/commit/?h=glib-2-48&id=fd6ca66c16b942d1f7d8c70468a438ad4c887c7b

Now it'd be a bit weird as it's really imply only adding support for the interface, right?

Created attachment 327253 [details] [review]
gdbus-codegen: Only generate autocleanup when instructed to

This adds a new --c-generate-autocleanup option to gdbus-codegen
which is now required for gdbus-codegen to emit autocleanup
definitions. Doing this unconditionally was found to interfere
with existing code out in the wild.

(In reply to Colin Walters from comment #42)
> (In reply to Matthias Clasen from comment #41)
> > My suggestion is that we should add a --generate-autocleanup option to
> > gdbus-codegen, and only generate autocleanup macros when it is specified.
> 
> That would have been a good solution *before* we shipped
> https://git.gnome.org/browse/glib/commit/?h=glib-2-
> 48&id=fd6ca66c16b942d1f7d8c70468a438ad4c887c7b
> 
> Now it'd be a bit weird as it's really imply only adding support for the
> interface, right?

Yeah, it sucks either way. We really need to be more careful with this kind of stuff.

We could make this slightly more complicated and allow

--c-generate-autocleanup=objects
--c-generate-autocleanup=interfaces
--c-generate-autocleanup=all

And then default to objects if nothing is explicitly specified.

Created attachment 327254 [details] [review]
gdbus-codegen: Only generate autocleanup when instructed to

This adds a new --c-generate-autocleanup option to gdbus-codegen
which can be used to instruct gdbus-codegen about what autocleanup
definitions to emit.

Doing this unconditionally was found to interfere with existing
code out in the wild.

The new option takes an argument that can be
none, objects or all; to indicate whether to generate no
autocleanup functions, only do it for object types, or do it
for interface types as well. The default is 'objects', which
matches the unconditional behavior of gdbus-codegen on the 2.48
branch.

Created attachment 327255 [details] [review]
Document the new gdbus-codegen option

Review of attachment 327254 [details] [review]:

::: gio/gdbus-2.0/codegen/codegen.py
@@ +458,3 @@
             self.h.write('GType %s_proxy_get_type (void) G_GNUC_CONST;\n'%(i.name_lower))
             self.h.write('\n')
+            if self.generate_autocleanup == 'objects':

Surely this, and all other tests for 'objects', should be:

if self.generate_autocleanup in ('objects', 'all'):

(In reply to Colin Walters from comment #37)
> Hopefully everyone knows they shouldn't define a symbol named `g_foo` in
> their app, but the autoptr stuff I feel is more subtle.

Can we document this in the G_DEFINE_AUTOPTR_ docstrings, then? I'll try to write some suitable wording when we have agreed what its meaning should be.

I think the only sensible policy is that the module that declared a typedef may make it an autoptr, and other modules must not. In particular, I don't think it makes sense to consider adding autocleanup to existing hand-written code to be an API break - if we did, commits like 73497c1 in GTK would not be allowed.

For generated code, it's a little less clear-cut because there are two modules that might claim to be responsible for declaring the type: the code-generator, and the project that embeds the generated code (e.g. xdg-app). Having options passed to the code-generator makes sense if we consider the project that embeds the generated code to be the one responsible for selecting its API.

(In reply to Matthias Clasen from comment #45)
> This adds a new --c-generate-autocleanup option to gdbus-codegen
> which can be used to instruct gdbus-codegen about what autocleanup
> definitions to emit.

I'm not sure how well this approach will scale over time. I'd be tempted to phrase it as more like an "API level", something like this (obviously using hypothetical examples for future features):

--c-glib-api=2.48 (default)
    generate autocleanups for objects

--c-glib-api=2.50
    as above, and also generate autocleanups for interface instances,
    and something we haven't thought of yet but will add in 2.49.3

--c-glib-api=2.52
    as above, and also generate something else we haven't thought of yet
    but will add in 2.51.4

(debhelper in Debian, with its "compat level" concept, is a major user of this pattern.)

(In reply to Simon McVittie from comment #47)
> Review of attachment 327254 [details] [review] [review]:
> 
> ::: gio/gdbus-2.0/codegen/codegen.py
> @@ +458,3 @@
>              self.h.write('GType %s_proxy_get_type (void)
> G_GNUC_CONST;\n'%(i.name_lower))
>              self.h.write('\n')
> +            if self.generate_autocleanup == 'objects':
> 
> Surely this, and all other tests for 'objects', should be:
> 
> if self.generate_autocleanup in ('objects', 'all'):

Obviously, yes. Thanks for catching that.

(In reply to Simon McVittie from comment #48)
> (In reply to Colin Walters from comment #37)
> > Hopefully everyone knows they shouldn't define a symbol named `g_foo` in
> > their app, but the autoptr stuff I feel is more subtle.
> 
> Can we document this in the G_DEFINE_AUTOPTR_ docstrings, then? I'll try to
> write some suitable wording when we have agreed what its meaning should be.

Yes, I've been meaning to say that we should have some best practice documentation around this.

> 
> --c-glib-api=2.48 (default)
>     generate autocleanups for objects
> 
> --c-glib-api=2.50
>     as above, and also generate autocleanups for interface instances,
>     and something we haven't thought of yet but will add in 2.49.3
> 
> --c-glib-api=2.52
>     as above, and also generate something else we haven't thought of yet
>     but will add in 2.51.4

Dunno. I can see that this will 'scale' better (whatever that means, precisely), but then,

--c-generate-autocleanup objects

pretty cleary says what it does, while

--c-glib-api=2.50

sends you running for the manual.

Lets see if we can get some more opinions on this.

I guess the question is - how likely is it that we'll introduce any future incompatibilities like this?

My instinct says it's pretty unlikely - we'll have learned our lesson from this one hopefully!

If that's the case, then --c-glib-api would be odd because people would feel the need to go and bump it whenever they bump glib version, but it wouldn't do anything.

I don't have a strong opinion in the end - adding a command line option to the code generator is okay by me.

(In reply to Matthias Clasen from comment #50)
> 'scale' better (whatever that means, precisely)

What I meant is: in a few years' time, we probably don't want there to be (say) 5 options that basically all mean "make the generated code better/more convenient" and all have to be copy/pasted into everything that generates new D-Bus interfaces, analogous to the way Autotools projects all get

AC_CONFIG_AUX_DIR([build-aux])
AC_CONFIG_MACRO_DIR([m4])
AC_CONFIG_HEADERS([config.h])
AC_SYS_LARGEFILE
(etc.)

pasted into their configure.ac (and if they don't, they probably should).

I can see your point about this having a more discoverable meaning, but in the long run, I would hope that a fraction of projects approaching 100% will be using it.

Let's say it's --c-generate-autocleanup=all for now, but we should seriously consider a "compat level" approach *next* time, if there is a next time?

(In reply to Colin Walters from comment #51)
> My instinct says it's pretty unlikely - we'll have learned our lesson from
> this one hopefully!

I hope the lesson being learned here isn't "after writing a code generator, we can never improve its output"?

*** Bug 765378 has been marked as a duplicate of this bug. ***

(In reply to Simon McVittie from comment #52)

> Let's say it's --c-generate-autocleanup=all for now, but we should seriously
> consider a "compat level" approach *next* time, if there is a next time?

Right.

> (In reply to Colin Walters from comment #51)
> > My instinct says it's pretty unlikely - we'll have learned our lesson from
> > this one hopefully!
> 
> I hope the lesson being learned here isn't "after writing a code generator,
> we can never improve its output"?

I don't think so - this case seems unique in downstream code trying to add something to the generated types after the fact.  I can't imagine many situations where that would come up again.

Matthias, do you want to squash your two patches (why would docs be separate?) and attach the updated bugfixed version?

Created attachment 327319 [details] [review]
gdbus-codegen: Only generate autocleanup when instructed to

This adds a new --c-generate-autocleanup option to gdbus-codegen
which can be used to instruct gdbus-codegen about what autocleanup
definitions to emit.

Doing this unconditionally was found to interfere with existing
code out in the wild.

The new option takes an argument that can be
none, objects or all; to indicate whether to generate no
autocleanup functions, only do it for object types, or do it
for interface types as well. The default is 'objects', which
matches the unconditional behavior of gdbus-codegen on the 2.48
branch.

Review of attachment 327319 [details] [review]:

One optional doc text update.

Otherwise LGTM.  We'll also need to update mutter as soon as this lands and then untag things in GContinuous.

::: docs/reference/gio/gdbus-codegen.xml
@@ +224,3 @@
+          'objects' means to generate them for object types, and
+          'all' means to generate them for object types and interfaces.
+          The default is 'objects'. This option was added in GLib 2.50.

I would say something like:

"The default is 'objects' due to a corner case in backwards compatibility with a few projects, but you should likely switch your project to use 'all'."

Attachment 327319 [details] pushed as 98f86be - gdbus-codegen: Only generate autocleanup when instructed to