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).

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

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).

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 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.

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

(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.

Try build in the works: https://tbpl.mozilla.org/?tree=Try&rev=fbea33f06051

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 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.

(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.

(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.

(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.

(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.

(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!

Try build for round 2: http://tbpl.mozilla.org/?tree=Try&rev=4ad2dc42f74f

* 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!

* 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.

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

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 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 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

This is no longer valid.