1160563 - Convert ANDROID_RES_DIRS and ANDROID_ASSETS_DIR to moz.build variables

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Nick Alexander :nalexander [he/him]]

We already have ANDROID_ASSET_DIR for APKs produced from raw Makefile.in descriptions (like robocop.apk).  We don't use that mechanism for Fennec; Fennec is a special snowflake.

We should add something like ANDROID_ASSET_FILES in moz.build to get files easily into the assets/ directory of the Fennec APK.

How to do this is documented at http://www.ncalexander.net/blog/2014/07/02/adding-assets-to-the-fennec-apk-file/.  The files can be consumed by using the resource android://assets/ -- see Bug 948465.

Comment 1

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba157a128887

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1160563 - Pre: Allow ANDROID_RES_DIRS that are not relative to srcdir. r?gps

Bug 1160563 - Make ANDROID_ASSETS_DIRS a list in moz.build. r?gps

We have had singular ANDROID_ASSETS_DIR in Makefile.in for a while.
Fennec itself does not use the existing Makefile.in Android code, for
complicated historical reasons.

This makes the existing variable moz.build-only; generalizes the
existing variable to an ordered list; and adds the equivalent use of
the new list to the Fennec build, with a simple example asset.

glandium and nfroyd requested that I use a
ContextDerivedTypedList(SourcePath), and gps suggested an Android
package wrapper type.  The former doesn't help for pass-thru
variables, and the latter is a larger change than I have time for.
Many consumers would like this, including future Fennec distributions,
possibly b2gdroid, and third-party consumers who are re-packaging
Fennec; this satisfies the need with minimal effort.

Comment 3

[Nick Alexander :nalexander [he/him]]

gps: the discussion I refer to above happened in Bug 1163082.  That ticket did much more than this one, so I'm moving this (much simpler) thing here.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8631231 [details]
MozReview Request: Bug 1160563 - Pre: Allow ANDROID_RES_DIRS that are not relative to srcdir. r?gps

https://reviewboard.mozilla.org/r/12349/#review11587

There are both changes to moz.build workings and AFAICT to packaging in this commit. But the commit message only details the former. Am I missing something? Please explain the packaging changes to me, preferably in the commit message.

::: python/mozbuild/mozbuild/frontend/context.py:682
(Diff revision 1)
> +    'ANDROID_ASSETS_DIRS': (List, list,

We really should be using SourcePath everywhere paths are captured in moz.build. If it weren't for `ANDROID_ASSETS_DIRS += [TOPSRCDIR + base + 'assets']` in one of the moz.build files, I'd be willing to let this slide.

Please write the extra few lines in emitter.py to assign to the passthru object properly.

::: toolkit/mozapps/installer/upload-files.mk:472
(Diff revision 1)
> +    $(ZIP) -r9D $(_ABS_DIST)/gecko.ap_ assets && \

What are the implications of adding assets to this package? This change seems unrelated to the moz.build refactoring.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Comment on attachment 8631231 [details]
> MozReview Request: Bug 1160563 - Make ANDROID_ASSETS_DIRS a list in
> moz.build. r?gps
> 
> https://reviewboard.mozilla.org/r/12349/#review11587
> 
> There are both changes to moz.build workings and AFAICT to packaging in this
> commit. But the commit message only details the former. Am I missing
> something? Please explain the packaging changes to me, preferably in the
> commit message.
> 
> ::: python/mozbuild/mozbuild/frontend/context.py:682
> (Diff revision 1)
> > +    'ANDROID_ASSETS_DIRS': (List, list,
> 
> We really should be using SourcePath everywhere paths are captured in
> moz.build. If it weren't for `ANDROID_ASSETS_DIRS += [TOPSRCDIR + base +
> 'assets']` in one of the moz.build files, I'd be willing to let this slide.

I explicitly want to allow paths outside of the source directory, for branding and especially for distributions.  We could assert that automation always puts distributions in the source directory?  I don't know what precedents desktop repacks, etc make.  I know for sure that l10n files are outside of the source directory, so clearly we don't expect all "extras" to the build system to be in the source directory.

> Please write the extra few lines in emitter.py to assign to the passthru
> object properly.

Sure, contingent on the discussion above.  NI to gps to comment.

> ::: toolkit/mozapps/installer/upload-files.mk:472
> (Diff revision 1)
> > +    $(ZIP) -r9D $(_ABS_DIST)/gecko.ap_ assets && \
> 
> What are the implications of adding assets to this package? This change
> seems unrelated to the moz.build refactoring.

Without this, the assets don't make it to the final APK.  What happens:

First mobile/android/base/Makefile.in produces gecko.ap_, which is a ZIP file with special content.  The new -A flag includes assets/ in the ap_.

Then upload-files.mk unpacks gecko.ap_, adds some additional things (libraries), rezips into apk, and then zipaligns and signs everything.

Without the packager change, the assets/ directory in the ap_ gets left out of the final apk.  This whole approach is totally non-standard but is more or less required to support our single-locale repack scheme.

Comment 6

[Gregory Szorc [:gps]]

(In reply to Nick Alexander :nalexander from comment #5)
> I explicitly want to allow paths outside of the source directory, for
> branding and especially for distributions.  We could assert that automation
> always puts distributions in the source directory?  I don't know what
> precedents desktop repacks, etc make.  I know for sure that l10n files are
> outside of the source directory, so clearly we don't expect all "extras" to
> the build system to be in the source directory.

Sorry, I meant to say Path, not SourcePath. And it has provisions for representing paths outside the source directory. See https://hg.mozilla.org/mozilla-central/file/f7e1f596d57d/python/mozbuild/mozbuild/frontend/context.py#l337.
 
> > Please write the extra few lines in emitter.py to assign to the passthru
> > object properly.
> 
> Sure, contingent on the discussion above.  NI to gps to comment.

Pretty sure Path will solve your problems.
 
> > ::: toolkit/mozapps/installer/upload-files.mk:472
> > (Diff revision 1)
> > > +    $(ZIP) -r9D $(_ABS_DIST)/gecko.ap_ assets && \
> > 
> > What are the implications of adding assets to this package? This change
> > seems unrelated to the moz.build refactoring.
> 
> Without this, the assets don't make it to the final APK.  What happens:
> 
> First mobile/android/base/Makefile.in produces gecko.ap_, which is a ZIP
> file with special content.  The new -A flag includes assets/ in the ap_.
> 
> Then upload-files.mk unpacks gecko.ap_, adds some additional things
> (libraries), rezips into apk, and then zipaligns and signs everything.
> 
> Without the packager change, the assets/ directory in the ap_ gets left out
> of the final apk.  This whole approach is totally non-standard but is more
> or less required to support our single-locale repack scheme.

Got it. Something in version control (preferably in-line comments or something in build/docs or secondary a commit message) is preferred. Grokking packaging from reading dozens of bugs is not conducive to understanding and easily changing things down the road.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #6)
> Sorry, I meant to say Path, not SourcePath. And it has provisions for
> representing paths outside the source directory. See
> https://hg.mozilla.org/mozilla-central/file/f7e1f596d57d/python/mozbuild/
> mozbuild/frontend/context.py#l337.

It actually doesn't. Yet. I remember I made a comment about using % as a prefix for absolute paths, btw.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Gregory Szorc [:gps] from comment #6)
> > Sorry, I meant to say Path, not SourcePath. And it has provisions for
> > representing paths outside the source directory. See
> > https://hg.mozilla.org/mozilla-central/file/f7e1f596d57d/python/mozbuild/
> > mozbuild/frontend/context.py#l337.
> 
> It actually doesn't. Yet. I remember I made a comment about using % as a
> prefix for absolute paths, btw.

gps: in light of this, and the context I provided, what is the absolute minimum that needs to happen to move this ahead?

Comment 9

[Mike Hommey [:glandium]]

I'd suggest adding an AbsolutePath class.

Comment 10

[Gregory Szorc [:gps]]

AbsolutePath class seems easiest. I don't think it is too much work.

Comment 11

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e38105b40e02

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8631231 [details]
MozReview Request: Bug 1160563 - Pre: Allow ANDROID_RES_DIRS that are not relative to srcdir. r?gps

Bug 1160563 - Pre: Allow ANDROID_RES_DIRS that are not relative to srcdir. r?gps

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1160563 - Part 1: Make ANDROID_RES_DIRS a moz.build variable. r?gps

Bug 1160563 - Part 1: Make ANDROID_RES_DIRS a moz.build variable. r?gps

This patch does a few things.  First, it adds an AbsolutePath data
type, sibling to SourcePath and ObjDirPath.  (Existing Path consumers
that accept an open set of Path subtypes, and that only use full_path,
should function fine with the new AbsolutePath subtype.)

Second, it moves ANDROID_RES_DIRS to a moz.build list of Paths
(ordered).  We test, but don't use in tree, the new AbsolutePath.

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1160563 - Part 2: Make ANDROID_ASSETS_DIRS a moz.build variable. r?gps

Bug 1160563 - Part 2: Make ANDROID_ASSETS_DIRS a moz.build variable. r?gps

We have had singular ANDROID_ASSETS_DIR in Makefile.in for a while.
Fennec itself does not use the existing Makefile.in Android code, for
complicated historical reasons.

This makes the existing variable moz.build-only; generalizes the
existing variable to an ordered list; and adds the equivalent use of
the new list to the Fennec build, with a simple example asset.

This patch also updates the packager to include assets packed into the
gecko.ap_.  Without the packager change, the assets/ directory in the
ap_ gets left out of the final apk.  This whole approach is totally
non-standard but is more or less required to support our single-locale
repack scheme.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8631231 [details]
MozReview Request: Bug 1160563 - Pre: Allow ANDROID_RES_DIRS that are not relative to srcdir. r?gps

https://reviewboard.mozilla.org/r/12349/#review14089

Ship It!

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8645949 [details]
MozReview Request: Bug 1160563 - Part 1: Make ANDROID_RES_DIRS a moz.build variable. r?gps

https://reviewboard.mozilla.org/r/15561/#review14091

I wish there were unit tests for AbsolutePath. But having a variable use it and tests for that variable should be sufficient.

::: python/mozbuild/mozbuild/frontend/context.py:351
(Diff revision 1)
> +      - '%/absolute/paths'

Should we bikeshed on `%` vs `//`? Other build systems use `/` and `//` to distinguish between absolute paths and project relative paths. Although, I think our use would be reversed (Bazel uses `//` to denote project relative paths IIRC and I believe its derivatives - Buck, Pants - do the same).

One concern I have with `//` is that when concatenation is performed, `'/' + variable` could inadvertently not reference an absolute path. Chances of collision should be low. But you never know.

I'm OK with `%` unless others insist on `//` or something else.

::: python/mozbuild/mozbuild/frontend/emitter.py:707
(Diff revision 1)
> +                    raise SandboxValidationError('Directory listed in '

At some point we should start doing this validation at moz.build variable evaluation time, as opposed to deferring it to the emitter. But that's for another day.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8645950 [details]
MozReview Request: Bug 1160563 - Part 2: Make ANDROID_ASSETS_DIRS a moz.build variable. r?gps

https://reviewboard.mozilla.org/r/15563/#review14095

::: python/mozbuild/mozbuild/frontend/context.py:701
(Diff revision 1)
> +        This variable contains a list of directories, each relative to
> +        the srcdir, containing static files to package into an 'assets/'
> +        directory and merge into an APK file.

It is a `Path` instance, so values can be not just "relative to the srcdir."

Comment 18

[Mike Hommey [:glandium]]

> Should we bikeshed on `%` vs `//`? Other build systems use `/` and `//` to
> distinguish between absolute paths and project relative paths. Although, I
> think our use would be reversed (Bazel uses `//` to denote project relative
> paths IIRC and I believe its derivatives - Buck, Pants - do the same).

FWIW, I have two concerns with '//', which is why I suggested something else:
- the fact that it would be the exact opposite of what those other build systems use
- the fact that absolute paths on windows don't start with a / even when normalizing path separators

Comment 19

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/15561/#review14097

::: python/mozbuild/mozbuild/frontend/context.py:351
(Diff revision 1)
> +      - '%/absolute/paths'

I'd put '%/filesystem/absolute/paths' to be even more explicit.

::: python/mozbuild/mozbuild/frontend/context.py:405
(Diff revision 1)
>          if value.startswith('!'):

You need to reject '%' here

::: python/mozbuild/mozbuild/frontend/context.py:406
(Diff revision 1)
>              raise ValueError('Object directory paths are not allowed')

And change this error message (as well as the one for ObjDirPath)

::: python/mozbuild/mozbuild/frontend/context.py:455
(Diff revision 1)
> +        self.full_path = mozpath.normpath(value[1:])

should probably check for isabs(value[1:]), because, what is '%foo' ?

Comment 20

[Nick Alexander :nalexander [he/him]]

gps, glandium: thanks for the prompt and thorough review.  I can't push due to tree closure, but I addressed most of your concerns.  In particular I added glandium's isabs predicate, and I added a test asserting the predicate is used for AbsolutePath.  So now we have an anchor point for future tests.

Comment 21

[Nick Alexander :nalexander [he/him]]

url:        https://hg.mozilla.org/integration/fx-team/rev/7ac5ef42b07a28d6a25a1eece7f0be113a9196fa
changeset:  7ac5ef42b07a28d6a25a1eece7f0be113a9196fa
user:       Nick Alexander <[e-mail]>
date:       Mon Aug 10 13:18:32 2015 -0700
description:
Bug 1160563 - Pre: Allow ANDROID_RES_DIRS that are not relative to srcdir. r=gps

url:        https://hg.mozilla.org/integration/fx-team/rev/f662067f106aa0b68adf797aac8d34a85f969064
changeset:  f662067f106aa0b68adf797aac8d34a85f969064
user:       Nick Alexander <[e-mail]>
date:       Wed Aug 12 11:03:44 2015 -0700
description:
Bug 1160563 - Part 1: Make ANDROID_RES_DIRS a moz.build variable. r=gps

This patch does a few things.  First, it adds an AbsolutePath data
type, sibling to SourcePath and ObjDirPath.  (Existing Path consumers
that accept an open set of Path subtypes, and that only use full_path,
should function fine with the new AbsolutePath subtype.)

Second, it moves ANDROID_RES_DIRS to a moz.build list of Paths
(ordered).  We test, but don't use in tree, the new AbsolutePath.

url:        https://hg.mozilla.org/integration/fx-team/rev/b288dff997a4db9ca5f280813232d60bd162568f
changeset:  b288dff997a4db9ca5f280813232d60bd162568f
user:       Nick Alexander <[e-mail]>
date:       Wed Aug 12 11:04:03 2015 -0700
description:
Bug 1160563 - Part 2: Make ANDROID_ASSETS_DIRS a moz.build variable. r=gps

We have had singular ANDROID_ASSETS_DIR in Makefile.in for a while.
Fennec itself does not use the existing Makefile.in Android code, for
complicated historical reasons.

This makes the existing variable moz.build-only; generalizes the
existing variable to an ordered list; and adds the equivalent use of
the new list to the Fennec build, with a simple example asset.

This patch also updates the packager to include assets packed into the
gecko.ap_.  Without the packager change, the assets/ directory in the
ap_ gets left out of the final apk.  This whole approach is totally
non-standard but is more or less required to support our single-locale
repack scheme.

Comment 22

[Nick Alexander :nalexander [he/him]]

This went in a different direction.  It doesn't add a list installing files into the Fennec APK's assets/ directory, it just adds the lower level primitives.  Progress.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ac5ef42b07a
https://hg.mozilla.org/mozilla-central/rev/f662067f106a
https://hg.mozilla.org/mozilla-central/rev/b288dff997a4

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8631231 [details]
MozReview Request: Bug 1160563 - Pre: Allow ANDROID_RES_DIRS that are not relative to srcdir. r?gps

This request applies to all patches.

Approval Request Comment
[Feature/regressing bug #]: The work here is a prerequisite for the parental controls theme we implemented and uplifted in bug 1182514. However without these patches we do not package the theme's png (bug 1205274)

[User impact if declined]: There's basically no theme applied when using a restricted profile (See bug 1182514)

[Describe test coverage new/current, TreeHerder]: I checked out aurora and applied the patches locally. They apply cleanly and bug 1205274 is fixed.

[Risks and why]: I talked to nalexander on #mobile and he said "it's a clean uplift" and "basically zero risk".

[String/UUID change made/needed]: -

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8645949 [details]
MozReview Request: Bug 1160563 - Part 1: Make ANDROID_RES_DIRS a moz.build variable. r?gps

See comment above.

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8645950 [details]
MozReview Request: Bug 1160563 - Part 2: Make ANDROID_ASSETS_DIRS a moz.build variable. r?gps

See comment above.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8631231 [details]
MozReview Request: Bug 1160563 - Pre: Allow ANDROID_RES_DIRS that are not relative to srcdir. r?gps

Build system changes are usually safe. Taking it to simplify your work.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fbb02d5e6784
https://hg.mozilla.org/releases/mozilla-aurora/rev/84e994df9849
https://hg.mozilla.org/releases/mozilla-aurora/rev/39fcc0e11d27