400382 - make webdav code link with xpcom_glue instead of xpcom directly, convert to frozen linkage

Status: VERIFIED FIXED

Description

[Daniel Boelzle [:dbo]]

Follow-up bug from bug 324440 comment #31.

Comment 1

[Daniel Boelzle [:dbo]]

Created attachment 285465 [details] [diff] [review]
change to frozen linkage on trunk

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Shaver's unlikely to be responsive for a while (have you seen his latest blog post? :), maybe there's someone else you could ask for review?

Comment 3

[Daniel Boelzle [:dbo]]

Yes, I know; he's off at least until end of october. I think we could wait some time with this one (because calendar still releases from branch), but if e.g. dmose wants to jump in, it would of course be great. :)

Comment 4

[Dan Mosedale (:dmose)]

Comment on attachment 285465 [details] [diff] [review]
change to frozen linkage on trunk

Looks good; r=dmose.  I don't think we ever had an explicit policy about whether sr is required in the WebDAV code, so I'll assert that it's not.

Comment 5

[Daniel Boelzle [:dbo]]

Checked in on trunk.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

This caused sunbird-trunk build to go red.
../../../staticlib/components/libwebdav.a(nsWebDAVService.o): In function `nsWebDAVService::PropfindInternal(nsIWebDAVResource*, unsigned int, char const**, int, nsIWebDAVOperationListener*, nsIInterfaceRequestor*, nsISupports*, int)':
/builds/tinderbox/Sunbird-Trunk/Linux_2.6.18-8.el5_Depend/mozilla/extensions/webdav/src/nsWebDAVService.cpp:420: undefined reference to `nsACString::RFindChar(char) const'

and a whole bunch of those errors.
http://tinderbox.mozilla.org/showlog.cgi?log=Sunbird/1196708040.1196711853.23287.gz
apparently, this mixes internal and external linkage.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

Created attachment 291275 [details] [diff] [review]
patch for bustage [checked in]

This patch makes sunbird-static work on mac. But I couldn't test thunderbird+lightning yet.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

An alternative would be some ifdef magic to set MOZILLA_INTERNAL_API when doing a static build. That would need to be combined with #include "nsStringGlue.h" instead of "nsStringApi.h"

Comment 9

[Dan Mosedale (:dmose)]

r=dmose to check in to fix build bustage.  I think it's worth investigating to understand better what the right fix really is, however.  mvl's proposal in comment 8 seems like a good option.

Comment 10

[ause]

Created attachment 291466 [details] [diff] [review]
fix breakage

not sure if this is a clean fix but it seems to work somehow...

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 291275 [details] [diff] [review]
patch for bustage [checked in]

I checked in this patch. Trees are getting green.
I even think that this might be the right fix. It treats webdav as an extension, for all builds. I think that's what we want.

Comment 12

[Daniel Boelzle [:dbo]]

Comment on attachment 291275 [details] [diff] [review]
patch for bustage [checked in]

mvl, it's not clear to me why your patch fixes trunk static builds. Could you please elaborate why it does?

>-#include "nsStringAPI.h"
>+#include "nsStringGlue.h"
Especially this confuses me since we don't want the non-frozen API to be included, so why not straight away include nsStringAPI.h?

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Sorry, I forget to mention that I didn't check in that part of the patch, since it indeed does not fix anything. (it was part of my other attempt of fixing this)

The patch fixes the builds because it makes webdav always shared. It can't be linked in static, because it uses the external string api, while sunbird itself uses the internal. Trying to make webdav link staticly would mix the two api's.

Comment 14

[Daniel Boelzle [:dbo]]

(In reply to comment #13)
> The patch fixes the builds because it makes webdav always shared. It can't be
> linked in static, because it uses the external string api, while sunbird itself
> uses the internal. Trying to make webdav link staticly would mix the two api's.
Yes makes sense. Yesterday ause and I were wondering about why we shouldn't take webdav dynamically, too.
But w.r.t. bug 324440, is it feasible at all to link a static (frozen only) sunbird?

Comment 15

[Daniel Boelzle [:dbo]]

Static sunbird builds again => FIXED.

Comment 16

[Daniel Boelzle [:dbo]]

verified on trunk