FNX-14633 ⁃ For #162 - Add OSS build flavor. #13453

gilbsgilbs · 2020-08-10T13:00:53Z

This PR adds an OSS build flavor allowing to build fenix without proprietary components, namely:

play-services-ads-identifier
play-services-oss-licenses (the plugin is FOSS, but not the library, so it was replaced with AboutLibraries)
lib-push-firebase (which isn't closed-source by itself yet bundles proprietary libraries such as firebase-core and play-services)
leanplum (which isn't closed-source by itself but includes dependencies to firebase and gms)

I didn't do the work for other SDKs that are open-source but promote closed-source tracking services (such as Adjust), as it is not strictly necessary to release fenix on F-Droid as far as I know.

Build the oss / non-oss flavor:

./gradlew assembleOssDebug assembleNonOssDebug

Simple check for proprietary dependencies:

$ dexdump app/build/outputs/apk/nonOss/debug/app-nonOss-arm64-v8a-debug.apk  | grep -E '(firebase|gms)' | wc -l
21121
$ dexdump app/build/outputs/apk/oss/debug/app-oss-arm64-v8a-debug.apk  | grep -E '(firebase|gms)' | wc -l      
0

So I guess it works.

I think this PR even closes #162 as the work of making fenix available on F-Droid will mostly be political after that.

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not - mostly consists in stubbing existing implementations
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not - no UI changes
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product. - no UI changes

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

ping @pocmo

firefoxci-taskcluster · 2020-08-10T13:01:11Z

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

…encies.

This is to ensure both free and non-free implementations are updated when an interface is added to the base service, like in 77263aa.

NotWoods · 2020-08-10T21:10:48Z

app/src/free/java/org/mozilla/fenix/components/metrics/LeanplumMetricsServiceImpl.kt

+import android.net.Uri
+import java.util.UUID
+
+class LeanplumMetricsServiceImpl(


LeanplumMetricsService is used for two cases, both of which already have interfaces. Rather than making a dummy metrics service, let's configure those two locations.

First one is easy, the service is used in MetricController.create and discarded if the Telemetry flag is set to false. No change needed.

Second location is for the DeepLinkIntentProcessor. It wants a DeepLinkVerifier instance. You could pass in a dummy version that always returns false based on build flags.

First one is easy, the service is used in MetricController.create and discarded if the Telemetry flag is set to false. No change needed.

Since you can't include LeanplumMetricsService in main sources because of the imports, you still need a dummy implementation somewhere at least to make the compiler happy when building the free variant. Or are you suggesting to defer this at runtime by using reflection? What am I missing?

Sorry, I think I was getting confused with BuildConfig flags and build variants. I thought this could be accomplished with an if statement.

Are we allowed to use any form of telemetry in the F-Droid version? If not, we may want to replace the entire MetricController. We have a dummy DebugMetricController already that could be used.

Are we allowed to use any form of telemetry in the F-Droid version?

I'm pretty sure that yes, telemetry is allowed as long as the frameworks being used are completely FLOSS (so Glean and Adjust should be OK, not firebase because of the dependency to gms). You can even display ads under the same conditions.

However, telemetry, especially when enabled by default like it is in Fenix, is considered an "anti-feature". This means there will be a warning prior downloading the app on the store. Such warning is already displayed for Fennec, so that's not too bad I guess.

NotWoods · 2020-08-10T21:12:16Z

app/src/free/java/org/mozilla/fenix/settings/about/OssLicensesMenuImpl.kt

+
+class OssLicensesMenuImpl : OssLicensesMenu {
+    override fun getIntent(context: Context?, title: String): Intent {
+        // This menu shouldn't displayed on the free variant anyways.


Why not? AFAIK open source libraries should be fine to leave in, and we'll need to credit them to use them in the app.

You're right, I don't know why I thought it wasn't required for OSS software 🤐 . So I replaced Google's library with AboutLibraries which is the most popular FLOSS alternative I could find and it just looks better.

Note that I had to pin android gradle because AboutLibraries has gradle:4.0.0 which isn't compatible with Fenix yet (#13262). For the records, I quickly checked what were the blockers to upgrading the android gradle plugin and I found that the glean-gradle-plugin is incompatible: getBuildConfigPackageName() now returns a Property so you have to call get() on it. Also, I also had to update the RepoMatching like this:

- const val comAndroid = "com\\.android\\..*" + const val comAndroid = "com\\.android(\\..+)?"

I don't think those two changes are sufficient to update because there are more subtle issues with the components, but at least these could easily be addressed in other PRs.

Out of curiosity, I also looked how the licenses menu worked in Fennec, and it turns out Fennec just opens about:license, which doesn't list Android dependencies. We could fallback to this, but I don't think it's a fair solution.

I'm still a bit confused here. Why can't we use the Google library? As far as I can tell its open source and just figures out the play services libraries we use, rather than depending on them.

If we need to change the license implementation I think it would be nice to have it split into a separate PR so its easier to review.

Actually it is confusing. The gradle plugin that lists the dependencies is open-source and under Apache 2 license, but the library that is shipped with the APK is proprietary and, as far as I can tell, closed-source. Morever, the library has dependencies to gms (which you can easily see with dexdump) which is unsuitable for OSS anyways.

Agreed that it would make sense to split this into a separate PR though.

Got it, thanks for breaking it down. Using the most popular FLOSS version instead makes sense to me.

NotWoods · 2020-08-10T21:13:34Z

app/build.gradle

@@ -17,6 +17,8 @@ import org.mozilla.fenix.gradle.tasks.LintUnitTestRunner

 import static org.gradle.api.tasks.testing.TestResult.ResultType

+def useFreeImplementation = project.properties['org.mozilla.fenix.useFreeImplementation'].toBoolean()


This might be better off as another build variant, like debug.

That was my original intention, but wasn't sure how to exclude dependencies for a specific build type. Sure you can add new dependencies like debugImplementation, but how do you "not include" them?

Then I thought the best solution would be to use a specific product flavor dimension, but that changes quite a lot of things in the release process (mainly the output APK path) therefore I wasn't very confident making this change. I ended up going for this procedural solution which isn't very elegant but at least shouldn't break anything.

I'm open to changing the implementation to build types or flavors (and I'd even prefer it as it would make the OSS variant a first-class citizen), but I need help to make sure I don't break the release process.

So this is done. I've used build flavors which definitely makes more sense. Needing help to update taskcluster.

NotWoods · 2020-08-10T21:14:47Z

app/src/free/java/org/mozilla/fenix/components/metrics/AdvertisingIDImpl.kt

+
+import android.content.Context
+
+interface AdvertisingIDImpl : AdvertisingID {


Should this be a class instead of an interface?

Done. It's a bit more verbose but probably better.

Rolledback to an interface because otherwise detekt was complaining of an abstract class containing only abstract method (see UnncessaryAbstractClass). Tell me if you think it would be better off keeping a class and ignoring the warning using an annotation.

app/src/main/java/org/mozilla/fenix/components/Push.kt

gilbsgilbs

Hey, thanks for the quick review 👍 . I made most of the requested changes but some details are still unclear (see my responses).

gilbsgilbs · 2020-08-11T07:38:47Z

app/build.gradle

@@ -17,6 +17,8 @@ import org.mozilla.fenix.gradle.tasks.LintUnitTestRunner

 import static org.gradle.api.tasks.testing.TestResult.ResultType

+def useFreeImplementation = project.properties['org.mozilla.fenix.useFreeImplementation'].toBoolean()


That was my original intention, but wasn't sure how to exclude dependencies for a specific build type. Sure you can add new dependencies like debugImplementation, but how do you "not include" them?

Then I thought the best solution would be to use a specific product flavor dimension, but that changes quite a lot of things in the release process (mainly the output APK path) therefore I wasn't very confident making this change. I ended up going for this procedural solution which isn't very elegant but at least shouldn't break anything.

I'm open to changing the implementation to build types or flavors (and I'd even prefer it as it would make the OSS variant a first-class citizen), but I need help to make sure I don't break the release process.

gilbsgilbs · 2020-08-11T07:50:34Z

app/src/free/java/org/mozilla/fenix/components/metrics/AdvertisingIDImpl.kt

+
+import android.content.Context
+
+interface AdvertisingIDImpl : AdvertisingID {


Done. It's a bit more verbose but probably better.

gilbsgilbs · 2020-08-11T08:32:31Z

app/src/free/java/org/mozilla/fenix/components/metrics/LeanplumMetricsServiceImpl.kt

+import android.net.Uri
+import java.util.UUID
+
+class LeanplumMetricsServiceImpl(


First one is easy, the service is used in MetricController.create and discarded if the Telemetry flag is set to false. No change needed.

Since you can't include LeanplumMetricsService in main sources because of the imports, you still need a dummy implementation somewhere at least to make the compiler happy when building the free variant. Or are you suggesting to defer this at runtime by using reflection? What am I missing?

gilbsgilbs · 2020-08-11T09:03:49Z

app/src/free/java/org/mozilla/fenix/settings/about/OssLicensesMenuImpl.kt

+
+class OssLicensesMenuImpl : OssLicensesMenu {
+    override fun getIntent(context: Context?, title: String): Intent {
+        // This menu shouldn't displayed on the free variant anyways.


You're right, I don't know why I thought it wasn't required for OSS software 🤐 . So I replaced Google's library with AboutLibraries which is the most popular FLOSS alternative I could find and it just looks better.

Note that I had to pin android gradle because AboutLibraries has gradle:4.0.0 which isn't compatible with Fenix yet (#13262). For the records, I quickly checked what were the blockers to upgrading the android gradle plugin and I found that the glean-gradle-plugin is incompatible: getBuildConfigPackageName() now returns a Property so you have to call get() on it. Also, I also had to update the RepoMatching like this:

- const val comAndroid = "com\\.android\\..*" + const val comAndroid = "com\\.android(\\..+)?"

I don't think those two changes are sufficient to update because there are more subtle issues with the components, but at least these could easily be addressed in other PRs.

Out of curiosity, I also looked how the licenses menu worked in Fennec, and it turns out Fennec just opens about:license, which doesn't list Android dependencies. We could fallback to this, but I don't think it's a fair solution.

app/src/main/java/org/mozilla/fenix/components/Push.kt

gilbsgilbs · 2020-08-11T20:06:49Z

I've moved to using build flavors instead of a build flag and fixed tests for both flavors. As expected, taskcluster will have to be updated to run the OSS tests suite and produce the OSS apks. I'd appreciate some help as I don't know how to test this locally.

gilbsgilbs · 2020-08-12T15:48:44Z

I've tried to blindly update the taskcluster config, but it'll be hard to go further without being able to test. Please tell me there's anything I can do to get this PR to the next step. Thanks!

(and thoughts for all the Mozilla employees who were laid off 😢)

pocmo · 2020-08-12T16:59:04Z

Yeah, challenging times. I'll get back to that PR. Thank you so far for all that work! 👍

This will be done in a separate PR: mozilla-mobile#13767

pocmo

Sorry for the confusion, I do think we want build flags for those (like in your initial PR) and not a separate flavor dimension. We just worked hard reducing all our dimensions to avoid an explosion of variants. :)

Overall I think the reason is that we want to be more fine-grained: "OSS" doesn't really capture it. For example Leanplum is OSS (code). But I understand you still want to exclude it for a fdroid build. Also lib-push itself is OSS (but depends on a closed transitive dependency).

I think we want different build flags here for (disabling push, disabling leanplum (or marketing SDKs in general, disabling telemetry?). In some combination we may end up using those too in some of our builds (e.g. not all devices come with firebase push support).

Changing the build variants as a really loooong ripple effect on automation and other systems and I would like us to not get into this.. :)

gilbsgilbs · 2020-08-14T16:49:06Z

@pocmo thanks for your response. Although build flavors are generally the intended way to do this, I can totally understand your reasoning. I'll bring back build flags then.

pocmo · 2020-08-14T17:22:12Z

Thank you! I am on PTO next week, so it may take a bit until can take another look.

gilbsgilbs · 2020-08-14T17:23:30Z

Enjoy yourself 😄 .

gilbsgilbs · 2020-08-15T09:41:41Z

I'll close this PR and make tinier, per-build flag PRs to make reviews easier.

gilbsgilbs force-pushed the oss-variant branch from 8df86dd to a893b22 Compare August 10, 2020 13:04

gilbsgilbs mentioned this pull request Aug 10, 2020

F-Droid #162

Closed

For mozilla-mobile#162 - Add build flag to exclude proprietary depend…

19a6825

…encies.

gilbsgilbs force-pushed the oss-variant branch from a893b22 to 19a6825 Compare August 10, 2020 13:12

pocmo self-requested a review August 10, 2020 13:54

Merge branch 'master' into oss-variant

a0aab2b

gilbsgilbs force-pushed the oss-variant branch from fa26bd8 to 6e1a949 Compare August 10, 2020 16:30

Make LeanplumService implementations derive from the same class.

3e021e4

This is to ensure both free and non-free implementations are updated when an interface is added to the base service, like in 77263aa.

gilbsgilbs force-pushed the oss-variant branch from 6e1a949 to 3e021e4 Compare August 10, 2020 21:03

NotWoods suggested changes Aug 10, 2020

View reviewed changes

data-sync-user changed the title ~~For #162 - Add build flag to exclude proprietary dependencies.~~ FNX-14633 ⁃ For #162 - Add build flag to exclude proprietary dependencies. Aug 11, 2020

gilbsgilbs commented Aug 11, 2020

View reviewed changes

gilbsgilbs added 4 commits August 11, 2020 17:19

Replace play-services-oss-licenses library with AboutLibraries.

1113120

Use a class instead of an interface for AdvertisingID.

3dffa78

Rename PushService to FirebasePushService

6b2434f

Add missing license header.

5f3de6e

gilbsgilbs force-pushed the oss-variant branch from 719f2e7 to 5f3de6e Compare August 11, 2020 15:20

pocmo self-assigned this Aug 11, 2020

Declare OSS variant as a build flavor and fix tests.

f0dfa28

Remove dropped gradle property.

fa93528

gilbsgilbs changed the title ~~FNX-14633 ⁃ For #162 - Add build flag to exclude proprietary dependencies.~~ FNX-14633 ⁃ For #162 - Add OSS build flavor. Aug 11, 2020

gilbsgilbs added 3 commits August 11, 2020 22:58

Remove redundant srcDirs declaration.

27e6bac

Rename flavors and setup scripts to use them.

e78c98d

Merge branch 'master' into oss-variant

08daf5a

gilbsgilbs mentioned this pull request Aug 12, 2020

For #162 - remove dependency to play-services-oss-licenses library #13767

Merged

4 tasks

Merge branch 'master' into oss-variant

5e1ebaf

Revert implementation of AboutLibraries.

8d948bf

This will be done in a separate PR: mozilla-mobile#13767

gilbsgilbs force-pushed the oss-variant branch from e76fe58 to 8d948bf Compare August 12, 2020 23:41

pocmo reviewed Aug 14, 2020

View reviewed changes

gilbsgilbs closed this Aug 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FNX-14633 ⁃ For #162 - Add OSS build flavor. #13453

FNX-14633 ⁃ For #162 - Add OSS build flavor. #13453

gilbsgilbs commented Aug 10, 2020 •

edited

Loading

firefoxci-taskcluster bot commented Aug 10, 2020

NotWoods Aug 10, 2020

gilbsgilbs Aug 11, 2020

NotWoods Aug 12, 2020

gilbsgilbs Aug 12, 2020

NotWoods Aug 10, 2020

gilbsgilbs Aug 11, 2020

NotWoods Aug 12, 2020

gilbsgilbs Aug 12, 2020

NotWoods Aug 12, 2020

gilbsgilbs Aug 12, 2020

NotWoods Aug 10, 2020

gilbsgilbs Aug 11, 2020 •

edited

Loading

gilbsgilbs Aug 11, 2020

NotWoods Aug 10, 2020

gilbsgilbs Aug 11, 2020

gilbsgilbs Aug 12, 2020

gilbsgilbs left a comment

gilbsgilbs Aug 11, 2020 •

edited

Loading

gilbsgilbs Aug 11, 2020

gilbsgilbs Aug 11, 2020

gilbsgilbs Aug 11, 2020

gilbsgilbs commented Aug 11, 2020

gilbsgilbs commented Aug 12, 2020

pocmo commented Aug 12, 2020

pocmo left a comment

gilbsgilbs commented Aug 14, 2020

pocmo commented Aug 14, 2020

gilbsgilbs commented Aug 14, 2020

gilbsgilbs commented Aug 15, 2020

		@@ -17,6 +17,8 @@ import org.mozilla.fenix.gradle.tasks.LintUnitTestRunner

		import static org.gradle.api.tasks.testing.TestResult.ResultType

		def useFreeImplementation = project.properties['org.mozilla.fenix.useFreeImplementation'].toBoolean()


		import android.content.Context

		interface AdvertisingIDImpl : AdvertisingID {

FNX-14633 ⁃ For #162 - Add OSS build flavor. #13453

FNX-14633 ⁃ For #162 - Add OSS build flavor. #13453

Conversation

gilbsgilbs commented Aug 10, 2020 • edited Loading

Pull Request checklist

After merge

To download an APK when reviewing a PR:

firefoxci-taskcluster bot commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilbsgilbs Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilbsgilbs left a comment

Choose a reason for hiding this comment

gilbsgilbs Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilbsgilbs commented Aug 11, 2020

gilbsgilbs commented Aug 12, 2020

pocmo commented Aug 12, 2020

pocmo left a comment

Choose a reason for hiding this comment

gilbsgilbs commented Aug 14, 2020

pocmo commented Aug 14, 2020

gilbsgilbs commented Aug 14, 2020

gilbsgilbs commented Aug 15, 2020

gilbsgilbs commented Aug 10, 2020 •

edited

Loading

gilbsgilbs Aug 11, 2020 •

edited

Loading

gilbsgilbs Aug 11, 2020 •

edited

Loading