Bug 24394

Comment on attachment 30157 [details] [review]
patch for making tp-glib build work on windows

>diff --git a/telepathy-glib/debug.c b/telepathy-glib/debug.c
>index 51e20ae..edc2c10 100755
>--- a/telepathy-glib/debug.c
>+++ b/telepathy-glib/debug.c
>@@ -71,6 +71,12 @@
> 
> #include "debug-internal.h"
> 
>+#ifdef WIN32
>+#define STDIN_FILENO    0       /* standard input file descriptor */
>+#define STDOUT_FILENO   1       /* standard output file descriptor */
>+#define STDERR_FILENO   2       /* standard error file descriptor */
>+#endif

I think it'd actually be cleaner to use 1 and 2 unconditionally, with a suitable comment, since these constants are universal - Unix shell scripts basically all rely on fd 1 and 2 being stdout and stderr, for instance, and GLib just uses 1 and 2. I've written a patch to do this.

>@@ -457,13 +463,28 @@ tp_debug_timestamped_log_handler (const gchar *log_domain,

I'd rather not have this #ifdef mess; I've done a patch that uses g_time_val_to_iso8601 instead (this is a small behaviour change - the format changes, and it's no longer in local time - but it's only debug output).

>diff --git a/telepathy-glib/proxy.h b/telepathy-glib/proxy.h
>index bd6b3a9..8bb3027 100755
>--- a/telepathy-glib/proxy.h
>+++ b/telepathy-glib/proxy.h
>@@ -25,8 +25,18 @@
> #include <dbus/dbus-glib.h>
> #include <glib-object.h>
> 
>+#ifdef WIN32
>+#include <glib/gquark.h>
>+#endif

Why is this needed? <glib-object.h> includes <glib.h>, which includes <glib/gquark.h>... so why do you need to include this header explicitly?

>+#ifdef WIN32
>+#ifdef interface
>+#undef interface
>+#endif
>+#endif

Ugh. What is interface defined to in Win32-land? :-(

In most of the cases where we mention "interface" as an identifier, this is easy to work around by renaming the function parameter to "iface".

However, TpProxyClass has a member called "interface", which is part of our API...

We could work around this like so:

#define _TP_CPP_PASTE_TOKENS(a,b) a ## b
GQuark _TP_CPP_PASTE_TOKENS(i,nterface);

or we could just #undef interface. However, #undef'ing interface seems likely to break whatever API defines it...

Anyone wanting to access the ->interface member would also have to perform a similar hack, unless we add a tp_proxy_class_set_interface() accessor (which might not be a bad idea anyway).

When we break API we should probably rename this to dbus_interface or interface_name, but we're not doing that yet.

>+#if WIN32
>+#undef interface;
>+#endif

That semicolon isn't syntactically right. I've fixed this file by renaming the "interface" argument to "iface", instead.

Could you try building the 08-portability branch from my git repository on Windows, please? It's based on telepathy-glib 0.8.0 (which is in turn basically identical to 0.7.37 - there were some minor bugfixes in the examples, and no changes in the library).

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/08-portability

git://git.collabora.co.uk/git/user/smcv/telepathy-glib-smcv.git

For the moment, until we decide how to deal with it, you'll still need to do the "#undef interface" hack in proxy.h, but you shouldn't need to make any other changes now.

(In reply to comment #2)

Tested that 08-portability branch and it did work with just #undef interface" in proxy.h and changing STDOUT_FILENO, STDERR_FILENO to 1, 2 in debug.c

Created attachment 30472 [details] [review]
Current patch against 08-portability branch

Attached this to keep track of changes, that are needed to make 08-portability branch work in windows.

Comment on attachment 30472 [details] [review]
Current patch against 08-portability branch

isn't it better if we define, if STDOUT_FILENO...etc  are not defined under windows, and not replace these with magical intergers like 1,2..etc ?

(In reply to comment #5)
> (From update of attachment 30472 [details] [review])
> isn't it better if we define, if STDOUT_FILENO...etc  are not defined under
> windows, and not replace these with magical intergers like 1,2..etc ?
> 

Oops. I missed the 1# comment about this from Simon, So.. :) it's fine I guess

(Removing patch keyword; this isn't ready for review until we have a git branch that compiles without changes.)

Snapshot of the 08-portability branch, which I've rebased onto 0.8.1: http://people.collabora.co.uk/~smcv/telepathy-glib-0.8.1+08-portability.tar.gz

I think this is ready for review. The one remaining issue is 'interface' which I'm fairly sure isn't our fault:

15:44 < mattire> to me it looks telepathy-glib 0.8.1.1 is windows compatible 
                 tested with roster app
15:44 < smcv> mattire: \o/
15:44 < mattire> :)
15:44 < smcv> mattire: I wonder why interface isn't #define'd any more...
15:45 < smcv> mattire: oh argh I might know
15:45 < smcv> mattire: were you compiling tp-qt4 against it?
15:45 < smcv> mattire: it wouldn't surprise me if Qt defines 'interface' under 
              some circumstances
15:45 < mattire> yes, i have qt-tp4 messing things

(In reply to comment #9)
I recently found out that interface is defined in ObjBase.h in windows header,
it is defined as struct..
it gets sometimes included and sometimes not, when just building telepathy-glib
dll interface keyword does not cause problems, when continuing to build 
telepathy-gabble compatibility branch it comes back to haunt me again, when 
compiling ft-channel that includes connection.h that includes 
dbus-properties-mixin.h.
also i had to do #undef interface in dbus-properties-mixin.h, the #undef in proxy.h seemed not to have effect, 
I'm not sure how to deal with this issue, any suggestions are welcome..

(In reply to comment #10)
> (In reply to comment #9)
> I recently found out that interface is defined in ObjBase.h in windows header,
> it is defined as struct..

In that case, I consider this namespace pollution to be not our bug; if you're including ObjBase.h somewhere (e.g. Gabble) then the inclusion of that header should do the #undef.

dbus-properties-mixin.h doesn't include proxy.h (I don't think) so one #undef won't affect the other file. dbus-properties-mixin.h no longer mentions 'interface' so this shouldn't be a problem for you any more.

When one of the other telepathy-glib core developers reviews and approves this, I think we can integrate this branch.

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > I recently found out that interface is defined in ObjBase.h in windows header,
> > it is defined as struct..

For reviewers' benefit: in the corresponding Gabble bug, it has been noted that you have to #undef interface if you've included winsock2.h.

Fixed in master, will be in either 0.9.2 or 0.10.0, whichever happens first (most likely 0.10.0).

Since we have a new stable-branch release on the way soon anyway, I'm not going to merge this to 0.8.x; we can consider 0.10.x to be the one to use for Windows.

A cmake build system might follow in 0.11.x, depending how we get on with it in telepathy-qt4 (which is our nominated cmake early-adopter).