1266995 - Remove XPIDL_MODULE

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

moz.build currently uses 3 variables to define XPIDLs: XPIDL_SOURCES, XPIDL_MODULE, and XPIDL_NO_MANIFEST. These were each ported mostly verbatim from the old make days.

AFAICT, XPIDL_MODULE isn't actually necessary. In theory, it provides a grouping so various interfaces get "linked" into the same xpt. However, I don't think this is used in practice. At the end of the day, xpt files are loaded from "interfaces" entries in XPCOM manifests and the names don't matter. Furthermore, we ship an interfaces.xpt that is itself a linking of all the individual .xpt files produced during the build. So these XPIDL_MODULE values from the build don't really have an impact on the final built product. There are also a few one-off .xpt files (xpctest.xpt and xpcomtest.xpt) used for testing. Again, the name doesn't matter.

I propose dropping XPIDL_MODULE and using the relative directory path to derive the xpt file name.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1266995 - Remove wrong docs about XPIDL_MODULE defaulting to MODULE; r?glandium

I can't find the code that makes this true. I don't think this has been
true since the XPIDL rules code was ported from make gunk to mostly
Python a few years ago.

Review commit: https://reviewboard.mozilla.org/r/48639/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48639/

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1266995 - Use DIST_SUBDIR instead of XPIDL_MODULE in export test; r?glandium

We're about to remove XPIDL_MODULE. Switch to another arbitrary string
variable.

Review commit: https://reviewboard.mozilla.org/r/48641/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48641/

Comment 3

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

PIDL module names aren't actually relevant because at the end of the day
an XPCOM manifest loads an XPT file and the name of the loaded file
doesn't really have any impact on run-time behavior: it's the content
in the XPT that matters.

Furthermore, we re-link the various intermediate XPT files into a
single interfaces.xpt, which is shipped as part of omni.ja. So the
XPIDL module names don't survive to a shipping Firefox. Again, only
the content in the XPT matters.

This commit changes the build system to derive the intermediate
XPT/"module" names from relative directory paths. A future commit
will remove remaining references of XPIDL_MODULE from the tree. We
keep the commits separate to make review easier.

For XPIDL files we ship in Firefox, this change "just works." However,
there are some test-only XPT files that now have new names. So we had
to adjust references to those paths in various locations.

This change should not require a clobber because chrome.manifest
in local builds loads the "interfaces.manifest" file which is rewritten
by config/makefiles/xpidl/Makefile to contain only the list of
active .xpt files. If we used the traditional "add entry to manifest"
method, this would orphan old .xpt paths/entries and could lead to
issues.

Review commit: https://reviewboard.mozilla.org/r/48643/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48643/

Comment 4

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

We previously changed it to do nothing. This gets rid of usages in
moz.build files and support for the variable in the sandbox.

As part of making this change, I noticed that several moz.build
files (notably under dom/) now only contain XPIDL_SOURCES variables.
We could probably aggressively consolidate these moz.build files
together.

Review commit: https://reviewboard.mozilla.org/r/48645/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48645/

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48643/diff/1-2/

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48645/diff/1-2/

Comment 7

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/48643/#review45373

Gah - I forgot that various package-manifest.in files reference the .xpt paths derived from XPIDL_MODULE_NAME.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8744637 [details]
MozReview Request: Bug 1266995 - Remove wrong docs about XPIDL_MODULE defaulting to MODULE; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48639/diff/1-2/

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8744638 [details]
MozReview Request: Bug 1266995 - Use DIST_SUBDIR instead of XPIDL_MODULE in export test; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48641/diff/1-2/

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48643/diff/2-3/

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48645/diff/2-3/

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48643/diff/3-4/

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48645/diff/3-4/

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48643/diff/4-5/

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48645/diff/4-5/

Comment 16

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/48643/#review45385

I'm still seeing failures with packaged builds with the latest patch. Local, pre-packaged builds run fine. Error messages when running a packaged build are similar to:

1461470356825   addons.xpi      WARN    Exception running bootstrap method startup on [e-mail]: [Exception... "Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]"  nsresult: "0x80570018 (NS_ERROR_XPC_BAD_IID)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/gps/src/firefox-unified/objdir/dist/firefox/browser/features/[e-mail]!/bootstrap.js :: startup :: line 1387"  data: no] Stack trace: startup()@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/gps/src/firefox-unified/objdir/dist/firefox/browser/features/[e-mail]!/bootstrap.js:1387 < this.XPIProvider.callBootstrapMethod()@resource://gre/modules/addons/XPIProvider.jsm:4715 < this.XPIProvider.startup()@resource://gre/modules/addons/XPIProvider.jsm:2693 < callProvider()@resource://gre/modules/AddonManager.jsm:227 < _startProvider()@resource://gre/modules/AddonManager.jsm:776 < AddonManagerInternal.startup()@resource://gre/modules/AddonManager.jsm:960 < this.AddonManagerPrivate.startup()@resource://gre/modules/AddonManager.jsm:2902 < amManager.prototype.observe()@resource://gre/components/addonManager.js:68
1461470356990   addons.manager  WARN    Exception calling callback: [Exception... "Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]"  nsresult: "0x80570018 (NS_ERROR_XPC_BAD_IID)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/gps/src/firefox-unified/objdir/dist/firefox/browser/features/[e-mail]!/bootstrap.js :: PocketOverlay.startup :: line 374"  data: no] Stack trace: PocketOverlay.startup()@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/gps/src/firefox-unified/objdir/dist/firefox/browser/features/[e-mail]!/bootstrap.js:374 < startup/<()@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/gps/src/firefox-unified/objdir/dist/firefox/browser/features/[e-mail]!/bootstrap.js:545 < safeCall()@resource://gre/modules/AddonManager.jsm:179 < makeSafe/<()@resource://gre/modules/AddonManager.jsm:195 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816

It's possible the automated conversion of package-manifests.in I wrote was a bit too aggressive. I may have to scour the diff with more scrutiny...

Comment 17

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/48643/#review45391

::: b2g/installer/package-manifest.in:325
(Diff revision 5)
>  #ifdef MOZ_USE_NATIVE_UCONV
>  @RESPATH@/components/ucnative.xpt
>  #endif
> -@RESPATH@/components/uconv.xpt
> -@RESPATH@/components/unicharutil.xpt
> -@RESPATH@/components/update.xpt
> +@RESPATH@/components/intl_uconv.xpt
> +@RESPATH@/components/intl_unicharutil.xpt
> +@RESPATH@/components/toolkit_components_timermanager.xpt

After some diffing of the final interfaces.xpt, I found the problem. It turns out there are a few shared XPIDL_MODULE values. The most aggregious I've found so far is here. toolkit/components/timermanager/moz.build lists its XPIDL_MODULE as "update" which is shared with toolkit/mozapps/update/moz.build.

Comment 18

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1266995 - Use unique XPIDL_MODULE for each moz.build; r?glandium

There were a few moz.build files specifying the same XPIDL_MODULE
value. Probably the craziest was timermanager's, which specified the
"update" module!

In preparation for making the xpt filename derived from the relative
source directory, we make all XPIDL_MODULE values unique so each
directory has its own xpt. This will make automatic conversion of
package-manifest.in easier.

Review commit: https://reviewboard.mozilla.org/r/48663/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48663/

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8744637 [details]
MozReview Request: Bug 1266995 - Remove wrong docs about XPIDL_MODULE defaulting to MODULE; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48639/diff/2-3/

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8744638 [details]
MozReview Request: Bug 1266995 - Use DIST_SUBDIR instead of XPIDL_MODULE in export test; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48641/diff/2-3/

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48643/diff/5-6/

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48645/diff/5-6/

Comment 23

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/48643/#review45393

The dump of the final interfaces.xpt for this set of diffs is identical to before the series. So I think this will finally pass try. Sorry for the patch spam.

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48643/diff/6-7/

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48645/diff/6-7/

Comment 26

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/48643/#review45427

::: mobile/android/installer/package-manifest.in:546
(Diff revision 7)
>  @BINPATH@/components/DirectoryProvider.js
>  @BINPATH@/components/FilePicker.js
>  @BINPATH@/components/HelperAppDialog.js
>  @BINPATH@/components/LoginManagerPrompter.js
>  @BINPATH@/components/MobileComponents.manifest
> -@BINPATH@/components/MobileComponents.xpt
> +@BINPATH@/components/mobile_android_components.xpt

This one wasn't converted automatically for some reason. That was causing the test failures on the last push. The main interfaces.xpt is idential after this change, so I'm pretty confident this will pass try.

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8744637 [details]
MozReview Request: Bug 1266995 - Remove wrong docs about XPIDL_MODULE defaulting to MODULE; r?glandium

https://reviewboard.mozilla.org/r/48639/#review45661

Since you're removing it, you might as well fold this with the XPIDL_MODULE removal patch.

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8744638 [details]
MozReview Request: Bug 1266995 - Use DIST_SUBDIR instead of XPIDL_MODULE in export test; r?glandium

https://reviewboard.mozilla.org/r/48641/#review45663

::: python/mozbuild/mozbuild/test/frontend/data/inheriting-variables/moz.build:7
(Diff revision 3)
>  # vim: set filetype=python:
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -XPIDL_MODULE = 'foobar'
> +SONAME = 'foobar'

You should probably make it an actual variable we use this way: DIST_SUBDIR

Comment 29

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/48639/#review45665

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

https://reviewboard.mozilla.org/r/48643/#review45667

This is both painful to review and pailful to maintain. Can we finish bug 1158018 first, instead?

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

https://reviewboard.mozilla.org/r/48645/#review45673

Comment 32

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/48643/#review45667

The package-manifest.in changes were performed automatically. I basically hacked up emitter.py to replace instances of "/<XPIDL_MODULE>.xpt" with "/<new_module>.xpt" in all the package-manifest.in files. With the exception of MobileComponents.xpt, this "just worked" (once the duplicate XPIDL_MODULE values were removed anyway).

I do want to kill all these .xpt files from package-manifest.in. But you didn't like the approach of consolidating the .xpts, so perhaps I'll come up with a different solution...

Comment 33

[Mike Hommey [:glandium]]

Finishing bug 1158018 is how to get rid of the xpts in package-manifest. Someone just needs to update the patch.

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 8744638 [details]
MozReview Request: Bug 1266995 - Use DIST_SUBDIR instead of XPIDL_MODULE in export test; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48641/diff/3-4/

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8744729 [details]
MozReview Request: Bug 1266995 - Use unique XPIDL_MODULE for each moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48663/diff/1-2/

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48643/diff/7-8/

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48645/diff/7-8/

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

I think we decided that I don't need to look at this.

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 8744638 [details]
MozReview Request: Bug 1266995 - Use DIST_SUBDIR instead of XPIDL_MODULE in export test; r?glandium

https://reviewboard.mozilla.org/r/48641/#review46183

Comment 40

[Mike Hommey [:glandium]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

https://reviewboard.mozilla.org/r/48643/#review46185

Please attach the script you used to generate this patch.

Comment 41

[Mike Hommey [:glandium]]

Comment on attachment 8744729 [details]
MozReview Request: Bug 1266995 - Use unique XPIDL_MODULE for each moz.build; r?glandium

https://reviewboard.mozilla.org/r/48663/#review46187

Comment 42

[Gregory Szorc [:gps]]

Attachment: HACK rewrite package-manifest.in files during xpidl processing

This is how I automatically converted package-manifest.in files to use
the new XPIDL module names.

There are theoretical issues with substring matching. However, I
surprisingly didn't hit them when running this. I confirmed from a few
local builds that the resulting interfaces.xpt is identical after the
rewrite.

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8744638 [details]
MozReview Request: Bug 1266995 - Use DIST_SUBDIR instead of XPIDL_MODULE in export test; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48641/diff/4-5/

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8744729 [details]
MozReview Request: Bug 1266995 - Use unique XPIDL_MODULE for each moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48663/diff/2-3/

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48643/diff/8-9/

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8744640 [details]
MozReview Request: Bug 1266995 - Remove XPIDL_MODULE from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48645/diff/8-9/

Comment 47

[Mike Hommey [:glandium]]

Comment on attachment 8744639 [details]
MozReview Request: Bug 1266995 - Derive XPIDL module name from relative source directory; r?glandium

https://reviewboard.mozilla.org/r/48643/#review46915

At least the following are not being replaced in package-manifests:
- dom_events
- dom_html
- dom_base
- dom_xul
- inputmethod

This also needs a coordinated change in package manifests in c-c.

Comment 48

[Gregory Szorc [:gps]]

I'm not actively working on this. I still think this is a good cleanup though.