887958 - Better express JS files in moz.build files

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #880245 +++

The simple port of EXTRA_JS_MODULES and JS_MODULES_PATH in bug 880245 is a good start and a means to an end. However, I don't want it to live forever.

In the real world, we have single Makefile.in that install JS files to multiple target directories. Currently, this can only be facilitated with INSTALL_TARGETS. We obviously want to facilitate this with moz.build files, so the next version of EXTRA_JS_MODULES should allow multiple sets installed to multiple targets (like EXPORTS currently does).

On top of that, the install location is dependent on $(FINAL_TARGET), which is dependent on $(XPI_NAME) and $(DIST_SUBDIR). $(DIST_SUBDIR) is effectively dependent on whether we're in a gre or app directory.

And we have preprocessed variations of these. And, some preprocessing even depends on other targets!

Furthermore, we have testing-only JS modules.

How should we model this in moz.build?

One idea I keep coming back to is making a JS_FILES moz.build symbol. This symbol is actually an object with well-defined keys or attributes. The value of each of these is an EXPORTS-like object that allows assigning to arbitrary nested levels which correspond to final install location. The main keys/attributes in JS_FILES would correspond to "classes" of JS files. e.g.

JS_FILES['gre'] += ['Foo.jsm', 'Bar.jsm']
JS_FILES['app'].devtools += ['Debugger.jsm']
JS_FILES.gre.services += ['Sync.jsm']

Preprocessed files are difficult, especially since some Makefiles define their own defines. To handle custom defines, we could assign a tuple instead of a scalar string. e.g.

JS_FILES['gre'] += [('Foo.jsm', 'I_AM_DEFINED')]

We may put preprocessed files in the same bucket as non-preprocessed files. Or, we could create a new bucket:

JS_FILES['gre_pp'] += ['Foo.jsm']

What about preprocessing with custom dependencies? I'm not sure. I'm optimistic bug 837792 will fix many of these. Worst case I suppose we can send a richer data structure to JS_FILES. e.g.

JS_FILES['gre'] += [Preprocess('Foo.jsm', depends=['foo.js'])]

Although, that should be a last resort and only implemented if we need it.

Reflecting on this, part of me really doesn't like having that extra level in JS_FILES to indicate e.g. app vs gre. However, I think it's necessary. The alternative is this information would live in a separate variable. And, that would constrain usage and prevent the same moz.build file from putting JS files in both app and gre directories. I'm pretty sure there are places in the tree where we have separate directories and Makefiles to work around this limitation.

What do people think?

Comment 1

[Joshua Cranmer [:jcranmer]]

(In reply to Gregory Szorc [:gps] from comment #0)
> On top of that, the install location is dependent on $(FINAL_TARGET), which
> is dependent on $(XPI_NAME) and $(DIST_SUBDIR). $(DIST_SUBDIR) is
> effectively dependent on whether we're in a gre or app directory.

One criterion I want to mention is that Lightning presently installs its files into a calendar-js directory, so we can't fully rely on .-qualified. Granted, something like the JS model where a.x and a['x'] are semantically the same thing would be sufficient.

> One idea I keep coming back to is making a JS_FILES moz.build symbol.

This is probably a bit too much like bikeshedding for you, but I think it needs to be discussed before it's committed. You mentioned when closing the INSTALL_TARGETS bug that you'd rather have semantically meaningful rules for installing than a catchall. Right now, EXTRA_JS_MODULES has been used as meaning "install this file to .../modules or some subdirectory thereof," which makes me wonder if we should try to be stricter about its semantic meaning. If this meaning is something more than "this is a file written in the JavaScript language," then I think that the name JS_FILES is inappropriate and something like JS_MODULES would be more appropriate.

What sort of meaning am I proposing? One example of something that we could conceivably run is a static analysis tool over our JS modules; in this sort of scenario, being named in JS_MODULES would mean "I am going to be a file that is going to be used from Components.utils.import"--which is much stronger semantically than how we use it now. In practice, EXTRA_JS_MODULES appears to be things that are either imported via Cu.i, used via ChromeWorkers, or loaded as a subscript of one of those, so the direct semantic shift would require a more complicated reasoning structure.

> Preprocessed files are difficult, especially since some Makefiles define
> their own defines. To handle custom defines, we could assign a tuple instead
> of a scalar string. e.g.
> 
> JS_FILES['gre'] += [('Foo.jsm', 'I_AM_DEFINED')]

This approach doesn't lend itself to a concise way to represent preprocessed files that use the default preprocessor definitions. Although, since I think preprocessing should die wherever possible, I'm not too displeased with that.

> What about preprocessing with custom dependencies? I'm not sure. I'm
> optimistic bug 837792 will fix many of these. Worst case I suppose we can
> send a richer data structure to JS_FILES. e.g.
> 
> JS_FILES['gre'] += [Preprocess('Foo.jsm', depends=['foo.js'])]

The essential problems with your preprocessing proposals so far is that they don't give a clear indication on how to preprocess several files with the same arguments easily. That said, as I've poked around some of the uglier Makefiles to understand (<http://mxr.mozilla.org/comm-central/source/calendar/lightning/Makefile.in> is my model here), I think moz.build can provide a lot of saner results with some simpler rules.

Note in the example Makefile I posted that some of the defines end up being:
-DTHUNDERBIRD_VERSION=$(shell cat $(topsrcdir)/some/ugly/file))

If we had a construct, say, PreprocessorRules, we could say:
rules = PreprocessorRules()
rules.set_from_file("THUNDERBIRD_VERSION", "mail/config/version.txt")

Then specifying a file could be something like:
JS_MODULES.gre += [rules.preprocess('Foo.jsm')]

This could also automatically regenerate Foo.jsm whenever the version file updates, assuming that this data sticks around as long as is needed.

Such a preprocessing framework is more general, and naturally extends to other rules where preprocessing may be desirable (e.g., EXTRA_COMPONENTS, which I'm guessing ought to be renamed JS_COMPONENTS or something).

> Reflecting on this, part of me really doesn't like having that extra level
> in JS_FILES to indicate e.g. app vs gre. However, I think it's necessary.

As someone who sticks with c-c development most of the time, I don't know the difference between the two and when to use one or the other. A counteroffer I'd like to make is to default to the one which is usually the most correct (gre, I think?) and have people explicitly use the other one when they need to.

Comment 2

[Gregory Szorc [:gps]]

I like the idea of a "preprocessor context" class or similar. I wish it weren't necessary. But in the back of my mind I know it is inevitable.

I do welcome your bikeshedding. The comments about lumping everything into JS_FILES despite different semantic application do have a point.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #0)
> JS_FILES['gre'] += ['Foo.jsm', 'Bar.jsm']
> JS_FILES['app'].devtools += ['Debugger.jsm']
> JS_FILES.gre.services += ['Sync.jsm']

You can't have files for both gre and app in the same moz.build. There's no incentive for doing this. Moreover, 'app' doesn't have the same meaning during a same build depending if you're building browser, metro, or webapprt.

Note that we have JS files going into modules/ and others going into components/. Some others are also going somewhere under chrome, but that's currently handled by jar.mn.

The more abstracted the definitions are, the easier it is to move stuff around. For example, say we want to move components elsewhere. Instead of having to change all moz.builds to deal with that, we'd just have to change the rules associated with components.

> JS_FILES['gre'] += [('Foo.jsm', 'I_AM_DEFINED')]
> JS_FILES['gre'] += [Preprocess('Foo.jsm', depends=['foo.js'])]

How about:
Foo_jsm = PreprocessedFile(source='Foo.jsm.in', defines={'I_AM_DEFINED': None, 'FOO': 1})
JS_COMPONENTS += [Foo_jsm]

The nice thing about using python here is that we could actually get the list of dependencies from the preprocessed file itself (from its includes, etc.)

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Joshua Cranmer [:jcranmer] from comment #1)
> (In reply to Gregory Szorc [:gps] from comment #0)
> > What about preprocessing with custom dependencies? I'm not sure. I'm
> > optimistic bug 837792 will fix many of these. Worst case I suppose we can
> > send a richer data structure to JS_FILES. e.g.
> > 
> > JS_FILES['gre'] += [Preprocess('Foo.jsm', depends=['foo.js'])]
> 
> The essential problems with your preprocessing proposals so far is that they
> don't give a clear indication on how to preprocess several files with the
> same arguments easily. That said, as I've poked around some of the uglier
> Makefiles to understand
> (<http://mxr.mozilla.org/comm-central/source/calendar/lightning/Makefile.in>
> is my model here), I think moz.build can provide a lot of saner results with
> some simpler rules.
> 
> Note in the example Makefile I posted that some of the defines end up being:
> -DTHUNDERBIRD_VERSION=$(shell cat $(topsrcdir)/some/ugly/file))
> 
> If we had a construct, say, PreprocessorRules, we could say:
> rules = PreprocessorRules()
> rules.set_from_file("THUNDERBIRD_VERSION", "mail/config/version.txt")

FWIW, on a first look, I don't see a reason those can't all be defined globally.

Comment 5

[Mike Hommey [:glandium]]

> Note in the example Makefile I posted that some of the defines end up being:
> -DTHUNDERBIRD_VERSION=$(shell cat $(topsrcdir)/some/ugly/file))

BTW, moz.build files are python, why not allow open('some/ugly/file').read() ?

Comment 6

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #3)
> How about:
> Foo_jsm = PreprocessedFile(source='Foo.jsm.in', defines={'I_AM_DEFINED':
> None, 'FOO': 1})
> JS_COMPONENTS += [Foo_jsm]
> 
> The nice thing about using python here is that we could actually get the
> list of dependencies from the preprocessed file itself (from its includes,
> etc.)

I was thinking about something like this as well. Presumably PreprocessedFile() could take a list of inputs?

Also, I noticed you used a '.in' extension - are you intending for this to now be 2 separate steps instead of one? Right now a file in EXTRA_PP_JS_MODULES just does:

Preprocess srcdir/foo.jsm -> dist/bin/modules/foo.jsm

Would this model require us to do:

Preprocess srcdir/foo.jsm.in -> objdir/foo.jsm
install objdir/foo.jsm -> dist/bin/modules/foo.jsm

If so, how do developers feel about a file extension change?

Comment 7

[Mike Hommey [:glandium]]

I meant preprocess srcdir/foo.jsm.in -> dist/bin/modules/foo.jsm
The source file can be named whatever developers like. It was just an example.

Comment 8

[(not currently active) Ted Mielczarek]

I would be fine with requiring preprocessed files to be named with a ".in" suffix, since it makes it clearer that they're going to be preprocessed. I'd also be fine with special-casing them based on that extension, so you could just have like:
JS_MODULES += ["Foo.jsm", "Bar.jsm.in"]

and the latter would get preprocessed with the default set of DEFINES. For more complicated setups the PreprocessedFile thing sounds pretty nice. We could also just do something simpler, like tuples:
JS_MODULES += ["Foo.jsm", ("Bar.jsm.in", {"I_AM_DEFINED": None, "FOO": 1})]

I think jcranmer and glandium are on the right track here, in that we should probably just be stricter about what our existing file classes mean, and move things that are abusing them into new classes, so that JS_MODULES is *always* modules, etc.

Comment 10

[Gregory Szorc [:gps]]

glandium recently wrote and landed the Java-y named mozbuild.util.StrictOrderingOnAppendListWithFlagsFactory class. This class extends StrictOrderingOnAppendList and allows us to attach whitelisted attributes to each entry in the list. We should be able to employ this along with HierarchicalStringList for defining JS/Chrome files. e.g.

CHROME_FILES += [
   'Foo.jsm',
   'Bar.jsm',
]

CHROME_FILES['Foo.jsm'].preprocess = True
CHROME_FILES['Foo.jsm'].defines['MOZ_FOO'] = True

CHROME_FILES.devtools += [
   'Devtool.jsm',
]

Also, I'm not convinced we need a separate variable for JS files. I like how we're now reading file extensions from SOURCES to derive meaning. That seems much easier than maintaining a handful of separate variables. If we play our cards right, we can probably tackle the generic problem of dist/bin file installation in this bug...

Comment 11

[Joshua Cranmer [:jcranmer]]

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #10)
> Also, I'm not convinced we need a separate variable for JS files. I like how
> we're now reading file extensions from SOURCES to derive meaning. That seems
> much easier than maintaining a handful of separate variables. If we play our
> cards right, we can probably tackle the generic problem of dist/bin file
> installation in this bug...

There is a distinction between JS components and JS modules, and we certainly don't universally use .jsm for modules.

Comment 12

[Nathan Froyd [:froydnj]]

A starting attempt at this is in bug 1044162, we can extend this incrementally as-needed.