I added support for JS_MODULES_PATH as well, since that's the directory where EXTRA_JS_MODULES are installed. Seems like they should be moved at the same time.

Comment on attachment 760665 [details] [diff] [review] Move EXTRA_JS_MODULES to moz.build (batch #1) Review of attachment 760665 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good, these files have minor conflicts: ./dom/alarm/Makefile.in ./dom/browser-element/Makefile.in ./dom/push/src/Makefile.in ./dom/messages/Makefile.in ./dom/contacts/Makefile.in ./dom/network/src/Makefile.in

Comment on attachment 760664 [details] [diff] [review] Move EXTRA_JS_MODULES to moz.build (logic) Review of attachment 760664 [details] [diff] [review]: ----------------------------------------------------------------- I imagine we'll someday come up with a better way to define how this is done. But this is sufficient for our immediate goals of moving things to moz.build files.

inbound: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/76a9571e5283 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d187d59f162 try: https://tbpl.mozilla.org/?tree=Try&rev=c8822ff7aaf3

https://hg.mozilla.org/mozilla-central/rev/76a9571e5283 https://hg.mozilla.org/mozilla-central/rev/0d187d59f162

(In reply to Michael Shal [:mshal] from comment #1) > I added support for JS_MODULES_PATH as well, since that's the directory > where EXTRA_JS_MODULES are installed. Seems like they should be moved at the > same time. I experimented with moving these at the same time in comm-central and the build failed. The problem is that all uses of JS_MODULES_PATH are used to install to $(FINAL_TARGET)/modules/subdir, and $(FINAL_TARGET) is not defined at the point of definition for backends.mk. It works in current code because we specify the path as JS_MODULES_PATH = $(FINAL_TARGET)/..., but backend.mk produces a variable that looks like :=, which is too early.

I haven't fully tested this patch on try (I'm building a copy of c-c locally, so I can confirm if this will work or not for c-c), but this is one way to fix the JS_MODULES_PATH/FINAL_TARGET issue. Another way is to make JS_MODULES_PATH act relative to $(FINAL_TARGET) in the first place, which I didn't consider until after I finished writing all the tests, etc., for this patch.

Comment on attachment 763039 [details] [diff] [review] Fix JS_MODULES_PATH issue Review of attachment 763039 [details] [diff] [review]: ----------------------------------------------------------------- Hmmm. Since you are inventing a new class for hackiness, I'd almost prefer we invent a new class for JS modules themselves. I don't want there to be another passthru class that violates compartmentalization of functionality. In comment #8 you proposed we should just make JS_MODULES_PATH relative to FINAL_TARGET. I like that idea as a compromise until we can define JS_MODULES intelligently (likely similar to how we define EXPORTS).

Move Android's .jsm modules from Makefile.in to moz.build.

Is this what you guys have in mind? If so I'll have a followup patch for joey that does a conversion in toolkit (eg: JS_MODULES_PATH = 'modules/identity' works in moz.build with this change).

Comment on attachment 764267 [details] [diff] [review] Convert JS_MODULES_PATH to be relative to $(FINAL_TARGET) Review of attachment 764267 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 764398 [details] [diff] [review] Move EXTRA_JS_MODULES to moz.build (batch #2) We need to fix the case/sorting at some point: EXTRA_JS_MODULES += [ 'DeferredTask.jsm', 'Deprecated.jsm', [snip] 'Timer.jsm', 'debug.js', Not a problem for this patch, looks good

Comment on attachment 763911 [details] [diff] [review] fix-android-modules-mozbuild.patch Looks good, maybe include results from a sanity check build on try.

Try build: https://tbpl.mozilla.org/?tree=Try&rev=246e1879236c

Unfortunately when I tested on try, the proposed patch broke an xpcshell test. The problem is that services/metrics/Makefile.in used JS_MODULES_PATH as a PP_TARGETS helper-variable, which is not relative to FINAL_TARGET. We also can't make all PP_TARGETS to be relative to FINAL_TARGET, since there are a handful of Makefile's that use the default install path (CURDIR), or install to other places relative to the current directory. The attached patch fixes the issue by making services/metrics/Makefile.in use EXTRA_PP_JS_MODULES, rather than setting PP_TARGETS explicitly for this set of files. However, it still uses PP_TARGETS for another set of files because we can't export multiple sets of files to different paths with EXTRA_PP_JS_MODULES (another case for where objects make sense instead of variables? :) I feel like we will need a long-term solution to decide how things are pre-processed and installed. Anyone have thoughts on that?

For the most part all of these special variables are just "install these files to a known location" or "install these files to a known location with preprocessing".

EXTRA_{PP_}COMPONENTS does a bit of manifests logic. TESTING_JS_MODULES is a special variant of EXTRA_JS_MODULES that installs to _tests/modules only if ENABLE_TESTS is set. AUTOCONFIG_JS_EXPORTS installs to $(FINAL_TARGET)/defaults/autoconfig PREF_JS_EXPORTS preprocesses and installs to $(FINAL_TARGET)/defaults/pref or $(FINAL_TARGET)/defaults/preferences based on a few other variables (>_>). DIST_FILES and DIST_CHROME_FILES preprocess to $(FINAL_TARGET) and $(FINAL_TARGET)/chrome, respectively. There's also MOCHITEST_* stuff, but that's supposedly moving to manifests and may not remain in Makefile.ins for long anyways. Moving outside of the predefined well-known locations, installing happens to a wide range of locations (_tests/xpcshell/mailnews, $(DIST)/bin/isp, $(DIST)/bin/defaults/messenger, $(DIST)/bin/chrome, _tests/mozmill, etc.), and sometimes wants to copy instead of symlink (NSDISTMODE=copy happens a few times). A suggestion I heard somewhere was a generic rule like EXPORTS: INSTALL.modules += [] INSTALL.modules.mime += [] (Except calendar wants to install to calendar-js)

We should definitely figure out the long-term solution for installing/copying/preprocessing. However, we have bug 853594 for that. Let's focus on just EXTRA_JS_MODULES here. Step 1) Move things into a more strict evaluation environment (moz.build) to take away the footguns and cargo culting. Step 2) Inject sanity into the "schema" in that environment.

Comment on attachment 765922 [details] [diff] [review] Convert JS_MODULES_PATH to be relative to $(FINAL_TARGET) Review of attachment 765922 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

inbound: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b799dc6cf9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6dc93537d1 try: https://tbpl.mozilla.org/?tree=Try&rev=c231cbbfe0dc

https://hg.mozilla.org/mozilla-central/rev/b7b799dc6cf9 https://hg.mozilla.org/mozilla-central/rev/5b6dc93537d1

try results: https://tbpl.mozilla.org/?tree=Try&rev=06c0fad18d8b

Comment on attachment 769246 [details] [diff] [review] Move EXTRA_JS_MODULES to moz.build (batch #3) Forgot to move over JS_MODULES_PATH in browser/components/sessionstore/src - will upload a new patch after try.

Try results with batch #3 and #4: https://tbpl.mozilla.org/?tree=Try&rev=1506b804e551

Comment on attachment 769852 [details] [diff] [review] Move EXTRA_JS_MODULES to moz.build (batch #3) Looks good

Comment on attachment 769853 [details] [diff] [review] Move EXTRA_JS_MODULES to moz.build (batch #4) Ready for race day

inbound for batch#3 & #4: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7099355d3666 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f9271afd56

https://hg.mozilla.org/mozilla-central/rev/7099355d3666 https://hg.mozilla.org/mozilla-central/rev/c5f9271afd56

One reference to the variable remains toolkit/crashreporter/test/Makefile.in ====================================== libs:: $(SHARED_LIBRARY) $(EXTRA_JS_MODULES) $(INSTALL) $^ $(DEPTH)/_tests/xpcshell/$(relativesrcdir)/unit/ $(INSTALL) $^ $(DEPTH)/_tests/xpcshell/$(relativesrcdir)/unit_ipc/

The EXTRA_JS_MODULES in toolkit/crashreporter/test are not shipped as part of the build, they're used for xpcshell unit tests. These could be switched to TESTING_JS_MODULES nowadays.

Let's call this one done. I'll file a followup to deal with CrashTestUtils.jsm.