Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

[Francois Gouget]

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

[Francois Gouget]

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", , , 0);
 surface.group_id = 0;
 surface.mem = QXLPHYSICAL_FROM_PTR(mem);
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 worker->create_primary_surface(worker, 0, );
+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;
 

Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

[Christophe Fergeau]

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

[Francois Gouget]

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

[Frediano Ziglio]

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

[Francois Gouget]

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

[Daniel P. Berrange]

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