1320310 - Local builds: Home screen icon disappears when switching between gradle/mach build

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Whenever I build with mach/gradle, install the app, add a home screen icon and then switch to the other build mechanism (mach -> gradle, gradle -> mach) then the home screen icon disappears on install.

For local builds this is not really an issue, but it's an indicator that the manifest entries might be slightly different and then this could be an issue if  we switch release builds to gradle - e.g. see bug 1268455, bug 1256615.

Comment 1

[Max Liu [:maliu]]

Attachment: Diff of AndroidManifest.xml between Mach and Gradle build.

Diff of AndroidManifest.xml between mach and Gradle build.

There are two diffs, first is the component name from activity-alias is not full class name but short class name starts with a dot(.App). Gradle build appends the prefix "org.mozilla.gecko" from the package of the manifest which mach build don't. This is the root cause of this bug because the component name are different in Launcher app design.

I think explicit specify the full class name is more consistent and better comparing to current dot prefix behavior. But on the other hand, one-time shortcut missing on home screen is still an impact to current user.

@Sebastian: What do you think?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

> I think explicit specify the full class name is more consistent and better comparing to current
> dot prefix behavior. But on the other hand, one-time shortcut missing on home screen is still an
> impact to current user.

Yeah, it would be nicer to use the full class name. However we can't purge launcher icons from all our users. And we have partnerships where partners add Firefox to the home screen - it would be counter productive to remove them again with an update. :)

I wonder if we can find a workaround so that this doesn't happen when building with gradle?

And we should file another bug for the AppMeasurement things. Even if we can't prevent them from getting into the manifest we at least want to disable the reporting (afaik you can do that with a specific meta-data tag in the manifest).

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> > I think explicit specify the full class name is more consistent and better comparing to current
> > dot prefix behavior. But on the other hand, one-time shortcut missing on home screen is still an
> > impact to current user.
> 
> Yeah, it would be nicer to use the full class name. However we can't purge
> launcher icons from all our users. And we have partnerships where partners
> add Firefox to the home screen - it would be counter productive to remove
> them again with an update. :)

Sadly, this is one of these things that I think the launcher(s) get wrong and we will have to support essentially forever.

> I wonder if we can find a workaround so that this doesn't happen when
> building with gradle?

It's not easy, but we can probably find a way to achieve this in Gradle.  Two thoughts:
- does inserting a dummy empty variable prevent Gradle doing the expansion?  Like "${dummyPlaceholder}.App".
- it is possible, but not easy, to post-process the various .xml files produced.  I've tried and failed to do this for certain things in the past.

> And we should file another bug for the AppMeasurement things. Even if we
> can't prevent them from getting into the manifest we at least want to
> disable the reporting (afaik you can do that with a specific meta-data tag
> in the manifest).

I concur.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1320310 - Post-process Gradle-produced Android manifest.

Layer on the hacks!  This:

1) Reinstates the <activity-alias android:name=".App"> that we have in
   the moz.build produced Android manifest.  I found no way to do this
   using placeholders or the manifest merger.

2) Culls manifest entries provided by
   com.google.android.gms.measurement.  We know they're not necessary,
   since they're not present in the existing Fennec Android manifest.

These are strictly workarounds to avoid doing the real work of fixing
the issues.  To fix 1), we'd need to migrate all existing users with
homescreen shortcuts to .App.  This could be difficult, especially if
partners have deployed packages out of our update control.  To fix 2),
we'll need to upgrade our Google Play Services to at least version
9.0.0 and then use the finer-grained AAR dependencies to not build
with the measurement split at all.

Review commit: https://reviewboard.mozilla.org/r/129278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129278/

Comment 5

[Max Liu [:maliu]]

https://android.googlesource.com/platform/tools/base/+/master/sdk-common/src/main/java/com/android/ide/common/xml/AndroidManifestParser.java#393

I also found there is no way to skip the prepend process. Looks like the post processing is the only workaround.

Nice hack! Thanks Nick.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8857313 [details]
Bug 1320310 - Post-process Gradle-produced Android manifest.

https://reviewboard.mozilla.org/r/129278/#review131902

Nice :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/364265dfdf2c
Post-process Gradle-produced Android manifest. r=sebastian

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for gradle related failures:

https://hg.mozilla.org/integration/autoland/rev/7da164b38b603436a1a13f9d372ffc1beaad1d9f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=364265dfdf2c2fc7ad8805ac88b2705f40681598&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8857313 [details]
Bug 1320310 - Post-process Gradle-produced Android manifest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129278/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c85665cc6260
Post-process Gradle-produced Android manifest. r=sebastian

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c85665cc6260

Comment 12

[Nick Alexander :nalexander [he/him]]

Clearing NI, since I fixed and landed this.

Comment 13

[Nick Alexander :nalexander [he/him]]

maliu: would you mind testing this?

Comment 14

[Max Liu [:maliu]]

(In reply to Nick Alexander :nalexander from comment #13)
> maliu: would you mind testing this?

Verify fixed; both of the AndroidManifest.xml are identical.

However sadly, there is a regression while integrating with android-studio. We can no longer launch activity from android-studio because the component it tried to launch is the one with prefix, not the modified one. Even when I try to specify manually, still fail because android-studio thinks the component does not exist.

```
$ adb shell am start -n "org.mozilla.fennec_maliu/org.mozilla.gecko.App" -a android.intent.action.MAIN -c android.intent.category.LAUNCHER
Error while executing: am start -n "org.mozilla.fennec_maliu/org.mozilla.gecko.App" -a android.intent.action.MAIN -c android.intent.category.LAUNCHER
Starting: Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] cmp=org.mozilla.fennec_maliu/org.mozilla.gecko.App }
Error type 3
Error: Activity class {org.mozilla.fennec_maliu/org.mozilla.gecko.App} does not exist.

Error while Launching activity
```

This regression with android-studio also blocks us using instant-run. T_T

@Nick
Consider sending a patch to android-studio? ;)

Comment 15

[Nick Alexander :nalexander [he/him]]

Huh, that's interesting.  I wonder what would happen if we added a second alias for .App as well?  I'll have to think that through.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Nick, I just remembered the bouncer and was wondering whether we need to apply the same post-processing there?

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Sebastian Kaspari (:sebastian) from comment #16)
> Nick, I just remembered the bouncer and was wondering whether we need to
> apply the same post-processing there?

Yeah, I guess we should.  I kind of want to get rid of the bouncer entirely, but until we do that, you're right -- we should ensure that the Gradle bouncer and app APKs agree.  I'll file.