1459004 - Generate built_in_addons.json from moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[(not currently active) Ted Mielczarek]

bug 1348981 added this as a Makefile rule. It would be better if it was not. It currently lists the directories under `dist/bin/browser/features` which is sorta crappy. I guess we could make it use `GENERATED_FILES` but read the dist_bin install manifest to get the list of files? A little unconventional, but not the worst idea.

Comment 1

[Robert Helmer [:rhelmer]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0)
> bug 1348981 added this as a Makefile rule. It would be better if it was not.
> It currently lists the directories under `dist/bin/browser/features` which
> is sorta crappy. I guess we could make it use `GENERATED_FILES` but read the
> dist_bin install manifest to get the list of files? A little unconventional,
> but not the worst idea.

Ideally we'd put any built-in add-ons into the omni jar (bug 1357205) and stop generating this file entirely.

I haven't been very active in that bug lately and won't stop anyone from doing further cleanup in the meantime though!

Comment 2

[Kris Maglione [:kmag]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0)
> It currently lists the directories under `dist/bin/browser/features` which
> is sorta crappy. I guess we could make it use `GENERATED_FILES` but read the
> dist_bin install manifest to get the list of files? A little unconventional,
> but not the worst idea.

Yeah, that was the best idea I could come up with.

(In reply to Robert Helmer [:rhelmer] from comment #1)
> Ideally we'd put any built-in add-ons into the omni jar (bug 1357205) and
> stop generating this file entirely.

I think, ideally, even with extensions packaged in omni jar, we'd still want a manifest. Iterating the omni jar index at startup will be expensive, and having an explicit manifest makes the add-on manager code much simpler, anyway.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1459004: Generate built_in_addons.json from moz.build definitions.

Review commit: https://reviewboard.mozilla.org/r/249870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249870/

Comment 4

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] (PTO through 06/05) from comment #3)
> Created attachment 8983990 [details]
> Bug 1459004: Generate built_in_addons.json from moz.build definitions.
> 
> Review commit: https://reviewboard.mozilla.org/r/249870/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/249870/

Note: This patch breaks tup builds, since they have their own method of generating built_in_addons.json which conflicts with this one. I'm ignoring this for now, since tup is currently tier 2, and the people working on that back-end probably have a better idea of the right way to fix this than I do.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8983990 [details]
Bug 1459004: Generate built_in_addons.json from moz.build definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249870/diff/1-2/

Comment 6

[Nick Alexander :nalexander [he/him]]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1466574, which is basically the same issue but arises more visibly in the context of trying to use Node.js in the build system with artifact builds.

Comment 7

[Mike Taylor [:miketaylr]]

*gentle review ping* 

(we're blocked on some webcompat research around Google Tier 1 experience for Fennec on this bug)

Comment 8

[Michael Shal [:mshal]]

(In reply to Kris Maglione [:kmag] from comment #4)
> Note: This patch breaks tup builds, since they have their own method of
> generating built_in_addons.json which conflicts with this one. I'm ignoring
> this for now, since tup is currently tier 2, and the people working on that
> back-end probably have a better idea of the right way to fix this than I do.

If we need to land these patches to unblock things, we can un-bust tup builds by adding built_in_addons.json to the _skip_files list:

diff --git a/python/mozbuild/mozbuild/backend/tup.py b/python/mozbuild/mozbuild/backend/tup.py
index 1529c147501f7..d8430e124cd9f 100644
--- a/python/mozbuild/mozbuild/backend/tup.py
+++ b/python/mozbuild/mozbuild/backend/tup.py
@@ -99,6 +99,7 @@ class BackendTupfile(object):
         # These files are special, ignore anything that generates them or
         # depends on them.
         self._skip_files = [
+            'built_in_addons.json',
             'signmar',
         ]

However, parsing install manifests to figure out which files are installed in features directories feels like the wrong approach. The install manifests are an implementation detail of the make backend, and adjusting gen_built_in_addons.py to accommodate multiple backends is not ideal. Additionally, parsing those install manifests to look for a few directories takes a long time - on my machine, gen_built_in_addons.py took ~20 seconds.

The approach chmanchester took in the tup backend to generate the file just keeps track of the FINAL_TARGET_FILES that are adding things into a features/ directory. We could generalize this idea by putting it in the common backend, and make it a first-class variable (so that we're not keying off of magic directory names in FINAL_TARGET_FILES). I think this could be done in a way to unify the structure of the moz.build files that install features, and have it work with all backends on desktop & android builds. Since it's done as we're already processing moz.build, keeping track of the list of features adds hardly any overhead as compared to parsing install manifests after the fact.

That said, I don't see anything wrong with the output that this creates, so if we need to we can try to land this and then optimize gen_built_in_addons.py away in a followup.

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8983990 [details]
Bug 1459004: Generate built_in_addons.json from moz.build definitions.

https://reviewboard.mozilla.org/r/249870/#review258348

::: toolkit/mozapps/extensions/gen_built_in_addons.py:42
(Diff revision 2)
> +    return path
> +
> +
> +def get_registry(paths):
> +    registry = FileRegistry()
> +    for base, path in manifest_paths:

This should be 'paths' instead of 'manifest_paths', since you pass in manifest_paths to get_registry.

::: toolkit/mozapps/extensions/gen_built_in_addons.py:87
(Diff revision 2)
> +
> +        listing["system"] = sorted(list(features))
>  
> -    with open(os.path.join(bindir, args.outputfile), 'w') as fh:
> +    json.dump(listing, output)
> -        json.dump(listing, fh, sort_keys=True)
>  

This should return a set() that lists the files read (eg: the manifest files). Right now if an install manifest changes, make doesn't see it as a dependency, and so it won't update built_in_addons.json with a new list of features.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Michael Shal [:mshal] from comment #8)
> However, parsing install manifests to figure out which files are installed
> in features directories feels like the wrong approach. The install manifests
> are an implementation detail of the make backend, and adjusting
> gen_built_in_addons.py to accommodate multiple backends is not ideal.
> Additionally, parsing those install manifests to look for a few directories
> takes a long time - on my machine, gen_built_in_addons.py took ~20 seconds.

I didn't particularly like the idea either, but it was the best solution I had for a short term fix, given the infrastructure we have now. Ideally, I'd like a more generalized way to do this that can handle otehr backends.

And, unfortunately, for the sake of dictionaries, just keeping track of FINAL_TARGET_FILES isn't really good enough, since a bunch of magic happens at build time to figure out what locale files we need to package.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8983990 [details]
Bug 1459004: Generate built_in_addons.json from moz.build definitions.

https://reviewboard.mozilla.org/r/249870/#review258348

> This should return a set() that lists the files read (eg: the manifest files). Right now if an install manifest changes, make doesn't see it as a dependency, and so it won't update built_in_addons.json with a new list of features.

When a GENERATED_FILES entry uses .flags, it automatically gets a dependency on buildconfig, and gets regenerated whenever any config file changes.

Comment 12

[Michael Shal [:mshal]]

(In reply to Kris Maglione [:kmag] from comment #10)
> I didn't particularly like the idea either, but it was the best solution I
> had for a short term fix, given the infrastructure we have now. Ideally, I'd
> like a more generalized way to do this that can handle otehr backends.

Ok, I filed bug 1470240 as a followup.

> And, unfortunately, for the sake of dictionaries, just keeping track of
> FINAL_TARGET_FILES isn't really good enough, since a bunch of magic happens
> at build time to figure out what locale files we need to package.

The dictionaries are in LOCALIZED_FILES, so presumably we could track those too. I'm not sure if all the l10n info required to generate a correct file for a localized build is available to mozbuild yet, but I think we should be able to support that.

Comment 13

[Michael Shal [:mshal]]

(In reply to Kris Maglione [:kmag] from comment #11)
> > This should return a set() that lists the files read (eg: the manifest files). Right now if an install manifest changes, make doesn't see it as a dependency, and so it won't update built_in_addons.json with a new list of features.
> 
> When a GENERATED_FILES entry uses .flags, it automatically gets a dependency
> on buildconfig, and gets regenerated whenever any config file changes.

This isn't true since bug 1402012 landed, so we'll need to specify the inputs explicitly. Consider this example:

./mach build
# Edit browser/extensions/moz.build, and delete the 'onboarding' line
./mach build

The second build should regenerate built_in_addons.json without the onboarding entry, but it doesn't with this patch.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 8983990 [details]
Bug 1459004: Generate built_in_addons.json from moz.build definitions.

Punting this to mshal for now, sorry for the delay!

Comment 15

[Kris Maglione [:kmag]]

(In reply to Michael Shal [:mshal] from comment #12)
> > And, unfortunately, for the sake of dictionaries, just keeping track of
> > FINAL_TARGET_FILES isn't really good enough, since a bunch of magic happens
> > at build time to figure out what locale files we need to package.
> 
> The dictionaries are in LOCALIZED_FILES, so presumably we could track those
> too. I'm not sure if all the l10n info required to generate a correct file
> for a localized build is available to mozbuild yet, but I think we should be
> able to support that.

We could. The problem is that localized files use globs for the locales, and the actual files that we package depends on the locales that we're building for, and there's not really any easy way to get to that logic when processing build config. The globs for the locales even get written into the build manifests, and the manifest processors map those to actual sources and targets based on which inputs are available.

I wish I had a more straightforward solution, but for now, I don't.

(In reply to Michael Shal [:mshal] from comment #13)
> (In reply to Kris Maglione [:kmag] from comment #11)
> > When a GENERATED_FILES entry uses .flags, it automatically gets a dependency
> > on buildconfig, and gets regenerated whenever any config file changes.
> 
> This isn't true since bug 1402012 landed, so we'll need to specify the
> inputs explicitly. Consider this example:
> 
> ./mach build
> # Edit browser/extensions/moz.build, and delete the 'onboarding' line
> ./mach build
> 
> The second build should regenerate built_in_addons.json without the
> onboarding entry, but it doesn't with this patch.

Hm. I believe this worked when I first tested, but I'll check again and update if necessary. Setting .flags on a generated source entry adds a separate dependency to the source which, as far as I could tell, caused it to be rebuilt whenever the build config changed.

Comment 16

[Michael Shal [:mshal]]

(In reply to Kris Maglione [:kmag] from comment #15)
> (In reply to Michael Shal [:mshal] from comment #13)
> > (In reply to Kris Maglione [:kmag] from comment #11)
> > > When a GENERATED_FILES entry uses .flags, it automatically gets a dependency
> > > on buildconfig, and gets regenerated whenever any config file changes.
> > 
> > This isn't true since bug 1402012 landed, so we'll need to specify the
> > inputs explicitly. Consider this example:
> > 
> > ./mach build
> > # Edit browser/extensions/moz.build, and delete the 'onboarding' line
> > ./mach build
> > 
> > The second build should regenerate built_in_addons.json without the
> > onboarding entry, but it doesn't with this patch.
> 
> Hm. I believe this worked when I first tested, but I'll check again and
> update if necessary. Setting .flags on a generated source entry adds a
> separate dependency to the source which, as far as I could tell, caused it
> to be rebuilt whenever the build config changed.

It looks like the .flags usage adds a dependency on that directory's backend.mk, but the backend.mk files are only updated during backend generation if the contents have changed (using FileAvoidWrite). Since this script uses other parts of the backend as inputs (the install manifests), we need to declare those manually by returning a set.

Comment 17

[Michael Shal [:mshal]]

Comment on attachment 8983990 [details]
Bug 1459004: Generate built_in_addons.json from moz.build definitions.

Feel free to re-flag me as review once it's updated. It should be a pretty straightforward r+ then.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8983990 [details]
Bug 1459004: Generate built_in_addons.json from moz.build definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249870/diff/2-3/

Comment 19

[Michael Shal [:mshal]]

Comment on attachment 8983990 [details]
Bug 1459004: Generate built_in_addons.json from moz.build definitions.

https://reviewboard.mozilla.org/r/249870/#review259696

Comment 20

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6120c2bb51e2057df51f4d52510bb5f4e8b4ca5
Bug 1459004: Generate built_in_addons.json from moz.build definitions. r=mshal

Comment 21

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d6120c2bb51e