Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros
On Mon, 17 Oct 2016, Francois Gouget wrote: [...] > > Imo this SPICE_NO_DEPRECATED is only meant to be used internally by > > spice-gtk even if this is not really documented. I would not consider it > > breakage if this was removed, or changed in incompatible ways. > > And it turns out it's actually useless as -Wno-deprecated-declarations > > is used during compilation (which also disables the > > warnings from GLIB_VERSION_MIN_REQUIRED/GLIB_VERSION_MAX_REQUIRED :( > > I'd tend to change that so that -Wno-deprecated-declarations is only > > used for spicy, and make selective use of > > G_GNUC_BEGIN_IGNORE_DEPRECATIONS / G_GNUC_END_IGNORE_DEPRECATIONS when > > needed. Seems to be working with quick and dirty local changes. > > I know you were talking about spice-gtk, but would the spice patch > below be what you had in mind? The dark side of G_GNUC_{BEGIN,END}_IGNORE_DEPRECATIONS is that it's not supported on RHEL 6.8: * GLib < 2.32 does not provide these macros, * and gcc <4.6 does not support the underlying pragmas. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros
On Thu, 1 Sep 2016, Christophe Fergeau wrote: > On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote: > > > > The following patch broke compilation of xf86-video-qxl: > > > > commit b41220b1441b8adea6db9a98e9da1b43a8f426bb > > Author: Christophe Fergeau > > Date: Thu Mar 5 15:28:22 2015 +0100 > > > > Mark unused public API methods/code as deprecated > > > > Acked-by: Jonathon Jongsma > > > > > > The reason is it introduces a #include in spice-server.h which > > is a *public* header! This means any application that needs to include > > spice-server.h, like xf86-video-qxl, must now check for GLib even if > > they don't use it, like xf86-video-qxl. > > > > This does not make sense and thus the glib.h include must be removed. > > As danpb pointed out, main reason for that is that we forgot to add > glib-2.0 as a dependency in the .pc file, if this had been done, this > change would probably have gone mostly unnoticed. No. The main reason is that the patch introduced a dependency which should not be there. Papering over it with a .pc file changes nothing to the fact that third-party projects will now be unable to use the header without installing glib first and that there is no good reason for that. [...] > Imo this SPICE_NO_DEPRECATED is only meant to be used internally by > spice-gtk even if this is not really documented. I would not consider it > breakage if this was removed, or changed in incompatible ways. > And it turns out it's actually useless as -Wno-deprecated-declarations > is used during compilation (which also disables the > warnings from GLIB_VERSION_MIN_REQUIRED/GLIB_VERSION_MAX_REQUIRED :( > I'd tend to change that so that -Wno-deprecated-declarations is only > used for spicy, and make selective use of > G_GNUC_BEGIN_IGNORE_DEPRECATIONS / G_GNUC_END_IGNORE_DEPRECATIONS when > needed. Seems to be working with quick and dirty local changes. I know you were talking about spice-gtk, but would the spice patch below be what you had in mind? commit 8b6bb2357ba41426d08c0f322440c19cb8e1b897 Author: Francois Gouget Date: Mon Oct 17 11:48:47 2016 +0200 server: Disable deprecation warnings only where needed Signed-off-by: Francois Gouget diff --git a/server/red-qxl.c b/server/red-qxl.c index e517b41..51e0dd6 100644 --- a/server/red-qxl.c +++ b/server/red-qxl.c @@ -971,6 +971,7 @@ void red_qxl_init(RedsState *reds, QXLInstance *qxl) qxl_state->dispatcher = dispatcher_new(RED_WORKER_MESSAGE_COUNT, NULL); qxl_state->qxl_worker.major_version = SPICE_INTERFACE_QXL_MAJOR; qxl_state->qxl_worker.minor_version = SPICE_INTERFACE_QXL_MINOR; +G_GNUC_BEGIN_IGNORE_DEPRECATIONS qxl_state->qxl_worker.wakeup = qxl_worker_wakeup; qxl_state->qxl_worker.oom = qxl_worker_oom; qxl_state->qxl_worker.start = qxl_worker_start; @@ -987,6 +988,7 @@ void red_qxl_init(RedsState *reds, QXLInstance *qxl) qxl_state->qxl_worker.reset_cursor = qxl_worker_reset_cursor; qxl_state->qxl_worker.destroy_surface_wait = qxl_worker_destroy_surface_wait; qxl_state->qxl_worker.loadvm_commands = qxl_worker_loadvm_commands; +G_GNUC_END_IGNORE_DEPRECATIONS qxl_state->max_monitors = UINT_MAX; qxl->st = qxl_state; diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c index b5baded..5dc173e 100644 --- a/server/red-replay-qxl.c +++ b/server/red-replay-qxl.c @@ -1241,7 +1241,9 @@ static void replay_handle_create_primary(QXLWorker *worker, SpiceReplay *replay) spice_printerr( "WARNING: %d: original recording event not preceded by a destroy primary", replay->counter); +G_GNUC_BEGIN_IGNORE_DEPRECATIONS worker->destroy_primary_surface(worker, 0); +G_GNUC_END_IGNORE_DEPRECATIONS } replay->created_primary = TRUE; @@ -1255,7 +1257,9 @@ static void replay_handle_create_primary(QXLWorker *worker, SpiceReplay *replay) read_binary(replay, "data", &size, &mem, 0); surface.group_id = 0; surface.mem = QXLPHYSICAL_FROM_PTR(mem); +G_GNUC_BEGIN_IGNORE_DEPRECATIONS worker->create_primary_surface(worker, 0, &surface); +G_GNUC_END_IGNORE_DEPRECATIONS } static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay, @@ -1264,15 +1268,21 @@ static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay, switch (message) { case RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE: case RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC: +G_GNUC_BEGIN_IGNORE_DEPRECATIONS replay_handle_create_primary(worker, replay); +G_GNUC_END_IGNORE_DEPRECATIONS break; case RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE: replay->created_primary = FALSE; +G_GNUC_BEGIN_IGNORE_DEPRECATIONS worker->destroy_primary_surface(worker, 0); +G_GNUC_END_IGNORE_DEPRECATIONS break; case RED_WORKER_MESSAGE_DESTROY_SURFACES: replay->created_primary
Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros
On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote: > > The following patch broke compilation of xf86-video-qxl: > > commit b41220b1441b8adea6db9a98e9da1b43a8f426bb > Author: Christophe Fergeau > Date: Thu Mar 5 15:28:22 2015 +0100 > > Mark unused public API methods/code as deprecated > > Acked-by: Jonathon Jongsma > > > The reason is it introduces a #include in spice-server.h which > is a *public* header! This means any application that needs to include > spice-server.h, like xf86-video-qxl, must now check for GLib even if > they don't use it, like xf86-video-qxl. > > This does not make sense and thus the glib.h include must be removed. As danpb pointed out, main reason for that is that we forgot to add glib-2.0 as a dependency in the .pc file, if this had been done, this change would probably have gone mostly unnoticed. > However it was added to get G_GNUC_DEPRECATED. Fortunately there is > SPICE_GNUC_DEPRECATED, or maybe that's SPICE_DEPRECATED? It gets a bit > confusing from here: > > First the macros to tag deprecated functions: > * spice uses G_GNUC_DEPRECATED directly in spice-server.h as seen above. > > * spice-gtk defines SPICE_DEPRECATED in spice-util.h as a wrapper around > G_GNUC_DEPRECATED, or an empty macro if SPICE_NO_DEPRECATED is > defined. > > * spice-gtk also has a SPICE_GNUC_DEPRECATED_FOR() macro which is not > defined anywhere else and a corresponding wrapper, > SPICE_DEPRECATED_FOR(), which is a no-op if SPICE_NO_DEPRECATED is > defined. Imo this SPICE_NO_DEPRECATED is only meant to be used internally by spice-gtk even if this is not really documented. I would not consider it breakage if this was removed, or changed in incompatible ways. And it turns out it's actually useless as -Wno-deprecated-declarations is used during compilation (which also disables the warnings from GLIB_VERSION_MIN_REQUIRED/GLIB_VERSION_MAX_REQUIRED :( I'd tend to change that so that -Wno-deprecated-declarations is only used for spicy, and make selective use of G_GNUC_BEGIN_IGNORE_DEPRECATIONS / G_GNUC_END_IGNORE_DEPRECATIONS when needed. Seems to be working with quick and dirty local changes. SPICE_NO_DEPRECATED is then not really needed. > > * spice-protocol defines SPICE_GNUC_DEPRECATED in spice/macro.h but > uses the gcc __attribute__ macro directly instead of using > G_GNUC_DEPRECATED. Yup, mostly because spice-protocol does not depend on glib (and does not use anything related to glib for that matter). > Second the macros to 'enable/disable' deprecated APIs: > * In spice-gtk defining SPICE_NO_DEPRECATED disables the warnings. See above, my thinking is that this is mainly meant for internal use. > > * In spice-protocol you must define SPICE_DEPRECATED to get access to > the deprecated vd_agent.h VD_AGENT_CLIPBOARD_MAX_SIZE_DEFAULT and > VD_AGENT_CLIPBOARD_MAX_SIZE_ENV macros. > > Of course spice-gtk's source files use both spice-util.h and vd_agent.h, > meaning they necessarily get the spice-protocol deprecated macros due to > the SPICE_DEPRECATED naming conflict. Ah, not a big deal, but not nice :( > So here is the proposed solution: > > * spice-protocol's SPICE_DEPRECATED is in a public header so keep it as > is. Extend it to mean the user wants to use deprecated APIs. This > includes: > - Defining deprecated APIs and macros (as before). > - Disabling warnings about the use of deprecated APIs so it takes > over spice-gtk's SPICE_NO_DEPRECATED role. > > * Disable spice-protocol's SPICE_GNUC_DEPRECATED warnings when > SPICE_DEPRECATED is defined. Hm, I can see some merit with having a way to disable deprecation warnings per-module rather than globally through -Wno-deprecated-declarations. How big of a mess would a more explicit/similar to glib name cause? SPICE_DISABLE_DEPRECATION_WARNINGS Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros
On Thu, 11 Aug 2016, Francois Gouget wrote: [...] > So here is the proposed solution: > > * spice-protocol's SPICE_DEPRECATED is in a public header so keep it as > is. Extend it to mean the user wants to use deprecated APIs. This > includes: > - Defining deprecated APIs and macros (as before). > - Disabling warnings about the use of deprecated APIs so it takes > over spice-gtk's SPICE_NO_DEPRECATED role. > > * Disable spice-protocol's SPICE_GNUC_DEPRECATED warnings when > SPICE_DEPRECATED is defined. > > * Add SPICE_GNUC_DEPRECATED_FOR() to spice-protocol next to > SPICE_GNUC_DEPRECATED. The drawback of that part is that it makes the new spice-protocol incompatible with the old spice-gtk code because it causes redefines in the latter. If that's not acceptable then it means we need to avoid macro names that spice-gtk defines without checking if they already exist: SPICE_GNUC_DEPRECATED_FOR SPICE_DEPRECATED_FOR SPICE_DEPRECATED Also it feels like GNUC makes these macro names too specific: they might be extended to work with other compilers later. So we could use: SPICE_DEPRECATED_API SPICE_DEPRECATED_API_FOR Suggestions? Should I go ahead with this? -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros
> > On Thu, 11 Aug 2016, Daniel P. Berrange wrote: > > > On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote: > > > > > > The following patch broke compilation of xf86-video-qxl: > > > > > > commit b41220b1441b8adea6db9a98e9da1b43a8f426bb > > > Author: Christophe Fergeau > > > Date: Thu Mar 5 15:28:22 2015 +0100 > > > > > > Mark unused public API methods/code as deprecated > > > > > > Acked-by: Jonathon Jongsma > > > > > > > > > The reason is it introduces a #include in spice-server.h which > > > is a *public* header! This means any application that needs to include > > > spice-server.h, like xf86-video-qxl, must now check for GLib even if > > > they don't use it, like xf86-video-qxl. > > > > They shouldn't have to check for it explicitly. The spice-server.pc > > pkgconfig file should have been updated in that commit so that glib-2.0 > > is listed under "Requires" instead of "Requires.private", then applications > > using spice-server would not have broken, as pkg-config would have just > > "done the right thing" and automatically added -I/path/to/glib/headers. > > Even so, while being Spice tradition, the naming conflicts and code > duplication should be fixed. So I think my patchset still stands and > removing the need for this glib.h include is a nice side benefit. > I agree on removing the header dependency. Patch is small and make sense instead of adding the dependency to all projects just using the headers. Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros
On Thu, 11 Aug 2016, Daniel P. Berrange wrote: > On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote: > > > > The following patch broke compilation of xf86-video-qxl: > > > > commit b41220b1441b8adea6db9a98e9da1b43a8f426bb > > Author: Christophe Fergeau > > Date: Thu Mar 5 15:28:22 2015 +0100 > > > > Mark unused public API methods/code as deprecated > > > > Acked-by: Jonathon Jongsma > > > > > > The reason is it introduces a #include in spice-server.h which > > is a *public* header! This means any application that needs to include > > spice-server.h, like xf86-video-qxl, must now check for GLib even if > > they don't use it, like xf86-video-qxl. > > They shouldn't have to check for it explicitly. The spice-server.pc > pkgconfig file should have been updated in that commit so that glib-2.0 > is listed under "Requires" instead of "Requires.private", then applications > using spice-server would not have broken, as pkg-config would have just > "done the right thing" and automatically added -I/path/to/glib/headers. Even so, while being Spice tradition, the naming conflicts and code duplication should be fixed. So I think my patchset still stands and removing the need for this glib.h include is a nice side benefit. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros
On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote: > > The following patch broke compilation of xf86-video-qxl: > > commit b41220b1441b8adea6db9a98e9da1b43a8f426bb > Author: Christophe Fergeau > Date: Thu Mar 5 15:28:22 2015 +0100 > > Mark unused public API methods/code as deprecated > > Acked-by: Jonathon Jongsma > > > The reason is it introduces a #include in spice-server.h which > is a *public* header! This means any application that needs to include > spice-server.h, like xf86-video-qxl, must now check for GLib even if > they don't use it, like xf86-video-qxl. They shouldn't have to check for it explicitly. The spice-server.pc pkgconfig file should have been updated in that commit so that glib-2.0 is listed under "Requires" instead of "Requires.private", then applications using spice-server would not have broken, as pkg-config would have just "done the right thing" and automatically added -I/path/to/glib/headers. 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 :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros
The following patch broke compilation of xf86-video-qxl: commit b41220b1441b8adea6db9a98e9da1b43a8f426bb Author: Christophe Fergeau Date: Thu Mar 5 15:28:22 2015 +0100 Mark unused public API methods/code as deprecated Acked-by: Jonathon Jongsma The reason is it introduces a #include in spice-server.h which is a *public* header! This means any application that needs to include spice-server.h, like xf86-video-qxl, must now check for GLib even if they don't use it, like xf86-video-qxl. This does not make sense and thus the glib.h include must be removed. However it was added to get G_GNUC_DEPRECATED. Fortunately there is SPICE_GNUC_DEPRECATED, or maybe that's SPICE_DEPRECATED? It gets a bit confusing from here: First the macros to tag deprecated functions: * spice uses G_GNUC_DEPRECATED directly in spice-server.h as seen above. * spice-gtk defines SPICE_DEPRECATED in spice-util.h as a wrapper around G_GNUC_DEPRECATED, or an empty macro if SPICE_NO_DEPRECATED is defined. * spice-gtk also has a SPICE_GNUC_DEPRECATED_FOR() macro which is not defined anywhere else and a corresponding wrapper, SPICE_DEPRECATED_FOR(), which is a no-op if SPICE_NO_DEPRECATED is defined. * spice-protocol defines SPICE_GNUC_DEPRECATED in spice/macro.h but uses the gcc __attribute__ macro directly instead of using G_GNUC_DEPRECATED. Second the macros to 'enable/disable' deprecated APIs: * In spice-gtk defining SPICE_NO_DEPRECATED disables the warnings. * In spice-protocol you must define SPICE_DEPRECATED to get access to the deprecated vd_agent.h VD_AGENT_CLIPBOARD_MAX_SIZE_DEFAULT and VD_AGENT_CLIPBOARD_MAX_SIZE_ENV macros. Of course spice-gtk's source files use both spice-util.h and vd_agent.h, meaning they necessarily get the spice-protocol deprecated macros due to the SPICE_DEPRECATED naming conflict. So here is the proposed solution: * spice-protocol's SPICE_DEPRECATED is in a public header so keep it as is. Extend it to mean the user wants to use deprecated APIs. This includes: - Defining deprecated APIs and macros (as before). - Disabling warnings about the use of deprecated APIs so it takes over spice-gtk's SPICE_NO_DEPRECATED role. * Disable spice-protocol's SPICE_GNUC_DEPRECATED warnings when SPICE_DEPRECATED is defined. * Add SPICE_GNUC_DEPRECATED_FOR() to spice-protocol next to SPICE_GNUC_DEPRECATED. * Extend spice-protocol's SPICE_GNUC_DEPRECATED to use G_GNUC_DEPRECATED if available. This makes it a portable drop-in replacement for G_GNUC_DEPRECATED in Spice's code, particularly in spice-server.h. This implies spice and spice-gtk will need an up-to-date spice-protocol to compile. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel