Closed Bug 1006158 Opened 5 years ago Closed 5 years ago

Build against Android v7 support library and Google Play Services

Categories

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

All
Android
defect
Not set

Tracking

(firefox33 fixed)

RESOLVED FIXED
mozilla33
Tracking Status
firefox33 --- fixed

People

(Reporter: nalexander, Assigned: wesj)

References

Details

Attachments

(1 file, 2 obsolete files)

This is a ticket to track landing the immediate build dependencies for ChromeCast streaming from Bug 901803.  The code in that ticket is very close, so no reason not to try to get a few odds and ends tied up and land that.
Is there any "political" issue in building with Google Play Services? IIRC, F-Droid marks applications using it as non-free.
Probably.

This will (I assume) mark it as a build time flag, and tbh I don't think we'll need it. So if we can avoid it, we will. That machinery to include it is basically the same as what's needed for v7 support library though.
(In reply to Marco Castelluccio [:marco] from comment #1)
> Is there any "political" issue in building with Google Play Services? IIRC,
> F-Droid marks applications using it as non-free.

Presumably, yes.  We have existing code to support add-ons with Java code, and I have some experimental code that makes this work better; we could, in theory, ship an add-on with the Play Services and build ChromeCast support that way.

For the moment, this will be behind a build flag, preffed off.  We don't have the requisite bits on the build bots yet, anyway.
(In reply to Nick Alexander :nalexander from comment #3)
> (In reply to Marco Castelluccio [:marco] from comment #1)
> > Is there any "political" issue in building with Google Play Services? IIRC,
> > F-Droid marks applications using it as non-free.
> 
> Presumably, yes.  We have existing code to support add-ons with Java code,
> and I have some experimental code that makes this work better; we could, in
> theory, ship an add-on with the Play Services and build ChromeCast support
> that way.

Hmm, I recall there was a small manifest change needed for Play Services; that's never going to work with an add-on.  Perhaps wesj knows if that is required.
Attached patch Path v1 (obsolete) — Splinter Review
Sorry for the delay. I ran down a rabbit hole a bit.

I think addresses most of the comments. It isn't conditional. Some investigation on our end showed the Google Play SDK had the exact same licensing as the rest of the SDK, so I don't have any worries there.
Attachment #8424495 - Flags: review?(nalexander)
Comment on attachment 8424495 [details] [diff] [review]
Path v1

Review of attachment 8424495 [details] [diff] [review]:
-----------------------------------------------------------------

I have one implementation problem that needs to be addressed: the library existence checks need to stay in configure.

But I also think this should be compile-time guarded.  We should follow our recent discussion about building our features so that they can be independent of the trains.  I'm not confident this will even build (do we have the GPS binaries on the buildbots?), and I'm not confident that the code bloat of shipping these libraries will be acceptable until we're really pleased with the resulting feature.  This is open to discussion, of course...

::: build/autoconf/android.m4
@@ +335,5 @@
>      ANDROID_PLATFORM_TOOLS="${android_platform_tools}"
>      ANDROID_BUILD_TOOLS="${android_build_tools}"
>      AC_SUBST(ANDROID_SDK_ROOT)
>      AC_SUBST(ANDROID_SDK)
> +    AC_SUBST(ANDROID_COMPAT_DIR_BASE)

Keep the tests for the sub-libraries here.  It's what configure is for, and we want to fail early and hard, not at the end of a build.

::: build/mobile/robocop/Makefile.in
@@ +7,5 @@
>  dir-tests    := $(DEPTH)/$(mobile-tests)
>  
> +JAVA_CLASSPATH = $(ANDROID_SDK)/android.jar
> +ANDROID_COMPAT_LIB = $(ANDROID_COMPAT_DIR_BASE)/v4/android-support-v4.jar
> +JAVA_BOOTCLASSPATH += $(JAVA_CLASSPATH):$(ANDROID_COMPAT_LIB)

Again, why is the BCP including the CP?  Am I missing something?

::: mobile/android/base/Makefile.in
@@ +68,5 @@
> +
> +ANDROID_COMPAT_LIB = $(ANDROID_COMPAT_DIR_BASE)/v4/android-support-v4.jar
> +ANDROID_APPCOMPAT_LIB = $(ANDROID_COMPAT_DIR_BASE)/v7/appcompat/libs/android-support-v7-appcompat.jar
> +ANDROID_MEDIAROUTER_LIB = $(ANDROID_COMPAT_DIR_BASE)/v7/mediarouter/libs/android-support-v7-mediarouter.jar
> +GOOGLE_PLAY_SERVICES = $(ANDROID_SDK)/../../extras/google/google_play_services/libproject/google-play-services_lib/libs/google-play-services.jar

Gentle preferences for _SERVICES_LIB, like the others.

@@ +70,5 @@
> +ANDROID_APPCOMPAT_LIB = $(ANDROID_COMPAT_DIR_BASE)/v7/appcompat/libs/android-support-v7-appcompat.jar
> +ANDROID_MEDIAROUTER_LIB = $(ANDROID_COMPAT_DIR_BASE)/v7/mediarouter/libs/android-support-v7-mediarouter.jar
> +GOOGLE_PLAY_SERVICES = $(ANDROID_SDK)/../../extras/google/google_play_services/libproject/google-play-services_lib/libs/google-play-services.jar
> +
> +JAVA_BOOTCLASSPATH += $(JAVA_CLASSPATH):$(ANDROID_COMPAT_LIB):$(ANDROID_APPCOMPAT_LIB):$(ANDROID_MEDIAROUTER_LIB):$(GOOGLE_PLAY_SERVICES)

I'm confused: why is the BCP including the CP?  If that's not necessary, then we can use the BCP to reduce duplication below.

@@ +96,5 @@
>  # indices.
>  
>  classes.dex: .proguard.deps
>  	$(REPORT_BUILD)
> +	$(DX) --dex --output=classes.dex jars-proguarded $(ANDROID_COMPAT_LIB) $(ANDROID_APPCOMPAT_LIB) $(ANDROID_MEDIAROUTER_LIB) $(GOOGLE_PLAY_SERVICES)

How about $(filter :,,$(BCP))?

@@ +120,5 @@
>  		@$(topsrcdir)/mobile/android/config/proguard.cfg \
>  		-optimizationpasses $(PROGUARD_PASSES) \
>  		-injars $(subst ::,:,$(subst $(NULL) ,:,$(strip $(ALL_JARS)))) \
>  		-outjars jars-proguarded \
> +		-libraryjars $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB):$(ANDROID_APPCOMPAT_LIB):$(ANDROID_MEDIAROUTER_LIB):$(GOOGLE_PLAY_SERVICES)

This is $(CP):$(BCP), right?

@@ +295,5 @@
>  
>  ANDROID_AAPT_IGNORE := !.svn:!.git:.*:<dir>_*:!CVS:!thumbs.db:!picasa.ini:!*.scc:*~:\#*:*.rej:*.orig
>  
> +define check_for_library
> +	$(if $(wildcard $(1)),,$(error $(2) (looked for $(1))))

This doesn't do what you think it does.  It's actually getting evaluated very early in the Make process.  I think you wanted

$$(if $$(wildcard $(1)),,$$(error $(2) (looked for $(1))))

or similar, so that it only gets evaluated as part of the rule.

@@ +309,5 @@
>  # thinking aapt's outputs are stale.  This is safe because Make
>  # removes the target file if any recipe command fails.
>  define aapt_command
>  $(1): $$(call mkdir_deps,$(filter-out ./,$(dir $(3) $(4) $(5)))) $(2)
> +	$(foreach lib, $(ANDROID_COMPAT_LIB) $(ANDROID_APPCOMPAT_LIB) $(ANDROID_MEDIAROUTER_LIB), $(call check_for_library, $(lib), \

I know it dupes, but this should really remain in configure.

@@ +326,3 @@
>  		--auto-add-overlay \
>  		$$(addprefix -S ,$$(ANDROID_RES_DIRS)) \
> +		-S $(GOOGLE_PLAY_SERVICES_RES) \

This is fine for now, but I'll probably follow-up to extract these extra packages into local Make variables.

::: mobile/android/config/proguard.cfg
@@ +193,5 @@
>  -keepclassmembers @org.mozilla.gecko.mozglue.generatorannotations.WrapEntireClassForJNI class * {
>      *;
>  }
>  
> +-keep class **.R*

From the "Class Specifications" section of http://proguard.sourceforge.net/index.html#manual/usage.html, this keeps any class starting with R, and not just R.java.
Attachment #8424495 - Flags: review?(nalexander) → feedback+
For the record, these JARs are big.  Of course, Proguard will save some (most?) of this space.  This does not include resources, which are always shipped.

  18301723      688 -rw-r--r--    1 nalexander       admin              349134 May  5 14:06 appcompat/libs/android-support-v7-appcompat.jar
  18301468       80 -rw-r--r--    1 nalexander       admin               38916 May  5 14:06 gridlayout/libs/android-support-v7-gridlayout.jar
  18300616      336 -rw-r--r--    1 nalexander       admin              168045 May  5 14:06 mediarouter/libs/android-support-v7-mediarouter.jar
  18302843     3680 -rw-r--r--    1 nalexander       admin             1881748 May  5 14:06 google_play_services/libproject/google-play-services_lib/libs/google-play-services.jar
Attached patch Patch v2 (obsolete) — Splinter Review
This conditionally includes the v7 and play libraries. The v7 conditionals are a bit useless since the sdk won't let you download the v4 compat libraries without the v7 ones.

I think that maybe this should be wrapped in a further define. Maybe the MOZ_DEVICES one? But it also seems a bit painful if we have multiple features (eventually) that all require play services or compat libraries.
Attachment #8424495 - Attachment is obsolete: true
Attachment #8428888 - Flags: review?(nalexander)
I ran some builds with and without this patch to look at sizes:

32148738 with.apk
28645800 without.apk

I wonder if we can do a better job pruning out unused resources (the only ones we actually need are some int attributes I think...).
Comment on attachment 8428888 [details] [diff] [review]
Patch v2

Review of attachment 8428888 [details] [diff] [review]:
-----------------------------------------------------------------

On what is in this patch, I have some nits, some legit concerns (JAVA_CP defines, that GPS resource), and a burning desire for try build.

But I think I wasn't clear about what "guarded" meant.  I really meant a feature flag, something in configure.in that says "We should look for GPS" or "we shouldn't even look for it".  As it stands, this will bloat our APK if it finds the new libraries, and still work if they're missing.  Is that something you see as follow-up?

::: build/autoconf/android.m4
@@ +340,5 @@
>          AC_MSG_ERROR([You must download the Android support library when targeting Android.   Run the Android SDK tool and install Android Support Library under Extras.  See https://developer.android.com/tools/extras/support-library.html for more info. (looked for $ANDROID_COMPAT_LIB)])
>      fi
> +    AC_SUBST(ANDROID_COMPAT_DIR_BASE)
> +
> +    AC_MSG_CHECKING([Wesj for google play services])

I think the Wesj's can go :)

@@ +342,5 @@
> +    AC_SUBST(ANDROID_COMPAT_DIR_BASE)
> +
> +    AC_MSG_CHECKING([Wesj for google play services])
> +    if test -e ${ANDROID_SDK_ROOT}/extras/google/google_play_services/libproject/google-play-services_lib/libs/google-play-services.jar ; then
> +        AC_MSG_CHECKING([Wesj found google play services])

I think you want AC_MSG_RESULT here.

::: config/android-common.mk
@@ -7,5 @@
>  ifndef ANDROID_SDK
>    $(error ANDROID_SDK must be defined before including android-common.mk)
>  endif
>  
> -ifndef JAVA_CLASSPATH

Why are we removing this guard?

@@ +26,5 @@
>  
>  JAVAC_FLAGS = \
>    -target $(JAVA_VERSION) \
>    -source $(JAVA_VERSION) \
> +  $(if JAVA_CLASSPATH,-classpath $(JAVA_CLASSPATH),) \

This should be $(if $(JAVA_CLASSPATH),...).  Otherwise, this will always expand, since $(if ANYSTRING,a,b) == a.

See http://sunsite.ualberta.ca/Documentation/Gnu/make-3.79/html_chapter/make_8.html#SEC80

::: mobile/android/base/AndroidManifest.xml.in
@@ +83,5 @@
>  
>          <meta-data android:name="com.sec.android.support.multiwindow" android:value="true"/>
> +#ifdef GOOGLE_PLAY_SERVICES
> +        <!-- Casting will throw and crash if this is not set. -->
> +        <meta-data android:name="com.google.android.gms.version" android:value="@integer/google_play_services_version" />

This resource comes from the GPS Android project, correct?  If so, say it here; if not, we need that resource :)

::: mobile/android/base/Makefile.in
@@ +64,5 @@
> +JAVA_BOOTCLASSPATH = $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB)
> +
> +ifdef GOOGLE_PLAY_SERVICES_LIB
> +JAVA_CLASSPATH += $(GOOGLE_PLAY_SERVICES_LIB)
> +DEFINES+=-DGOOGLE_PLAY_SERVICES=1

Best to add this define in moz.build.

@@ +99,5 @@
>  # Sync dependencies are provided in a single jar. Sync classes themselves are delivered as source,
>  # because Android resource classes must be compiled together in order to avoid overlapping resource
>  # indices.
>  
> +LIBRARY_JARS = $(JAVA_CLASSPATH):$(JAVA_BOOTCLASSPATH)

nit: for things that are local (not shared between files), prefer lower case, so library_jars.

@@ +312,5 @@
>  
>  ANDROID_AAPT_IGNORE := !.svn:!.git:.*:<dir>_*:!CVS:!thumbs.db:!picasa.ini:!*.scc:*~:\#*:*.rej:*.orig
>  
> +ifdef ANDROID_APPCOMPAT_LIB
> +EXTRA_PACKAGES += android.support.v7.appcompat

ditto for lower case throughout, but it's not a huge deal.

@@ +344,5 @@
>  $(1): $$(call mkdir_deps,$(filter-out ./,$(dir $(3) $(4) $(5)))) $(2)
>  	@$$(TOUCH) $$@
> +	$$(AAPT) package -f -m \
> +		-M AndroidManifest.xml \
> +		-I $(ANDROID_SDK)/android.jar \

Prefer $$.

@@ +349,3 @@
>  		--auto-add-overlay \
>  		$$(addprefix -S ,$$(ANDROID_RES_DIRS)) \
> +		$(if EXTRA_RES_DIRS,$$(addprefix -S ,$$(EXTRA_RES_DIRS)),) \

This isn't quite right.  $(if is evaluated immediately, but EXTRA_RES_DIRS needs to be evaluated.  I think this should be:

$(if $(EXTRA_RES_DIRS),$$(addprefix -S ,$$(EXTRA_RES_DIRS)),) \

And the line below needs the same fix, and putting the final $ as a $$ makes it easier to understand:

$(if $(EXTRA_PACKAGES),--extra-packages $$(EXTRA_PACKAGES),) \

Alternately, you could double *every* $ and keep your life simple; then it's straight text substitution.

@@ +365,5 @@
>  # further no-op build do work.  See also
>  # toolkit/mozapps/installer/packager.mk.
>  
>  # .aapt.deps: $(all_resources)
> +$(eval $(call aapt_command,.aapt.deps,$(all_resources),gecko.ap_,generated//,./))

generated// seems funky.
Attachment #8428888 - Flags: review?(nalexander) → feedback+
ni to me to investigate how to make this work with Eclipse.  Tricksy.
Attached patch Patch v3Splinter Review
OK Take 3. I added a flag for MOZ_NATIVE_DEVICES to determine if we build with these resources. I sorta don't like that because it assumes one use case for them here. I'm happy to take ideas on how you'd rather see that.

> I think the Wesj's can go :)
Heh. I also fixed this up quite a bit, since we can now just refuse to build if the libraries are requested but don't exist.

> > -ifndef JAVA_CLASSPATH
> 
> Why are we removing this guard?

I don't think its really needed. We keep defining android.jar in the classpath just to have something there so that this guard doesn't complain, but the world works fine without CLASSPATH defined at all.
Attachment #8428888 - Attachment is obsolete: true
Attachment #8432786 - Flags: review?(nalexander)
Comment on attachment 8432786 [details] [diff] [review]
Patch v3

Review of attachment 8432786 [details] [diff] [review]:
-----------------------------------------------------------------

This ended up really tight, thanks for going through the review cycle multiple times.

try build needed.

::: build/mobile/robocop/Makefile.in
@@ +94,5 @@
>  # The test APK needs to know the contents of the target APK while not
>  # being linked against them.  This is a best effort to avoid getting
>  # out of sync with base's build config.
>  JARS_DIR := $(DEPTH)/mobile/android/base
> +JAVA_BOOTCLASSPATH := $(JAVA_BOOTCLASSPATH):$(subst $(NULL) ,:,$(wildcard $(JARS_DIR)/*.jar)):$(ANDROID_COMPAT_LIB)

I think this will work, providing we never access a GPS/v7 support library symbol in Robocop.  Good enough for me.

::: config/android-common.mk
@@ +26,5 @@
>  
>  JAVAC_FLAGS = \
>    -target $(JAVA_VERSION) \
>    -source $(JAVA_VERSION) \
> +  $(if $(JAVA_CLASSPATH),-classpath $(JAVA_CLASSPATH),) \

Ah, I like this.

::: mobile/android/base/Makefile.in
@@ +65,5 @@
> +    $(NULL)
> +
> +JAVA_BOOTCLASSPATH := $(subst $(NULL) ,:,$(strip $(JAVA_BOOTCLASSPATH)))
> +
> +# If native devices are enabled, add Google Play Services and some of the v7 compat libraries

nit: punctuation, plz.

@@ +243,5 @@
>  .locales.deps: FORCE
>  	$(TOUCH) $@
>  	$(MAKE) -C locales
>  
> +

nit: remove extraneous ws.
Attachment #8432786 - Flags: review?(nalexander) → review+
That try build failed because settings MOZ_NATIVE_DEVICES=0 makes a bunch of ifdef's pass. I just changed it to MOZ_NATIVE_DEVICES=. Hope thats ok?

https://hg.mozilla.org/integration/fx-team/rev/a2b9ea476e59
https://hg.mozilla.org/mozilla-central/rev/a2b9ea476e59
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment on attachment 8432786 [details] [diff] [review]
Patch v3

Approval Request Comment
[Feature/regressing bug #]: new feature
[User impact if declined]: Can't enable sending video to chromecast
[Describe test coverage new/current, TBPL]: None :(
[Risks and why]: This is pretty low risk. Just enabling a slightly different sdk package for aurora that is the same sdk as before, but now includes google play services as well. That code isn't even bundled with fennec until bug 1042238 (already approved for uplift, but dependent on this).
[String/UUID change made/needed]: none
Attachment #8432786 - Flags: approval-mozilla-aurora?
Attachment #8432786 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This landed on m-c when it was still tracking Gecko 33. What needs uplifting to Aurora here?
Flags: needinfo?(wjohnston)
Attachment #8432786 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Pulling back uplift approval until we clarify with Wesley why it was needed.
Nope. Just confused this with bug 1016529
Flags: needinfo?(wjohnston)
Attachment #8432786 - Flags: approval-mozilla-aurora?
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 33 → mozilla33
You need to log in before you can comment on or make changes to this bug.