Closed Bug 1234629 (bouncerapk) Opened 9 years ago Closed 8 years ago

Create bouncer APK for OTA distribution installs

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 affected, firefox46 disabled, firefox47 fixed, fennec47+)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- disabled
firefox47 --- fixed
fennec 47+ ---

People

(Reporter: Margaret, Assigned: nalexander)

References

Details

Attachments

(10 files)

MozReview Request: Bug 1234629 - Pre: Make Distribution look in /data/data/.../distribution first. r?rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 1234629 - Create bouncer APK for OTA distribution installs. f?margaret
58 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1234629 - Follow-up: Include assets. f?margaret
58 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1234629 - Part 1: Create bouncer APK for OTA distribution installs. r?margaret,gps
58 bytes, text/x-review-board-request
Margaret
: review+
Details
MozReview Request: Bug 1234629 - Don't track distribution assets in version control. r?nalexander
58 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1234629 - Copy distribution files in bouncer APK. f?rnewman
58 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1234629 - Part 2: Fail packaging if bouncer APK and main APK have different permissions. r?gps
58 bytes, text/x-review-board-request
gps
: review+
Details
MozReview Request: Bug 1234629 - Part 3: Make bouncer's <intent-filter> set a larger subset of Fennec's <intent-filter> set. r?margaret
58 bytes, text/x-review-board-request
Margaret
: review+
Details
MozReview Request: Bug 1234629 - Post: Add Gradle support for bouncer. r=me
58 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1234629 - Post: Make bouncer APK Java package org.mozilla.gecko, like the main APK. r=me
58 bytes, text/x-review-board-request
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details 
Partners want to be able to install distribution builds OTA, but our APK is too large. In the long term, we should work on reducing our APK size for all users, as well as moving more parts of the app to DLC, but in the very short term, let's explore creating a very small bouncer APK.

This APK should copy distribution files to /data/data, and then later bounce users to the play store to update to a full Firefox build.

We started exploring the technical details here:
https://public.etherpad-mozilla.org/p/bouncer

Nick offered to help modify our build system to create this bouncer APK at build time.
Flags: needinfo?(nalexander)
margaret: see if the commit I pushed works for you locally.  Make sure to run |mach configure| first.  Everything should be 100% artifact build compliant.
Flags: needinfo?(nalexander) → needinfo?(margaret.leibovic)
https://reviewboard.mozilla.org/r/28983/#review25771

Note that the bouncer should have a substantially similar manifest to Fennec itself. One partner requested that it have all the same intent filters. I don't think we can get exact parity -- no accounts, no content providers, etc. -- but the closer we can get, the better.

::: mobile/android/bouncer/AndroidManifest.xml.in:6
(Diff revision 1)
>        android:versionCode="@ANDROID_VERSION_CODE@"

This should be a manually specified very small number, like '1'. It should definitely be less than any released Fennec version. Because we don't include native code, we don't need multiple version codes and multiple APKs.
https://reviewboard.mozilla.org/r/28983/#review25771

Hmm.  sebastian was fairly strongly of the opinion that starting from nothing and adding whatever manifest entries were needed was the way to go, so I implemented that (and am leaving it to margaret to flesh out the intent filters, etc.)  But if we want to start from Fennec and then override launchers, etc, we can arrange that.

> This should be a manually specified very small number, like '1'. It should definitely be less than any released Fennec version. Because we don't include native code, we don't need multiple version codes and multiple APKs.

Yeah, it's 1 -- it's set in `mobile/android/bouncer/moz.build`.
Comment on attachment 8701551 [details]
MozReview Request: Bug 1234629 - Follow-up: Include assets. f?margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28983/diff/1-2/
Attachment #8701551 - Attachment description: MozReview Request: Bug 1234629 - Add main activity for bouncer APK → MozReview Request: Bug 1234629 - Follow-up: Include assets. f?margaret
Comment on attachment 8701551 [details]
MozReview Request: Bug 1234629 - Follow-up: Include assets. f?margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28983/diff/1-2/
Comment on attachment 8701860 [details]
MozReview Request: Bug 1234629 - Don't track distribution assets in version control. r?nalexander

Review commit: https://reviewboard.mozilla.org/r/29043/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29043/
Flags: needinfo?(margaret.leibovic)
Oh, reviewboard/mozreview. I miss Splinter, tbh.



AndroidManifest:

I think the reason a partner wants these to have the same names as our real activities is twofold:

1. IIRC, Android is not smart when the set of supported activities changes across package upgrades. I've seen weird stuff as a user, and I've seen confused developers, too.
2. If a user reorders their launcher icons prior to clicking through the bouncer, this will screw that up -- our launcher icon will disappear and be replaced on upgrade.


>                 Log.i(LOGTAG, "Copying file: " + path);

Consider whether to leave this in, and at which level to log.


71: 
>                paths.addAll(getFiles(newPath));

Rename `getFiles` to `accumulateFiles`, take an `ArrayList` as input, and avoid the need to allocate a new one for each subdirectory. (You can keep the `return` to make using this easier.)


88:
>         if (!dir.exists()) {

`getParentFile` can return null. You should null-check here.
Status: NEW → ASSIGNED
OS: Unspecified → Android
Hardware: Unspecified → All
Version: Firefox 35 → Trunk
Attachment #8701861 - Flags: review?(rnewman) → review-
Oh, and a final review comment: you're opening N input streams as you copy, but you're only closing the last one. The finally block should be inside the loop.
(In reply to Richard Newman [:rnewman] from comment #14)

> AndroidManifest:
> 
> I think the reason a partner wants these to have the same names as our real
> activities is twofold:
> 
> 1. IIRC, Android is not smart when the set of supported activities changes
> across package upgrades. I've seen weird stuff as a user, and I've seen
> confused developers, too.
> 2. If a user reorders their launcher icons prior to clicking through the
> bouncer, this will screw that up -- our launcher icon will disappear and be
> replaced on upgrade.

This point about a shortcut icon is a good one. I agree we'll need to make sure the main activity name stays the same between updates in order to keep the same shortcut icon around and working. This will be important if a partner is creating their own shortcut icon from this activity.

We should talk more about what to do with the manifest, since there are other things to think about, like whether we should register to handle VIEW intents. We probably should, to promote Firefox's existence on the device, but we'll have a less-than-optimal UX if the user chooses to open a link with Firefox, and then we just take them to the Play store.
If we're willing to do a small amount of work, we can write clicked URLs into the same place that Tab Queue is expecting them.
Depends on: 1236621
Comment on attachment 8701860 [details]
MozReview Request: Bug 1234629 - Don't track distribution assets in version control. r?nalexander

In general, we shouldn't be hiding stuff like this.  (And if we do, add it to .gitignore too.)

If assets/ is empty, aapt complains.  We really do want the assets/ directory to exist; and the only way to make hg insure that is to have a file in there.

What's the motivation here?  You don't have to commit everything in your local tree while testing.  It seems that what we really want is to make https://bugzilla.mozilla.org/show_bug.cgi?id=1163082 stick the distribution into assets/, and to include that distribution in the bouncer as well.  Reasonable?
Attachment #8701860 - Flags: review?(nalexander) → review-
(In reply to Nick Alexander :nalexander [Vacation PTO from Dec 23--Jan 4] from comment #18)
> Comment on attachment 8701860 [details]
> MozReview Request: Bug 1234629 - Don't track distribution assets in version
> control. r?nalexander
> 
> In general, we shouldn't be hiding stuff like this.  (And if we do, add it
> to .gitignore too.)
> 
> If assets/ is empty, aapt complains.  We really do want the assets/
> directory to exist; and the only way to make hg insure that is to have a
> file in there.
> 
> What's the motivation here?  You don't have to commit everything in your
> local tree while testing.  It seems that what we really want is to make
> https://bugzilla.mozilla.org/show_bug.cgi?id=1163082 stick the distribution
> into assets/, and to include that distribution in the bouncer as well. 
> Reasonable?

Yeah, that seems like it would be a better long-term solution.

Mainly, I wanted to avoid accidentally committing any distribution files, since they can contain partner-sensitive information. It seems risky to require developers to copy these sensitive files to a place tracked in hg in order to create a distribution re-pack.
Comment on attachment 8701861 [details]
MozReview Request: Bug 1234629 - Copy distribution files in bouncer APK. f?rnewman

When I tested this, the distribution filed were copied, but the distribution didn't apply on update.

However, I realized this is likely because this patch doesn't set the distribution state to STATE_SET. That's the only time when we'll look for the distribution in /data/data:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java?force=1#464
Comment on attachment 8701550 [details]
MozReview Request: Bug 1234629 - Create bouncer APK for OTA distribution installs. f?margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28981/diff/1-2/
Comment on attachment 8701551 [details]
MozReview Request: Bug 1234629 - Follow-up: Include assets. f?margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28983/diff/2-3/
Comment on attachment 8701860 [details]
MozReview Request: Bug 1234629 - Don't track distribution assets in version control. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29043/diff/1-2/
Attachment #8701860 - Flags: review- → review?(nalexander)
Comment on attachment 8701861 [details]
MozReview Request: Bug 1234629 - Copy distribution files in bouncer APK. f?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29045/diff/1-2/
Attachment #8701861 - Flags: review- → review?(rnewman)
Nick offered to help take this over. The two main things I think we need to do here before landing are:

1) Make sure the distribution is applied to the full browser app on update.
2) Include full app permissions in bouncer app manifest, so that app can update automatically in the background.

I think we can push off adding more to the bouncer manifest to follow-up bugs.
Assignee: margaret.leibovic → nalexander
Comment on attachment 8701861 [details]
MozReview Request: Bug 1234629 - Copy distribution files in bouncer APK. f?rnewman

https://reviewboard.mozilla.org/r/29045/#review25909

::: mobile/android/bouncer/AndroidManifest.xml.in:34
(Diff revision 1)
> +            android:name="org.mozilla.bouncer.BouncerActivity"

I think the reason a partner wants these to have the same names as our real activities is twofold:

1. IIRC, Android is not smart when the set of supported activities changes across package upgrades. I've seen weird stuff as a user, and I've seen confused developers, too.
2. If a user reorders their launcher icons prior to clicking through the bouncer, this will screw that up -- our launcher icon will disappear and be replaced on upgrade.

::: mobile/android/bouncer/java/org/mozilla/bouncer/BouncerService.java:37
(Diff revision 1)
> +                Log.i(LOGTAG, "Copying file: " + path);

Consider whether to leave this in, and at which level to log.

::: mobile/android/bouncer/java/org/mozilla/bouncer/BouncerService.java:49
(Diff revision 1)
> +                    in.close();

This `finally` block should be *inside* the `for` loop. You're leaking `n-1` `InputStream`s.

::: mobile/android/bouncer/java/org/mozilla/bouncer/BouncerService.java:71
(Diff revision 1)
> +                paths.addAll(getFiles(newPath));

Rename `getFiles` to `accumulateFiles`, take an `ArrayList` as input, and avoid the need to allocate a new one for each subdirectory. (You can keep the `return` to make using this easier.)

::: mobile/android/bouncer/java/org/mozilla/bouncer/BouncerService.java:88
(Diff revision 1)
> +        if (!dir.exists()) {

`getParentFile` can return null. You should null-check here.

::: mobile/android/bouncer/java/org/mozilla/gecko/BrowserApp.java:32
(Diff revision 2)
> +        startActivity(new Intent(Intent.ACTION_VIEW, Uri.parse("market://details?id=" + getPackageName())));

Does the package name need to be escaped? (See shit like Bug 1188550…)
Attachment #8701861 - Flags: review?(rnewman)
It looks like this will be a requirement as part of a partner deal, but likely won't be needed sooner than 46, since that deal also requires Android 6.0 runtime permissions.
tracking-fennec: --- → ?
tracking-fennec: ? → 47+
This wouldn't be tracking to a particular release in practical terms, therefore I'm assuming this is a stake in the ground?
Alias: bouncerapk
Depends on: 1163082
Comment on attachment 8701237 [details]
MozReview Request: Bug 1234629 - Pre: Make Distribution look in /data/data/.../distribution first. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28919/diff/1-2/
Attachment #8701237 - Attachment description: MozReview Request: Bug 1234629 - Create bouncer APK for OTA distribution installs. f?margaret → MozReview Request: Bug 1234629 - Pre: Make Distribution look in /data/data/.../distribution first. r?rnewman
Attachment #8701237 - Flags: review?(rnewman)
Comment on attachment 8701601 [details]
MozReview Request: Bug 1234629 - Part 1: Create bouncer APK for OTA distribution installs. r?margaret,gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28997/diff/1-2/
Attachment #8701601 - Attachment description: MozReview Request: Bug 1234629 - Follow-up: Include assets. f?margaret → MozReview Request: Bug 1234629 - Part 1: Create bouncer APK for OTA distribution installs. r?margaret,gps
Attachment #8701601 - Flags: review?(margaret.leibovic)
Attachment #8701601 - Flags: review?(gps)
This isn't sensible for b2gdroid, but that project should never enable
the bouncer APK anyway.

Review commit: https://reviewboard.mozilla.org/r/32707/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32707/
Attachment #8712934 - Flags: review?(gps)
This small change is a follow-up to Bug 1242213, which did the same
for the main package.  This is a nod to the future and Gradle, which
cleanly splits the internal Java package (org.mozilla.gecko) from the
external Android package (org.mozilla.fennec and friends).

Review commit: https://reviewboard.mozilla.org/r/32713/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32713/
Comment on attachment 8701601 [details]
MozReview Request: Bug 1234629 - Part 1: Create bouncer APK for OTA distribution installs. r?margaret,gps

https://reviewboard.mozilla.org/r/28997/#review29523

Fennec's build system makes my head hurt.

::: mobile/android/bouncer/AndroidManifest.xml.in:1
(Diff revision 2)
> +#filter substitution

Should this file have been marked as copied from an existing, similar one?

::: mobile/android/bouncer/Makefile.in:15
(Diff revision 2)
> +PP_TARGETS += manifest
> +manifest := $(srcdir)/AndroidManifest.xml.in
> +manifest_TARGET := export
> +# Special 'cuz they are set in mobile/android/defs.mk.
> +manifest_FLAGS += \
> +  -DMOZ_ANDROID_SHARED_ID="$(MOZ_ANDROID_SHARED_ID)" \
> +  -DMOZ_ANDROID_SHARED_ACCOUNT_TYPE="$(MOZ_ANDROID_SHARED_ACCOUNT_TYPE)" \
> +  -DMOZ_ANDROID_SHARED_FXACCOUNT_TYPE="$(MOZ_ANDROID_SHARED_FXACCOUNT_TYPE)" \
> +  $(NULL)
> +
> +libs:: $(ANDROID_APK_NAME).apk

Can't we do this in moz.build?
Attachment #8701601 - Flags: review?(gps)
Attachment #8712934 - Flags: review?(gps) → review+
Comment on attachment 8712934 [details]
MozReview Request: Bug 1234629 - Part 2: Fail packaging if bouncer APK and main APK have different permissions. r?gps

https://reviewboard.mozilla.org/r/32707/#review29527
https://reviewboard.mozilla.org/r/28997/#review29523

> Should this file have been marked as copied from an existing, similar one?

Is that a good idea?  I wouldn't mind marking it as a copy of `base/AndroidManifest.xml.in`, if you think it's valuable.

> Can't we do this in moz.build?

I don't think so, for two reasons: first, as the comment notes, these are set using `defs.mk`, and it's my belief that such things are not available to `moz.build`.  I considered removing `mobile/android/defs.mk` entirely, but felt it as too much of a diversion from the task at hand to block on.  See https://bugzilla.mozilla.org/show_bug.cgi?id=870396 for tracking there.

Second, some APK files are rightly part of `libs`, and some part of `tools`.  I suppose we could grow a `moz.build` declaration for the APK tier, but again -- it's a diversion.  I'm not trying to improve the `moz.build`-Java-Android stuff any more, I'm spending time transitioning to Gradle.

If you feel strongly, tell me the absolute minimum you want to improve and I'll address it.
Comment on attachment 8701237 [details]
MozReview Request: Bug 1234629 - Pre: Make Distribution look in /data/data/.../distribution first. r?rnewman

https://reviewboard.mozilla.org/r/28919/#review29545

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:469
(Diff revision 2)
> +                checkDataDistribution() ||

I have a sneaking suspicion that this should come *last*, not first.

That is:

If we were launched through a marketing link, it should win over the bouncer.

If we somehow have a distro in the installed APK, that should win.

If we're installing via a bouncer, but there's actually a distribution in /system, that should win.

(I might be persuaded on the last point.)

Thoughts?
Attachment #8701237 - Flags: review?(rnewman) → review+
Comment on attachment 8712935 [details]
MozReview Request: Bug 1234629 - Part 3: Make bouncer's <intent-filter> set a larger subset of Fennec's <intent-filter> set. r?margaret

https://reviewboard.mozilla.org/r/32709/#review29571

While these changes look fine to me, I think we should come up with some system/policy for which <intent-filter> items we include, since this doesn't directly mirror our main manifest. We should also consider adding a comment in the main manifest to explain which entries are mapped to the bouncer.

We can always make this better in the future, so I'm fine with landing this as it is right now.
Attachment #8712935 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8701601 [details]
MozReview Request: Bug 1234629 - Part 1: Create bouncer APK for OTA distribution installs. r?margaret,gps

https://reviewboard.mozilla.org/r/28997/#review29573

Java bits look fine to me, mostly because it looks like what I wrote before :)

As I said in my last review comment, this looks like solid progress and good enough to land. We can make it better later if we need to, the goal right now is a POC.
Attachment #8701601 - Flags: review?(margaret.leibovic) → review+
(In reply to Nick Alexander :nalexander from comment #37)
> https://reviewboard.mozilla.org/r/28997/#review29523
> 
> > Should this file have been marked as copied from an existing, similar one?
> 
> Is that a good idea?  I wouldn't mind marking it as a copy of
> `base/AndroidManifest.xml.in`, if you think it's valuable.
> 
> > Can't we do this in moz.build?
> 
> I don't think so, for two reasons: first, as the comment notes, these are
> set using `defs.mk`, and it's my belief that such things are not available
> to `moz.build`.  I considered removing `mobile/android/defs.mk` entirely,
> but felt it as too much of a diversion from the task at hand to block on. 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=870396 for tracking there.
> 
> Second, some APK files are rightly part of `libs`, and some part of `tools`.
> I suppose we could grow a `moz.build` declaration for the APK tier, but
> again -- it's a diversion.  I'm not trying to improve the
> `moz.build`-Java-Android stuff any more, I'm spending time transitioning to
> Gradle.
> 
> If you feel strongly, tell me the absolute minimum you want to improve and
> I'll address it.

Adding needinfo for gps.  gps, there are two separate side quests here that I don't want to address.
Flags: needinfo?(gps)
https://reviewboard.mozilla.org/r/28997/#review29523

> I don't think so, for two reasons: first, as the comment notes, these are set using `defs.mk`, and it's my belief that such things are not available to `moz.build`.  I considered removing `mobile/android/defs.mk` entirely, but felt it as too much of a diversion from the task at hand to block on.  See https://bugzilla.mozilla.org/show_bug.cgi?id=870396 for tracking there.
> 
> Second, some APK files are rightly part of `libs`, and some part of `tools`.  I suppose we could grow a `moz.build` declaration for the APK tier, but again -- it's a diversion.  I'm not trying to improve the `moz.build`-Java-Android stuff any more, I'm spending time transitioning to Gradle.
> 
> If you feel strongly, tell me the absolute minimum you want to improve and I'll address it.

It's scope bloat. Ignore my requests.
Flags: needinfo?(gps)
https://hg.mozilla.org/integration/fx-team/rev/7a62e97f07c2c90d9f34c2c7de40f3cb193f6312
Bug 1234629 - Pre: Make Distribution look in /data/data/$PACKAGE/distribution last. r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/aaa420ed66d754ecc17b19f5a12297d24371f1ca
Bug 1234629 - Part 1: Create bouncer APK for OTA distribution installs. r=margaret,gps

https://hg.mozilla.org/integration/fx-team/rev/50bcadca213183aaa64e39632892b8f00957dcfc
Bug 1234629 - Part 2: Fail packaging if bouncer APK and main APK have different permissions. r=gps

https://hg.mozilla.org/integration/fx-team/rev/f80912ecfa87584394d22e28ed09e68c182d1b35
Bug 1234629 - Part 3: Make bouncer's <intent-filter> set a larger subset of Fennec's <intent-filter> set. r=margaret

https://hg.mozilla.org/integration/fx-team/rev/54a3d0851bc60d26fb4744c605787031070052c8
Bug 1234629 - Post: Add Gradle support for bouncer. r=me

https://hg.mozilla.org/integration/fx-team/rev/400c030d3c01d8818fe07d4547e0c145e19f099e
Bug 1234629 - Post: Make bouncer APK Java package org.mozilla.gecko, like the main APK. r=me

https://hg.mozilla.org/integration/fx-team/rev/c79f4a8c3e86aa0c1c693f637b9d5ac053a6fd2f
Bug 1234629 - Post: Add simple bouncer APK docs. r=me
https://hg.mozilla.org/integration/fx-team/rev/9aa32d9a25df32ef740835d14836ff20db0a0cc4
Bug 1234629 - Follow-up: clear distributions from /data/data during test. r=bustage
https://hg.mozilla.org/integration/fx-team/rev/985ecc25db90a9af5e75287ed049a883cdbbf134
Bug 1234629 - Part 0: Make Distribution look in /data/data/$PACKAGE/distribution last. r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/f66289e50d72401059c907080a3e56e52c2b390c
Bug 1234629 - Part 1: Create bouncer APK for OTA distribution installs. r=margaret,gps

https://hg.mozilla.org/integration/fx-team/rev/41547137d9f97f66ba34a2dfcc91c32c1f5180a2
Bug 1234629 - Part 2: Fail packaging if bouncer APK and main APK have different permissions. r=gps

https://hg.mozilla.org/integration/fx-team/rev/c1bf75444e88795d9c35cc5b90e13a498f1aa1b4
Bug 1234629 - Part 3: Make bouncer's <intent-filter> set a larger subset of Fennec's <intent-filter> set. r=margaret

https://hg.mozilla.org/integration/fx-team/rev/9f28798c041f5e7ec84fc96f8986b5c667a73a98
Bug 1234629 - Post: Add Gradle support for bouncer. r=me

https://hg.mozilla.org/integration/fx-team/rev/3c8e2be83df67250aca88bb55981c752eb67db73
Bug 1234629 - Post: Add simple bouncer APK docs. r=me
Sheriff: folded follow-up down and relanded.  The test failure was caused by Bug 1242213; I've backed out the offending piece, so this should be green now.
This broke single-locale APKs, 

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=76733110704b&exclusion_profile=false&filter-job_group_symbol=L10n

log section in question:

05:07:10     INFO -  28158279 application.ini (OK - compressed)
05:07:10     INFO -  28158760 package-name.txt (OK)
05:07:10     INFO -  28158847 ua-update.json (OK - compressed)
05:07:10     INFO -  28159225 platform.ini (OK - compressed)
05:07:10     INFO -  28159446 chrome.manifest (OK - compressed)
05:07:10     INFO -  28159557 removed-files (OK - compressed)
05:07:10     INFO -  28159728 assets/omni.ja (OK)
05:07:10     INFO -  37044484 classes.dex (OK)
05:07:10     INFO -  Verification succesful
05:07:10     INFO -  Testing is disabled - No Android Robocop for you
05:07:10     INFO -  cp: cannot stat `/builds/slave/m-cen-and-api-9-l10n_3-0000000/build/mozilla-central/obj-l10n/mobile/android/bouncer/bouncer-unsigned-unaligned.apk': No such file or directory
05:07:10    ERROR -  make[1]: *** [repackage-zip] Error 1
05:07:10     INFO -  make[1]: Leaving directory `/builds/slave/m-cen-and-api-9-l10n_3-0000000/build/mozilla-central/obj-l10n/mobile/android/locales'
05:07:10     INFO -  make: *** [repackage-zip-nl] Error 2
05:07:10    ERROR - Return code: 2
05:07:10     INFO - Setting buildbot property nl_failure to nl failed in make installers-nl!
05:07:10     INFO - Writing buildbot properties ['nl_failure'] to /builds/slave/m-cen-and-api-9-l10n_3-0000000/properties/nl_failure
05:07:10     INFO - Writing to file /builds/slave/m-cen-and-api-9-l10n_3-0000000/properties/nl_failure
05:07:10     INFO - Contents:
05:07:10     INFO -  nl_failure:nl failed in make installers-nl!
05:07:10    ERROR - nl failed in make installers-nl!
05:07:10  WARNING - setting return code to 9
05:07:10    ERROR - Repacked 0 of 9 binaries successfully.
05:07:10     INFO - #####
05:07:10     INFO - ##### Running upload-repacks step.
05:07:10     INFO - #####
Nick, Margaret, do you have an idea for a fix, or should this be backed out in the meantime?
Flags: needinfo?(nalexander)
Flags: needinfo?(margaret.leibovic)
(In reply to Axel Hecht [:Pike] from comment #51)
> Nick, Margaret, do you have an idea for a fix, or should this be backed out
> in the meantime?

Yeah, I'm pretty sure we should just handle this like Robocop.  I'll look tomorrow AM PST.  Keeping NI.
Flags: needinfo?(margaret.leibovic)
(In reply to Nick Alexander :nalexander from comment #52)
> (In reply to Axel Hecht [:Pike] from comment #51)
> > Nick, Margaret, do you have an idea for a fix, or should this be backed out
> > in the meantime?
> 
> Yeah, I'm pretty sure we should just handle this like Robocop.  I'll look
> tomorrow AM PST.  Keeping NI.

There's no particularly good way to determine in https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files.mk that we're repacking.  Robocop avoids this by setting --disable-tests in single locale repack mozconfigs.

Pike, before trying to address this, can you explain why we're still doing this at all?  We don't ship these in the play store and are unlikely to ever do so.
Flags: needinfo?(nalexander) → needinfo?(l10n)
We're doing single-locale builds so that we can test localizations before getting them out to the general public.

It's a catch-22, can we add this to the build? Well, we should test. Well, we'll need to add it to the build.
Flags: needinfo?(l10n)
Comment on attachment 8701860 [details]
MozReview Request: Bug 1234629 - Don't track distribution assets in version control. r?nalexander

This ticket is closed.  This change is not needed.
Attachment #8701860 - Flags: review?(nalexander)
re-flagging Nick per comment 54
Flags: needinfo?(nalexander)
(In reply to Axel Hecht [:Pike] from comment #56)
> re-flagging Nick per comment 54

I'm not thrilled but see the value to the l10n community.  I'll address this shortly.  I don't see adding a new flag for this edge case, so I will make packaging the bouncer conditional on building tests, so bouncer.apk is handled like robocop.apk.
Flags: needinfo?(nalexander)
Blocks: 1247470
Comment on attachment 8712937 [details]
MozReview Request: Bug 1234629 - Post: Make bouncer APK Java package org.mozilla.gecko, like the main APK. r=me

Approval Request Comment
[Feature/regressing bug #]: bouncer APK -- a small APK that redirects to the play store.

[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.  Testing requires a "real" APK in the play store; this uplift is to let us test this new APK.

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

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

There's a dependency here: https://bugzilla.mozilla.org/show_bug.cgi?id=1163082
.  All the above applies there (but I don't want to duplicate everything; this system sucks when uplifting MozReview-ed patches!).

I'm happy to co-ordinate uplift patches myself.
Attachment #8712937 - Flags: approval-mozilla-beta?
Attachment #8712937 - Flags: approval-mozilla-aurora?
Comment on attachment 8712937 [details]
MozReview Request: Bug 1234629 - Post: Make bouncer APK Java package org.mozilla.gecko, like the main APK. r=me

Helps partners, seems low risk, taking it.
Attachment #8712937 - Flags: approval-mozilla-beta?
Attachment #8712937 - Flags: approval-mozilla-beta+
Attachment #8712937 - Flags: approval-mozilla-aurora?
Attachment #8712937 - Flags: approval-mozilla-aurora+
This is going to need rebasing work for at least beta. Haven't tried aurora yet.
I had to back these out of aurora for android build bustage, so this'll need work for both aurora and beta: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/8fb854b34485

https://treeherder.mozilla.org/logviewer.html#?job_id=1969165&repo=mozilla-aurora
status-firefox46: fixed → affected
Flags: needinfo?(nalexander)
will miss beta 9 i think
Flags: needinfo?(sledru)
OK. I pinged Nick. He won't be mad/too sad if it doesn't ship.
Flags: needinfo?(sledru)
Hopefully this can wait till beta 2.
Nick, do we still want to try uplifting this to 46, or should we set this to wontfix for 46?
(In reply to Wes Kocher (:KWierso) from comment #66)
> Nick, do we still want to try uplifting this to 46, or should we set this to
> wontfix for 46?

KWierso -- thanks for keeping tabs on this.  I pushed this and it's (trivial) pre-req:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/45a841d2c188
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/b49a3c403769
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/07f3e1db69aa
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/59f660dca352
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/4badb36f71b9
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/808d8b35a357
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/46772bf935c3

Let's see how it does!
Flags: needinfo?(nalexander)
Blocks: 1255078
Depends on: 1255119
I would argue that the default for MOZ_ANDROID_PACKAGE_INSTALL_BOUNCER (in mobile/android/confvars.sh) should probably be unset.  Most people who are going to be wanting to use the bouncer (i.e. distributors) can set that value when they are packaging things up...and they are going to be the ones who will want to enable it.
(In reply to Nathan Toone [:toonetown] from comment #70)
> I would argue that the default for MOZ_ANDROID_PACKAGE_INSTALL_BOUNCER (in
> mobile/android/confvars.sh) should probably be unset.  Most people who are
> going to be wanting to use the bouncer (i.e. distributors) can set that
> value when they are packaging things up...and they are going to be the ones
> who will want to enable it.

Yeah, I'm going to disable it (for other reasons).  It was default on to get testing in Nightly (which didn't even help, since Beta is burning).
It appears to be default on for beta as well - because I just checked out beta and it bit me. :)
Blocks: 1258452
Blocks: 1360587
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: