Closed Bug 880245 Opened 7 years ago Closed 6 years ago

Move EXTRA_JS_MODULES to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox24 fixed)

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- fixed

People

(Reporter: joey, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(8 files, 2 obsolete files)

No description provided.
Blocks: 880246
No longer blocks: 880246
Depends on: 880260
No longer depends on: 870370, 880260
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.
Attachment #760664 - Flags: review?(gps)
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
Attachment #760665 - Flags: review?(joey) → review+
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.
Attachment #760664 - Flags: review?(gps) → review+
(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.
Attachment #763039 - Flags: review?(gps)
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).
Attachment #763039 - Flags: review?(gps)
Move Android's .jsm modules from Makefile.in to moz.build.
Attachment #763911 - Flags: review?(joey)
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).
Attachment #764267 - Flags: review?(gps)
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.
Attachment #764267 - Flags: review?(gps) → review+
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
Attachment #764398 - Flags: review?(joey) → review+
Comment on attachment 763911 [details] [diff] [review]
fix-android-modules-mozbuild.patch

Looks good, maybe include results from a sanity check build on try.
Attachment #763911 - Flags: review?(joey) → review+
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?
Attachment #764267 - Attachment is obsolete: true
Attachment #765922 - Flags: review?(gps)
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.
Attachment #765922 - Flags: review?(gps) → review+
Blocks: 887958
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.
Attachment #769246 - Flags: review?(joey)
Attachment #769246 - Attachment is obsolete: true
Attachment #769852 - Flags: review?(joey)
Comment on attachment 769852 [details] [diff] [review]
Move EXTRA_JS_MODULES to moz.build (batch #3)

Looks good
Attachment #769852 - Flags: review?(joey) → review+
Comment on attachment 769853 [details] [diff] [review]
Move EXTRA_JS_MODULES to moz.build (batch #4)

Ready for race day
Attachment #769853 - Flags: review?(joey) → review+
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Whiteboard: [leave open]
Blocks: 948851
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.