GNOME Bugzilla – Bug 738896
Vala API changed at b1331919, breaks gnome-calculator
Last modified: 2014-10-26 20:32:41 UTC
b13319198f3ed38db92b777407249bfbf51628ee changes the signature of gtk_source_completion_context_get_iter to return a gboolean when the old version returned void. When a function returns void and has an out struct argument Vala's GIR parser will automatically convert convert the signature to a return value. Usually this is quite nice, as it lets people use the API much more naturally (iter = foo.get_iter() instead of foo.get_iter(out iter)), but in this case it has created a rather annoying problem. Since the return value is no longer void Vala can no longer change the signature, so we end up with an API break since the VAPI changes from public Gtk.TextIter get_iter (); to public bool get_iter (out Gtk.TextIter iter); AFAICT there are basically three ways to deal with this: 1. Revert the change in the C API, maybe adding new function to provide the new functionality. 2. Use metadata to hide the return value and revert to the old signature. We could also add an alternative binding for the new signature if Vala users need to be able to access the new functionality. 3. Live with the API break, patch gnome-calculator (and any other software which uses this API from Vala, if there is any). I'm happy to provide a patch for gtksourceview (or gnome-calculator), depending on how you want to proceed.
Thanks Evan for tracking this down! General note: once this is fixed, please revert https://git.gnome.org/browse/gnome-continuous/commit/?id=fa9179b34fe58fd16d4d48906ca3a7f26203eaa4
as a side note: this change is perfectly API and ABI-compatible in C. functions returning void can be compatibly changed to return a value; we do this in many places. in this particular case, it's the interaction between Vala and a C function with an out parameter — which may be an indication that this ought to be fixed at the VAPI level.
I'd rather keep the new API in C. It is an unfortunate incident, but given the amount of vala users of this API I'd just go ahead and fix gnome-calculator. If the maintainers do not agree we can explore working around this in the vapi
Vala.... Vala tries to simplify the life of programmers, by doing things like changing an out parameter by a return value. This is initially a bad idea in my opinion. The Vala code should be fixed. If you still want to use Vala, you have to live with that. Adding an extra layer to the C language (here valac) adds more bugs and maintenance problems. The time saved with the higher-level syntax is completely lost with many bugs in the vapis, a few bugs in valac, maintenance problems like this one, reading the C documentation when programming in Vala (so sometimes the developer needs to read the vapi), etc. Is Vala really worth it?
Paolo, the added code doesn't seem to add any gain in adding a bool return there [1]. Those should be assertions: "This should never happen: context should be always be created providing a position iter" if (mark_buffer == NULL) <<-- programmer error, the mark has been deleted from the buffer if (view == NULL) <<-- should never happen, isn't the completion associated with a view always available? if (completion_buffer != mark_buffer) <<-- programmer error, using a mark from a different buffer The first and last assertion are the same reason for which gtk_text_buffer_get_iter_at_mark does not return a boolean value. I'm not here to defend Vala in any way, however by acknowledging the bool return as useless we solve this issue painlessly for everybody. Unless I'm missing it's usefulness of course :-) [1] https://git.gnome.org/browse/gtksourceview/commit/?h=b13319198f3ed38db92b777407249bfbf51628ee
GtkSourceCompletion has a weak ref to GtkSourceView. So the view can be NULL. A view can change its buffer, so the buffer can be different or NULL. There was also a FIXME comment. Thus the boolean return value is useful, even if in practice there was no crash in gedit, it's better to make the code more robust, maybe other applications than gedit use the API a bit differently. Paolo actually had a crash in gedit with the completion, but unfortunately without a lot of debug info, so it was difficult to diagnose, and the CompletionContext is maybe the cause (maybe). The code change in Vala is really trivial, you can even ignore the return value, but it's better to handle it. So I close this bug, there is nothing we can really do, a Vala codebase has simply more maintenance burden. So I move this bug to gnome-calculator.
Created attachment 289078 [details] [review] vala: provide backwards-compatibleSourceCompletionContext.get_iter (In reply to comment #6) > The code change in Vala is really trivial, you can even ignore the return > value, but it's better to handle it. So I close this bug, there is nothing we > can really do, a Vala codebase has simply more maintenance burden. This is is what you can do, as described in my original comment. Old code keeps working, people who want can use try_get_iter instead of get_iter (IDK if the name makes sense, I'm not familiar with the API). I'm not saying this is how you *should* fix it, but it seems like you don't understand that this is a possibility, hopefully seeing code will help with that. It results in public Gtk.TextIter get_iter (); [CCode (cname = "gtk_source_completion_context_get_iter")] public bool try_get_iter (out Gtk.TextIter iter);
Created attachment 289079 [details] [review] Update code to handle GtkSourceView API break. Patch for gnome-calculator which is necessary if no changes are made to gtksourceview.
(In reply to comment #4) > Vala.... Vala tries to simplify the life of programmers, by doing things like > changing an out parameter by a return value. This is initially a bad idea in my > opinion. Vala would not be very fun if we had to use out parameters all over the place.
Review of attachment 289079 [details] [review]: This is desirable regardless of whether you use get_iter() or try_get_iter().
Sébastien: I do not think bashing Vala in general is a good use of this bugzilla report, there are other forums for those kind of discussion, so please refrain from that. Luca: whether the bool is useful or not, at this point it is out in a release so there is code out there that uses foo = get_iter() in C, so even reverting the change would be an API break. Between the two API breaks I prefer breaking vala, sorry :-) Given that the gnome-calculator patch actually makes the code nicer, it seems to me that in the grand scheme of things the return value is useful, so I'd prefer to avoid the vapi workaround unless more complains pop up
(In reply to comment #11) > Given that the gnome-calculator patch actually makes the code nicer, it seems > to me that in the grand scheme of things the return value is useful, so I'd > prefer to avoid the vapi workaround unless more complains pop up Well since you've chosen to distribute a vapi, you really ought to consider yourselves responsible for the Vala API as well as the C API. Evan's patch in comment #7 is a small fix and my recommendation is to just apply it. Then gnome-calculator can switch to using try_get_iter() anyway, since it's nicer, without breaking the build with earlier versions of gtksourceview.
try_get_iter doesn't exist in earlier builds of gtksourceview, so switching to it will bump your dependency regardless. The advantage of the patch it is that consumers don't *have* to switch, and that projects which don't switch will continue working with past and future versions of gtksourceview.
(In reply to comment #13) > try_get_iter doesn't exist in earlier builds of gtksourceview, so switching to > it will bump your dependency regardless. The advantage of the patch it is that > consumers don't *have* to switch, and that projects which don't switch will > continue working with past and future versions of gtksourceview. Sorry, I meant "without breaking the build of earlier versions of gnome-calculator with newer versions of gtksourceview." E.g. I was bisecting Calculator earlier today, and I had to patch GtkSourceView; for today, not a problem, but when I have to do this again two years from now, searching for this bug to find the GtkSourceView patch that makes Calculator build, that's going to be pretty annoying....
I understand that this is annoying I am sorry we caused this. However also adding the vapi compatibility layer has its costs: it means every app from now on should use the try_ variant, which differs from the C api and thus has all the related problems in terms of consistency and documentation. Each solution has its drawbacks... all in all I still feel like in the long run just fixing this in calculator is the better way forward in this particular case
Sorry for the late entry in the discussion. I have another doubt regarding the stable 3.14 branch. Do we bump the requirement in stable branch as well? Or do we bump it only for development branch. The issue is, when distros upgrade one of the softwares, and not both togather (which at least Arch already did), there will be errors while building Calculator, and they won't be able to release newer 3.14 stable branch of Calculator. Regarding the VAPI vs Calculator patch, I think it would be better, if instead of changing the signature of current get_iter (), if we can implement new API as try_get_iter (), and keep the get_iter () as it is right now, so that the final VAPI has these two methods. (similar to what Evan suggested) public Gtk.TextIter get_iter (); public bool try_get_iter (out Gtk.TextIter iter); We can also mark get_iter () as deprecated in current build (so that softwares will be forced to switch to try_get_iter () in upcoming major releases). This will make sure interoperability of all stable softwares with GtkSourceView.
(In reply to comment #16) > I have another doubt regarding the stable 3.14 branch. > Do we bump the requirement in stable branch as well? Or do we bump it only for > development branch. > The modified API is in gtksourceview 3.14.0. I think you should require that both in the devel version and in the 3.14 version of calculator. > The issue is, when distros upgrade one of the softwares, and not both togather > (which at least Arch already did), there will be errors while building > Calculator, and they won't be able to release newer 3.14 stable branch of > Calculator. > gtksourceview is part of gnome and distros should ship 3.14 version of all gnome components. > > Regarding the VAPI vs Calculator patch, I think it would be better, if instead > of changing the signature of current get_iter (), if we can implement new API > as try_get_iter (), and keep the get_iter () as it is right now, so that the > final VAPI has these two methods. (similar to what Evan suggested) > This is what was suggested above, but as I explained in comment #15 I think the drawbacks of this approach are greater than the advantages. > This will make sure interoperability of all stable softwares with > GtkSourceView. At the end of the day the ABI compatibility at runtime is OK. What we are requiring is that gnome-calculator 3.14 requires gtksourceview 3.14. As said above I *am* sorry that this happened, but the cat is already out of the bag...
No the change is only for git master, not gnome-3-14. For what it's worth, I've already updated LaTeXila: https://git.gnome.org/browse/latexila/commit/?id=482a94936cf363ac9246e00b1d12b632aab2e42c The patch is really straightforward, and it improves the robustness of the code. And GtkSource-3.0.metadata should not exist in the first place, its content should shrink, not grow.
(In reply to comment #18) > And GtkSource-3.0.metadata should not exist in the first place, its content > should shrink, not grow. Honestly, if you're going to break the vala bindings just two months after adding them, and have no intention of maintaining the metadata file, then maybe you shouldn't distribute them at all... when bindings distributed by Vala break, that reflects on the qualify of Vala as a programming language, but when bindings distributed by a library break, that reflects on the library. Anyway, Arth, I guess the thing for us to do is firstly branch gnome-3-14, then bump the GtkSourceView dependency to unreleased 3.15.1, then apply this patch.
(In reply to comment #17) > The modified API is in gtksourceview 3.14.0. I think you should require that > both in the devel version and in the 3.14 version of calculator. (In reply to comment #19) > Anyway, Arth, I guess the thing for us to do is firstly branch gnome-3-14, then > bump the GtkSourceView dependency to unreleased 3.15.1, then apply this patch. Michael, I'm a little confused here. Both of these are contradictory. The new API is already in 3.14.0. This means that if we don't bump the requirement for gnome-3-14 branch, soon people won't be able to compile Calculator 3.14, on 3.14 development environment. Also, now that we have fix for many other issues in Calculator 3.14, we can release 3.14.1 after we stabilize this issue.
No, I repeat, the *Vala* API change is only for GtkSourceView 3.15 (git master, *not* for GNOME 3.14).
Comment on attachment 289079 [details] [review] Update code to handle GtkSourceView API break. Actually, thinking about this some more, I'm concerned that the bindings distributed by gtksourceview are clearly considered a secondary concern ("the *vala* API change" when nothing has changed except in gtksourceview) and I'm not looking forward to doing this again in two months. Please look for a solution that preserves API compatibility, so that users can compile calculator regardless of whether they are using the gtksourceview bindings distributed by vala or the ones distributed by gtksourceview. If you are really sure you want to break API, then the vapi should be distributed by gnome-calculator so we don't have to worry about future changes either.
Just update the GtkSourceView version requirement in gnome-calculator and you're done. Both GtkSourceView and gnome-calculator are part of GNOME, so it's anyway a bad idea to install e.g. GtkSourceView 3.16 with gnome-calculator 3.14. And GtkSourceView isn't to blame here, the problem comes from Vala. An out parameter should remain an out parameter, not a return value. If you think a return value is more convenient, then you have to accept maintenance problems such as this one without blaming the library. And adding an hack in the metadata is not a good solution, it would add more confusion for those using the API in Vala in the future.
No, I don't want to depend on your bindings; you're just going to cause more breakage in the future. Seriously, you should not distribute those at all; things worked fine when they were distributed and maintained by the Vala developers.
Created attachment 289278 [details] [review] Add gtksourceview bindings The gtksourceview developers don't intend to maintain a stable vala API.
Created attachment 289289 [details] [review] Add gtksourceview bindings Forgot to distribute the bindings
(In reply to comment #25) > The gtksourceview developers don't intend to maintain a stable vala API. I don't understand how maintaining our own copy of vapi helps us in this situation. If you have to run bisect, that would anyways not work with the added VAPI either. Now, from the perspective of maintaining the VAPI file, IMO, it's better that either vala or GtkSourceView teams maintain them (assuming, there won't be many sudden and significant API changes in future).
(In reply to comment #27) > (In reply to comment #25) > > The gtksourceview developers don't intend to maintain a stable vala API. > > I don't understand how maintaining our own copy of vapi helps us in this > situation. If you have to run bisect, that would anyways not work with the > added VAPI either. > > Now, from the perspective of maintaining the VAPI file, IMO, it's better that > either vala or GtkSourceView teams maintain them (assuming, there won't be many > sudden and significant API changes in future). I really don't expect there to be many API breaks in the future. API breaks used to be common in Vala bindings, but now that most projects (including gtksourceview) have mostly correct GObject Introspection annotations for us to work with things are pretty stable. I've been maintaining a lot of Vala bindings for quite a while now, and I don't remember this particular issue ever popping up before. A method which takes an out argument, has a void return value, and the argument is a struct (the vast majority of types are classes, not structs) is fairly uncommon. The odds of one of those methods being changed to have a return value are quite low. The odds of all those things coming together are *very* low. The are really only two other things I can think of which might cause an API break in the future: * Changing the nullability of an argument to a virtual method will break implementations of that method in Vala software which is implementing that vfunc in a subclass (Vala currently requires the nullability to match). If someone who knows the API wants to go through and check the nullability of those arguments now that should pretty much eliminate that issue. * Changing the ownership of a return value from unowned (transfer none) to owned (transfer full or container) could cause an issue if a consumers is assigning the value to an explicitly typed unowned variable instead of allowing Vala to infer the type. Most people don't do that, and at this point it's likely that the return value transfer annotations in gtksourceview are already correct. Whether or not you want to ship a copy of the gtksourceview bindings is up to you. I understand your frustration at gtksourceview deciding not to maintain backwards compatibility in this case, but TBH I doubt there will be much opportunity for it to become a pattern.
My recommendation is to not depend on bindings if the maintainers of the bindings have clearly expressed that they are not stable. There is little downside to carrying our own stable copy of the bindings; they could break if the C API changes, but that seems very unlikely. I'm not sure why relying on unstable API is even worth considering, regardless of the chances of future breakage, just as a matter of principle. This feels like the wild west.... Anyway, Yosef pushed a build fix already, so let's clean that up at least.
The following fix has been pushed: 35b8638 Better handle GtkSourceView API break
Created attachment 289381 [details] [review] Better handle GtkSourceView API break Use the return value instead of weird hacks to see if this succeeded or not; this is identical to Evan Nemerson's original patch.
The following fix has been pushed: abc1c90 Depend on unreleased GtkSourceView
Created attachment 289382 [details] [review] Depend on unreleased GtkSourceView For their API break
The following fix has been pushed: 4494638 Revert "manifest: Remove gnome-calculator for now"
Created attachment 289383 [details] [review] Revert "manifest: Remove gnome-calculator for now" This reverts commit fa9179b34fe58fd16d4d48906ca3a7f26203eaa4.