954410 - Support Microsoft Office Communicator protocol (SIPE)

Status: RESOLVED FIXED

(Chat Core :: General, enhancement)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 976 at 2011-08-22 00:59:00 UTC ***

It'd be beneficial (for me at least) to have support for the Microsoft Office Communicator protocol in Instantbird: there's a libpurple prpl that supports it called SIPE (see [1], [2]).

I'm attaching my progress so far. (It builds and I can add accounts, but it's untested. I'll test it tomorrow.)

This has some significant changes from the original SIPE library, I'll attach a diff soon.

[1] - http://sourceforge.net/projects/sipe/
[2] - http://sipe.sourceforge.net/

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Changes to SIPE prpl

*** Original post on bio 976 as attmnt 783 at 2011-08-22 01:21:00 UTC ***

So my patch is too big to attach, for now I'll attach my differences to the SIPE prpl.

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 976 at 2011-08-22 13:46:06 UTC ***

Comment on attachment 8352525 [details] [diff] [review] (bio-attmnt 783)
Changes to SIPE prpl

>diff -u -U 8 -p sipe/src/api/sipe-common.h instantbird/purple/libpurple/protocols/sipe/api/sipe-common.h

>+/* Redefine some glib functions to the libpurple versions. */
>+#define g_base64_encode purple_base64_encode
>+#define g_base64_decode purple_base64_decode

I don't understand why this caused an issue in the first place, but... I'm surprised that you have to copy this in so many different files.

>diff -u -U 8 -p sipe/src/api/sipe-nls.h instantbird/purple/libpurple/protocols/sipe/api/sipe-nls.h
>--- sipe/src/api/sipe-nls.h	2011-07-22 22:00:29 -0400
>+++ instantbird/purple/libpurple/protocols/sipe/api/sipe-nls.h	2011-07-22 22:11:47 -0400
>@@ -15,14 +15,16 @@
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  * GNU General Public License for more details.
>  *
>  * You should have received a copy of the GNU General Public License
>  * along with this program; if not, write to the Free Software
>  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  */
> 
>+#undef ENABLE_NLS
>+
> #ifdef ENABLE_NLS
> #include <glib/gi18n.h>

Isn't this the right place to include our l10n code? (and remove the gi18n inclusion)

> #else
> #define _(String) ((const char *) (String))
> #define N_(String) ((const char *) (String))
> #endif /* ENABLE_NLS */
>

>diff -u -U 8 -p sipe/src/core/sipe-utils.c instantbird/purple/libpurple/protocols/sipe/core/sipe-utils.c
>--- sipe/src/core/sipe-utils.c	2011-05-30 20:42:43 -0400
>+++ instantbird/purple/libpurple/protocols/sipe/core/sipe-utils.c	2011-08-21 20:15:52 -0400
>@@ -25,17 +25,17 @@
> #include <string.h>
> #include <ctype.h>
> #include <time.h>
> 
> #include <glib.h>
> 
> #ifdef _WIN32
> /* for network */
>-#include "win32/libc_interface.h"
>+#include "libc_interface.h"

It's unfortunate that you had to make this change in so many places. Could this be solved instead by adding the libpurple source folder in the include path?

>@@ -240,17 +242,20 @@ void sipe_utils_message_debug(const gcha
> 		GTimeVal currtime;
> 		gchar *time_str;
> 		const char *marker   = sending ?
> 			">>>>>>>>>>" :
> 			"<<<<<<<<<<";
> 		gchar *tmp;
> 
> 		g_get_current_time(&currtime);
>-		time_str = g_time_val_to_iso8601(&currtime);
>+#if 0
>+    time_str = g_time_val_to_iso8601(&currtime);
>+#endif
>+    time_str = g_strdup(purple_utf8_strftime("%Y-%m-%dT%H:%M:%SZ", NULL));

The indentation here needs to use tabs (yes, that's painful) to match the style of the original file.

When replacing some code that doesn't work for us, you used this pattern:
#if 0
 <old code>
#endif
 <new code>

To explicitely show where the new code ends, I think it would be more readable to do:
#if 0
 <old code>
#else
 <new code>
#endif


> gchar *
> sipe_utils_time_to_str(time_t timestamp)
> {
>+#if 0
> 	GTimeVal time = { timestamp, 0 };
> 	return g_time_val_to_iso8601(&time);
>+#endif
>+  return g_strdup(purple_utf8_strftime("%Y-%m-%dT%H:%M:%SZ", gmtime(&timestamp)));
> }

Same things here.


>+/* Copied from gtimer.c. */
>+void
>+g_usleep(gulong microseconds)
>+{
>+#ifdef _WIN32
>+  Sleep (microseconds / 1000);
>+#else
>+  struct timespec request, remaining;
>+  request.tv_sec = microseconds / G_USEC_PER_SEC;
>+  request.tv_nsec = 1000 * (microseconds % G_USEC_PER_SEC);
>+  while (nanosleep (&request, &remaining) == -1 && errno == EINTR)
>+    request = remaining;
>+#endif
>+}

I would really prefer if calling sleep could be avoided. As EionRobb pointed out on IRC, prpl code isn't supposed to be blocking.

>diff -u -U 8 -p sipe/src/core/sipe.c instantbird/purple/libpurple/protocols/sipe/core/sipe.c
>--- sipe/src/core/sipe.c	2011-08-21 18:55:00 -0400
>+++ instantbird/purple/libpurple/protocols/sipe/core/sipe.c	2011-08-21 19:29:05 -0400
>@@ -37,21 +37,31 @@
> #include "config.h"
> #endif
> 
> #include <time.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <errno.h>
> #include <string.h>
>+#ifdef HAVE_UNISTD_H
> #include <unistd.h>
>+#endif
> 
> #include <glib.h>
> 
>-#include <libintl.h>
>+// #include <libintl.h>

C++ style comments, boo ;).


>diff -u -U 8 -p sipe/src/purple/purple-ft.c instantbird/purple/libpurple/protocols/sipe/purple-ft.c

>@@ -175,17 +176,17 @@ tftp_read(guchar **buffer, PurpleXfer *x
> }
> 
> static void
> ft_outgoing_init(PurpleXfer *xfer)
> {
> 	sipe_core_ft_outgoing_init(PURPLE_XFER_TO_SIPE_FILE_TRANSFER,
> 				   purple_xfer_get_filename(xfer),
> 				   purple_xfer_get_size(xfer),
>-				   xfer->who); 
>+				   xfer->who);

White space only changes may not be a good idea, but let's blame your editor! ;).


>diff -u -U 8 -p sipe/src/purple/purple-plugin.c instantbird/purple/libpurple/protocols/sipe/purple-plugin.c
>--- sipe/src/purple/purple-plugin.c	2011-05-30 20:42:43 -0400
>+++ instantbird/purple/libpurple/protocols/sipe/purple-plugin.c	2011-08-20 22:28:12 -0400
>@@ -30,23 +30,25 @@
> #include <glib.h>
> 
> #include "sipe-common.h"
> 
> /* Flag needed for correct version of PURPLE_INIT_PLUGIN() */
> #ifndef PURPLE_PLUGINS
> #define PURPLE_PLUGINS
> #endif
>+#undef PURPLE_PLUGINS
> 
> /* for LOCALEDIR 
>  * as it's determined on runtime, as Pidgin installation can be anywhere.
>  */
> #ifdef _WIN32
>-#include "win32/win32dep.h"
>+#include "win32dep.h"
> #endif
>+#define LOCALEDIR NULL

What's the code using this LOCALEDIR variable?


It looks like you made some fantastic progress! Congrats! :)

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 976 at 2011-08-22 14:24:41 UTC ***

Just a note that the git repo on the website moved apparently and is now at http://repo.or.cz/w/siplcs.git

Comment 4

[Florian Quèze [:florian]]

*** Original post on bio 976 at 2011-09-23 22:28:21 UTC ***

Most likely too late for 1.1.

Comment 5

[Patrick Cloke [:clokep]]

Attachment: SIPE protocol dependencies v1

*** Original post on bio 976 as attmnt 1087 at 2011-12-22 15:21:00 UTC ***

This adds the SIPE prpl: I've build it (and connected!) on Windows. I haven't tried building yet on Linux, but I'd appreciate some comments on the Makefile (in particular the code paths for non-Windows builds). I think we'd want to add the dependency of libkrb5 on Linux as SIPE is rather useless without Kerberos. It also could require gmime, but this probably isn't necessary. (Note that on Windows, it links to secur32 for Kerberos, so there's no extra dependencies.) Additionally, a few files from libxml2 are necessary that weren't available, I've added them.

Other issues:
 - I'm using a 22x22 icon as the 32x32. It's what came in the SIPE repo, there is also an SVG, so I can make a 32x32 icon. Is it possible to just use the SVG though?
 - I do have the locale files that I or flo created a while ago, but it's possible they need to be updated. (In fact, the properties file I'm using might need to be updated, as well.)
 - Some of the protocol option strings use \n in them, which doesn't play well with our XUL interface (which just eats \n as empty space). I'm not sure what we can do about this.  I'm assuming we're using <label> elements which don't even support line breaks. It causes the options dialog to look awful, however.

I will upload a diff of my changes to the SIPE prpl, but they're similar to the previous patch.

Comment 6

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl part 1 v1

*** Original post on bio 976 as attmnt 1088 at 2011-12-22 15:22:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 7

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl part 2 v1

*** Original post on bio 976 as attmnt 1089 at 2011-12-22 15:23:00 UTC ***

Note that SIPE prpl parts 1 & 2 are split simply because they're over Bugzilla's filesize limit. If there's a better way you'd like me to split patches out, please let me know.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8352829 [details] [diff] [review]
SIPE protocol dependencies v1

*** Original change on bio 976 attmnt 1087 at 2011-12-22 15:28:57 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Changes to SIPE prpl v1

*** Original post on bio 976 as attmnt 1090 at 2011-12-22 15:30:00 UTC ***

Updated changes to the SIPE prpl (some of these should be upstreamed, most likely).

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8352525 [details] [diff] [review]
Changes to SIPE prpl

*** Original change on bio 976 attmnt 783 at 2011-12-22 15:30:04 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[Patrick Cloke [:clokep]]

Attachment: SIPE protocol dependencies v1.1

*** Original post on bio 976 as attmnt 1114 at 2012-01-08 19:15:00 UTC ***

Updated patch for changes from bug 954193 (bio 759) (http://hg.instantbird.org/instantbird/rev/c663d78550e9). (Also fixes spacing in the libxml Makefile.)

Comment 12

[Patrick Cloke [:clokep]]

Comment on attachment 8352829 [details] [diff] [review]
SIPE protocol dependencies v1

*** Original change on bio 976 attmnt 1087 at 2012-01-08 19:15:08 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[Patrick Cloke [:clokep]]

Comment on attachment 8352830 [details] [diff] [review]
SIPE prpl part 1 v1

*** Original change on bio 976 attmnt 1088 at 2012-06-27 13:02:16 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8352831 [details] [diff] [review]
SIPE prpl part 2 v1

*** Original change on bio 976 attmnt 1089 at 2012-06-27 13:02:23 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8352857 [details] [diff] [review]
SIPE protocol dependencies v1.1

*** Original change on bio 976 attmnt 1114 at 2012-06-27 13:15:29 UTC ***

This needs a bit more work + needs to be updated for SIPE 1.13.2.

Comment 16

[Patrick Cloke [:clokep]]

Attachment: SIPE protocol dependencies v2.0

*** Original post on bio 976 as attmnt 1918 at 2012-09-28 01:09:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[Patrick Cloke [:clokep]]

Comment on attachment 8352857 [details] [diff] [review]
SIPE protocol dependencies v1.1

*** Original change on bio 976 attmnt 1114 at 2012-09-28 01:09:54 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 18

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl part 1 v2.0

*** Original post on bio 976 as attmnt 1919 at 2012-09-28 01:15:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 19

[Patrick Cloke [:clokep]]

Comment on attachment 8352830 [details] [diff] [review]
SIPE prpl part 1 v1

*** Original change on bio 976 attmnt 1088 at 2012-09-28 01:15:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 20

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl part 2 v2.0

*** Original post on bio 976 as attmnt 1920 at 2012-09-28 01:16:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 21

[Patrick Cloke [:clokep]]

Comment on attachment 8352831 [details] [diff] [review]
SIPE prpl part 2 v1

*** Original change on bio 976 attmnt 1089 at 2012-09-28 01:16:40 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 22

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl part 3 v2.0

*** Original post on bio 976 as attmnt 1923 at 2012-09-28 01:21:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 23

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl part 4 v2.0

*** Original post on bio 976 as attmnt 1924 at 2012-09-28 01:21:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 24

[Patrick Cloke [:clokep]]

Attachment: Changes to SIPE prpl v2.0

*** Original post on bio 976 as attmnt 1925 at 2012-09-28 01:22:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 25

[Patrick Cloke [:clokep]]

Comment on attachment 8352832 [details] [diff] [review]
Changes to SIPE prpl v1

*** Original change on bio 976 attmnt 1090 at 2012-09-28 01:22:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 26

[Patrick Cloke [:clokep]]

*** Original post on bio 976 at 2012-09-28 01:32:40 UTC ***

(In reply to comment #16)
> Created attachment 8353681 [details] [diff] [review] (bio-attmnt 1925) [details]
> Changes to SIPE prpl v2.0

I pushed the parts of this patch that would be appropriate for SIPE to take upstream: http://repo.or.cz/w/siplcs.git/commit/faaad867f420a8c302aa0cc8324bab82183d1873

Hopefully it'll be merged to their master branch.

Comment 27

[Florian Quèze [:florian]]

*** Original post on bio 976 at 2012-09-28 10:13:38 UTC ***

So what should be reviewed here exactly? Is it attachment 8353676 [details] [diff] [review] (bio-attmnt 1918) and attachment 8353681 [details] [diff] [review] (bio-attmnt 1925)?
I don't think you expect me to read all the SIPE code, do you? :)

In attachment 8353676 [details] [diff] [review] (bio-attmnt 1918), I don't really see why you put together in the same attachment the l10n changes and the libxml2 changes.

About the xml2 changes:
Are these new files taken from the same version that we were using, or are they from a newer upstream version?

About the l10n changes:
There's enough dead code in tools/l10n/convert-purple-po-files-to-properties-files.py that I don't think you really intended to attach that.


About attachment 8353679 [details] [diff] [review] (bio-attmnt 1923):
Do you really think we should commit in our code repository the purple/libpurple/protocols/sipe/po/ folder?
(We didn't do it for libpurple)

Comment 28

[Patrick Cloke [:clokep]]

*** Original post on bio 976 at 2012-09-28 10:21:54 UTC ***

(In reply to comment #18)
> So what should be reviewed here exactly? Is it attachment 8353676 [details] [diff] [review] (bio-attmnt 1918) [details] and attachment
> 1925 [details]?
> I don't think you expect me to read all the SIPE code, do you? :)
> 
> In attachment 8353676 [details] [diff] [review] (bio-attmnt 1918) [details], I don't really see why you put together in the same
> attachment the l10n changes and the libxml2 changes.
It was pretty much the stuff not in the sipe/ directory.

> About the xml2 changes:
> Are these new files taken from the same version that we were using, or are they
> from a newer upstream version?
Same version. :)

> About the l10n changes:
> There's enough dead code in
> tools/l10n/convert-purple-po-files-to-properties-files.py that I don't think
> you really intended to attach that.
Yes, I don't think I did. This will probably need a newer properties file, by the way. But last time I checked I couldn't do that on Windows.

> About attachment 8353679 [details] [diff] [review] (bio-attmnt 1923) [details]:
> Do you really think we should commit in our code repository the
> purple/libpurple/protocols/sipe/po/ folder?
> (We didn't do it for libpurple)
Oops, no. Definitely not. (Those also weren't updated...)

Comment 29

[Patrick Cloke [:clokep]]

*** Original post on bio 976 at 2012-09-28 10:22:43 UTC ***

(In reply to comment #18)
> So what should be reviewed here exactly? Is it attachment 8353676 [details] [diff] [review] (bio-attmnt 1918) [details] and attachment
> 1925 [details]?
> I don't think you expect me to read all the SIPE code, do you? :)
No, not at all. Pretty much I expect the makefiles and the changes to SIPE to be looked over. Does that seem reasonable?

Comment 30

[Florian Quèze [:florian]]

*** Original post on bio 976 at 2012-09-28 10:30:06 UTC ***

(In reply to comment #20)

> > I don't think you expect me to read all the SIPE code, do you? :)
> No, not at all. Pretty much I expect the makefiles and the changes to SIPE to
> be looked over. Does that seem reasonable?

Yes.

By the way, about the changes to SIPE, wouldn't it be easier to include g_strcmp0 to our version of glib, rather than working around all its calls?

Comment 31

[Patrick Cloke [:clokep]]

*** Original post on bio 976 at 2012-09-29 11:56:17 UTC ***

(In reply to comment #21)
> By the way, about the changes to SIPE, wouldn't it be easier to include
> g_strcmp0 to our version of glib, rather than working around all its calls?

I could do that. I think I wasn't sure how we felt about adding more stuff, but I'd definitely like to patch SIPE as little as possible.

I'll make that change.

Also, the makefile is more of a f? not a r?

Comment 32

[Patrick Cloke [:clokep]]

*** Original post on bio 976 at 2012-10-02 22:36:38 UTC ***

Note to self: switch ifdef HAVE_UNISTD to HAVE_UNISTD_H.

Comment 33

[Patrick Cloke [:clokep]]

Attachment: SIPE protocol dependencies v3.0

*** Original post on bio 976 as attmnt 1948 at 2012-10-12 03:49:00 UTC ***

This includes more files from glib so less hacking on SIPE code.

Comment 34

[Patrick Cloke [:clokep]]

Comment on attachment 8353676 [details] [diff] [review]
SIPE protocol dependencies v2.0

*** Original change on bio 976 attmnt 1918 at 2012-10-12 03:49:36 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 35

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl part 1 v3.0

*** Original post on bio 976 as attmnt 1949 at 2012-10-12 03:52:00 UTC ***

The actual SIPE code (part 1 of 2).

Comment 36

[Patrick Cloke [:clokep]]

Comment on attachment 8353677 [details] [diff] [review]
SIPE prpl part 1 v2.0

*** Original change on bio 976 attmnt 1919 at 2012-10-12 03:52:35 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 37

[Patrick Cloke [:clokep]]

Comment on attachment 8353678 [details] [diff] [review]
SIPE prpl part 2 v2.0

*** Original change on bio 976 attmnt 1920 at 2012-10-12 03:52:35 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 38

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl part 2 v3.0

*** Original post on bio 976 as attmnt 1950 at 2012-10-12 03:54:00 UTC ***

Part 2 of 2 of the SIPE code.

Comment 39

[Patrick Cloke [:clokep]]

Comment on attachment 8353679 [details] [diff] [review]
SIPE prpl part 3 v2.0

*** Original change on bio 976 attmnt 1923 at 2012-10-12 03:54:58 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 40

[Patrick Cloke [:clokep]]

Comment on attachment 8353680 [details] [diff] [review]
SIPE prpl part 4 v2.0

*** Original change on bio 976 attmnt 1924 at 2012-10-12 03:54:58 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 41

[Patrick Cloke [:clokep]]

Attachment: Instantbird stuff for SIPE (makefiles, etc.)

*** Original post on bio 976 as attmnt 1951 at 2012-10-12 03:58:00 UTC ***

This contains the SIPE Makefile.in as well as other changes to Makefiles, etc.

Comment 42

[Patrick Cloke [:clokep]]

Attachment: Changes to SIPE prpl v3.0

*** Original post on bio 976 as attmnt 1952 at 2012-10-12 04:01:00 UTC ***

The hacks done on the SIPE code for us to compile. Minimized a bit more than previously, but let me know if we can remove even more of these.

Comment 43

[Patrick Cloke [:clokep]]

Comment on attachment 8353681 [details] [diff] [review]
Changes to SIPE prpl v2.0

*** Original change on bio 976 attmnt 1925 at 2012-10-12 04:01:02 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 44

[Instantbot]

*** Original post on bio 976 by Cameron Murray <cam AT camm.id.au> at 2012-12-10 12:20:08 UTC ***

I would like to apply these patches to test this. I've downloaded the source for instantbird and can compile no problems without the patch. I've attempted to download all five patch files and run patch -p1 with them, but can't get it to compile after. Am I doing something wrong?

Comment 45

[Patrick Cloke [:clokep]]

*** Original post on bio 976 at 2012-12-10 12:58:58 UTC ***

(In reply to comment #29)
> I would like to apply these patches to test this. I've downloaded the source
> for instantbird and can compile no problems without the patch. I've attempted
> to download all five patch files and run patch -p1 with them, but can't get it
> to compile after. Am I doing something wrong?

I personally usually use hg import, but they should apply OK with patch. You would need to provide your error message, operating system, etc. for me to even begin to guess what's wrong. Maybe hopping onto IRC would be easier (#instantbird on irc.mozilla.org)?

Comment 46

[Instantbot]

*** Original post on bio 976 by Cameron Murray <cam AT camm.id.au> at 2012-12-10 13:26:04 UTC ***

Hi ClokeP,

I'll hop on to IRC, but for your reference:

Operating System: Windows 7
VC Version 10. (Visual Studio 2010)

Using make (not pymake due to other errors).

Patches applied by running

patch -p1 < sipe-1.diff
patch -p1 < sipe-2.diff
patch -p1 < sipe-aux.diff
patch -p1 < sipe-deps.diff

After patching the following command is run:

make -f client.mk configure

Followed by:

make -C ../obj-instantbird tier_app

Compile gets all the way to SIPE protocol, and then bombs out with the following:

C:\mozilla-build\msys\build\instantbird\purple\libpurple\protocols\sipe\core\htt
p-conn.c : fatal error C1083: Cannot open compiler generated file: 'core/http-co
nn.obj': No such file or directory
make[5]: *** [core/http-conn.obj] Error 1
make[5]: Leaving directory `/build/obj-instantbird/purple/libpurple/protocols/si
pe'
make[4]: *** [dynamic-prpls] Error 2
make[4]: Leaving directory `/build/obj-instantbird/purple/libpurple'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/build/obj-instantbird/purple/purplexpcom/src'
make[2]: *** [libs] Error 2
make[2]: Leaving directory `/build/obj-instantbird/purple'
make[1]: *** [libs_tier_app] Error 2
make[1]: Leaving directory `/build/obj-instantbird'
make: *** [tier_app] Error 2
make: Leaving directory `/build/obj-instantbird'

Comment 47

[Instantbot]

*** Original post on bio 976 by Cameron Murray <cam AT camm.id.au> at 2012-12-10 14:41:47 UTC ***

Changes required for compilation:

directories are not created by makefile:
obj-instantbird\purple\libpurple\protocols\sipe\api
obj-instantbird\purple\libpurple\protocols\sipe\core

sipe-2.diff contains duplicate reference to sipe-incomming.c

Comment 49

[Florian Quèze [:florian]]

Comment on attachment 8353710 [details] [diff] [review]
Instantbird stuff for SIPE (makefiles, etc.)

This needs an update for the Makefile.in -> moz.build change.

Comment 50

[Florian Quèze [:florian]]

Comment on attachment 8353706 [details] [diff] [review]
SIPE protocol dependencies v3.0

Same here.

Comment 51

[Patrick Cloke [:clokep]]

Attachment: SIPE protocol dependencies v4

Fixed the paths and Makefile.in -> moz.build.

Comment 52

[Patrick Cloke [:clokep]]

Attachment: SIPE prpl v4

The actual SIPE prpl with paths changed.

Comment 53

[Patrick Cloke [:clokep]]

Attachment: 1 - SIPE protocol dependencies v5

This new set of patches compiles on both Mac and Windows successfully. It does NOT support Kerberos on !Windows. I'm planning to do that in a separate bug since it's giving me issues.

Comment 54

[Patrick Cloke [:clokep]]

Attachment: 2 - SIPE prpl v5 (SIPE r1.18.0)

This updates the SIPE prpl code to v1.18.0, the newest released upstream version.

Comment 55

[Patrick Cloke [:clokep]]

Attachment: 3 - Instantbird SIPE stuff (moz.build files, icons, etc.) v5

Comment 56

[Patrick Cloke [:clokep]]

Attachment: 4 - Changes to SIPE prpl v5

The changes I made to the SIPE prpl to actually get things to compile, most of these are for MSVC, but not all. This is NOT included in attachment 8399442 [details] [diff] [review] and must be applied separately. I can fold these together before pushing if that's desired.

Comment 57

[Florian Quèze [:florian]]

Comment on attachment 8399444 [details] [diff] [review]
4 - Changes to SIPE prpl v5

Review of attachment 8399444 [details] [diff] [review]:
-----------------------------------------------------------------

::: libpurple/protocols/sipe/core/sip-sec-gssapi.c
@@ +38,5 @@
>  #endif
>  
>  #include <glib.h>
>  #include <string.h>
> +#include <gssapi.h>

What's the reason for needing this change? (Why is our path different?)

::: libpurple/protocols/sipe/purple-buddy.c
@@ -464,5 @@
>  					    _("User name should be a valid SIP URI\nExample: [e-mail]"),
>  					    NULL
> -#if PURPLE_VERSION_CHECK(3,0,0)
> -					    , NULL
> -#endif

Why this removal?

::: libpurple/protocols/sipe/purple-notify.c
@@ -88,5 @@
>  
>  	purple_notify_error(purple_private->gc, NULL, title, msg
> -#if PURPLE_VERSION_CHECK(3,0,0)
> -			    , NULL
> -#endif

Same here.

::: libpurple/protocols/sipe/purple-status.c
@@ +43,5 @@
>  gboolean sipe_backend_status_changed(struct sipe_core_public *sipe_public,
>  				     guint activity,
>  				     const gchar *message)
>  {
> +#if 0

What's this function trying to do, and are we sure we can't make it work without using functions from savedstatuses.h?

@@ +80,5 @@
>  void sipe_backend_status_and_note(struct sipe_core_public *sipe_public,
>  				  guint activity,
>  				  const gchar *message)
>  {
> +#if 0

This one seems pretty reasonable to turn off, but it may be nice to add a comment explaining why we did it for future reference.

Comment 58

[Florian Quèze [:florian]]

Comment on attachment 8399442 [details] [diff] [review]
3 - Instantbird SIPE stuff (moz.build files, icons, etc.) v5

Review of attachment 8399442 [details] [diff] [review]:
-----------------------------------------------------------------

If you actually want to ship this as a dynamic prpl, you'll need to add a line to package-manifest.in.

You likely want to figure out what's broken about linking it statically though; and if you link statically into purplexpcom, you'll need all the linker flags to be in the purplexpcom/src/ Makefile.in/moz.build files, rather than in libpurple/protocols/sipe/.

::: jar.mn
@@ +49,5 @@
>  	skin/classic/prpl/bilboed-netsoul/icon.png	(icons/prpl-bilboed-netsoul.png)
> +% skin prpl-sipe classic/1.0 %skin/classic/prpl/sipe/
> +	skin/classic/prpl/sipe/icon32.png	(icons/prpl-sipe-32.png)
> +	skin/classic/prpl/sipe/icon48.png	(icons/prpl-sipe-48.png)
> +	skin/classic/prpl/sipe/icon.png	(icons/prpl-sipe.png)

This line (the part in ()) looks misaligned.

::: libpurple/protocols/sipe/Makefile.in
@@ +11,5 @@
> +include $(srcdir)/../prpl.mk
> +
> +# Include the API directory & the libpurple directory.
> +LOCAL_INCLUDES	+= -I$(srcdir)/api -I$(srcdir)/../..
> +#$(NSS_CFLAGS)

remove commented out dead code

@@ +23,5 @@
> +DEFINES += -DPACKAGE_NAME=\"pidgin-sipe\"
> +DEFINES += -DPACKAGE_URL=\"http://www.instantbird.org/\"
> +DEFINES += -DPACKAGE_VERSION=\"$(VERSION)\"
> +DEFINES += -DSIPE_TRANSLATIONS_URL=\"https://www.transifex.net/projects/p/pidgin-sipe/r/mob/\"
> +#DEFINES += -DENABLE_OCS2005_MESSAGE_HACK=1

I think I would prefer the:
DEFINES += \
 -D... \
 $(NULL)
format.

Also, remove the lines of dead code here.

@@ +32,5 @@
> +    $(DIST)/lib/$(LIB_PREFIX)nssutil3.$(LIB_SUFFIX) \
> +    $(DIST)/lib/$(LIB_PREFIX)pkcs7.$(LIB_SUFFIX) \
> +    $(NULL)
> +else
> +EXTRA_LIBS += $(NSS_LIBS) $(NSPR_LIBS)

Why isn't this working for both Windows and other platforms?

@@ +43,5 @@
> +	$(DIST)/lib/$(LIB_PREFIX)nsspki.$(LIB_SUFFIX) \
> +	$(DIST)/lib/$(LIB_PREFIX)nssutil.$(LIB_SUFFIX) \
> +	$(DIST)/lib/$(LIB_PREFIX)pkcs7.$(LIB_SUFFIX) \
> +	$(DIST)/lib/$(LIB_PREFIX)pk11wrap.$(LIB_SUFFIX) \
> +	$(DIST)/lib/$(LIB_PREFIX)plc4.$(LIB_SUFFIX) \

I really doubt you need to link by hand against all this long list of libraries.

@@ +51,5 @@
> +# On Windows, use native SSPI security package (Kerberos, NTLM; SSO).
> +# Otherwise use SIPE custom NTLMv2 implementation only (No Kerberos; No
> +# SSO).
> +ifeq (WINNT,$(OS_ARCH))
> +  DEFINES += -DHAVE_SSPI=1 -DSECURITY_WIN32=1

No indent in ifeq blocks in Makefiles.

Comment 59

[Florian Quèze [:florian]]

Comment on attachment 8399435 [details] [diff] [review]
1 - SIPE protocol dependencies v5

Review of attachment 8399435 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not excited about increasing the size of our libxml2, but adding these parts (assuming they are actually used by SIPE) seems fine.

Some of the stuff added to glib is problematic though.

::: libraries/glib/glib.symbols
@@ +1290,5 @@
>  g_assertion_message G_GNUC_NORETURN
>  g_assertion_message_cmpnum G_GNUC_NORETURN
>  g_assertion_message_cmpstr G_GNUC_NORETURN
>  g_assertion_message_expr G_GNUC_NORETURN
>  g_strcmp0

g_strcmp0 seems the only function you needed in that file.

@@ +1342,5 @@
>  g_timer_stop
>  g_time_val_add
>  g_time_val_from_iso8601
>  g_time_val_to_iso8601 G_GNUC_MALLOC
>  g_usleep

I don't think g_usleep is something we should let glib expose. SIPE uses it twice, but that's most likely wrong.

g_time_val_{from,to}_iso8601 seem to be the only other functions used from that file.

Comment 60

[Patrick Cloke [:clokep]]

Note that if your review comments that were straight forward (i.e. "remove this line") I've just deleted from this comment and taken care of. If I wasn't sure what to do it should be below. Once the questions in here are answered I'll upload v6. Thanks for the review.

(In reply to Florian Quèze [:florian] [:flo] from comment #57)
> ::: libpurple/protocols/sipe/core/sip-sec-gssapi.c
> @@ +38,5 @@
> >  #endif
> >  
> >  #include <glib.h>
> >  #include <string.h>
> > +#include <gssapi.h>
> 
> What's the reason for needing this change? (Why is our path different?)
Ignore this for now, it's part of the Kerberos changes. (I've moved this out into a separate patch.)

> ::: libpurple/protocols/sipe/purple-buddy.c
> @@ -464,5 @@
> >  					    _("User name should be a valid SIP URI\nExample: [e-mail]"),
> >  					    NULL
> > -#if PURPLE_VERSION_CHECK(3,0,0)
> > -					    , NULL
> > -#endif
> 
> Why this removal?
MSVC explodes on this, Mook and I had a conversation on IRC [1]. purple_notify_error (which seems to be have removed from the context...) is a macro and our guess is that an ifdef is not allowed in a macro on MSVC. Clang seems OK w/ it. This same reason is for the other change.

> ::: libpurple/protocols/sipe/purple-status.c
> @@ +43,5 @@
> >  gboolean sipe_backend_status_changed(struct sipe_core_public *sipe_public,
> >  				     guint activity,
> >  				     const gchar *message)
> >  {
> > +#if 0
> 
> What's this function trying to do, and are we sure we can't make it work
> without using functions from savedstatuses.h?
This lets the SIPE prpl change the status of Pidgin. (Because the SIPE prpl has access to your calendar, it will mark you busy when you have meetings, etc.) I believe during our last discussion you said we did not want this. (Unless I'm misunderstanding what this function does...)

> @@ +80,5 @@
> >  void sipe_backend_status_and_note(struct sipe_core_public *sipe_public,
> >  				  guint activity,
> >  				  const gchar *message)
> >  {
> > +#if 0
> 
> This one seems pretty reasonable to turn off, but it may be nice to add a
> comment explaining why we did it for future reference.
Will do. Thanks.

(In reply to Florian Quèze [:florian] [:flo] from comment #58)
> If you actually want to ship this as a dynamic prpl, you'll need to add a
> line to package-manifest.in.
OK. I made this change, I'll upload it with the next round unless we statically link it.

> You likely want to figure out what's broken about linking it statically
> though; and if you link statically into purplexpcom, you'll need all the
> linker flags to be in the purplexpcom/src/ Makefile.in/moz.build files,
> rather than in libpurple/protocols/sipe/.
Is there some large benefit to statically linking I'm not understanding? The issue with statically linking it is...

The error when statically linking is:
12:25.69 duplicate symbol _sipmsg_parse_msg in:
12:25.69     ../../libpurple/protocols/simple/sipmsg.o
12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
12:25.70 duplicate symbol _sipmsg_parse_header in:
12:25.70     ../../libpurple/protocols/simple/sipmsg.o
12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
12:25.70 duplicate symbol _sipmsg_free in:
12:25.70     ../../libpurple/protocols/simple/sipmsg.o
12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
12:25.70 duplicate symbol _sipmsg_find_header in:
12:25.70     ../../libpurple/protocols/simple/sipmsg.o
12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
12:25.70 duplicate symbol _sipmsg_add_header in:
12:25.70     ../../libpurple/protocols/simple/sipmsg.o
12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
12:25.70 duplicate symbol _sipmsg_to_string in:
12:25.70     ../../libpurple/protocols/simple/sipmsg.o
12:25.70     ../../libpurple/protocols/sipe/sipmsg.o

I think at one point I experimented with using a #define somewhere to change this. I'm not tied to dynamically linking it, but it seemed like this was the "right thing to do". Please advise.

> @@ +23,5 @@
> > +DEFINES += -DPACKAGE_NAME=\"pidgin-sipe\"
> > +DEFINES += -DPACKAGE_URL=\"http://www.instantbird.org/\"
> > +DEFINES += -DPACKAGE_VERSION=\"$(VERSION)\"
Should I have changed these to Instantbird links or should I have kept them the same as they were?

> @@ +32,5 @@
> > +    $(DIST)/lib/$(LIB_PREFIX)nssutil3.$(LIB_SUFFIX) \
> > +    $(DIST)/lib/$(LIB_PREFIX)pkcs7.$(LIB_SUFFIX) \
> > +    $(NULL)
> > +else
> > +EXTRA_LIBS += $(NSS_LIBS) $(NSPR_LIBS)
> 
> Why isn't this working for both Windows and other platforms?
This seems to work for both without all the extra libs, not sure why I did this.

(In reply to Florian Quèze [:florian] [:flo] from comment #59)
> I'm not excited about increasing the size of our libxml2, but adding these
> parts (assuming they are actually used by SIPE) seems fine.
They are last time I checked. It actually "wants" gmime too, but you can disable that.

> Some of the stuff added to glib is problematic though.
> 
> ::: libraries/glib/glib.symbols
> @@ +1290,5 @@
> >  g_assertion_message G_GNUC_NORETURN
> >  g_assertion_message_cmpnum G_GNUC_NORETURN
> >  g_assertion_message_cmpstr G_GNUC_NORETURN
> >  g_assertion_message_expr G_GNUC_NORETURN
> >  g_strcmp0
> 
> g_strcmp0 seems the only function you needed in that file.
Do you want me to only include that function? What action is associated with this comment?

> @@ +1342,5 @@
> >  g_timer_stop
> >  g_time_val_add
> >  g_time_val_from_iso8601
> >  g_time_val_to_iso8601 G_GNUC_MALLOC
> >  g_usleep
> 
> I don't think g_usleep is something we should let glib expose. SIPE uses it
> twice, but that's most likely wrong.
Yes, we use to override this inside of sipe-backend.h [2], I seem to have missed that change. Would it be better to override it in glib itself?

> g_time_val_{from,to}_iso8601 seem to be the only other functions used from
> that file.
Again, I'm unsure what action is supposed to be associated with this comment.

[1] http://log.bezut.info/instantbird/140328/#m29
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8353711&action=diff#a/src/api/sipe-backend.h_sec2

Comment 61

[Patrick Cloke [:clokep]]

(In reply to Patrick Cloke [:clokep] from comment #60)
> Note that if your review comments that were straight forward (i.e. "remove
> this line") I've just deleted from this comment and taken care of. If I
> wasn't sure what to do it should be below. Once the questions in here are
> answered I'll upload v6. Thanks for the review.
> 
> (In reply to Florian Quèze [:florian] [:flo] from comment #57)
> > ::: libpurple/protocols/sipe/core/sip-sec-gssapi.c
> > @@ +38,5 @@
> > >  #endif
> > >  
> > >  #include <glib.h>
> > >  #include <string.h>
> > > +#include <gssapi.h>
> > 
> > What's the reason for needing this change? (Why is our path different?)
> Ignore this for now, it's part of the Kerberos changes. (I've moved this out
> into a separate patch.)
I just got the GSSAPI stuff working. This was changed because the paths on Mac OS X can't have the gssapi folder in front of them, it doesn't match the path given here.

Comment 62

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #60)

> > ::: libpurple/protocols/sipe/purple-buddy.c
					    NULL
> > > -#if PURPLE_VERSION_CHECK(3,0,0)
> > > -					    , NULL
> > > -#endif
> > 
> > Why this removal?
> MSVC explodes on this, Mook and I had a conversation on IRC [1].
> purple_notify_error (which seems to be have removed from the context...) is
> a macro and our guess is that an ifdef is not allowed in a macro on MSVC.
> Clang seems OK w/ it. This same reason is for the other change.

Ah, I remember reading that conversation, sorry.

> > ::: libpurple/protocols/sipe/purple-status.c
> > @@ +43,5 @@
> > >  gboolean sipe_backend_status_changed(struct sipe_core_public *sipe_public,
> > >  				     guint activity,
> > >  				     const gchar *message)
> > >  {
> > > +#if 0
> > 
> > What's this function trying to do, and are we sure we can't make it work
> > without using functions from savedstatuses.h?
> This lets the SIPE prpl change the status of Pidgin.

That's not how I understand this function. To me it seems to look at the SIPE account's status (+ the global idle/away status) and return a value indicating if the status has changed compared to a previously saved value (which I would assume matches what the prpl has previously sent to the server).

> > @@ +80,5 @@
> > >  void sipe_backend_status_and_note(struct sipe_core_public *sipe_public,
> > >  				  guint activity,
> > >  				  const gchar *message)
> > >  {
> > > +#if 0
> > 
> > This one seems pretty reasonable to turn off, but it may be nice to add a
> > comment explaining why we did it for future reference.
> Will do. Thanks.

The reason why I said "seems pretty reasonable to turn off" is that my reading of this function is that it does what you just described for the other function: attempt to change Pidgin's global status.

> > You likely want to figure out what's broken about linking it statically
> > though; and if you link statically into purplexpcom, you'll need all the
> > linker flags to be in the purplexpcom/src/ Makefile.in/moz.build files,
> > rather than in libpurple/protocols/sipe/.
> Is there some large benefit to statically linking I'm not understanding?

The benefit of statically linking is related to performance, by reducing the amount of files we need to touch on the disk at (libpurple's) startup. So we tried to dynamically link only the prpls that were linked into libraries that may be missing on the user's system (eg. libavahi on Linux).

> The
> issue with statically linking it is...
> 
> The error when statically linking is:
> 12:25.69 duplicate symbol _sipmsg_parse_msg in:
> 12:25.69     ../../libpurple/protocols/simple/sipmsg.o
> 12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
> 12:25.70 duplicate symbol _sipmsg_parse_header in:
> 12:25.70     ../../libpurple/protocols/simple/sipmsg.o
> 12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
> 12:25.70 duplicate symbol _sipmsg_free in:
> 12:25.70     ../../libpurple/protocols/simple/sipmsg.o
> 12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
> 12:25.70 duplicate symbol _sipmsg_find_header in:
> 12:25.70     ../../libpurple/protocols/simple/sipmsg.o
> 12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
> 12:25.70 duplicate symbol _sipmsg_add_header in:
> 12:25.70     ../../libpurple/protocols/simple/sipmsg.o
> 12:25.70     ../../libpurple/protocols/sipe/sipmsg.o
> 12:25.70 duplicate symbol _sipmsg_to_string in:
> 12:25.70     ../../libpurple/protocols/simple/sipmsg.o
> 12:25.70     ../../libpurple/protocols/sipe/sipmsg.o

Are protocols/simple/sipmsg.c and protocols/sipe/sipmsg.c identical enough that we could compile only one of the two when both prpls are statically linked?

If not, putting 5 #define in a header file included by all other (relevant) .c SIPE files is the way to go.

> > @@ +23,5 @@
> > > +DEFINES += -DPACKAGE_NAME=\"pidgin-sipe\"
> > > +DEFINES += -DPACKAGE_URL=\"http://www.instantbird.org/\"
> > > +DEFINES += -DPACKAGE_VERSION=\"$(VERSION)\"
> Should I have changed these to Instantbird links or should I have kept them
> the same as they were?

Changing them to Instantbird things seems to already be what you've done. I don't see any problem with that. The one that you haven't changed is SIPE_TRANSLATIONS_URL. I don't think it's ever going to be visible, so we could as well shorten it (eg. with an empty string) after verifying it isn't actually going to ever be loaded.

> > ::: libraries/glib/glib.symbols
> > @@ +1290,5 @@
> > >  g_assertion_message G_GNUC_NORETURN
> > >  g_assertion_message_cmpnum G_GNUC_NORETURN
> > >  g_assertion_message_cmpstr G_GNUC_NORETURN
> > >  g_assertion_message_expr G_GNUC_NORETURN
> > >  g_strcmp0
> > 
> > g_strcmp0 seems the only function you needed in that file.
> Do you want me to only include that function? What action is associated with
> this comment?

You can either ifdef out the rest of the file, or in this case as the g_strcmp0 function is very small, you may just copy it to another file we already build eg. gutils.c.

By the way, I'm pretty sure that we have worked around the lack of g_strcmp0 in a few places before; if we now include g_strcmp0, we should check and remove our hacks the next time we update libpurple.

> > @@ +1342,5 @@
> > >  g_timer_stop
> > >  g_time_val_add
> > >  g_time_val_from_iso8601
> > >  g_time_val_to_iso8601 G_GNUC_MALLOC
> > >  g_usleep
> > 
> > I don't think g_usleep is something we should let glib expose. SIPE uses it
> > twice, but that's most likely wrong.
> Yes, we use to override this inside of sipe-backend.h [2], I seem to have
> missed that change. Would it be better to override it in glib itself?

I prefer g_usleep to not be built at all, so that if someone ever tries to compile a prpl using it, the compilation will fail and the developer will notice the code is attempting to do something wrong.

The hack in sipe-backend.h is fine with me.

> > g_time_val_{from,to}_iso8601 seem to be the only other functions used from
> > that file.
> Again, I'm unsure what action is supposed to be associated with this comment.

Either copy the relevant parts of gtimer.c (they seem all at the end of the file, to make your life easier :-)) to another file or ifdef out the parts we don't need. These functions are slightly larger than g_strcmp0 so in this case I would likely go the ifdef way, but both solutions are OK with me.

Comment 63

[Florian Quèze [:florian]]

I've looked a bit more at sipe_backend_status_changed. I still think we could implement it without savedstatuses.h BUT the only code path actually using it will result in calling sipe_backend_status_and_note which we don't want anyway.

So I think what you had in your patch (ifdef'ing out the content and returning FALSE all the time) is a reasonable thing to do. An additional comment would be appreciated though ;).

My understanding of how this function is used is: SIPE has access to the user's calendar and is trying to automatically mark the user as busy when the user has meetings. The sipe_backend_status_changed call is a check to verify that the user isn't already busy or idle, before setting the status.

Comment 64

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] [:flo] from comment #62)
> > The issue with statically linking it is...
> >
> > The error when statically linking is:
[...]
I've changed this all to be statically linking.

> Are protocols/simple/sipmsg.c and protocols/sipe/sipmsg.c identical enough
> that we could compile only one of the two when both prpls are statically
> linked?
>
> If not, putting 5 #define in a header file included by all other (relevant)
> .c SIPE files is the way to go.
They differ significantly, I've added some defines.

> > > @@ +23,5 @@
> > > > +DEFINES += -DPACKAGE_NAME=\"pidgin-sipe\"
> > > > +DEFINES += -DPACKAGE_URL=\"http://www.instantbird.org/\"
> > > > +DEFINES += -DPACKAGE_VERSION=\"$(VERSION)\"
> > Should I have changed these to Instantbird links or should I have kept them
> > the same as they were?
>
> Changing them to Instantbird things seems to already be what you've done. I
> don't see any problem with that. The one that you haven't changed is
> SIPE_TRANSLATIONS_URL. I don't think it's ever going to be visible, so we
> could as well shorten it (eg. with an empty string) after verifying it isn't
> actually going to ever be loaded.
It looks like they're only used in a method that is used in the protocol actions part of the plugin [1]. Do we even implement that?

> > > ::: libraries/glib/glib.symbols
> > > g_strcmp0 seems the only function you needed in that file.
> You can either ifdef out the rest of the file, or in this case as the
> g_strcmp0 function is very small, you may just copy it to another file we
> already build eg. gutils.c.
I added it to gutils.c

> By the way, I'm pretty sure that we have worked around the lack of g_strcmp0
> in a few places before; if we now include g_strcmp0, we should check and
> remove our hacks the next time we update libpurple.
I have another patch that upgrades libpurple (bug 964828), so I'll check that out as part of that.

> I prefer g_usleep to not be built at all, so that if someone ever tries to
> compile a prpl using it, the compilation will fail and the developer will
> notice the code is attempting to do something wrong.
>
> The hack in sipe-backend.h is fine with me.
Done.

> > > g_time_val_{from,to}_iso8601 seem to be the only other functions used from
> > > that file.
> Either copy the relevant parts of gtimer.c (they seem all at the end of the
> file, to make your life easier :-)) to another file or ifdef out the parts
> we don't need. These functions are slightly larger than g_strcmp0 so in this
> case I would likely go the ifdef way, but both solutions are OK with me.
I added a couple of ifdefs, I also needed mktime_utc, which is used in one of the above functions.

This works fine on Windows and Mac OS X, but I haven't tried it on Linux yet. Do we want to attempt to compile Kerberos into this by default (and probably not statically link on Linux) or just disable it in the Linux mozconfigs or...something else?

[1] https://developer.pidgin.im/doxygen/dev2.x.y/html/struct__PurplePluginInfo.html#a3c8791f1d3875851584d09643381d528

Comment 65

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #64)

> > > > @@ +23,5 @@
> > > > > +DEFINES += -DPACKAGE_NAME=\"pidgin-sipe\"
> > > > > +DEFINES += -DPACKAGE_URL=\"http://www.instantbird.org/\"
> > > > > +DEFINES += -DPACKAGE_VERSION=\"$(VERSION)\"
> > > Should I have changed these to Instantbird links or should I have kept them
> > > the same as they were?
> >
> > Changing them to Instantbird things seems to already be what you've done. I
> > don't see any problem with that. The one that you haven't changed is
> > SIPE_TRANSLATIONS_URL. I don't think it's ever going to be visible, so we
> > could as well shorten it (eg. with an empty string) after verifying it isn't
> > actually going to ever be loaded.
> It looks like they're only used in a method that is used in the protocol
> actions part of the plugin [1]. Do we even implement that?

We don't.

> This works fine on Windows and Mac OS X, but I haven't tried it on Linux
> yet. Do we want to attempt to compile Kerberos into this by default (and
> probably not statically link on Linux) or just disable it in the Linux
> mozconfigs or...something else?

Up to you I guess :-).

Is it the whole SIPE prpl that you would need to disable on Linux, or just kerberos support? If we link to an external library that isn't guaranteed to be here, we can't statically link into purplexpcom.

Comment 66

[Patrick Cloke [:clokep]]

Attachment: 1 - SIPE protocol dependencies v6

I'm putting up a set of updated patches that compile for me on Windows, Linux and Mac OS. Windows and Mac statically link SIPE into purplexpcom, Linux dynamically links it. Kerberos is enabled on all 3 platforms. We'll need to re-evaluate if that's reasonable to enable by default on Kerberos once we have a builder running for Linux again.

Comment 67

[Patrick Cloke [:clokep]]

Attachment: 3 - Instantbird SIPE stuff (moz.build files, icons, etc.) v6

Comment 68

[Patrick Cloke [:clokep]]

Attachment: 4 - Changes to SIPE prpl v6

Comment 69

[Patrick Cloke [:clokep]]

Attachment: A - SIPE packaging (for Linux) v6

This diff gets applied to c-c while the rest are to purple.

Comment 70

[Florian Quèze [:florian]]

Comment on attachment 8407556 [details] [diff] [review]
3 - Instantbird SIPE stuff (moz.build files, icons, etc.) v6

Review of attachment 8407556 [details] [diff] [review]:
-----------------------------------------------------------------

::: libpurple/protocols/sipe/Makefile.in
@@ +31,5 @@
> +# Otherwise use SIPE custom NTLMv2 implementation only (No Kerberos; No
> +# SSO).
> +ifeq (WINNT,$(OS_ARCH))
> +DEFINES += -DHAVE_SSPI=1 -DSECURITY_WIN32=1
> +EXTRA_LIBS += secur32.lib

I'm surprised you don't need this EXTRA_LIBS line in purplexpcom/src/Makefile.in now that you are statically linking SIPE on Windows. But if you are sure this compiles OK, it's fine with me the way it is now.

Comment 71

[Florian Quèze [:florian]]

Comment on attachment 8407562 [details] [diff] [review]
A - SIPE packaging (for Linux) v6

Happy landing! :-)

Comment 72

[Patrick Cloke [:clokep]]

Long time in the making, but finally did land! :)

https://hg.mozilla.org/comm-central/rev/f9b8f3cd6389
https://hg.mozilla.org/users/florian_queze.net/purple/rev/49d193ad08b1
https://hg.mozilla.org/users/florian_queze.net/purple/rev/5a66232458e7
https://hg.mozilla.org/users/florian_queze.net/purple/rev/336d872cd3a4
https://hg.mozilla.org/users/florian_queze.net/purple/rev/783630602d6f

I'll look at upstreaming some of those SIPE changes we discussed. Thanks for hanging with me through all those reviews!