1351573 - Replace Leanplum moz.build integration with Gradle sub-module or external dependency

Status: RESOLVED DUPLICATE of bug 1351585

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In order to use the Leanplum SDK we will have to add it to our thirdparty module. 

Leanplum is going to open-source their SDK for us. We can work on some of the related bugs with the closed source SDK but for landing the patches we have to integrate the open-source version.

This should be behind a build flag so that we can still build Fennec with and without the SDK added. If the SDK has transitive dependencies then those should only be included based on the build flag too. See Adjust for an example.

Comment 1

[Nick Alexander :nalexander [he/him]]

I looked at https://github.com/Leanplum/Leanplum-Android-SDK and it looks possible to integrate into our moz.build system, but we will want to make some small changes:

1) we should remove a pointless resource (app_name in res/values/strings.xml) that we can just get rid of, and then we don't have to include com.leanplum.**.R (saving many DEX fields);

2) we will want to turn their manifest into a manifest fragment for including in Fennec's AndroidManifest.xml.in (Gradle would handle this directly);

3) we will want to incorporate their Proguard configuration into Fennec's (Gradle would also handle this);

4) we might want to remove their FCM code if it can't run in our configuration.

After these small tweaks, we need to arrange:

1) a build flag, like MOZ_INSTALL_TRACKING, to turn this feature on and off at build time;

2) a conditionally compiled helper, like we have in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/adjust, to handle the case when Leanplum is _not_ turned on.

This last is required 'cuz Java needs to be able to resolve all symbols at compile time.  We use a different implementation of a simple helper interface, determined at compile time, to enable and disable the feature.  (A better system would use dependency injection (a la Dagger or similar) to do this more dynamically.  We can't do that easily using moz.build.)

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Nick Alexander :nalexander from comment #1)
> I looked at https://github.com/Leanplum/Leanplum-Android-SDK and it looks
I'm putting a needinfo to Daniel for this likely requires security review or at least inputs from the expert.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Has there also been legal review for putting this into Fennec? Does it need some sort of data/privacy review as well?  

Looking at this for the first time today, it seems unrealistic to try to ship this in 54, as we are already halfway through Beta 54.

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1351573 - Add Leanplum SDK as module

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

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1351573 - Update project files

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

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867045 [details]
Bug 1351573 - Add Leanplum SDK as module

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

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8867045 [details]
Bug 1351573 - Add Leanplum SDK as module

https://reviewboard.mozilla.org/r/138624/#review142166

This commit would have been easier to read in two parts: Part 1, import; Part 2, change our existing build system to reference the new stuff.

In addition, this should be rooted in `mobile/android/thirdparty/leanplum`, not in `mobile/android/leanplum/AndroidSDK`.  (External reviewers, specifically Gerv, really want us to divide internal from external code, and `thirdparty/` is already flagged as external.)  That is, move the contents of the `AndroidSDK` directory into `m/a/thirdparty/leanplum`.

Next steps:

1) add a project flag to `mobile/android/moz.configure` for `MOZ_ANDROID_LEANPLUM`, _or_ explain why you're going to use `MOZ_INSTALL_TRACKING` and take the risk of not being able to turn off Leanplum while keeping Adjust.  Speaking of which, are we going to _remove_ Adjust at the same time as we enable Leanplum?  Don't these two libraries do a lot of the same things?

2) start working on a `LeanplumHelper`, so that the code compiles and works when the library is _not_ included.

3) start building a leanplum JAR in the moz.build system, working through the dependencies, etc.  I can help with this eventually (and hopefully next week).

Good start!

::: commit-message-ad45e:1
(Diff revision 2)
> +Bug 1351573 - Add Leanplum SDK as module r?maliu,sebastian,nalexander

Always include the imported version, a link to the imported version (preferably a github permalink embedding the commit hash), and a description of any changes required in the commit message.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:15
(Diff revision 2)
>  import android.content.SharedPreferences;
>  import android.content.res.Configuration;
>  import android.os.SystemClock;
>  import android.util.Log;
>  
> +import com.leanplum.Leanplum;

This is where we'll need to follow the Adjust process and use a layer of indirection for the build flag.  See `AdjustHelper.java` and the dummy helper when the flag isn't set.

::: mobile/android/leanplum/.gitignore:1
(Diff revision 2)
> +## macOS

We should drop this; it doesn't make sense to check it into our repository.

We should also drop all of `.gradle`; those are caches that shouldn't be checked in.

::: mobile/android/leanplum/AndroidSDK/build.gradle:22
(Diff revision 2)
> +
> +android {
> +    compileSdkVersion 23
> +    buildToolsVersion mozconfig.substs.ANDROID_BUILD_TOOLS_VERSION
> +    useLibrary 'org.apache.http.legacy'
> +    publishNonDefault true

I doubt we want this.

::: mobile/android/leanplum/AndroidSDK/build.gradle:35
(Diff revision 2)
> +            minifyEnabled true
> +            proguardFiles 'proguard-rules.pro'
> +
> +        }
> +        buildTypes.each {
> +            def packageIdentifier = '\"' + (System.getenv("LEANPLUM_PACKAGE_IDENTIFIER") ?: "s") +

We probably want to this to be `ANDROID_PACKAGE_NAME`.  This will be much clearer when done as a second patch on top of the import patch.

::: mobile/android/leanplum/AndroidSDK/out.map:1
(Diff revision 2)
> +com.leanplum.AESCrypt -> com.leanplum.a:

This shouldn't be in their repository, let alone in ours.  This is a Proguard artifact that doesn't make sense for folks building from source, like us.

::: mobile/android/leanplum/AndroidSDK/res/values/strings.xml:1
(Diff revision 2)
> +<?xml version="1.0" encoding="utf-8"?>

As I pointed out in a comment on the ticket, we can and should get rid of this.  Do it as a Post: or final part after import, etc.

::: mobile/android/leanplum/local.properties:11
(Diff revision 2)
> +#
> +# Location of the SDK. This is only used by Gradle.
> +# For customization when using a Version Control System, please read the
> +# header note.
> +#Thu May 11 17:01:15 PDT 2017
> +ndk.dir=/Users/nevin/.android-sdk/ndk-bundle

Clearly this doesn't make sense.  All of the files in `leanplum/` itself arent' relevant for us.

::: settings.gradle:31
(Diff revision 2)
>  // local.properties (first 'sdk.dir', then 'android.dir') and then the
>  // environment variable ANDROID_HOME will override this.  That's unfortunate,
>  // but it's hard to automatically arrange better.
>  System.setProperty('android.home', json.substs.ANDROID_SDK_ROOT)
>  
> -include ':app'
> +include ':app', ':leanplum'

Nit: separate include, in alphabetical order (so before `:omnijar`.)

This will need to be conditional on a build flag.

::: settings.gradle:56
(Diff revision 2)
>  // We use this ext property to pass the per-object-directory mozconfig
>  // between scripts.  This lets us execute set-up code before we gradle
>  // tries to configure the project even once, and as a side benefit
>  // saves invoking |mach environment| multiple times.
>  gradle.ext.mozconfig = json
> +project(':leanplum').projectDir = new File('mobile/android/leanplum/AndroidSDK')

nit: this will need to be conditional on a build flag, and should be in the same place as all the others (in alphabetical order).

Comment 8

[Elvin Lee [:ellee] (no longer with Mozilla)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #3)
> Has there also been legal review for putting this into Fennec? Does it need
> some sort of data/privacy review as well?  

Thanks for the NI, Liz. Yes, legal has been looped in.

> 
> Looking at this for the first time today, it seems unrealistic to try to
> ship this in 54, as we are already halfway through Beta 54.

Comment 9

[Max Liu [:maliu]]

Comment on attachment 8867045 [details]
Bug 1351573 - Add Leanplum SDK as module

https://reviewboard.mozilla.org/r/138624/#review142834

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:162
(Diff revision 2)
>          mInBackground = false;
>      }
>  
>      @Override
>      public void onCreate() {
> +        Leanplum.start(this);

If this patch is for adding Leanplum SDK as a module, we should init this only if it is allowed to  in Switchboard.

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #2)
> I'm putting a needinfo to Daniel for this likely requires security review or at least inputs from the expert.

This needs to be a "PI Request", not just a needinfo in a bug. https://mana.mozilla.org/wiki/display/PI/PI+Request

There has already been mail requesting that so that's covered, but curiously Mike Han claimed there had already been a security review from "platform security". Any links to that?

Comment 11

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> (In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #2)
> > I'm putting a needinfo to Daniel for this likely requires security review or at least inputs from the expert.
> 
> This needs to be a "PI Request", not just a needinfo in a bug.
> https://mana.mozilla.org/wiki/display/PI/PI+Request
> 
> There has already been mail requesting that so that's covered, but curiously
> Mike Han claimed there had already been a security review from "platform
> security". Any links to that?
So Mike created a separate bug for that afterwards. --> Bug 1365108

Comment 12

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Looks like the patches were included to bug 1351585 and landed.

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8867045 [details]
Bug 1351573 - Add Leanplum SDK as module

Since this bug is targeting Gradle build, and Bug 1351585 completes the mozbuild part, I temporary remove the review flag due to the short time frame for 55. I'll request a review again later. Sorry for the confusion!

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Nevin Chen [:nechen] from comment #13)
> Comment on attachment 8867045 [details]
> Bug 1351573 - Add Leanplum SDK as module
> 
> Since this bug is targeting Gradle build, and Bug 1351585 completes the
> mozbuild part, I temporary remove the review flag due to the short time
> frame for 55. I'll request a review again later. Sorry for the confusion!

Bug 1351585 handles both moz.build and Gradle; I won't r+ patches that break the Gradle build.  (Knowingly!)

This ticket could be used to track replacing the moz.build thirdparty integration with a Gradle sub-module that is checked into the tree; we might want to follow-up Bug 1355625 (and friends) with these types of tickets, moving from thirdparty into proper sub-modules (or external dependencies).

I'll change the title.

Comment 15

[Nick Alexander :nalexander [he/him]]

maliu: do you really want re-review on that patch?  If so, you should request it from me, since I r-'ed it once.

Comment 16

[Max Liu [:maliu]]

(In reply to Nick Alexander :nalexander from comment #15)
> maliu: do you really want re-review on that patch?  If so, you should
> request it from me, since I r-'ed it once.

Sorry, I was just removing myself from reviewer of this patch on mozreview. It turns out looks like I'm requesting the review from Sebastian without you.
Should I remove my review request from here and leave this to Nevin request for review someday?

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Max Liu [:maliu] from comment #16)
> (In reply to Nick Alexander :nalexander from comment #15)
> > maliu: do you really want re-review on that patch?  If so, you should
> > request it from me, since I r-'ed it once.
> 
> Sorry, I was just removing myself from reviewer of this patch on mozreview.
> It turns out looks like I'm requesting the review from Sebastian without you.
> Should I remove my review request from here and leave this to Nevin request
> for review someday?

Yep, sounds good.