Closed Bug 1541793 Opened 8 months ago Closed 8 months ago

Convert comm-central components to static registration

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

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.

No longer depends on: 1541792
Depends on: 1541792

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.

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.

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.

Flags: needinfo?(benc)

(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()?

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)

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.

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)

Flags: needinfo?(benc)

(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!

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.

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

(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.

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.

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

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

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.

Ready to go, Ben?

Flags: needinfo?(benc)

I'll get it landed.

Flags: needinfo?(benc)
Keywords: checkin-needed

(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).

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.

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.

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.

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

Status: NEW → RESOLVED
Closed: 8 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

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

Target Milestone: --- → Thunderbird 68.0
Duplicate of this bug: 1542167
Assignee: nobody → mh+mozilla
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.