Bug 27244

Summary: Wocky does not build on Windows with MSVC
Product: Wocky Reporter: Ole André Vadla Ravnås <ole.andre.ravnas>
Component: GeneralAssignee: Sjoerd Simons <sjoerd>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 27488    
Attachments: Don't include unistd.h unless available
Add a wrapper for realpath to support non-POSIX OSes
Fix a trivial warning where guint was used instead of WockyTLSCertStatus
Add a missing cast to make MSVC a bit happier warning-wise
Fix warnings related to DEBUG() usage without arguments
Make MUC timestamp parsing portable
Fix OpenSSL include issue on Windows
Replace CA/CRL stat logic with g_file_test to improve portability
Avoid C99 named initializers
Replace isXXX() with g_ascii_isXXX() to improve portability
Fix error_to_string() crash

Description Ole André Vadla Ravnås 2010-03-22 08:41:37 UTC
Created attachment 34312 [details] [review]
Don't include unistd.h unless available

See the attached patches (which are a bit rough).
Comment 1 Ole André Vadla Ravnås 2010-03-22 08:42:23 UTC
Created attachment 34313 [details] [review]
Add a wrapper for realpath to support non-POSIX OSes
Comment 2 Ole André Vadla Ravnås 2010-03-22 08:43:12 UTC
Created attachment 34314 [details] [review]
Fix a trivial warning where guint was used instead of WockyTLSCertStatus
Comment 3 Ole André Vadla Ravnås 2010-03-22 08:43:36 UTC
Created attachment 34315 [details] [review]
Add a missing cast to make MSVC a bit happier warning-wise
Comment 4 Ole André Vadla Ravnås 2010-03-22 08:44:00 UTC
Created attachment 34316 [details] [review]
Fix warnings related to DEBUG() usage without arguments
Comment 5 Ole André Vadla Ravnås 2010-03-22 08:44:38 UTC
Created attachment 34317 [details] [review]
Make MUC timestamp parsing portable

Could need some careful review.
Comment 6 Ole André Vadla Ravnås 2010-03-22 08:45:06 UTC
Created attachment 34318 [details] [review]
Fix OpenSSL include issue on Windows
Comment 7 Ole André Vadla Ravnås 2010-03-22 08:45:36 UTC
Created attachment 34319 [details] [review]
Replace CA/CRL stat logic with g_file_test to improve portability
Comment 8 Ole André Vadla Ravnås 2010-03-22 08:46:06 UTC
Created attachment 34320 [details] [review]
Avoid C99 named initializers
Comment 9 Ole André Vadla Ravnås 2010-03-22 08:46:33 UTC
Created attachment 34321 [details] [review]
Replace isXXX() with g_ascii_isXXX() to improve portability
Comment 10 Ole André Vadla Ravnås 2010-03-22 08:47:25 UTC
Created attachment 34322 [details] [review]
Fix error_to_string() crash
Comment 11 Ole André Vadla Ravnås 2010-03-22 08:50:19 UTC
That's all so far. Am now able to connect to Google Talk from my Vala/GTK+ test application.

Regarding the first patch: configure.ac might need to be updated, not certain if any of the existing M4 macros check for unistd.h. (Haven't had time to test this on a UNIX system yet.)
Comment 12 Simon McVittie 2010-03-23 05:48:13 UTC
Do you have a git repo/branch online somewhere for this sort of thing? We can resurrect your git.collabora.co.uk account if that'd help?
Comment 13 Simon McVittie 2010-03-23 05:50:47 UTC
Comment on attachment 34312 [details] [review]
Don't include unistd.h unless available

>Subject: [PATCH 01/12] Don't include unistd.h unless available

Looks good in principle. We don't normally wrap config.h in HAVE_CONFIG_H: is creating a stub config.h problematic on Windows?

> #include <stdio.h>
>+#ifdef HAVE_UNISTD_H
> #include <unistd.h>
>+#endif
> #include <stdlib.h>
> #include <string.h>

I might add a patch to do the unconditional and conditional includes separately, and add a bit of whitespace:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#ifdef HAVE_HYPOTHETICAL_H
# include <hypothetical.h>
#endif
#ifdef HAVE_UNISTD_H
# include <unistd.h>
#endif
Comment 14 Simon McVittie 2010-03-23 05:58:46 UTC
Comment on attachment 34313 [details] [review]
Add a wrapper for realpath to support non-POSIX OSes

>Subject: [PATCH 02/12] Add a wrapper for realpath to support non-POSIX OSes

>+gchar *
>+wocky_normalise_path (const gchar *path)
>+{
>+#ifdef G_OS_WIN32
>+  if (!g_path_is_absolute (path))
>+    return NULL;
>+  return g_strdup (path);
>+#else
>+  return realpath (path, NULL);
>+#endif
>+}

I'm pretty sure that's wrong for relative paths. Wouldn't we be better off using g_file_resolve_relative_path (g_file_new_for_path (g_get_current_dir ()), path) on Windows?

(If we don't really care about normalizing symlinks, we could probably even do the same on Unix.)
Comment 15 Simon McVittie 2010-03-23 06:00:56 UTC
Comment on attachment 34314 [details] [review]
Fix a trivial warning where guint was used instead of WockyTLSCertStatus

>Subject: [PATCH 03/12] Fix a trivial warning where guint was used instead of WockyTLSCertStatus

Looks good
Comment 16 Simon McVittie 2010-03-23 06:01:31 UTC
Comment on attachment 34315 [details] [review]
Add a missing cast to make MSVC a bit happier warning-wise

>Subject: [PATCH 04/12] Add a missing cast to make MSVC a bit happier warning-wise

I approve
Comment 17 Simon McVittie 2010-03-23 06:01:59 UTC
Comment on attachment 34316 [details] [review]
Fix warnings related to DEBUG() usage without arguments

>Subject: [PATCH 05/12] Fix warnings related to DEBUG() usage without arguments

Looks like a necessary evil, and would be nice to put on the /Portability page too
Comment 18 Simon McVittie 2010-03-23 06:03:58 UTC
Comment on attachment 34317 [details] [review]
Make MUC timestamp parsing portable

>Subject: [PATCH 06/12] Make MUC timestamp parsing portable

I made a very similar change in Gabble a while ago, which never got merged...

>+          GTimeVal when;
>+
>+          if (g_time_val_from_iso8601 (tm, &when))
>             DEBUG ("Malformed date string '%s' for " WOCKY_XMPP_NS_DELAY, tm);
>           else
>-            stamp = timegm (&when);
>+            stamp = when.tv_sec;

... but I think you're missing a "!" at the beginning of the if condition :-)
Comment 19 Simon McVittie 2010-03-23 06:04:37 UTC
Comment on attachment 34318 [details] [review]
Fix OpenSSL include issue on Windows

>Subject: [PATCH 07/12] Fix OpenSSL include issue on Windows
>
>OpenSSL's Windows headers apparently require windows.h to be included
>before including them. Yikes.

Seems to be a necessary evil...
Comment 20 Simon McVittie 2010-03-23 06:05:31 UTC
Comment on attachment 34319 [details] [review]
Replace CA/CRL stat logic with g_file_test to improve portability

>Subject: [PATCH 08/12] Replace CA/CRL stat logic with g_file_test to improve portability

Looks good to me
Comment 21 Simon McVittie 2010-03-23 06:07:41 UTC
Comment on attachment 34320 [details] [review]
Avoid C99 named initializers

>Subject: [PATCH 09/12] Avoid C99 named initializers

wjt will be sad, but it seems necessary.

In existing code we've generally used a style more like:

foo,    /* foo */
NULL,   /* bar */

> static xmlSAXHandler parser_handler = {
>-  .initialized = XML_SAX2_MAGIC,
>-  .startElementNs = _start_element_ns,
>-  .endElementNs   = _end_element_ns,
>-  .characters     = _characters,
>-  .serror         = _error,
>+  /* internalSubset         */ NULL,
>+  /* isStandalone           */ NULL,
>+  /* hasInternalSubset      */ NULL,
>+  /* hasExternalSubset      */ NULL,
>+  /* resolveEntity          */ NULL,
>+  /* getEntity              */ NULL,
>+  /* entityDecl             */ NULL,
>+  /* notationDecl           */ NULL,
>+  /* attributeDecl          */ NULL,
>+  /* elementDecl            */ NULL,
>+  /* unparsedEntityDecl     */ NULL,
>+  /* setDocumentLocator     */ NULL,
>+  /* startDocument          */ NULL,
>+  /* endDocument            */ NULL,
>+  /* startElement           */ NULL,
>+  /* endElement             */ NULL,
>+  /* reference              */ NULL,
>+  /* characters             */ _characters,
>+  /* ignorableWhitespace    */ NULL,
>+  /* processingInstruction  */ NULL,
>+  /* comment                */ NULL,
>+  /* warning                */ NULL,
>+  /* error                  */ NULL,
>+  /* fatalError             */ NULL,
>+  /* getParameterEntity     */ NULL,
>+  /* cdataBlock             */ NULL,
>+  /* externalSubset         */ NULL,
>+  /* initialized            */ XML_SAX2_MAGIC,
>+  /* _private               */ NULL,
>+  /* startElementNs         */ _start_element_ns,
>+  /* endElementNs           */ _end_element_ns,
>+  /* serror                 */ _error
> };

I haven't yet verified that these are in the right order.
Comment 22 Simon McVittie 2010-03-23 06:08:24 UTC
Comment on attachment 34321 [details] [review]
Replace isXXX() with g_ascii_isXXX() to improve portability

>Subject: [PATCH 10/12] Replace isXXX() with g_ascii_isXXX() to improve portability

Yes please!
Comment 23 Simon McVittie 2010-03-23 06:11:29 UTC
Comment on attachment 34322 [details] [review]
Fix error_to_string() crash

>Subject: [PATCH 11/12] Fix error_to_string() crash
>
>Declaring the static ssl_error array as const means that it may be read-
>only at runtime, which is not what you want in this case.

Yes please! I don't know why anyone thought this was right... also, the (gchar *) cast ~10 lines later is a symptom of this mistake (which can now be removed), and the (const gchar *) cast the line after that is unnecessary.
Comment 24 Simon McVittie 2010-03-23 06:15:16 UTC
(In reply to comment #11)
> not certain
> if any of the existing M4 macros check for unistd.h

I'll herd the patches I approved into a branch, and make sure that check is there, unless someone beats me to it.
Comment 25 Simon McVittie 2010-03-23 07:41:24 UTC
http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows contains all those patches except for the realpath wrapper and MUC timestamp parsing, plus some minor changes from me.
Comment 26 Simon McVittie 2010-03-23 07:47:18 UTC
(In reply to comment #18)
> (From update of attachment 34317 [details] [review])
> >Subject: [PATCH 06/12] Make MUC timestamp parsing portable
> 
> I made a very similar change in Gabble a while ago

which is this:

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=7c0dac9e3fb52cb1ef

The timestamps seen in MUC do not contain an explicit timezone, but according to the XEP, they are understood to be in UTC, therefore we need to append "Z" before calling g_time_val_to_iso8601.
Comment 27 Ole André Vadla Ravnås 2010-03-23 07:52:59 UTC
(In reply to comment #12)
> Do you have a git repo/branch online somewhere for this sort of thing? We can
> resurrect your git.collabora.co.uk account if that'd help?

I do have a fdo account, but I didn't bother pushing there as I got pointed to this bugzilla, so I assumed I should attach patches here as well. :) Should I push there, or what's the easiest for you?
Comment 28 Ole André Vadla Ravnås 2010-03-23 07:54:44 UTC
(In reply to comment #13)
> (From update of attachment 34312 [details] [review])
> >Subject: [PATCH 01/12] Don't include unistd.h unless available
> 
> Looks good in principle. We don't normally wrap config.h in HAVE_CONFIG_H: is
> creating a stub config.h problematic on Windows?

Ah good. That was a broken assumption on my side -- hsbuild (which I'm using on Windows), auto-generates a config.h, so that's fine.

> > #include <stdio.h>
> >+#ifdef HAVE_UNISTD_H
> > #include <unistd.h>
> >+#endif
> > #include <stdlib.h>
> > #include <string.h>
> 
> I might add a patch to do the unconditional and conditional includes
> separately, and add a bit of whitespace:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> 
> #ifdef HAVE_HYPOTHETICAL_H
> # include <hypothetical.h>
> #endif
> #ifdef HAVE_UNISTD_H
> # include <unistd.h>
> #endif

Yup, that definitely looks better!
Comment 29 Ole André Vadla Ravnås 2010-03-23 07:55:19 UTC
(In reply to comment #14)
> (From update of attachment 34313 [details] [review])
> >Subject: [PATCH 02/12] Add a wrapper for realpath to support non-POSIX OSes
> 
> >+gchar *
> >+wocky_normalise_path (const gchar *path)
> >+{
> >+#ifdef G_OS_WIN32
> >+  if (!g_path_is_absolute (path))
> >+    return NULL;
> >+  return g_strdup (path);
> >+#else
> >+  return realpath (path, NULL);
> >+#endif
> >+}
> 
> I'm pretty sure that's wrong for relative paths. Wouldn't we be better off
> using g_file_resolve_relative_path (g_file_new_for_path (g_get_current_dir ()),
> path) on Windows?
> 
> (If we don't really care about normalizing symlinks, we could probably even do
> the same on Unix.)

Yes, definitely, that'd be great! I meant to include a comment that this one could need a real implementation on Windows, the whole thing was a bit rushed. :)
Comment 30 Ole André Vadla Ravnås 2010-03-23 07:56:16 UTC
(In reply to comment #18)
> (From update of attachment 34317 [details] [review])
> >Subject: [PATCH 06/12] Make MUC timestamp parsing portable
> 
> I made a very similar change in Gabble a while ago, which never got merged...
> 
> >+          GTimeVal when;
> >+
> >+          if (g_time_val_from_iso8601 (tm, &when))
> >             DEBUG ("Malformed date string '%s' for " WOCKY_XMPP_NS_DELAY, tm);
> >           else
> >-            stamp = timegm (&when);
> >+            stamp = when.tv_sec;
> 
> ... but I think you're missing a "!" at the beginning of the if condition :-)
> 

Oops! -ENOCAFFEINE :)
Comment 31 Ole André Vadla Ravnås 2010-03-23 07:58:40 UTC
(In reply to comment #26)
> (In reply to comment #18)
> > (From update of attachment 34317 [details] [review] [details])
> > >Subject: [PATCH 06/12] Make MUC timestamp parsing portable
> > 
> > I made a very similar change in Gabble a while ago
> 
> which is this:
> 
> http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=7c0dac9e3fb52cb1ef
> 
> The timestamps seen in MUC do not contain an explicit timezone, but according
> to the XEP, they are understood to be in UTC, therefore we need to append "Z"
> before calling g_time_val_to_iso8601.
> 

Great stuff! Thanks a lot!
Comment 32 Ole André Vadla Ravnås 2010-03-23 08:02:25 UTC
(In reply to comment #25)
> http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows
> contains all those patches except for the realpath wrapper and MUC timestamp
> parsing, plus some minor changes from me.
> 

Sweet! Thanks! I'll pull and look into the remaining bits.

One more thing, slightly off-topic, would you be okay with changing wocky_connector_{connect,register}_finish so the GError is put as the last argument instead of middle? That would make Vala async binding "just work". :-)
Comment 33 Simon McVittie 2010-03-23 08:12:23 UTC
(In reply to comment #14)
> (From update of attachment 34313 [details] [review])
> >Subject: [PATCH 02/12] Add a wrapper for realpath to support non-POSIX OSes

I now realise that the previous use of realpath() was wrong: we g_free its result, but g_free is not guaranteed to be compatible with free().

> I'm pretty sure that's wrong for relative paths. Wouldn't we be better off
> using g_file_resolve_relative_path (g_file_new_for_path (g_get_current_dir ()),
> path) on Windows?
> 
> (If we don't really care about normalizing symlinks, we could probably even do
> the same on Unix.)

The two uses of this are to load CRLs and CA certificates, neither of which seems to me to really need this functionality? I've added a patch to the branch which implements the rest, using GFile.
Comment 34 Simon McVittie 2010-03-23 08:14:34 UTC
(In reply to comment #32)
> One more thing, slightly off-topic, would you be okay with changing
> wocky_connector_{connect,register}_finish so the GError is put as the last
> argument instead of middle? That would make Vala async binding "just work". :-)

I for one would be in favour of this change.
Comment 35 Ole André Vadla Ravnås 2010-03-23 08:17:01 UTC
(In reply to comment #33)
> (In reply to comment #14)
> > (From update of attachment 34313 [details] [review] [details])
> > >Subject: [PATCH 02/12] Add a wrapper for realpath to support non-POSIX OSes
> 
> I now realise that the previous use of realpath() was wrong: we g_free its
> result, but g_free is not guaranteed to be compatible with free().

Sorry, I remember going "what?!" when I read that bit, but forgot entirely about
it. Good thing you noticed it!

> > I'm pretty sure that's wrong for relative paths. Wouldn't we be better off
> > using g_file_resolve_relative_path (g_file_new_for_path (g_get_current_dir ()),
> > path) on Windows?
> > 
> > (If we don't really care about normalizing symlinks, we could probably even do
> > the same on Unix.)
> 
> The two uses of this are to load CRLs and CA certificates, neither of which
> seems to me to really need this functionality? I've added a patch to the branch
> which implements the rest, using GFile.

Rock! \m/
Comment 36 Simon McVittie 2010-03-23 08:18:49 UTC
Ole André, Sjoerd: care to review <http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows>? I think it's ready.
Comment 37 Ole André Vadla Ravnås 2010-03-24 02:54:14 UTC
(In reply to comment #36)
> Ole André, Sjoerd: care to review
> <http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows>?
> I think it's ready.

Looks great!
Comment 38 Simon McVittie 2010-04-06 07:42:47 UTC
Fixed in git, thanks!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.