Re: [libvirt] [libvirt-glib] Correct namespace prefix for GVirConfig symbols
On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Breaks API and ABI on the fundamental level but lets fix this now while we don't guarantee any API/ABI stability.
Forgot to mention that this patch is on top of Christophe's ACK'ed but unmerged 'Add GVirConfigDomainSound' tree. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Thu, Dec 22, 2011 at 1:46 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Breaks API and ABI on the fundamental level but lets fix this now while we don't guarantee any API/ABI stability.
Forgot to mention that this patch is on top of Christophe's ACK'ed but unmerged 'Add GVirConfigDomainSound' tree.
And seems my patch went over the limit so it got chopped. You can find the patch here as well: https://gitorious.org/~zeenix/libvirt/zeenix-libvirt-glib/commit/d5e5c64732b... -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Thu, Dec 22, 2011 at 02:26:48AM +0200, Zeeshan Ali (Khattak) wrote:
On Thu, Dec 22, 2011 at 1:46 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Breaks API and ABI on the fundamental level but lets fix this now while we don't guarantee any API/ABI stability.
Forgot to mention that this patch is on top of Christophe's ACK'ed but unmerged 'Add GVirConfigDomainSound' tree.
And seems my patch went over the limit so it got chopped. You can find the patch here as well: https://gitorious.org/~zeenix/libvirt/zeenix-libvirt-glib/commit/d5e5c64732b...
For what it's worth, I don't think this patch improves the situation much if we can't express nested namespaces (ie put all the GVirConfigDomain* objects to a GVir::Config::Domain or GVirConfig::Domain namespace). Since it's pretty invasive, I'd lean toward not applying it, but I have no strong opinion either way, I'm fine if it goes in too. Let's see what danpb thinks about it :) Christophe
On Thu, Dec 22, 2011 at 5:17 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Thu, Dec 22, 2011 at 02:26:48AM +0200, Zeeshan Ali (Khattak) wrote:
On Thu, Dec 22, 2011 at 1:46 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Breaks API and ABI on the fundamental level but lets fix this now while we don't guarantee any API/ABI stability.
Forgot to mention that this patch is on top of Christophe's ACK'ed but unmerged 'Add GVirConfigDomainSound' tree.
And seems my patch went over the limit so it got chopped. You can find the patch here as well: https://gitorious.org/~zeenix/libvirt/zeenix-libvirt-glib/commit/d5e5c64732b...
For what it's worth, I don't think this patch improves the situation much if we can't express nested namespaces (ie put all the GVirConfigDomain* objects to a GVir::Config::Domain or GVirConfig::Domain namespace).
The patch improves the situation as it makes the whole API very consistent w.r.t what exactly is the namespace here. Curently that is supposed to be GVirConfig, judging from GIR and Vala bindings. I also agree that nested namespaces will be better. If we decide/manage to go towards nested namespaces, this patch actually helps in that regard as well since existing API is not consistent/correct for that purpose either.
I'm fine if it goes in too. Let's see what danpb thinks about it :)
I think his intention is pretty clear from the bindings but I can wait.
Christophe
-- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Thu, Dec 22, 2011 at 07:33:22PM +0200, Zeeshan Ali (Khattak) wrote:
The patch improves the situation as it makes the whole API very consistent w.r.t what exactly is the namespace here.
Imo the namespace really is GVir::Config, not a GVirConfig namespace totally separate from the GVir namespace, so it does not make the whole API "very consistent", it just changes things.
I also agree that nested namespaces will be better. If we decide/manage to go towards nested namespaces, this patch actually helps in that regard as well since existing API is not consistent/correct for that purpose either.
It helps *but breaks every library user*. Which is why you have to carefully weight the pros and cons. It makes things slightly nicer, slightly more consistent but *it breaks every user*. This is what makes it special and worth more considerations than a quick ack while everyone is on holidays.
I'm fine if it goes in too. Let's see what danpb thinks about it :)
I think his intention is pretty clear from the bindings but I can wait.
Yeah, under the assumption that what is in the bindings is right... Really, let's just wait until the holidays are over, as far as I'm concerned I wouldn't like having such a patch go in before I get a chance to see it even if I agree with it. Christophe
On Thu, Dec 22, 2011 at 7:55 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Thu, Dec 22, 2011 at 07:33:22PM +0200, Zeeshan Ali (Khattak) wrote:
The patch improves the situation as it makes the whole API very consistent w.r.t what exactly is the namespace here.
Imo the namespace really is GVir::Config, not a GVirConfig namespace totally separate from the GVir namespace, so it does not make the whole API "very consistent", it just changes things.
Even if the namespace is 'GVir::Config', my assertion that 'Config' is part of the namespace and not just a symbol prefix is pretty much correct so I don't understand what you are trying to discuss here.
I also agree that nested namespaces will be better. If we decide/manage to go towards nested namespaces, this patch actually helps in that regard as well since existing API is not consistent/correct for that purpose either.
It helps *but breaks every library user*.
Which user? Currently the API is hardly used by 2 apps. Keeping in mind that library is still at its infancy and missing a lot of essential API, this shouldn't be a concern at all since breaking things now is preferable to breaking it later when apps really depend on it and we promise some stability.
Which is why you have to carefully weight the pros and cons. It makes things slightly nicer, slightly more consistent but *it breaks every user*. This is what makes it special and worth more considerations than a quick ack while everyone is on holidays.
I disagree and think you sometimes worry way too much. :)
Really, let's just wait until the holidays are over, as far as I'm concerned I wouldn't like having such a patch go in before I get a chance to see it even if I agree with it.
Thats what I am going to do if there is no ACK for it anyway. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Thu, Dec 22, 2011 at 04:17:32PM +0100, Christophe Fergeau wrote:
On Thu, Dec 22, 2011 at 02:26:48AM +0200, Zeeshan Ali (Khattak) wrote:
On Thu, Dec 22, 2011 at 1:46 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Breaks API and ABI on the fundamental level but lets fix this now while we don't guarantee any API/ABI stability.
Forgot to mention that this patch is on top of Christophe's ACK'ed but unmerged 'Add GVirConfigDomainSound' tree.
And seems my patch went over the limit so it got chopped. You can find the patch here as well: https://gitorious.org/~zeenix/libvirt/zeenix-libvirt-glib/commit/d5e5c64732b...
For what it's worth, I don't think this patch improves the situation much if we can't express nested namespaces (ie put all the GVirConfigDomain* objects to a GVir::Config::Domain or GVirConfig::Domain namespace). Since it's pretty invasive, I'd lean toward not applying it, but I have no strong opinion either way, I'm fine if it goes in too. Let's see what danpb thinks about it :)
AFAICT, at the C level this is pretty much a no-op in terms of changes, just changing naming conventions for types. What is the actual effect on non-C language bindings that makes this compelling to change ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jan 3, 2012 at 4:31 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Dec 22, 2011 at 04:17:32PM +0100, Christophe Fergeau wrote:
On Thu, Dec 22, 2011 at 02:26:48AM +0200, Zeeshan Ali (Khattak) wrote:
On Thu, Dec 22, 2011 at 1:46 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Breaks API and ABI on the fundamental level but lets fix this now while we don't guarantee any API/ABI stability.
Forgot to mention that this patch is on top of Christophe's ACK'ed but unmerged 'Add GVirConfigDomainSound' tree.
And seems my patch went over the limit so it got chopped. You can find the patch here as well: https://gitorious.org/~zeenix/libvirt/zeenix-libvirt-glib/commit/d5e5c64732b...
For what it's worth, I don't think this patch improves the situation much if we can't express nested namespaces (ie put all the GVirConfigDomain* objects to a GVir::Config::Domain or GVirConfig::Domain namespace). Since it's pretty invasive, I'd lean toward not applying it, but I have no strong opinion either way, I'm fine if it goes in too. Let's see what danpb thinks about it :)
AFAICT, at the C level this is pretty much a no-op in terms of changes, just changing naming conventions for types. What is the actual effect on non-C language bindings that makes this compelling to change ?
I haven't really checked with other languages but vala tools get confused because we claim that GVirConfig is the namespace but then the macros aren't named accordingly. I can get you the exact errors I got from valac if you like but to me the problem is obvious and possible solutions are: 1. Use nested namespaces (not supported by GIR and based on past discussions about it, its unlikely GIR ever will). 2. Use same namespace 'GVir' (also not supported by GIR if we continue to have different gir/typelib files for each lib). 3. Continue using different namespaces for each lib and ensure all API is consistent with that approach. As it should be obvious now, #3 is the only viable solution here and hence my patch. :) -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Tue, Jan 03, 2012 at 05:18:41PM +0200, Zeeshan Ali (Khattak) wrote:
I haven't really checked with other languages but vala tools get confused because we claim that GVirConfig is the namespace but then the macros aren't named accordingly. I can get you the exact errors I got from valac if you like
Confused how? I was under the impression that you had been using libvirt-gconfig with vala in gnome-boxes without hitting any huge issues, but mostly missing API, is this a wrong impression I have? Christophe
On Tue, Jan 3, 2012 at 5:31 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Tue, Jan 03, 2012 at 05:18:41PM +0200, Zeeshan Ali (Khattak) wrote:
I haven't really checked with other languages but vala tools get confused because we claim that GVirConfig is the namespace but then the macros aren't named accordingly. I can get you the exact errors I got from valac if you like
Confused how? I was under the impression that you had been using libvirt-gconfig with vala in gnome-boxes without hitting any huge issues, but mostly missing API, is this a wrong impression I have?
*Mostly*, yes! During all these vacation, I lost track of which change exactly triggers this issue but it was some new API you added to the lib. Do you really need me to find out which one was it?
Christophe
-- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Tue, Jan 03, 2012 at 05:35:39PM +0200, Zeeshan Ali (Khattak) wrote:
On Tue, Jan 3, 2012 at 5:31 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Tue, Jan 03, 2012 at 05:18:41PM +0200, Zeeshan Ali (Khattak) wrote:
I haven't really checked with other languages but vala tools get confused because we claim that GVirConfig is the namespace but then the macros aren't named accordingly. I can get you the exact errors I got from valac if you like
Confused how? I was under the impression that you had been using libvirt-gconfig with vala in gnome-boxes without hitting any huge issues, but mostly missing API, is this a wrong impression I have?
*Mostly*, yes! During all these vacation, I lost track of which change exactly triggers this issue but it was some new API you added to the lib. Do you really need me to find out which one was it?
Well, you're trying to push a huge invasive change, it would be helpful to know understand what problems exactly you are having without this change so that an informed decision can be made... Christophe
On Tue, Jan 3, 2012 at 6:06 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Tue, Jan 03, 2012 at 05:35:39PM +0200, Zeeshan Ali (Khattak) wrote:
On Tue, Jan 3, 2012 at 5:31 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Tue, Jan 03, 2012 at 05:18:41PM +0200, Zeeshan Ali (Khattak) wrote:
I haven't really checked with other languages but vala tools get confused because we claim that GVirConfig is the namespace but then the macros aren't named accordingly. I can get you the exact errors I got from valac if you like
Confused how? I was under the impression that you had been using libvirt-gconfig with vala in gnome-boxes without hitting any huge issues, but mostly missing API, is this a wrong impression I have?
*Mostly*, yes! During all these vacation, I lost track of which change exactly triggers this issue but it was some new API you added to the lib. Do you really need me to find out which one was it?
Well, you're trying to push a huge invasive change, it would be helpful to know understand what problems exactly you are having without this change so that an informed decision can be made...
I already provided the needed information: vala gets confused because of the inconsistency in the API. Even if vala doesn't get confused, the inconsistency is a problem and will most definitely cause issues later so it needs to be resolved anyway. I also presented all the possible solutions to the problem (including yours) and showed the issues with every one of them but the one I provided. What exactly would the exact error from valac tell you that I haven't already told? FYI no matter how you solve this, there is no non-invasive solution possible since the problem is of a fundamental nature. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Tue, Jan 3, 2012 at 6:24 PM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
On Tue, Jan 3, 2012 at 6:06 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Tue, Jan 03, 2012 at 05:35:39PM +0200, Zeeshan Ali (Khattak) wrote:
On Tue, Jan 3, 2012 at 5:31 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Tue, Jan 03, 2012 at 05:18:41PM +0200, Zeeshan Ali (Khattak) wrote:
I haven't really checked with other languages but vala tools get confused because we claim that GVirConfig is the namespace but then the macros aren't named accordingly. I can get you the exact errors I got from valac if you like
Confused how? I was under the impression that you had been using libvirt-gconfig with vala in gnome-boxes without hitting any huge issues, but mostly missing API, is this a wrong impression I have?
*Mostly*, yes! During all these vacation, I lost track of which change exactly triggers this issue but it was some new API you added to the lib. Do you really need me to find out which one was it?
Well, you're trying to push a huge invasive change, it would be helpful to know understand what problems exactly you are having without this change so that an informed decision can be made...
I already provided the needed information: vala gets confused because of the inconsistency in the API. Even if vala doesn't get confused, the inconsistency is a problem and will most definitely cause issues later so it needs to be resolved anyway. I also presented all the possible solutions to the problem (including yours) and showed the issues with every one of them but the one I provided. What exactly would the exact error from valac tell you that I haven't already told?
So FWIW I found the patch in Boxes that causes a problem with current API: https://bugzilla.gnome.org/attachment.cgi?id=204140&action=edit Compiling that patch should lead you to this: vm-configurator.c: In function ‘boxes_vm_configurator_set_video_config’: vm-configurator.c:713:62: error: ‘GVIR_TYPE_DOMAIN_VIDEO_MODEL’ undeclared (first use in this function) vm-configurator.c:713:62: note: each undeclared identifier is reported only once for each function it appears in vm-configurator.c: In function ‘boxes_vm_configurator_set_sound_config’: vm-configurator.c:758:62: error: ‘GVIR_TYPE_DOMAIN_SOUND_MODEL’ undeclared (first use in this function) vm-configurator.c: In function ‘boxes_vm_configurator_set_tablet_config’: vm-configurator.c:804:62: error: ‘GVIR_TYPE_DOMAIN_INPUT_BUS’ undeclared (first use in this function) -- Regards, Zeeshan Ali (Khattak) FSF member#5124 P.S. It needs your recently patches to libvirt-glib.
On Tue, Jan 03, 2012 at 08:47:02PM +0200, Zeeshan Ali (Khattak) wrote:
So FWIW I found the patch in Boxes that causes a problem with current API:
https://bugzilla.gnome.org/attachment.cgi?id=204140&action=edit
Compiling that patch should lead you to this:
vm-configurator.c: In function ‘boxes_vm_configurator_set_video_config’: vm-configurator.c:713:62: error: ‘GVIR_TYPE_DOMAIN_VIDEO_MODEL’ undeclared (first use in this function) vm-configurator.c:713:62: note: each undeclared identifier is reported only once for each function it appears in vm-configurator.c: In function ‘boxes_vm_configurator_set_sound_config’: vm-configurator.c:758:62: error: ‘GVIR_TYPE_DOMAIN_SOUND_MODEL’ undeclared (first use in this function) vm-configurator.c: In function ‘boxes_vm_configurator_set_tablet_config’: vm-configurator.c:804:62: error: ‘GVIR_TYPE_DOMAIN_INPUT_BUS’ undeclared (first use in this function)
Ah great, thanks! At first I thought your patch was just about making things cleaner, so I was a bit surprised that you mentioned issues with vala just now. And having a clear problem to think about really helps to focus rather than " I promise you, I've seen some problem once, but I forgot all the details" The issue comes from typeof(GVirConfig.DomainVideoModel). Vala wants to change this to GVIR_TYPE_CONFIG_DOMAIN_VIDEO_MODEL but it doesn't really have a way of getting this right (I couldn't find an obvious way of getting this name from the .gir) so it ends up with GVIR_TYPE_DOMAIN_VIDEO_MODEL which doesn't exist. However, the .gir has: <enumeration name="DomainVideoModel" glib:type-name="GVirConfigDomainVideoModel" glib:get-type="gvir_config_domain_video_model_get_type" c:type="GVirConfigDomainVideoModel"> My feeling is that there would be less magic involved on the Vala side if it were to use the value of the glib:get-type attribute instead of trying to build a GVIR_TYPE_DOMAIN_VIDEO_MODEL symbol, but it may have very good reasons for doing things the way it does. Then I'm not saying because of what is above, your patch is not needed, it's just me trying to understand what kind of breakage we have because of the unusual namespacing. Christophe
On Wed, Jan 04, 2012 at 11:48:31AM +0400, Christophe Fergeau wrote:
On Tue, Jan 03, 2012 at 08:47:02PM +0200, Zeeshan Ali (Khattak) wrote:
So FWIW I found the patch in Boxes that causes a problem with current API:
https://bugzilla.gnome.org/attachment.cgi?id=204140&action=edit
Compiling that patch should lead you to this:
vm-configurator.c: In function ‘boxes_vm_configurator_set_video_config’: vm-configurator.c:713:62: error: ‘GVIR_TYPE_DOMAIN_VIDEO_MODEL’ undeclared (first use in this function) vm-configurator.c:713:62: note: each undeclared identifier is reported only once for each function it appears in vm-configurator.c: In function ‘boxes_vm_configurator_set_sound_config’: vm-configurator.c:758:62: error: ‘GVIR_TYPE_DOMAIN_SOUND_MODEL’ undeclared (first use in this function) vm-configurator.c: In function ‘boxes_vm_configurator_set_tablet_config’: vm-configurator.c:804:62: error: ‘GVIR_TYPE_DOMAIN_INPUT_BUS’ undeclared (first use in this function)
Ah great, thanks! At first I thought your patch was just about making things cleaner, so I was a bit surprised that you mentioned issues with vala just now. And having a clear problem to think about really helps to focus rather than " I promise you, I've seen some problem once, but I forgot all the details"
The issue comes from typeof(GVirConfig.DomainVideoModel). Vala wants to change this to GVIR_TYPE_CONFIG_DOMAIN_VIDEO_MODEL but it doesn't really have a way of getting this right (I couldn't find an obvious way of getting this name from the .gir) so it ends up with GVIR_TYPE_DOMAIN_VIDEO_MODEL which doesn't exist. However, the .gir has: <enumeration name="DomainVideoModel" glib:type-name="GVirConfigDomainVideoModel" glib:get-type="gvir_config_domain_video_model_get_type" c:type="GVirConfigDomainVideoModel"> My feeling is that there would be less magic involved on the Vala side if it were to use the value of the glib:get-type attribute instead of trying to build a GVIR_TYPE_DOMAIN_VIDEO_MODEL symbol, but it may have very good reasons for doing things the way it does.
Then I'm not saying because of what is above, your patch is not needed, it's just me trying to understand what kind of breakage we have because of the unusual namespacing.
So I think we're agreed now that Zeeshan's patch should be merged ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Jan 04, 2012 at 11:21:22AM +0000, Daniel P. Berrange wrote:
So I think we're agreed now that Zeeshan's patch should be merged ?
I don't really care either way :) At this point we should be able to update the users quite easily, so why not. Christophe
On Thu, Dec 22, 2011 at 01:46:12AM +0200, Zeeshan Ali (Khattak) wrote:
On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) <zeeshanak@gnome.org> wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Breaks API and ABI on the fundamental level but lets fix this now while we don't guarantee any API/ABI stability.
Forgot to mention that this patch is on top of Christophe's ACK'ed but unmerged 'Add GVirConfigDomainSound' tree.
I pushed it now. examples/config-demo.py needs fixing with this patch though. Christophe
participants (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Zeeshan Ali (Khattak)