On Thu, Dec 22, 2011 at 1:46 AM, Zeeshan Ali (Khattak) <zeeshanak(a)gnome.org&gt; wrote: > On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) > <zeeshanak(a)gnome.org&gt; wrote: >> From: "Zeeshan Ali (Khattak)" <zeeshanak(a)gnome.org&gt; >> >> 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/d5e5c647...

The patch improves the situation as it makes the whole API very consistent w.r.t what exactly is the namespace 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.

> 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.

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(a)gnome.org&gt; wrote: > > On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) > > <zeeshanak(a)gnome.org&gt; wrote: > >> From: "Zeeshan Ali (Khattak)" <zeeshanak(a)gnome.org&gt; > >> > >> 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/d5e5c647... 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 :)

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

On Tue, Jan 3, 2012 at 5:31 PM, Christophe Fergeau <cfergeau(a)redhat.com&gt; 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?

On Tue, Jan 3, 2012 at 6:06 PM, Christophe Fergeau <cfergeau(a)redhat.com&gt; 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(a)redhat.com&gt; 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?

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.

On Thu, Dec 22, 2011 at 1:43 AM, Zeeshan Ali (Khattak) <zeeshanak(a)gnome.org&gt; wrote: > From: "Zeeshan Ali (Khattak)" <zeeshanak(a)gnome.org&gt; > > 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.