I'll hijack this bug, if you don't mind. :-) If moz.build can accurately compute FINAL_TARGET, then we can use install manifests to represent various rules. Subject to the condition, of course, that the files exist at install time. The problem is that FINAL_TARGET itself is relatively difficult to compute: it defaults to $(DIST)/$(XPI_NAME) or $(DIST)/$(DIST_SUBDIR) or $(DIST)/bin (depending on variable presence), and a few people set it manually themselves. DIST_SUBDIR itself is inherited via defs.mk files, and comm-central uses export XPI_NAME to achieve the same effect (hacky, I know). XPI_NAME has some different semantics from DIST_SUBDIR (it causes some DEFINES to be set), although the few manual uses of FINAL_TARGET itself appear to be able to replace DIST_SUBDIR (DIST_SUBDIR complains if you have a jar.mn and are missing another define), although I don't know the reason for the distinction personally. We can probably handle the defs.mk by use of a make_available(string) function that exports the variable name to all subdirectories transitively traversed (this doesn't quite work for memory/replace/defs.mk, but some small refactoring could fix that).

Getting FINAL_TARGET working requires a lot of work, apparently. This lets us use something like defs.mk... except it's actually closer to (a safer form of) |export FOO| in Makefiles, in that it is inherited by dir traversal as opposed to from parent directories. It's a conscientious and intentional change for this patch, but it does have consequences.

I know I don't have tests for this... but this is complicated enough I want some direction on this. FINAL_TARGET needs to be lazily computed from XPI_NAME and DIST_SUBDIR for its default value. I chose to allow functions as default values for moz.build variables, which seems to work properly. This seems to work gracefully enough as far as the sandboxing is concerned. The problem is handing off from the sandbox to the backend, since this isn't something that implies that actions need to be done but rather something that affects the results of other actions. It's also complicated how it is handed off to Makefiles themselves...

Not requesting review or feedback because this apparently doesn't pass try. But this should be a reference for what ends up being done for mozilla-central.

Comment on attachment 804240 [details] [diff] [review] Part 3: Move FINAL_TARGET to moz.build Review of attachment 804240 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/branding/aurora/moz.build @@ +6,5 @@ > > DIRS += ['content', 'locales'] > > +DIST_SUBDIR = 'browser' > +export('DIST_SUBDIR') it kind of sucks to have to add that to branding.

Comment on attachment 804210 [details] [diff] [review] Part 1: Allow inheritable moz.build variables Review of attachment 804210 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/reader.py @@ +632,5 @@ > raise BuildReaderError(list(self._execution_stack), > sys.exc_info()[2], other_error=e) > > + def _read_mozbuild(self, path, read_tiers, filesystem_absolute, descend, > + extra_vars): this should probably use the metadata stuff from bug 910453

Comment on attachment 804210 [details] [diff] [review] Part 1: Allow inheritable moz.build variables Review of attachment 804210 [details] [diff] [review]: ----------------------------------------------------------------- The concept of exported variables somewhat terrifies me. On one hand, it is a nifty feature and I can definitely see how it can be useful. On the other, this starts to break the promise that each moz.build file is isolated and its execution "environment" is well-defined. What this patch does is effectively makes any single moz.build file dependent on all its "parents." Over time, the parent-child relationship should diminish. This is because the current relationship is mostly defined by the make directory traversal mechanism. I think having an individual moz.build file be as standalone as possible is a trait worth working towards. Otherwise reading them will become much more complicated. Concerns notes, I'm going to say yes to this now. I would like us to keep usage of this variable in check, however. I don't want it abused. ::: python/mozbuild/mozbuild/frontend/reader.py @@ +247,5 @@ > + > + try: > + _ = self._globals[varname] > + except KeyError: > + raise Exception('Variable is not a known global variable: %s' % varname) Performing a __getitem__ has side-effects (e.g. assigning the default variable). I'd prefer this test to be side-effect free. You'll probably need to stuff a reference to the allowed globals set somewhere and use that. @@ +632,5 @@ > raise BuildReaderError(list(self._execution_stack), > sys.exc_info()[2], other_error=e) > > + def _read_mozbuild(self, path, read_tiers, filesystem_absolute, descend, > + extra_vars): To echo what glandium said: this should use the metadata facility from bug 910453. I just r+'d the patch, so safe to build on.

Comment on attachment 804239 [details] [diff] [review] Part 2: Define FINAL_TARGET in moz.build Review of attachment 804239 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/data.py @@ +304,5 @@ > + def is_custom(self): > + """Returns whether or not the target is not derived from the default > + given xpiname and subdir.""" > + > + from .sandbox_symbols import compute_final_target Move import to top of file. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +194,5 @@ > yield GeneratedWebIDLFile(sandbox, webidl) > > + if sandbox.get('FINAL_TARGET') or sandbox.get('XPI_NAME') or \ > + sandbox.get('DIST_SUBDIR'): > + yield InstallationTarget(sandbox) IMO InstallationTarget is really metadata attached to something else. I understand that "something else" may not be present in the sandbox, so we have to start somewhere. That being said, since recursivemake.py is simply writing out simple variables in backend.mk, perhaps we should just fold this metadata into the pass-thru variables list. What are your thoughts? ::: python/mozbuild/mozbuild/frontend/sandbox.py @@ +118,5 @@ > + # not a class that can be called), then it is actually a rule to > + # generate the default that should be used. > + defaultRule = default[2] > + if isinstance(defaultRule, type(lambda: None)): > + defaultRule = defaultRule(self) Nit: default_rule (no camelCase, please). hasattr(default_rule, '__call__') to determine if an object is callable. ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +53,5 @@ > > +def compute_final_target(variables): > + """Convert the default value for FINAL_TARGET""" > + basedir = 'dist/' > + if variables['XPI_NAME'] != '': if variables['XPI_NAME']:

(In reply to Gregory Szorc [:gps] from comment #7) > The concept of exported variables somewhat terrifies me. > > On one hand, it is a nifty feature and I can definitely see how it can be > useful. I think the use case which is the most justified is what Calendar does (although it doesn't need to do so anymore): the core calendar was built "normally" for Sunbird but the Lightning build exported the XPI_NAME to make them all go into the Lightning extension instead. Much of the defs.mk usage feels more like laziness to me, personally. (Hence why I made the semantics closer to the export model than defs.mk) > Concerns notes, I'm going to say yes to this now. I would like us to keep > usage of this variable in check, however. I don't want it abused. I can make the documentation emphasize to use it as a last resort. :-) (In reply to Gregory Szorc [:gps] from comment #8) > ::: python/mozbuild/mozbuild/frontend/emitter.py > @@ +194,5 @@ > > yield GeneratedWebIDLFile(sandbox, webidl) > > > > + if sandbox.get('FINAL_TARGET') or sandbox.get('XPI_NAME') or \ > > + sandbox.get('DIST_SUBDIR'): > > + yield InstallationTarget(sandbox) > > IMO InstallationTarget is really metadata attached to something else. I > understand that "something else" may not be present in the sandbox, so we > have to start somewhere. That being said, since recursivemake.py is simply > writing out simple variables in backend.mk, perhaps we should just fold this > metadata into the pass-thru variables list. > > What are your thoughts? FINAL_TARGET is super magical, in particular because people like doing make -C <blah> XPI_NAME. I originally made it a variable pass-thru, but I changed it because it was a bit too unwieldy. I was hoping you'd have a better suggestion... :-/

(In reply to Joshua Cranmer [:jcranmer] from comment #9) > (In reply to Gregory Szorc [:gps] from comment #7) > > The concept of exported variables somewhat terrifies me. > > > > On one hand, it is a nifty feature and I can definitely see how it can be > > useful. > > I think the use case which is the most justified is what Calendar does > (although it doesn't need to do so anymore): the core calendar was built > "normally" for Sunbird but the Lightning build exported the XPI_NAME to make > them all go into the Lightning extension instead. Much of the defs.mk usage > feels more like laziness to me, personally. (Hence why I made the semantics > closer to the export model than defs.mk) It's not laziness, it's there to avoid frontend developers having to worry about setting it when they add directories. I'm pretty sure that without DIST_SUBDIRS forced set to browser under browser/ and metro under browser/metro, we'd already have browser/ files in omni.ja instead of browser/omni.ja, and references with a resource://gre/ url instead of resource:/// url as a consequence, because the latter doesn't work.

We ought to have done this longer ago. This means we won't catch people who include config.mk, define banned variables, and then include rules.mk, though...

The final part is waiting on a green try run for Windows.

Comment on attachment 815342 [details] [diff] [review] Part 2: Move backend.mk inclusion to config.mk instead of rules.mk Oops, forgot to respond to prior requests here... :-[

(In reply to Gregory Szorc [:gps] from comment #8) > hasattr(default_rule, '__call__') to determine if an object is callable. If you read the documentation slightly above, you'll see that I expressly wanted to avoid such cases. My concern is that we could end up with a "smart" object that is callable by default, which would do the wrong thing here.

(In reply to Joshua Cranmer [:jcranmer] from comment #12) > Created attachment 815342 [details] [diff] [review] > Part 2: Move backend.mk inclusion to config.mk instead of rules.mk > > We ought to have done this longer ago. This means we won't catch people who > include config.mk, define banned variables, and then include rules.mk, > though... If you save the value in config.mk and check it didn't change in rules.mk, that should work.

Comment on attachment 815341 [details] [diff] [review] Part 1: Allow inheritable moz.build variables Review of attachment 815341 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/reader.py @@ +250,5 @@ > + """Export the variable to all subdirectories of the current path.""" > + > + if 'exports' not in self.metadata: > + self.metadata['exports'] = dict() > + exports = self.metadata['exports'] exports = self.metadata.setdefault('exports', dict()) @@ +260,5 @@ > + # effect. By calling the dict method instead, we don't have any > + # side effects. > + exports[varname] = dict.__getitem__(self._globals, varname) > + except KeyError: > + raise Exception('Variable is not a known global variable: %s' % varname) This is going to be hacky. But the way to do this so errors report proper context is: except KeyError: self.last_name_error = KeyError('global_ns', 'get_unknown', varname) raise self.last_name_error ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +562,5 @@ > add_tier_dir('base', 'bar', external=True) > """), > > + 'export': ('_export', (str,), > + """Make the specified variable to all child directories. I think you are missing a verb here. @@ +589,5 @@ > + > + export('XPI_NAME') > + > + # Subdirectories that will act as if XPI_NAME were defined in them > + DIRS += ['components', 'base'] You probably want a :: in here to make this proper rst. I believe you also want ^^^^ for the section instead of -----. See the file as it exists in m-c as of a few hours ago.

Comment on attachment 815341 [details] [diff] [review] Part 1: Allow inheritable moz.build variables Review of attachment 815341 [details] [diff] [review]: ----------------------------------------------------------------- I happened to be in the area. ::: python/mozbuild/mozbuild/frontend/reader.py @@ +180,5 @@ > # Register functions. > for name, func in FUNCTIONS.items(): > d[name] = getattr(self, func[0]) > > + # Initialize the exports that we need in the global nit: full sentence. @@ +252,5 @@ > + if 'exports' not in self.metadata: > + self.metadata['exports'] = dict() > + exports = self.metadata['exports'] > + if varname in exports: > + raise Exception('Variable has already been exported: %s' % varname) Is there a problem with doing this twice? It seems unnecessary to fail so hard here. ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +562,5 @@ > add_tier_dir('base', 'bar', external=True) > """), > > + 'export': ('_export', (str,), > + """Make the specified variable to all child directories. nit: variable *available* to.

Hmm, mid-aired a bit here.

Comment on attachment 815342 [details] [diff] [review] Part 2: Move backend.mk inclusion to config.mk instead of rules.mk Review of attachment 815342 [details] [diff] [review]: ----------------------------------------------------------------- I like the idea, but we lose enforcement of variables set between config.mk and rules.mk. Check out FREEZE_VARIABLES in rules.mk and do something similar for moz.build variables at the top of rules.mk.

(In reply to Gregory Szorc [:gps] from comment #17) > > + 'export': ('_export', (str,), > > + """Make the specified variable to all child directories. > > I think you are missing a verb here. I think you're mixing up verbs and adjectives :-P (In reply to Nick Alexander :nalexander from comment #18) > @@ +252,5 @@ > > + if 'exports' not in self.metadata: > > + self.metadata['exports'] = dict() > > + exports = self.metadata['exports'] > > + if varname in exports: > > + raise Exception('Variable has already been exported: %s' % varname) > > Is there a problem with doing this twice? It seems unnecessary to fail so > hard here. I felt it was better to be strict here, since it means we could change the semantics slightly in the future and not affect existing code.

Comment on attachment 818491 [details] [diff] [review] Part 2: Move backend.mk inclusion to config.mk instead of rules.mk Review of attachment 818491 [details] [diff] [review]: ----------------------------------------------------------------- While better than the last revision, this still regresses strictness checking. e.g. The following will no longer be rejected: include config.mk XPIDL_FLAGS := foo include rules.mk You need to have rules.mk via CHECK_MOZBUILD_VARIABLES do checking of _DEPRECATED_VARIABLES and that XPIDLSRCS block in addition to what it does today.

Comment on attachment 818497 [details] [diff] [review] Part 3: Define FINAL_TARGET in moz.build Review of attachment 818497 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

I know I'm missing an XPI_NAME, in config/tests/src-simple, but the corresponding moz.build is unused (that Makefile is processed via CONFIG_SUBST_FILES), and if I do migrate it, jsapi-tests breaks instead (WTF?). Hence why XPI_NAME is not added to the ban list. I'll add FINAL_TARGET to the list when DIST_SUBDIR comes around.

I dropped the XPIDL stuff altogether on the basis that they've been migrated for a sufficiently long period of time that people are unlikely to reintroduce them.

Turns out I needed an extra copy in part 1 to make this all work. With this patch, everybody should be migrated, but banning it looks difficult thanks to idiotic makefiles without moz.build pairs.

Comment on attachment 819036 [details] [diff] [review] Part 4: Migrate FINAL_TARGET/XPI_NAME Review of attachment 819036 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/widgetutils/src/moz.build @@ +16,5 @@ > > +XPI_NAME = 'widgetutils' > + > +if CONFIG['TARGET_XPCOM_ABI']: > + FINAL_TARGET += '/platform/%(OS_TARGET)s_%(TARGET_XPCOM_ABI)s' % CONFIG += seems weird for string values.

Comment on attachment 819792 [details] [diff] [review] Part 5: Port DIST_SUBDIR Review of attachment 819792 [details] [diff] [review]: ----------------------------------------------------------------- Again, I'm not a huge fan of export(). But it's the stop-gap we have. ::: python/mozbuild/mozbuild/frontend/reader.py @@ +709,5 @@ > 'parent': sandbox['RELATIVEDIR'], > 'var': var} > if 'exports' in sandbox.metadata: > sandbox.recompute_exports() > + recurse_info[d]['exports'] = dict(sandbox.metadata['exports']) Sneaky bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5709e2db35ff https://hg.mozilla.org/integration/mozilla-inbound/rev/fb42a764f90b https://hg.mozilla.org/integration/mozilla-inbound/rev/6916000f7b8c https://hg.mozilla.org/integration/mozilla-inbound/rev/5452527770cb https://hg.mozilla.org/integration/mozilla-inbound/rev/ec37f434044a \o/

This patchset enables so much future awesomeness. I'm salivating.

https://hg.mozilla.org/mozilla-central/rev/5709e2db35ff https://hg.mozilla.org/mozilla-central/rev/fb42a764f90b https://hg.mozilla.org/mozilla-central/rev/6916000f7b8c https://hg.mozilla.org/mozilla-central/rev/5452527770cb https://hg.mozilla.org/mozilla-central/rev/ec37f434044a