1541793 - Convert comm-central components to static registration

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Mike Hommey [:glandium]]

Bug 1541792 is going to remove the mechanism that registers static xpcom components declared with NSMODULE_DEFN (NSMODULE_DEFN will go away as well).

Static components in comm-central will need to either be converted as per bug 1524687, or registered "manually" via XRE_AddStaticComponent.

Comment 1

[Magnus Melin [:mkmelin]]

Any timeline on the removal?
We'll get started with the conversion but this is not a very small amount of work that needs to be done.

Comment 2

[Mike Hommey [:glandium]]

Patch is up for review in the depending bug to remove NSMODULE_DEFN. Presumably, switching comm-central shouldn't be a ton of work, as there aren't that many uses of NSMODULE_DEFN. XRE_AddStaticComponent might be the quickest to implement if you're really short on time.

Comment 3

[Jorg K (CEST = GMT+2)]

We use this eight times:
calendar/base/backend/libical/build/calBaseModule.cpp
66 NSMODULE_DEFN(calBaseModule) = &kCalBaseModule;
common/src/nsCommonModule.cpp
83 NSMODULE_DEFN(nsCommonModule) = &kCommonModule;
db/mork/build/nsMorkFactory.cpp
46 NSMODULE_DEFN(nsMorkModule) = &kMorkModule;
ldap/xpcom/src/nsLDAPProtocolModule.cpp
133 NSMODULE_DEFN(nsLDAPProtocolModule) = &kLDAPProtocolModule;
mail/components/build/nsMailComps.cpp
107 NSMODULE_DEFN(nsMailCompsModule) = &kMailCompsModule;
mailnews/build/nsMailModule.cpp
1340 NSMODULE_DEFN(nsMailModule) = &kMailNewsModule;
mailnews/import/build/nsImportModule.cpp
191 NSMODULE_DEFN(nsImportServiceModule) = &kMailNewsImportModule;
mailnews/mapi/mapihook/src/msgMapiSupport.cpp
146 NSMODULE_DEFN(msgMapiModule) = &kMAPIModule;

Is this such a big deal. Maybe some hackery for Ben.

Comment 4

[Mike Hommey [:glandium]]

Testing this:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=805f08374c330f2270984935ea52389c78da3bad

Comment 5

[Ben Campbell]

(In reply to Mike Hommey [:glandium] from comment #4)

Testing this:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=805f08374c330f2270984935ea52389c78da3bad

I think adding a "public:" before the ModulesInit ctor definition will get it building (and I'll try it myself in a minute)...

but isn't it a little dicey relying on global object initialisation to register these? Surely there's some place in the standard app startup we can hook into? ie somewhere called from main()?

Comment 6

[Mike Hommey [:glandium]]

XRE_AddStaticComponent actually handles that nicely. It adds the components to a list that is processed during XPCOM initialization.

(I already have another try with the public, but that failed too because MAPI is Windows-only)

Comment 7

[Mike Hommey [:glandium]]

Note the ideal fix would be to convert to the new xpcom static registration (https://firefox-source-docs.mozilla.org/build/buildsystem/defining-xpcom-components.html), but that's more work than I'm willing to put to avoid breaking comm-central hard.

Comment 8

[Mike Hommey [:glandium]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c3eb80ca12ac2417fe704914a346f7ddd86478a0

Comment 9

[Ben Campbell]

With a couple of minor tweaks it compiles and runs for me locally. Let's see what the try server makes of it:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dd96e14231e38a09b7041845a36f0f1d7c585c1b

(also not sure about the MOZ_CALENDAR #ifdef I used)

Comment 10

[Ben Campbell]

(In reply to Mike Hommey [:glandium] from comment #8)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c3eb80ca12ac2417fe704914a346f7ddd86478a0(In reply to

Oops - snap!
I had to add the MOZ_CALENDAR guard for my local build.

Mike Hommey [:glandium] from comment #7)

Note the ideal fix would be to convert to the new xpcom static registration (https://firefox-source-docs.mozilla.org/build/buildsystem/defining-xpcom-components.html), but that's more work than I'm willing to put to avoid breaking comm-central hard.

Ahh, cool - I'll look into that (but probably not today). For now, if we've got a preemptive bustage fix that's a step in the right direction, so thanks very much!

Comment 11

[Mike Hommey [:glandium]]

Yeah, MOZ_CALENDAR would seem necessary, although it seems to work on try (probably because calendar is enabled there)
Note that there's actually no global define for MOZ_CALENDAR, so a simple #define would actually fail with calendar enabled.

There might be a MOZ_LDAP_XPCOM needed for ldap too.

Comment 12

[Mike Hommey [:glandium]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=13938ecc7fca67efb847cdec139d89541f3eb05c

Comment 13

[Mike Hommey [:glandium]]

That built, but I'm not sure about the oranges that show up on try. Still pending Windows results, though.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Bug 1541793 - Replace NSMODULE_DEFN component registration with a static initializer using XRE_AddStaticComponent

Comment 15

[Geoff Lankow (:darktrojan)]

(In reply to Mike Hommey [:glandium] from comment #13)

That built, but I'm not sure about the oranges that show up on try. Still pending Windows results, though.

That appears to be just the usual array of broken things.

Comment 16

[Mike Hommey [:glandium]]

I'm putting this out there in Phabricator. Feel free to review and land as appropriate. It's worth noting that this actually doesn't depend on bug 1541792, this can land before it, and will only prevent bug 1541792 from busting comm-central when it lands. I'm not sure when bug 1541792 will land, there's still a pending review for one dependency. Hopefully next week.

Comment 17

[Ben Campbell]

Thanks Mike, that's all hugely helpful. I'll look at converting to the new static registration (another crash course in XPCOM for me!).

Comment 18

[Mike Hommey [:glandium]]

Please note that, like the attached patch, the new static registration also doesn't depend on bug 1541792 either.

Comment 19

[Jorg K (CEST = GMT+2)]

Ben, you're not subscribed on https://phabricator.services.mozilla.com/D26268. I've just accepted it, but please take a look as well. As for the oranges: Yes, we sadly have many failures in debug builds. Opt builds should be green.

Comment 20

[Jorg K (CEST = GMT+2)]

Ready to go, Ben?

Comment 21

[Jorg K (CEST = GMT+2)]

I'll get it landed.

Comment 22

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #20)

Ready to go, Ben?

Yes, it all looks sound to me. Do we need to shift the patch over from phabricator to here to land it or not? (more a procedural decision than a technical I guess).

Comment 23

[Ben Campbell]

Oh, ignore me! I see the attachment is a direct phabricator link. Was worried that an old attachment on this bug might get landed by mistake.

Comment 24

[Jorg K (CEST = GMT+2)]

The Phab stuff can be landed via Lando it you want to land it by itself, which is typically NOT the C-C workflow. I'll handle it.

Comment 25

[Mike Hommey [:glandium]]

I landed bug 1541792 to autoland, so expect comm-central to break when that's merged to mozilla-central, if that's merged before this bug lands.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/450bf8638ff1
Replace NSMODULE_DEFN component registration with a static initializer using XRE_AddStaticComponent. r=BenC,jorgk

Comment 27

[Jorg K (CEST = GMT+2)]

Thanks, Mike, I was in the process of landing this when your comment #25 arrived.