853594 - Port INSTALL_TARGETS to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

We should start thinking about how we're going to handle INSTALL_TARGETS in moz.build files.

At it's heart, INSTALL_TARGETS is just a mechanism to copy/symlink files from the source directory into somewhere else. About the only complication is whether we should set the execute bit on the target.

We have an important decision to make up front: do we support arbitrary destinations (like INSTALL_TARGETS does today) or do we define an explicit variable/function/target name for each distinct target type. I'm thinking the latter is more in-line with the vision of moz.build files. However, it might be easier to support the former initially and deprecate it long term. No matter what we go with, I imagine the recursive make backend could eventually emit a single manifest denoting all installs and we could employ mozpack.copier to install all files in one go. Should be a nice performance win!

Anyone have opinions on how we should tackle this?

Comment 1

[(not currently active) Ted Mielczarek]

It would probably be worthwhile to do a survey of the tree and see what we're currently using this for. There may be a bunch of uses that we could encapsulate a little further.

We're already using this as the backend machinery for a lot of things like EXPORTS.

Comment 2

[Mike Hommey [:glandium]]

IIRC it's also used in some cases where we copy source files around from other directories (which, with VPATH, we might as well avoid altogether).

Comment 4

[Gregory Szorc [:gps]]

Having thought about this some more and looking at uses of INSTALL_TARGETS (and lingering $(INSTALL) rules), I believe that the proper solution is to provide domain-specific functionality in moz.build files for declaring files of different types. A side-effect of that declaration is a build system rule that installs files into a destination directory.

e.g. instead of using a generic "install X to location Y" in moz.build we should say "X is a Z." Then, in the build backend, it knows that "Z's get installed to Y."

The benefit of this is future build backends (like Visual Studio project generators) will be able to examine richer build config metadata and make proper use of it.

I concede there may eventually be a need for an open-ended INSTALL_TARGETS-like functionality in moz.build. But, we should avoid it at all costs.

I'm marking this bug WONTFIX. If we need it later, we can reopen. In the mean time, let's implement domain-specific variables for accomplishing what's done with INSTALL_TARGETS today.

Comment 5

[Gregory Szorc [:gps]]

glandium and I talked about this today. And we think we have a solution that will work.

We want to create an INSTALLS variable that contains a non-trivial amount of magic. It will be used something like this:

INSTALLS.modules += ['Bar.jsm', 'Foo.jsm']
INSTALLS.modules.devtools += ['**/*.jsm']
INSTALLS.modules.devtools['**/*.jsm'].preprocess = True
INSTALLS.modules.devtools['SomeModule.jsm'].preprocess = False
INSTALLS.components += ['MyJSService.js', 'MyJSService.manifest']
INSTALLS.components['MyJSService.manifest'].preprocess = True
INSTALLS.components['MyJSService.manifest'].defines['EXTRA_DEFINE'] = True

The data structure would consist of a whitelist of root-level attributes that correspond to directories under dist/bin to install files to. After the root level, the data structure behaves like EXPORTS in that arbitrary hierarchy is allowed through attribute access.

Things get slightly complicated through [] access. glandium wrote a data structure that allows [] access to StrictOrderingOnAppendLists and the returned object allows you to define a whitelist of attributes. We would allow setting of certain attributes that can control things like whether to preprocess files and how to preprocess them.

So far, I think that's pretty reasonable and an easy pill to swallow. That may well be the output of this bug.

Where things get complicated is wildcards. People have asked for a way to glob source paths so they don't have to keep moz.build files in sync (e.g. bug 949205). I think this is a reasonable request. Let's do a brief history of wildcards in the build config.

Wildcards have performance and clobber implications. Filesystem scanning can be expensive, especially on Windows and where slow I/O is involved. Therefore, we have a strong incentive to avoid wildcards as much as possible. That being said, wildcards do make maintaining the build config much saner, especially where lists of hundreds or thousands of files is involved. There are clobber considerations because if wildcards are scanned at config.status time, a file addition or removal may not make it into the install rules due to config.status not detecting that. We can do a filesystem walk or config.status at the top of the build to see if anything has changed, but that is expensive. The latter unnecessarily slows down builds by ~5s. We might be able to make config.status faster, but I'm not holding my breath. Doing this wrong with support-files resulted in needed clobbers and bug 934739.

We solved the problem of wildcards by deferring their resolution to install manifest processing time. Essentially, we store the wildcard expression in the install manifest. This works and it works well.

Now, the problem with this bug is getting wildcards to work well when custom attributes are present. We want to do things like "add * and enable preprocessing on *". That gets complicated when you add "but don't preprocess Foo."

The proposal glandium and I came up with is for the install manifest to contain an ordered set of discovery and modification operations. In the example above, the output would look something like:

  [
     ('**/*.jsm', {}),
     ('**/*.jsm', {preprocess=True}),
     ('SomeModule.jsm', {preprocess=False}),
  ]

At processing time, the install manifest would do something like:

1) FileFinder.find('**/*.jsm')
   -> {'SomeModule.jsm': {}, 'SomeOtherModule.jsm': {}}

2) for path in files: if mozpath.match(path, '**/*.jsm'): files[path].preprocess = True
   -> {'SomeModule.jsm': {preprocess=True}, 'SomeOtherModule.jsm': {preprocess=True}}

3) files['SomeModule.jsm'].preprocess = False
   -> {'SomeModule.jsm': {preprocess=False}, 'SomeOtherModule.jsm': {preprocess=True}}

In other words, would store the operations executed in moz.build files in the order they were executed in and would later replay that execution to derive the final set of install rules.

It's quite complicated, but we are both convinced we can make this work. It's going to be a hairy set of Python classes.

Anyway, the wildcard matching is quite complicated and can be built on top of an implementation that doesn't support wildcards. So I think I want to move ahead without wildcards for now so we can unblock things moving to moz.build.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Part 1: Make HierarchicalStringList support whitelists

Will eventually need this.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Part 3: Replace HierarchicalStringList accessors with walk()

get_strings() and get_children() always felt a bit kludgy to me.
Consumers needed to reconstruct the hierarchy by hand, which was
annoying. These accessors have been replaced by walk(), which generates
all entries in hierarchical order. Recursion is now handled inside the
class rather than in callers. Callers can still get subsets of
information by using list comprehension. While this isn't as performant
as direct access, I don't believe performance is an issue for this data
structure.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8347052 [details] [diff] [review]
Part 1: Make HierarchicalStringList support whitelists

Review of attachment 8347052 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/util.py
@@ +426,3 @@
>          self._strings = StrictOrderingOnAppendList()
>          self._children = {}
> +        self._allowed_roots = set(allowed_roots) if allowed_roots else None

You could save a few ifs if you used an empty set instead of None.

@@ +451,5 @@
>          exports = self._get_exportvariable(name)
>          if not isinstance(value, HierarchicalStringList):
>              exports._check_list(value)
>              exports._strings = value
> +            exports._allowed_roots = None

I don't see a reason for this since get_exportvariable is going to create a HierarchicalStringList with no allowed_roots given

Comment 9

[(not currently active) Ted Mielczarek]

This seems fixed by FINAL_TARGET_FILES et. al.