GNOME Bugzilla – Bug 710133
Emit backward compatible code with gdbus-codegen
Last modified: 2013-10-29 16:04:51 UTC
If we don't take into consideration GLIB_VERSION_MAX_ALLOWED we're going to have a bad time. Also, we're asking everyone to bump up the dependency of GLib to 2.38. The additional couple of documentation patches ensure that we are explicit about the ABI of instance and class structures, as well as to the fact that generated code should not be kept in revision control or tarballs.
Created attachment 257288 [details] [review] gdbus-codegen: Take into consideration MAX_ALLOWED for private data The G_ADD_PRIVATE() macro, and the auto-generated get_instance_private() internal function, should be used conditionally depending on the maximum allowed version of GLib, as defined by the GLIB_VERSION_MAX_ALLOWED pre-processor symbol. This allows generating code that can be compiled in projects that wish to use an older API version of GLib through the use of the GLIB_VERSION_MAX_ALLOWED symbol.
Created attachment 257289 [details] [review] gdbus-codegen maintains ABI for type structures Make it explicit, to avoid changes that could potentially lead to breakage in user code.
Created attachment 257290 [details] [review] docs: Mention that generated code should not be kept Code generated by gdbus-codegen should neither be checked in into revision control, nor should be distributed.
Comment on attachment 257288 [details] [review] gdbus-codegen: Take into consideration MAX_ALLOWED for private data Looks good to me (assuming unit tests pass) but someone else should probably approve.
If gdbus-codegen doesn't break ABI, and the generated code includes checks for glib API availability, then why NOT check in / dist the generated code? Or on the flip side, if disting/checking in the generated code is bad, and you are only allowed to compile the generated code against the same version of glib as you used to generate it, then you don't need any compile-time checks in the generated code. If you are going to keep the checks, then you want GLIB_CHECK_VERSION(2,38,0), not GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_38, because the question is "is G_ADD_PRIVATE() available here and now", not "is G_ADD_PRIVATE available in every version of glib that the enclosing project supports".
(In reply to comment #5) > If gdbus-codegen doesn't break ABI, and the generated code includes checks for > glib API availability, then why NOT check in / dist the generated code? > > Or on the flip side, if disting/checking in the generated code is bad, and you > are only allowed to compile the generated code against the same version of glib > as you used to generate it, then you don't need any compile-time checks in the > generated code. Actually I don't think it makes sense to check for glib API availability. I only realized now that it does that. It probably shouldn't. The other reason for this is not API availability - it also has to do with bug-fixes in the codegen for the generated code. > If you are going to keep the checks, then you want GLIB_CHECK_VERSION(2,38,0), > not GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_38, because the question is "is > G_ADD_PRIVATE() available here and now", not "is G_ADD_PRIVATE available in > every version of glib that the enclosing project supports".
(In reply to comment #5) > If you are going to keep the checks, then you want GLIB_CHECK_VERSION(2,38,0), > not GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_38, because the question is "is > G_ADD_PRIVATE() available here and now", not "is G_ADD_PRIVATE available in > every version of glib that the enclosing project supports". no, that was actually the bug: a project using GLIB_VERSION_MAX_ALLOWED does not want to use G_ADD_PRIVATE and friends, because G_ADD_PRIVATE is using API available since GLib 2.38 onwards. so you *really* want to use MAX_ALLOWED to check the availability of the API, otherwise you'd implicitly bump up the requirement for the generated code.
(In reply to comment #7) > no, that was actually the bug: a project using GLIB_VERSION_MAX_ALLOWED does > not want to use G_ADD_PRIVATE and friends, because G_ADD_PRIVATE is using API > available since GLib 2.38 onwards. so you *really* want to use MAX_ALLOWED to > check the availability of the API, otherwise you'd implicitly bump up the > requirement for the generated code. I have always assumed that we don't support compiling against 2.38 and then running the resulting binary against 2.36's .so's, even if you only use 2.36's APIs. If that's the case, then GLIB_VERSION_MAX_ALLOWED is irrelevant; if the code is being compiled against 2.38, then you can use 2.38's APIs, because the resulting binary will only ever run against 2.38+. If running against older .so's than you compiled against *is* supported, then you need to check both GLIB_VERSION_MAX_ALLOWED (to see if you're allowed to use the API) and GLIB_CHECK_VERSION (to see if you actually have the API; the project might have MIN_REQUIRED=2.36 and MAX_ALLOWED=2.38, in which case the code needs to be able to compile on 2.36 or 2.38).
(In reply to comment #8) > (In reply to comment #7) > > no, that was actually the bug: a project using GLIB_VERSION_MAX_ALLOWED does > > not want to use G_ADD_PRIVATE and friends, because G_ADD_PRIVATE is using API > > available since GLib 2.38 onwards. so you *really* want to use MAX_ALLOWED to > > check the availability of the API, otherwise you'd implicitly bump up the > > requirement for the generated code. > > I have always assumed that we don't support compiling against 2.38 and then > running the resulting binary against 2.36's .so's, even if you only use 2.36's > APIs. that's not the issue of this bug. if a project uses GLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_36 and generates code with a gdbus-codegen coming from GLib 2.38, then the project will get an implicit dependency bump on GLib 2.38 because of the presence of G_ADD_PRIVATE() and the lack of g_type_class_add_private(); this will cause compiler warnings (because G_ADD_PRIVATE() is implemented in terms of API that violates the MAX_ALLOWED requirement). the only two ways to avoid this implicit dependency bump are: • check GLIB_VERSION_MAX_ALLOWED in the generated code • always generate the old style private data allocation (with deprecation warnings suppressed for g_type_class_add_private()) given that we want to transition people to the new instance private data style, attachment 257228 [details] [review] implements the first option.
(In reply to comment #8) > I have always assumed that we don't support compiling against 2.38 and then > running the resulting binary against 2.36's .so's, even if you only use 2.36's > APIs. The entire goal of the GLIB_VERSION_MAX_ALLOWED stuff is to enable that, no?
(In reply to comment #10) > (In reply to comment #8) > > I have always assumed that we don't support compiling against 2.38 and then > > running the resulting binary against 2.36's .so's, even if you only use 2.36's > > APIs. > > The entire goal of the GLIB_VERSION_MAX_ALLOWED stuff is to enable that, no? As I understood it, the point of MAX_ALLOWED is so that you can say "People on Fedora 18 are supposed to be able to compile this package, so warn me if I use any APIs that weren't available then". (As opposed to "People on Fedora 18 are supposed to be able to use binaries of this package that I compiled on my Fedora 20 box", which probably wouldn't work because of glibc versioning anyway...)
(In reply to comment #11) oint of MAX_ALLOWED is so that you can say "People on > Fedora 18 are supposed to be able to compile this package, so warn me if I use > any APIs that weren't available then". Ah sorry, yes, you're right. So I think the key point from comment 9 is: "this will cause compiler warnings" And that's just what we're trying to fix right? That'd make this bug just a variant of https://bugzilla.gnome.org/show_bug.cgi?id=703191 with s/G_DEFINE_TYPE/gdbus-codegen/
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > no, that was actually the bug: a project using GLIB_VERSION_MAX_ALLOWED does > > > not want to use G_ADD_PRIVATE and friends, because G_ADD_PRIVATE is using API > > > available since GLib 2.38 onwards. so you *really* want to use MAX_ALLOWED to > > > check the availability of the API, otherwise you'd implicitly bump up the > > > requirement for the generated code. > > > > I have always assumed that we don't support compiling against 2.38 and then > > running the resulting binary against 2.36's .so's, even if you only use 2.36's > > APIs. > > that's not the issue of this bug. It is though. If we don't support running new binaries against old glibs, and we don't support people disting/checking in generated files, then we don't need any version checks, because anyone who generates with the 2.38 gdbus-codegen will also compile against glib 2.38 and run against only glib 2.38 or later. If we don't support running new binaries against old glibs, and we do support (or at least tolerate) people disting/checking in generated files, then we need to do GLIB_VERSION_CHECK() (in case someone using 2.36 tries to build from a tree generated by someone using 2.38), but we still don't need to check GLIB_VERSION_MAX_ALLOWED. Your patch is correct if and only if we don't support disting/checking in generated files, and we *do* support running binaries against old glib as long as you specified MAX_ALLOWED when building. > if a project uses GLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_36 and generates code > with a gdbus-codegen coming from GLib 2.38, then the project will get an > implicit dependency bump on GLib 2.38 No, the *project* doesn't get a dependency, a single *build* of the project gets a dependency. Which is only a problem if you expect that build to be usable on older machines as well.
(In reply to comment #12) > (In reply to comment #11) > oint of MAX_ALLOWED is so that you can say "People on > > Fedora 18 are supposed to be able to compile this package, so warn me if I use > > any APIs that weren't available then". > > Ah sorry, yes, you're right. > > So I think the key point from comment 9 is: > "this will cause compiler warnings" > And that's just what we're trying to fix right? > > That'd make this bug just a variant of > https://bugzilla.gnome.org/show_bug.cgi?id=703191 > with s/G_DEFINE_TYPE/gdbus-codegen/ precisely.
ah, right, I wasn't considering the warnings themselves... if you generate+build+run against 2.38, you definitely *have* G_DEFINE_PRIVATE, and can safely use it, but if MAX_ALLOWED < 2.38, then you'll get warnings, which is obviously bad. So... ok, in that case, I think the patch is correct, codewise (as long as we don't support disting/checking in the generated code, which, based on the third patch, we don't). Since I got all confused about this, other people might too, so it might be nice to clarify in the comment or commit message that this is only about avoiding warnings, not about avoiding unavailable API.
This bug bit us badly in a downstream project which was using -DGLIB_VERSION_MIN_REQUIRED in combination with gdbus-codegen; the failure mode is that you get weird crashes since the private data isn't where it's expected. I tested these patches against master; the docs patch needed a closing </para>, which I've now rebased in and will push. Thanks Emmanuele!
Hello, Unfortunately this breaks the build of GIO on Windows/Visual C++ as it does not like #ifdef checks within a macro definition/usage. I have opened a bug[1] for this there, along with replacing snprintf() with g_snprintf() in gsubprocess.c. With blessings, thank you! [1]: https://bugzilla.gnome.org/show_bug.cgi?id=711049
*** Bug 711082 has been marked as a duplicate of this bug. ***