All users were logged out of Bugzilla on October 13th, 2018

Add configure option and build system for specifying Firefox for Android distribution files

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: nalexander, Assigned: nalexander, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 wontfix, firefox45 fixed, firefox46 fixed, firefox47 fixed)

Details

(Whiteboard: [lang=python][good next bug])

Attachments

(2 attachments, 4 obsolete attachments)

Right now, producing a Fennec APK with an embedded distribution requires an easy-but-magic-and-fragile step of manually copying distribution files into dist/bin at just the right time [1].  This ticket tracks adding a --with-mobile-android-distribution=DIR configure flag and the supporting build system work.

[1] https://wiki.mozilla.org/Mobile/Distribution_Files#Testing_a_distribution_locally
So the workflow here would be something like:

hg clone https://.../mozilla-central src
git clone https://.../disto.git distro-src
export MOZ_MOBILE_ANDROID_DISTRIBUTION_DIR=${PWD}/distro-src

and have the mozconfig pick up ${MOZ_MOBILE_ANDROID_DISTRIBUTION_DIR}?
(Assignee)

Comment 2

4 years ago
(In reply to Chris AtLee [:catlee] from comment #1)
> So the workflow here would be something like:
> 
> hg clone https://.../mozilla-central src
> git clone https://.../disto.git distro-src
> export MOZ_MOBILE_ANDROID_DISTRIBUTION_DIR=${PWD}/distro-src
> 
> and have the mozconfig pick up ${MOZ_MOBILE_ANDROID_DISTRIBUTION_DIR}?

Yes, exactly.  Right now we manually copy into objdir/dist/bin, but I'll update the build system to make this less fragile.
(Assignee)

Comment 3

4 years ago
rnewman, margaret: first, Distribution.java is pretty nice.  Bravo!

I see that the distribution files are expected to be in the root of the APK (well, in /distribution/).  This is not convenient for the build system and not future friendly, because Gradle and others expect such files to go into assets/.  Luckily, this expectation occurs only at one place [1].

I'd like to consider expecting distribution files in the APK to be in /assets/distribution/.  Can you comment on any problems that might cause?  It's possible assets/ doesn't work, but I'll test that.  Since this is baked into the APK, there is no legacy issue.  The only problem might be an existing process that needs to change -- but that's what this ticket is about.  Thoughts?

If we can do this, we can piggy back off the separately interesting Bug 1160563.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/distribution/Distribution.java#698
(Assignee)

Updated

4 years ago
Flags: needinfo?(rnewman)
Flags: needinfo?(margaret.leibovic)

Comment 4

4 years ago
I don't see that move being a problem. This logic was originally added back in bug 834681, and I don't see any particular reason we chose the root of the APK, other than convenience.

FYI, you'll need to update the mock-package.zip we use for testDistribution.
Flags: needinfo?(margaret.leibovic)
My only concern is that it would turn a single path entry into two path entries, so there's some maintenance and testing burden on adjusting the ZipEntry walker.

Other than testing to make sure it works, I don't see any issues with changing the in-APK path.

(This is ignoring the change for anyone who's doing distro-based repacks, of course -- an email to m-f-d when this lands wouldn't go amiss.)
Flags: needinfo?(rnewman)
(Assignee)

Comment 6

4 years ago
Created attachment 8604454 [details]
MozReview Request: bz://1163082/nalexander

/r/8629 - Bug 1163082 - Part 1: Add ANDROID_ASSETS_DIRS to moz.build. r=gps
/r/8631 - Bug 1163082 - Part 2: Prepare existing TEST_HARNESS_FILES code for re-use. r=froydnj
/r/8633 - Bug 1163082 - Part 3: Add ANDROID_ASSETS_FILES and bake assets into the APK. r=froyndj
/r/8635 - Bug 1163082 - Part 4: Add flexible pattern break |. r=gps
/r/8637 - Bug 1163082 - Part 5: Add --with-android-distribution-directory. r=gps

Pull down these commits:

hg pull -r 58b130436e85f010d6fe34d015becdc841c9ffb4 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8604454 - Flags: review?(nfroyd)
Attachment #8604454 - Flags: review?(gps)
(Assignee)

Comment 7

4 years ago
gps, froydnj: figure y'all can work out a reasonable review split.  I elected to approach this by addressing a more general case, namely Bug 1160563, as part of the distribution specific work.  (Observe that this allows writing to anywhere in assets/, not just into assets/distribution or similar.)
(Assignee)

Comment 8

4 years ago
Comment on attachment 8604454 [details]
MozReview Request: bz://1163082/nalexander

glandium: I expect you have an opinion on these path changes too.

For context, I tried to make FINAL_TARGET_FILES use this infrastructure as well but decided not to risk busting a complicated thing.
Attachment #8604454 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/8629/#review7311

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

Make that a ContextDerivedTypedList(SourcePath) instead of a List, and you get automatic support of topsrcdir-relative paths.
https://reviewboard.mozilla.org/r/8633/#review7313

::: mobile/android/base/moz.build:951
(Diff revision 1)
> +ANDROID_ASSETS_DIRS += [TOPOBJDIR + '/dist/android_assets']

FWIW, I have a patch in my queue that adds support for (top)objdir-relative paths for SourcePath (well, the patch renames it to Path)
https://reviewboard.mozilla.org/r/8635/#review7315

::: python/mozbuild/mozbuild/test/frontend/data/test-harness-files-patterns/moz.build:9
(Diff revision 1)
> +TEST_HARNESS_FILES.e += ["foo/inner|*"]

I get what you're trying to get at, but if I put myself in the boots of someone reading such a moz.build without knowing much about it, I have not the remote idea what the hell this is about. The TEST_HARNESS_FILES.path thing is already kind of confusing, this is adding another layer that I'm not sure is good to have.

Too much magic kills the magic. I think it's time to think about exposing FileFinder-like functions to moz.build so that one can do something like:
for file in find('foo/*'):
    TEST_HARNESS_FILES.a += [f[len('foo/'):]]

which I think reads better. (and we could have helper functions to remove top-levels because f[something:] is kind of ugly)
(In reply to Mike Hommey [:glandium] from comment #10)
> FWIW, I have a patch in my queue that adds support for (top)objdir-relative
> paths for SourcePath (well, the patch renames it to Path)

Also note I also want to change TEST_HARNESS_FILES to use that Path class instead of what it currently uses.
(In reply to Mike Hommey [:glandium] from comment #11)
> Too much magic kills the magic. I think it's time to think about exposing
> FileFinder-like functions to moz.build so that one can do something like:
> for file in find('foo/*'):
>     TEST_HARNESS_FILES.a += [f[len('foo/'):]]

In fact, this could even be a one liner:
  TEST_HARNESS_FILES.a += [f[len('foo/'):] for f in find('foo/*')]
(In reply to Mike Hommey [:glandium] from comment #11)
> https://reviewboard.mozilla.org/r/8635/#review7315
> 
> :::
> python/mozbuild/mozbuild/test/frontend/data/test-harness-files-patterns/moz.
> build:9
> (Diff revision 1)
> > +TEST_HARNESS_FILES.e += ["foo/inner|*"]
> 
> I get what you're trying to get at, but if I put myself in the boots of
> someone reading such a moz.build without knowing much about it, I have not
> the remote idea what the hell this is about. The TEST_HARNESS_FILES.path
> thing is already kind of confusing, this is adding another layer that I'm
> not sure is good to have.

More explicitly spelling out things for globbing is a Good Thing.  I honestly get confused about how glob patterns (especially recursive ones) get translated to install paths in TEST_HARNESS_FILES (maybe I'm the only one), and anything that would make globs more explicit gets +1 from me.
https://reviewboard.mozilla.org/r/8629/#review7359

r=me with glandium's comment addressed.  Please followup and fix ANDROID_RES_DIRS the same way.
https://reviewboard.mozilla.org/r/8631/#review7361

r=me with some name bikeshedding

::: python/mozbuild/mozbuild/backend/recursivemake.py:912
(Diff revision 1)
> -    def _process_test_harness_files(self, obj, backend_file):
> +    def _process_files(self, obj, backend_file, varname, destdir, install_manifest):

_process_files sounds a bit generic.  WDYT about calling this _process_abstract_file_list?

::: python/mozbuild/mozbuild/frontend/emitter.py:847
(Diff revision 1)
> +    def _process_files(self, context, varname, factory, allow_install_to_root=True):

Same comment from recursivemake.py applies here.
https://reviewboard.mozilla.org/r/8629/#review7367

::: python/mozbuild/mozbuild/frontend/context.py:563
(Diff revision 1)
> +    'ANDROID_ASSETS_DIRS': (List, list,
> +        """Android resource directories.
> +
> +        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.
> +        """, 'export'),

ANDROID_RES_DIRS already exists. At some point we'll probably want a generic hierarchical container for defining what goes into the APK. Is now that time?

::: build/mobile/robocop/Makefile.in:14
(Diff revision 1)
> -ANDROID_ASSETS_DIR := $(TESTPATH)/assets
> +ANDROID_ASSETS_DIRS := \
> +  $(TESTPATH)/assets \
> +  $(NULL)

Typically when we add variables to moz.build, we move them explicitly. This includes adding variables to a blacklist so occurrences in Makefile.in files trigger an error.

Is there a reason you aren't doing that here?

::: mobile/android/base/Makefile.in:428
(Diff revision 1)
> +		$$(addprefix -A ,$$(ANDROID_ASSETS_DIRS)) \

So we weren't packaging these directories before? What was the point of ANDROID_ASSET_DIR before this patch?
https://reviewboard.mozilla.org/r/8631/#review7369

::: python/mozbuild/mozbuild/backend/recursivemake.py:912
(Diff revision 1)
> -    def _process_test_harness_files(self, obj, backend_file):
> +    def _process_files(self, obj, backend_file, varname, destdir, install_manifest):

I wouldn't be surprised if this had a noticeable performance impact! Attribute and key lookups in tight loops are often noticeable in profile output!

::: python/mozbuild/mozbuild/frontend/data.py:230
(Diff revision 1)
> +    pass

You don't need `pass` when you have a docstring. That's because docstrings are part of the AST and satisfy the requirement that a block have content.

::: python/mozbuild/mozbuild/frontend/data.py:226
(Diff revision 1)
> +class TestHarnessFiles(AbstractFileList):

I'm assuming this class will do something useful in a subsequent patch. Please try to document these things in your commit message next time.
https://reviewboard.mozilla.org/r/8633/#review7371

::: python/mozbuild/mozbuild/backend/recursivemake.py:496
(Diff revision 1)
>          elif isinstance(obj, TestHarnessFiles):
>              self._process_test_harness_files(obj, backend_file)
>  
> +        elif isinstance(obj, AndroidAssetsFiles):
> +            self._process_android_assets_files(obj, backend_file)

Part of me thinks you should inline these methods while you are here...

::: python/mozbuild/mozbuild/frontend/data.py:236
(Diff revision 1)
> +    pass

Don't need pass.
https://reviewboard.mozilla.org/r/8635/#review7375

Why? You've introduced complexity for no documented reason.
Comment on attachment 8604454 [details]
MozReview Request: bz://1163082/nalexander

I think I have done enough damage with my reviews here.
Attachment #8604454 - Flags: review?(nfroyd)

Updated

3 years ago
Attachment #8604454 - Flags: review?(gps)
Attachment #8604454 - Flags: review?(mh+mozilla)
(Assignee)

Comment 22

3 years ago
Comment on attachment 8604454 [details]
MozReview Request: bz://1163082/nalexander
Attachment #8604454 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8620256 [details]
MozReview Request: Bug 1163082 - Part 3: Add ANDROID_ASSETS_FILES and bake assets into the APK. r=froyndj
(Assignee)

Comment 24

3 years ago
Created attachment 8620257 [details]
MozReview Request: Bug 1163082 - Part 4: Add flexible pattern break |. r=gps
(Assignee)

Comment 25

3 years ago
Created attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps
(Assignee)

Comment 26

3 years ago
Created attachment 8620259 [details]
MozReview Request: Bug 1163082 - Part 5: Add --with-android-distribution-directory. r=gps
(Assignee)

Comment 27

3 years ago
Created attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman
(Assignee)

Comment 28

3 years ago
Comment on attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/8629/diff/1-2/
Attachment #8620258 - Attachment description: MozReview Request: Bug 1163082 - Part 1: Add ANDROID_ASSETS_DIRS to moz.build. r=gps → MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps
Attachment #8620258 - Flags: review?(nfroyd)
Attachment #8620258 - Flags: review?(gps)
(Assignee)

Comment 29

3 years ago
Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/8631/diff/1-2/
Attachment #8620260 - Attachment description: MozReview Request: Bug 1163082 - Part 2: Prepare existing TEST_HARNESS_FILES code for re-use. r=froydnj → MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r?rnewman
Attachment #8620260 - Flags: review?(rnewman)
Attachment #8620260 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8620256 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8620257 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8620259 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1234629
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment on attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

Confirmed with nalexander that I was not supposed to review this patch.
Attachment #8620258 - Flags: review?(nfroyd)
Attachment #8620260 - Flags: review?(nfroyd)
Attachment #8620260 - Flags: review?(rnewman) → review+
Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

https://reviewboard.mozilla.org/r/8631/#review29261
(Assignee)

Updated

3 years ago
Attachment #8620258 - Flags: review?(nfroyd)
(Assignee)

Comment 33

3 years ago
Comment on attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/8629/diff/2-3/
(Assignee)

Updated

3 years ago
Attachment #8620260 - Attachment description: MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r?rnewman → MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman
Attachment #8620260 - Flags: review?(nfroyd)
(Assignee)

Comment 34

3 years ago
Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/8631/diff/2-3/
Attachment #8620258 - Flags: review?(nfroyd)
Attachment #8620260 - Flags: review?(nfroyd)
Comment on attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

https://reviewboard.mozilla.org/r/8629/#review29905
Attachment #8620258 - Flags: review?(gps) → review+
(Assignee)

Comment 36

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/e228040a044b7ff7363a178da2cb0b8b42724048
Bug 1163082 - Part 1: Add --with-android-distribution-directory. r=gps

https://hg.mozilla.org/integration/fx-team/rev/baf25be8d4917e6dcc52eede79e61e1837328c86
Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman
(Assignee)

Comment 37

3 years ago
Developer documentation and sample repository updated.  Mailing list post is at https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-February/001739.html.
(Assignee)

Comment 38

3 years ago
We might want to uplift this to make it easier to get distributions out with partners.  We can do it as needed.
(Assignee)

Comment 41

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/489b15a7dce6b8d26dddc26387ea0d02d057c298
Bug 1163082 - Part 1: Add --with-android-distribution-directory. r=gps

https://hg.mozilla.org/integration/fx-team/rev/926d1440ce98c618194236956342a627fdec4a93
Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Comment 42

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/489b15a7dce6
https://hg.mozilla.org/mozilla-central/rev/926d1440ce98
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
status-firefox45: --- → affected
status-firefox46: --- → affected
Nick, could you fill the uplift request for this bug as it is needed for bug 1234629? Thanks
status-firefox40: affected → wontfix
Flags: needinfo?(nalexander)
(Assignee)

Comment 44

3 years ago
Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman


Approval Request Comment
[Feature/regressing bug #]: needed by bouncer APK (Bug 1234629).

[User impact if declined]: delayed testing from partners.

[Describe test coverage new/current, TreeHerder]: very little automated -- it's almost impossible to automate.  I've tested it manually.

[Risks and why]: very low.  It's possible we'd see build issues in beta; they'd be immediately obvious.  Otherwise this changes no code in Fennec (or any other product) itself.  It's possible it would interact with partner distributions in the wild, but I've taken care that the existing automated tests don't change in that regard.

[String/UUID change made/needed]: none.

I'm happy to co-ordinate uplift patches myself.

There's a dependency on getting the Android partner sample builds to work so that we can actually check out the partner distribution and get it into the bouncer APK.  I'll work with jlund to make the actual builds happen.
Flags: needinfo?(nalexander)
Attachment #8620260 - Flags: approval-mozilla-beta?
Attachment #8620260 - Flags: approval-mozilla-aurora?
Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Taking it.
Should be in 45 beta 8
If you can take of the uplift, it would be great.
Attachment #8620260 - Flags: approval-mozilla-beta?
Attachment #8620260 - Flags: approval-mozilla-beta+
Attachment #8620260 - Flags: approval-mozilla-aurora?
Attachment #8620260 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.