854530 - Move RESOURCE_* declarations in mobile/android/base/Makefile.in to moz.build

Status: RESOLVED INVALID

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

mobile/android/base/Makefile.in has lots of Android resource declarations.  I'd like to transition them to moz.build.

I see three types of resources:

* local: copied from $SRCDIR/mobile/android/base/resources to $OBJDIR/mobile/android/base/res
* external: copied from elswhere in $SRCDIR to $OBJDIR/mobile/android/base/res (branding, icon)
* preprocessed from $SRCDIR/mobile/android/base/resources/*.in to $OBJDIR/mobile/android/base/res

I propose an `ANDROID_RESOURCES` moz.build object that encapsulates all these types of resources.  That would include maintaining the $OBJDIR/mobile/android/base/res directory tree (see Bug 750497).

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: pre: Remove unused MOZ_APP_ICON from mobile/android/base/Makefile.in.

MOZ_APP_ICON was a Makefile dependency not mentioned elsewhere in the
tree.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: pre: Remove unused LINUX_BRANDING_FILES from mobile/android/branding/**/Makefile.in.

The text `LINUX_BRANDING` does not appear in the tree.  There are no
references to `fennec_48x48.png` or `fennec_72x72.png` outside of
mobile/android/base/Makefile.in, and that file explicitly copies the
referenced files (so does not depend on the deleted export rules).

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Move resource declarations in mobile/android/base/Makefile.in to moz.build.

This patch:

* adds a new sandbox symbol, 'ANDROID_PACKAGE', to collect resource
  declarations;
* adds a new TreeMetadata subclass, AndroidPackageData to hold said
  declarations;
* adds a new backend AndroidPackageMakefileFragment class to write
  backend.mk rules to process the resources;
* moves lots of things out of mobile/android/base/Makefile.in and into
  moz.build;
* moves branding files out of android-resources.mn and into
  android-resources.build files.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 729888 [details] [diff] [review]
Move resource declarations in mobile/android/base/Makefile.in to moz.build.

Could I get some feedback on this approach for moving `mobile/android/base/Makefile.in` to moz.build?

gps: this might be the first rich type in moz.build.

mfinkle: can you see anything missing?

rnewman: I just wouldn't feel right not bug-spamming you.

Happy to add others as suggested.

Comment 5

[Gregory Szorc [:gps]]

Nice patches! I suspect at least some of the things you removed are actually used by l10n repacks. I could be wrong.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #5)
> Nice patches! I suspect at least some of the things you removed are actually
> used by l10n repacks. I could be wrong.

It's entirely possible; the l10n repack world seems like a weird and wonderful minefield.  In any case, l10n should only be touching strings.xml, which is generated by mobile/android/base/locales/Makefile.in.  Famous last words.

The two symbols (MOZ_APP_ICON and LINUX_BRANDING_FILES) are not referenced elsewhere in the tree, so they should be fine.

Comment 7

[Nick Alexander :nalexander [he/him]]

Try build in the works:

https://tbpl.mozilla.org/?tree=Try&rev=fbea33f06051

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 729888 [details] [diff] [review]
Move resource declarations in mobile/android/base/Makefile.in to moz.build.

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

I am positively weeping with joy. It looks good to me, but -- assuming the build and l10n folks are happy -- let's make sure:

* An Aurora-branded push works correctly (better to find out now rather than in a week)
* Incremental builds work correctly
* Our services landing code is up-to-date with this
* We haven't regressed clean or 0-work incremental build times.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 729888 [details] [diff] [review]
Move resource declarations in mobile/android/base/Makefile.in to moz.build.

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

My initial reaction is: \o/

The new API makes it clear what you are doing: attaching files to an Android package. Before, you just saw a bunch of variables and had to know how they were all hooked up. Winning!

While there are many imperfections with the code, this is much better than what we have today. More importantly, it curbs the existing Makefile.in footgun and allows the build team to iterate on improvements without involving major changes to Makefile.in or moz.build files. I'm very tempted to ignore most of the faults and see this committed ASAP.

Blockers to landing:

* Catch unknown attribute assignments to ANDROID_PACKAGE
* Write some tests
* Consider future "schema" evolution
* Maybe make the auto-generated make rules suck less

::: mobile/android/base/moz.build
@@ +6,5 @@
>  DIRS += ['locales']
> +
> +include('android-services-files.build')
> +
> +ANDROID_PACKAGE.add_preprocessed_resources([

This file could use a number of comments. These variables are obviously grouped. How so? Is it worth preserving the grouping or is one giant list sufficient?

::: mobile/android/branding/aurora/android-resources.build
@@ +1,1 @@
> +ANDROID_PACKAGE.add_external_resource(

This file needs a license.

And, all these files are essentially the same. Why don't you declare a local variable with the branding name and include a common file that uses that variable. This might close out bug 786538.

::: python/mozbuild/mozbuild/backend/android.py
@@ +94,5 @@
> +        self._write_external_resource_copies(backend_file)
> +        self._write_preprocessed_resources(backend_file)
> +        self._write_resource_directories(backend_file)
> +        self._write_android_package_resources(backend_file)
> +        self._write_garbage(backend_file)

I understand you are copying things verbatim from existing Makefile.in. That's fine. And I might let it slide for an initial landing. But, we should eventually convert these explicit rules to rules.mk conventions. e.g. INSTALL_TARGETS, PP_TARGETS, etc.

Even better, I'd love for us to be able to write out a manifest of some kind and have mozpack or something else perform rsync-like behavior to symlink/copy/preprocess/whatever those files into a destination directory. This generic solution would solve so many problem in our build system (headers, IDLs, JS modules, ...). Implementing it is beyond the scope of this bug.

::: python/mozbuild/mozbuild/frontend/android.py
@@ +11,5 @@
> +    SandboxDerived,
> +)
> +
> +
> +class AndroidPackageData(TreeMetadata):

This should just inherit from object. TreeMetadata is for things emitted by the emitter. SandboxDerived isa TreeMetadata, so you've already got that covered.

@@ +23,5 @@
> +    * preprocessed resources
> +
> +    External resources are those outside of the source directory
> +    describing the package -- usually, these are branding resources.
> +    """

I think I like what you are trying to do with the data model. However, I'm not sure how correct it is because I don't know much about Java/Android packaging.

I like the idea of having a rich data type defining Java/Android packages. We can more easily attach metadata to a single object than have said metadata represented by N independent global variables.

I would like us to consider the generic application of what you are doing here. As implemented, we've tightly coupled things like "res" and "resources" in file names and the existence of a single AndroidPackageData in a single moz.build file. Is it worth allowing the definition of multiple AndroidPackageData per moz.build file? What other attributes do you foresee us adding to this? Is it really an "Android" specific class or are we really talking just Java here? See https://src.chromium.org/svn/trunk/src/build/java.gypi for how GYP does it.

@@ +36,5 @@
> +    def add_resources(self, resources):
> +        for dst in resources:
> +            # dst like 'res/layout/foo.xml', src like 'resources/layout/foo.xml'
> +            src = dst.replace('res/', 'resources/')
> +            self.resources[dst] = src

Instead of passing in a list, you can use positional arguments:

def add_resources(self, *resources):
    # resources is a list.
    for dst in resources:
        # do stuff

This makes callers a little cleaner:

ANDROID_PACKAGE.add_resources(
    'res/drawable-mdpi/desktop.png',
    'res/drawable-mdpi/mobile.png'
)

If "res" and "resources" are Mozilla conventions and might be variable, we should consider having them passed in as arguments.

@@ +41,5 @@
> +
> +    def _add_preprocessed_resource(self, src, dst):
> +        self.preprocessed_resources[dst] = src
> +
> +    def add_preprocessed_resources(self, resources):

This seems like a special case of add_resources(preprocessed=True, files). This is purely an API design issue.

I guess the question is whether we should carry around separate containers for each resource flavor or have a unified container of resources and attach rich metadata to each resource entry (is_preprocessed, is_external, etc). I don't know.

@@ +56,5 @@
> +        outset = set()
> +        for r in [self.resources,
> +                  self.preprocessed_resources,
> +                  self.external_resources]:
> +            outset.update(r.keys())

This is Python: we can do awesome set operations:

outset = set()
outset |= self.resources.keys()
outset |= self.preprocessed_resources.keys()
outset |= self.external_resources.keys()

@@ +57,5 @@
> +        for r in [self.resources,
> +                  self.preprocessed_resources,
> +                  self.external_resources]:
> +            outset.update(r.keys())
> +        return outset

This class will allow assignment of arbitrary attributes. e.g. package.foo = 'foo'. One of the goals of moz.build files is to disallow this type of no-op operation (to prevent cargo culting and to ensure that every operation does something).

You should define a __setattr__ that disallows arbitrary attribute assignment.

It's worth noting that mshal is running into the same issue in bug 846634. If there's not a base class in the stdlib that disallows arbitrary attribute assignment (that doesn't use __slots__), perhaps we should create one - possibly based on a metaclass. That's out of scope for this patch, I think.

@@ +66,5 @@
> +    A thin wrapper around an `AndroidPackageData` instance.
> +    """
> +    __slots__ = ('package',)
> +
> +    def __init__(self, sandbox, package):

Perhaps this class can just inherit from AndroidPackageData *and* SandboxDerived and you can copy AndroidPackageData's data into the new class instance.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #5)
> Nice patches! I suspect at least some of the things you removed are actually
> used by l10n repacks. I could be wrong.

It /should/ be ok.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Comment on attachment 729888 [details] [diff] [review]
> Move resource declarations in mobile/android/base/Makefile.in to moz.build.
> 
> Review of attachment 729888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am positively weeping with joy. It looks good to me, but -- assuming the
> build and l10n folks are happy -- let's make sure:
> 
> * An Aurora-branded push works correctly (better to find out now rather than
> in a week)

To test this, I would just push to try with a mozconfig configuring aurora branding, and then test the resulting APK?  I can do this, but I'll wait one round of improvements.

> * Incremental builds work correctly

I did a good deal of manual testing: touching resources and preprocessed inputs, and removing resources and preprocessed outputs, etc.  I'm pretty certain the incremental build dependencies are no worse than they were before.

> * Our services landing code is up-to-date with this

It isn't.  That's a separate thing: Bug 855318.

> * We haven't regressed clean or 0-work incremental build times.

There should be no changes to `make -C mobile/android/base clean`, but I haven't tested this with any rigor.  This is so dependent on the disk cache that I don't think ad hoc tests are valuable.  As for 0-work incremental build time, I laugh in your face -- for reasons I don't understand, mobile/android/base/Makefile.in FORCEs classes.dex to be built.  In theory this approach does significantly less incremental work than the existing approach (for example -- not preprocessing everything when one thing changes) but compared to the actual dexing, this work is just noise.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 729888 [details] [diff] [review]
> Move resource declarations in mobile/android/base/Makefile.in to moz.build.
> 
> Review of attachment 729888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My initial reaction is: \o/

Thanks for this excellent feedback!  I will iterate ASAP.

> The new API makes it clear what you are doing: attaching files to an Android
> package. Before, you just saw a bunch of variables and had to know how they
> were all hooked up. Winning!
> 
> While there are many imperfections with the code, this is much better than
> what we have today. More importantly, it curbs the existing Makefile.in
> footgun and allows the build team to iterate on improvements without
> involving major changes to Makefile.in or moz.build files. I'm very tempted
> to ignore most of the faults and see this committed ASAP.
> 
> Blockers to landing:
> 
> * Catch unknown attribute assignments to ANDROID_PACKAGE

Happy to do this.

> * Write some tests

Feels dirty to post anything *without* tests.

> * Consider future "schema" evolution

There is some funky stuff in mobile/android/base/Makefile.in around
generating classes for web apps (WebApps*) and generating boilerplate
for Android custom views.  I'm not sure how that would fit in to
ANDROID_PACKAGE, so I punted.

> * Maybe make the auto-generated make rules suck less

More on this below.

> ::: mobile/android/base/moz.build
> @@ +6,5 @@
> >  DIRS += ['locales']
> > +
> > +include('android-services-files.build')
> > +
> > +ANDROID_PACKAGE.add_preprocessed_resources([
> 
> This file could use a number of comments. These variables are obviously
> grouped. How so? Is it worth preserving the grouping or is one giant list
> sufficient?

Happy to comment this, for sure.  The semantic grouping for Android is
by destination directory.  I considered having something like

Resource(mdpi, hdpi='', xhdpi='', ...)

to make it clear that resources come in flavours, but there are too many
axes: density, device size, Android version, etc.  In the end, the files
just need to be in the correct output directories, and the semantic
grouping maps directly to the output directory, so it can be easily
recovered.

> ::: mobile/android/branding/aurora/android-resources.build
> @@ +1,1 @@
> > +ANDROID_PACKAGE.add_external_resource(
> 
> This file needs a license.
> 
> And, all these files are essentially the same. Why don't you declare a local
> variable with the branding name and include a common file that uses that
> variable. This might close out bug 786538.

I am happy to take guidance here.  Two reasons that I didn't coalesce
these `android-resources.build` files:

* (minor) at some point, /someone/ thought they might be branding specific;
* (major) it's helpful to have the manifest that references the branding
  files be close to the files themselves.  It's a huge pain to crawl the
  tree looking for references to some icon file, and it gets harder if
  that reference looks like '%s/%s/icon.png' % (BASE, BRANDING) or
  similar.  If you actually see
  'mobile/android/branding/content/icon.png' in a text file, that's much
  easier for searching and updating.

> ::: python/mozbuild/mozbuild/backend/android.py
> @@ +94,5 @@
> > +        self._write_external_resource_copies(backend_file)
> > +        self._write_preprocessed_resources(backend_file)
> > +        self._write_resource_directories(backend_file)
> > +        self._write_android_package_resources(backend_file)
> > +        self._write_garbage(backend_file)
> 
> I understand you are copying things verbatim from existing Makefile.in.
> That's fine. And I might let it slide for an initial landing. But, we should
> eventually convert these explicit rules to rules.mk conventions. e.g.
> INSTALL_TARGETS, PP_TARGETS, etc.

I explicitly *didn't* do this because I'm not convinced that having
backend.mk write to rules.mk's API is sensible: I'd rather write
Makefile logic in Python than in rules.mk.  The backend.mk the script
produces is not fit for human consumption but it's not intended to be
consumed by humans.  I am happy to write to rules.mk's API, if that is
the decision, but this is adding a level of indirection that might not
be needed.

> Even better, I'd love for us to be able to write out a manifest of some kind
> and have mozpack or something else perform rsync-like behavior to
> symlink/copy/preprocess/whatever those files into a destination directory.
> This generic solution would solve so many problem in our build system
> (headers, IDLs, JS modules, ...). Implementing it is beyond the scope of
> this bug.

As a first step, I will split out a InstallManifestMakefileFragment that
does what I'm doing a little more generically.  That'll be an entry
point for mozbuild.backend to all do the same thing re: install
manifests (even if it just writes Makefile rules, or to rules.mk's API,
for now).  I will want this for handling Java code very shortly in any
case.

> ::: python/mozbuild/mozbuild/frontend/android.py
> @@ +11,5 @@
> > +    SandboxDerived,
> > +)
> > +
> > +
> > +class AndroidPackageData(TreeMetadata):
> 
> This should just inherit from object. TreeMetadata is for things emitted by
> the emitter. SandboxDerived isa TreeMetadata, so you've already got that
> covered.

The comment for `TreeMetadata` says """Base class for all data being
captured.""".  `AndroidPackageData` originally inherited from object :)

> @@ +23,5 @@
> > +    * preprocessed resources
> > +
> > +    External resources are those outside of the source directory
> > +    describing the package -- usually, these are branding resources.
> > +    """
> 
> I think I like what you are trying to do with the data model. However, I'm
> not sure how correct it is because I don't know much about Java/Android
> packaging.
> 
> I like the idea of having a rich data type defining Java/Android packages.
> We can more easily attach metadata to a single object than have said
> metadata represented by N independent global variables.
> 
> I would like us to consider the generic application of what you are doing
> here. As implemented, we've tightly coupled things like "res" and
> "resources" in file names and the existence of a single AndroidPackageData
> in a single moz.build file. Is it worth allowing the definition of multiple
> AndroidPackageData per moz.build file? What other attributes do you foresee
> us adding to this? Is it really an "Android" specific class or are we really
> talking just Java here? See
> https://src.chromium.org/svn/trunk/src/build/java.gypi for how GYP does it.

I spent some time thinking about it, and decided that it wasn't worth
allowing multiple `AndroidPackageData` declarations per moz.build yet.
(When we want that, we could just make ANDROID_PACKAGE a dictionary
mapping string names to `AndroidPackageData` instances, possibly with
restrictions on key creation.)  One immediate complication is that
string resources (namely assembling `strings.xml`) is split between
base/ and base/locales.

The resource stuff is definitely specific to Android, since we want to
do Android packaging things to it.  At the moment, I've left those
things in Makefile.in (see the rules for `R.java` and `gecko.ap_`), but
this effort won't be complete until those things are out standardized
away from Makefile.in.  The patch on Bug 747540 is relevant here: it's
standardizing the rules.mk API for this stuff.

Re: GYP: That's not Java-generic, it's an Android-specific mess :(  We
will essentially do the same, since (AFAIK) we build nothing with Java
that's not Android.

> @@ +36,5 @@
> > +    def add_resources(self, resources):
> > +        for dst in resources:
> > +            # dst like 'res/layout/foo.xml', src like 'resources/layout/foo.xml'
> > +            src = dst.replace('res/', 'resources/')
> > +            self.resources[dst] = src
> 
> Instead of passing in a list, you can use positional arguments:
> 
> def add_resources(self, *resources):
>     # resources is a list.
>     for dst in resources:
>         # do stuff
> 
> This makes callers a little cleaner:
> 
> ANDROID_PACKAGE.add_resources(
>     'res/drawable-mdpi/desktop.png',
>     'res/drawable-mdpi/mobile.png'
> )
> 
> If "res" and "resources" are Mozilla conventions and might be variable, we
> should consider having them passed in as arguments.

Sure, not a huge fan of * and ** arguments but will follow convention.

> @@ +41,5 @@
> > +
> > +    def _add_preprocessed_resource(self, src, dst):
> > +        self.preprocessed_resources[dst] = src
> > +
> > +    def add_preprocessed_resources(self, resources):
> 
> This seems like a special case of add_resources(preprocessed=True, files).
> This is purely an API design issue.
> 
> I guess the question is whether we should carry around separate containers
> for each resource flavor or have a unified container of resources and attach
> rich metadata to each resource entry (is_preprocessed, is_external, etc). I
> don't know.

I didn't know either, so I did the simplest possible thing.  I'll try
the `is_*` interface in the `InstallManifestMakefileFragment` I propose
and see how it feels.

> @@ +56,5 @@
> > +        outset = set()
> > +        for r in [self.resources,
> > +                  self.preprocessed_resources,
> > +                  self.external_resources]:
> > +            outset.update(r.keys())
> 
> This is Python: we can do awesome set operations:
> 
> outset = set()
> outset |= self.resources.keys()
> outset |= self.preprocessed_resources.keys()
> outset |= self.external_resources.keys()

Yeah, as a mathematician I can't get over the fact this isn't typeset as
\cup.

> @@ +57,5 @@
> > +        for r in [self.resources,
> > +                  self.preprocessed_resources,
> > +                  self.external_resources]:
> > +            outset.update(r.keys())
> > +        return outset
> 
> This class will allow assignment of arbitrary attributes. e.g. package.foo =
> 'foo'. One of the goals of moz.build files is to disallow this type of no-op
> operation (to prevent cargo culting and to ensure that every operation does
> something).
> 
> You should define a __setattr__ that disallows arbitrary attribute
> assignment.
> 
> It's worth noting that mshal is running into the same issue in bug 846634.
> If there's not a base class in the stdlib that disallows arbitrary attribute
> assignment (that doesn't use __slots__), perhaps we should create one -
> possibly based on a metaclass. That's out of scope for this patch, I think.

Thanks for pointing me at Bug 846634.  I'll suggest factoring out some
of this functionality on that ticket.

> @@ +66,5 @@
> > +    A thin wrapper around an `AndroidPackageData` instance.
> > +    """
> > +    __slots__ = ('package',)
> > +
> > +    def __init__(self, sandbox, package):
> 
> Perhaps this class can just inherit from AndroidPackageData *and*
> SandboxDerived and you can copy AndroidPackageData's data into the new class
> instance.

Not a big fan of multiple inheritance, but we could do this.  Just to be
clear, I separated `AndroidPackage` from `AndroidPackageData` for two
reasons:

* (minor) `SandboxDerived`'s constructor expects a `Sandbox`, which
  means that specifying default objects gets more complicated (you want
  a factory that takes `Sandbox` instances for this).
* (major) In theory, all path operations should happen at the emitter
  level.  I came to this conclusion and just saw it echoed in a comment
  you made on Bug 846634, so huzzah!

Again, thanks for great feedback.

Comment 13

[Gregory Szorc [:gps]]

(In reply to Nick Alexander :nalexander from comment #12)
> > ::: mobile/android/branding/aurora/android-resources.build
> > @@ +1,1 @@
> > > +ANDROID_PACKAGE.add_external_resource(
> > 
> > This file needs a license.
> > 
> > And, all these files are essentially the same. Why don't you declare a local
> > variable with the branding name and include a common file that uses that
> > variable. This might close out bug 786538.
> 
> I am happy to take guidance here.  Two reasons that I didn't coalesce
> these `android-resources.build` files:
> 
> * (minor) at some point, /someone/ thought they might be branding specific;
> * (major) it's helpful to have the manifest that references the branding
>   files be close to the files themselves.  It's a huge pain to crawl the
>   tree looking for references to some icon file, and it gets harder if
>   that reference looks like '%s/%s/icon.png' % (BASE, BRANDING) or
>   similar.  If you actually see
>   'mobile/android/branding/content/icon.png' in a text file, that's much
>   easier for searching and updating.

Fair enough. I really don't like DRY violations so I always point them out. We still have the old bug on file in case we want to tackle it. Let's not bloat scope.

> > ::: python/mozbuild/mozbuild/backend/android.py
> > @@ +94,5 @@
> > > +        self._write_external_resource_copies(backend_file)
> > > +        self._write_preprocessed_resources(backend_file)
> > > +        self._write_resource_directories(backend_file)
> > > +        self._write_android_package_resources(backend_file)
> > > +        self._write_garbage(backend_file)
> > 
> > I understand you are copying things verbatim from existing Makefile.in.
> > That's fine. And I might let it slide for an initial landing. But, we should
> > eventually convert these explicit rules to rules.mk conventions. e.g.
> > INSTALL_TARGETS, PP_TARGETS, etc.
> 
> I explicitly *didn't* do this because I'm not convinced that having
> backend.mk write to rules.mk's API is sensible: I'd rather write
> Makefile logic in Python than in rules.mk.  The backend.mk the script
> produces is not fit for human consumption but it's not intended to be
> consumed by humans.  I am happy to write to rules.mk's API, if that is
> the decision, but this is adding a level of indirection that might not
> be needed.

The problem (at least from my perspective) is duplication of install logic between Python and rules.mk. We already have INSTALL_TARGETS to install files "the right way." I'd prefer we use it.

That being said, I like the idea of a verbatim or near-verbatim switchover first followed by later optimizations.

> > Even better, I'd love for us to be able to write out a manifest of some kind
> > and have mozpack or something else perform rsync-like behavior to
> > symlink/copy/preprocess/whatever those files into a destination directory.
> > This generic solution would solve so many problem in our build system
> > (headers, IDLs, JS modules, ...). Implementing it is beyond the scope of
> > this bug.
> 
> As a first step, I will split out a InstallManifestMakefileFragment that
> does what I'm doing a little more generically.  That'll be an entry
> point for mozbuild.backend to all do the same thing re: install
> manifests (even if it just writes Makefile rules, or to rules.mk's API,
> for now).  I will want this for handling Java code very shortly in any
> case.

That could work! We can always refactor later.

> > ::: python/mozbuild/mozbuild/frontend/android.py
> > @@ +11,5 @@
> > > +    SandboxDerived,
> > > +)
> > > +
> > > +
> > > +class AndroidPackageData(TreeMetadata):
> > 
> > This should just inherit from object. TreeMetadata is for things emitted by
> > the emitter. SandboxDerived isa TreeMetadata, so you've already got that
> > covered.
> 
> The comment for `TreeMetadata` says """Base class for all data being
> captured.""".  `AndroidPackageData` originally inherited from object :)

The purpose of all the classes in mozbuild.frontend.data is to represent the output of the emitter. Classes inside moz.build sandboxes are (in theory) not chained up to anything in mozbuild.frontend.data. Although, that may need to change.

Comment 14

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #11)

> To test this, I would just push to try with a mozconfig configuring aurora
> branding, and then test the resulting APK?  I can do this, but I'll wait one
> round of improvements.

We used to have a branding patch that needed to be applied to get a "real" Aurora build. Not sure what the current state of the world is there. Just make sure this doesn't break when it merges to Aurora!

Comment 15

[Nick Alexander :nalexander [he/him]]

Try build for round 2:

http://tbpl.mozilla.org/?tree=Try&rev=4ad2dc42f74f

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: Address review comments in frontend (AndroidPackageData).

* inherit from object instead of `TreeMetadata`.
* define `__slots__` so that arbitrary attributes cannot be written.
* use set |= operator.
* take parameters for `input_base` and `output_base`.
* tests!

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Make ANDROID_PACKAGE look for resource files in `resources/`.

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: Address review comments in backend (AndroidPackageMakefileFragment).

* write PP_TARGETS and INSTALL_TARGETS rules for preprocessed and
  regular resources, respectively.
* use set |= operator.
* tests!

I explored breaking out a generic `InstallManifestMakefileFragment`
and it wasn't a clear win.  We can improve the implementation as use
cases become more clear.

Comment 19

[Nick Alexander :nalexander [he/him]]

Attachment: Address review comments in .build files.

* adds a single .build file to mobile/android/branding
* comments on resource grouping
* updates calling conventions

Comment 20

[Nick Alexander :nalexander [he/him]]

Since the review requests were significant, I've addressed them explicitly in separate patches.  This second round of patches applies on top of the initial patches.  After I get r+, I will rebase the review comments down to 3-4 patches.

You can view the patches on github at

https://github.com/ncalexan/mozilla-central/tree/nalexander/bug-854530-moz-build-resources

The mozbuild/backend/test_android.py shows what the backend.mk files look like.  I used rules.mk's API where possible and raw `cp` where not.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 729888 [details] [diff] [review]
Move resource declarations in mobile/android/base/Makefile.in to moz.build.

This looks fine to me. Given that Mozilla is moving to the moz.build approach, it will be nice to have this restructure.

My only request is that we wait for early in a cycle to land this and we make builds for aurora and beta channels (locally) so we can be ahead of the curve for any issues after merges.

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 730961 [details] [diff] [review]
Address review comments in frontend (AndroidPackageData).

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

::: python/mozbuild/mozbuild/frontend/android.py
@@ +55,3 @@
>          for dst in resources:
>              # dst like 'res/layout/foo.xml', src like 'resources/layout/foo.xml'
> +            src = dst.replace(self._output_base, self._input_base)

Shouldn't you only replace if the substring occurs at the beginning of the string?

@@ +68,3 @@
>          for dst in resources:
>              # dst like 'res/layout/foo.xml', src like 'resources/layout/foo.xml.in'
> +            src = dst.replace(self._output_base, self._input_base) + '.in'

Ditto.

::: python/mozbuild/mozbuild/test/frontend/test_android.py
@@ +12,5 @@
> +
> +
> +class TestAndroidPackageData(unittest.TestCase):
> +    def test_attributes(self):
> +        r"""Unrecognized attributes cannot be written."""

Nit: No need for r""" """ for docstrings. (You only typically use raw literals if you need the "raw" feature.)

@@ +19,5 @@
> +
> +        # Unknown attributes raise.
> +        def _test(p):
> +            return p.attribute_that_does_not_exist
> +        self.assertRaises(AttributeError, _test, package)

with self.assertRaises(AttributeError):
    p.attribute_that_does_not_exist

Comment 23

[Nick Alexander :nalexander [he/him]]

This is no longer valid.