Palm Sync extension problems on trunk builds for Thunderbird & Suiterunner.

RESOLVED FIXED

Status

MailNews Core Graveyard
Palm Sync
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
x86
Windows XP
Dependency tree / graph

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
Wayne has been trying out the palm sync extension on tpol's zip builds of suiterunner (which should also work for Thunderbird).

We've come across several problems so far:

a) The extension doesn't register in Add-on manager if used straight out of the zip builds (or in theory, developer builds).

I think this is due to the Makefile (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/extensions/palmsync/Makefile.in&rev=1.8&mark=55#51) registering the INSTALL_EXTENSION_ID incorrectly (using palmsync@mozilla.org rather than p@m as per the install.rdf (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/extensions/palmsync/install.rdf&rev=1.3)

Whilst this change is necessary for zip builds to work (and possibly installer builds of SeaMonkey), I'm a bit dubious about it as it'll mean that when using zip builds you'll get a prompt to install the conduit on first start up with the extension in place (its decided on via a pref). However I think it makes sense to make it work if we're going to keep shipping palmsync with SeaMonkey (which AFAIK is what we intend to do).

b) PalmSyncInstall.exe which installs the conduit that palm sync needs can't find msvc*.dll (CRT). This is because we only currently dump the msvc*.dlls in the application dir (bug 350616 has some info about the problems with the xulrunner-stub and this bug).

We have two solutions currently: 1) Link PalmSyncInstall.exe against the CRT statically or 2) include a second copy of the dlls.

My vote here is for 1).

c) Even when PalmSyncInstall.exe has found the dlls, it doesn't run due to R6034="attempt to load C runtime library incorrectly".

More info on the error here: http://msdn2.microsoft.com/en-us/library/ms235560(vs.80).aspx

Basically we need a PalmSyncInstall.exe.manifest in the same directory as where it gets built with a couple of options specified for how it loads the C runtime library.
I don't think you need a manifest if you statically link.
(Assignee)

Comment 2

11 years ago
(In reply to comment #1)
> I don't think you need a manifest if you statically link.
> 
That'd be nice.

Static linking may be a problem though (from http://msdn2.microsoft.com/en-us/library/abx4dbyh(VS.80).aspx):

"Because a DLL built by linking to a static CRT will have its own CRT state, it is not recommended to link statically to the CRT in a DLL unless the consequences of this are specifically desired and understood."

I hadn't realised earlier that we have:

PalmSyncInstall.exe
mozABConduit.dll
PalmSyncProxy.dll

I think that mozAbConduit could be linked staticially, but I'm not sure about PalmSyncProxy as there seems to be some registering going on:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpinstall/wizard/windows/palmsync/PalmSyncInstall.cpp&rev=1.10&mark=378-391#378
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/extensions/palmsync/src/Registry.cpp&rev=1.2&mark=174-182#152

Maybe David knows if we really need these to be separate dlls.

Comment 3

11 years ago
The extension ID should be palmsync@mozilla.org everywhere, so the install.rdf is probably wrong here.

The VC8 manifest stuff should probably be the same as for any DLLs we have in the components/ dir - what are we doing there? Can we do the same for the PalmSync stuff?
As a note, this should only be a problem if the CRTs are not installed into the system with the VC8_redist installer, everything should work flawlessly if that's done.
(Assignee)

Comment 4

11 years ago
(In reply to comment #3)
> The extension ID should be palmsync@mozilla.org everywhere, so the install.rdf
> is probably wrong here.

The install.rdf was purposely changed to p@m to help with a problem with the palmsync code whereby directory names are limited to 110 characters.

> The VC8 manifest stuff should probably be the same as for any DLLs we have in
> the components/ dir - what are we doing there? Can we do the same for the
> PalmSync stuff?

I think that's under discussion in bug 350616.

> As a note, this should only be a problem if the CRTs are not installed into the
> system with the VC8_redist installer, everything should work flawlessly if
> that's done.

Ted, why isn't FF/Xulrunner using the VC8_redist installer?

Comment 5

11 years ago
(In reply to comment #4)
> (In reply to comment #3)
> > The extension ID should be palmsync@mozilla.org everywhere, so the install.rdf
> > is probably wrong here.
> 
> The install.rdf was purposely changed to p@m to help with a problem with the
> palmsync code whereby directory names are limited to 110 characters.

bug 322628 discusses this point
(In reply to comment #4)
> 
> Ted, why isn't FF/Xulrunner using the VC8_redist installer?
> 

So we can install without admin rights.
(Assignee)

Comment 7

11 years ago
Created attachment 262669 [details] [diff] [review]
Make palmsync dlls link statically to the crt.

Just like bug 376041 we need to link the MS CRT statically into the dlls for the palmsync extension. This fixes the installation issue that Wayne was having on trunk when running the conduit installer. There are other issues we need to fix, one or two here, others probably elsewhere.

Ted can you check this is right from the build-config aspect please?
Attachment #262669 - Flags: superreview?(bienvenu)
Attachment #262669 - Flags: review?(ted.mielczarek)

Updated

11 years ago
Attachment #262669 - Flags: superreview?(bienvenu) → superreview+
Attachment #262669 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 8

11 years ago
I believe (apart from bug 379588) that we're building palmsync ok again on trunk now. Note that bug 322628 had the patch to change the installation location because I'd forgotten about this one.

So I think we're safe to close this one down now.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Depends on: 322628, 379588
Resolution: --- → FIXED
Product: Core → MailNews Core

Updated

9 years ago
Component: Palm Sync → Palm Sync
Product: MailNews Core → MailNews Core Graveyard
You need to log in before you can comment on or make changes to this bug.