979438 - Unify android target SDK versions

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

We define Android target SDK (like android-16) and Android target SDK version (like android:targetSdkVersion="16") in many places throughout the tree.  We should unify these to make it easier to bump the SDK version.

I think we should probably set ANDROID_TARGET_SDK=$(notdir $(ANDROID_SDK)) and ANDROID_TARGET_SDK_VERSION=$(patsubst android-%,%,$(ANDROID_TARGET_SDK)) in configure, and use them throughout.  It's not clear to me that we'd ever want to set ANDROID_TARGET_SDK to be different than ANDROID_SDK.

This is follow-up from Bug 978587; see https://bugzilla.mozilla.org/show_bug.cgi?id=978587#c4.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: target_android_sdk.patch

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: target_android_sdk.patch

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8396434 [details] [diff] [review]
target_android_sdk.patch

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

This should do the job for the geckoview bits, but there are a few other AndroidManifest's in the tree (not all preprocessed as yet) that we should update.  And since we're auditing them all, why don't we do ANDROID_MIN_SDK as well, for the glorious day when we finally drop Android 2.2?

I have a preference for ANDROID_TARGET_SDK, so that all the Android specific stuff starts ANDROID_, but it's not critical.

Finally, help me understand why you're not determining these in configure.in.  I imagine that it's something with the fact that the geckoview_library package target is called special?  But I see ANDROID_CPU_ARCH there, which is produced by configure.in: http://mxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4#222, so why not do this the same way?

::: embedding/android/geckoview_example/Makefile.in
@@ +49,5 @@
>  assets/libxul.so: $(DIST)/geckoview_library/geckoview_assets.zip FORCE
>  	$(UNZIP) -o $(DIST)/geckoview_library/geckoview_assets.zip
>  
> +build.xml: AndroidManifest.xml
> +	mv AndroidManifest.xml AndroidManifest.xml.save

$(MV)?

::: mobile/android/geckoview_library/Makefile.in
@@ +10,5 @@
>    project.properties \
>    build.xml \
>    $(NULL)
>  
> +TARGET_ANDROID_SDK= $(lastword $(subst -, ,$(notdir $(ANDROID_SDK))))

nit: comment that this is intended to turn android-17 into 17.

@@ +12,5 @@
>    $(NULL)
>  
> +TARGET_ANDROID_SDK= $(lastword $(subst -, ,$(notdir $(ANDROID_SDK))))
> +
> +DEFINES += -DTARGET_ANDROID_SDK=$(TARGET_ANDROID_SDK)

Make this manifest_DEFINES; it's not needed globally.

@@ +40,4 @@
>  
>  _ABS_DIST = $(abspath $(DIST))
>  
> +package: $(properties_deps)  AndroidManifest.xml FORCE

nit: remove extra space.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8396434 [details] [diff] [review]
> target_android_sdk.patch
> 
> Review of attachment 8396434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should do the job for the geckoview bits, but there are a few other
> AndroidManifest's in the tree (not all preprocessed as yet) that we should
> update.  
Which AndroidManifests are you concerned with that this patch doesn't address? Robocop?

> And since we're auditing them all, why don't we do ANDROID_MIN_SDK
> as well, for the glorious day when we finally drop Android 2.2?
sure
> 
> I have a preference for ANDROID_TARGET_SDK, so that all the Android specific
> stuff starts ANDROID_, but it's not critical.
> 
> Finally, help me understand why you're not determining these in
> configure.in.  I imagine that it's something with the fact that the
> geckoview_library package target is called special?  But I see
> ANDROID_CPU_ARCH there, which is produced by configure.in:
> http://mxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4#222,
> so why not do this the same way?
No reason other than I tend to not touch configure.in if a change is only required for a Makfile or two.
> 
> ::: embedding/android/geckoview_example/Makefile.in
> @@ +49,5 @@
> >  assets/libxul.so: $(DIST)/geckoview_library/geckoview_assets.zip FORCE
> >  	$(UNZIP) -o $(DIST)/geckoview_library/geckoview_assets.zip
> >  
> > +build.xml: AndroidManifest.xml
> > +	mv AndroidManifest.xml AndroidManifest.xml.save
> 
> $(MV)?
there is no MV defined (at least on my system). This surprised me as well.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > Comment on attachment 8396434 [details] [diff] [review]
> > target_android_sdk.patch
> > 
> > Review of attachment 8396434 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This should do the job for the geckoview bits, but there are a few other
> > AndroidManifest's in the tree (not all preprocessed as yet) that we should
> > update.  
> Which AndroidManifests are you concerned with that this patch doesn't
> address? Robocop?

Robocop and friends, via mxr:

/build/mobile/robocop/AndroidManifest.xml.in
/build/mobile/sutagent/android/fencp/AndroidManifest.xml
/build/mobile/sutagent/android/ffxcp/AndroidManifest.xml
/build/mobile/sutagent/android/watcher/AndroidManifest.xml
/build/mobile/sutagent/android/AndroidManifest.xml
/embedding/android/geckoview_example/AndroidManifest.xml
/mobile/android/base/AndroidManifest.xml.in
/mobile/android/geckoview_library/AndroidManifest.xml
/mobile/android/tests/background/junit3/AndroidManifest.xml.in
/mobile/android/tests/browser/junit3/AndroidManifest.xml.in

And the following is on me:

/python/mozbuild/mozbuild/backend/templates/android_eclipse/AndroidManifest.xml

> > And since we're auditing them all, why don't we do ANDROID_MIN_SDK
> > as well, for the glorious day when we finally drop Android 2.2?
> sure
> > 
> > I have a preference for ANDROID_TARGET_SDK, so that all the Android specific
> > stuff starts ANDROID_, but it's not critical.
> > 
> > Finally, help me understand why you're not determining these in
> > configure.in.  I imagine that it's something with the fact that the
> > geckoview_library package target is called special?  But I see
> > ANDROID_CPU_ARCH there, which is produced by configure.in:
> > http://mxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4#222,
> > so why not do this the same way?
> No reason other than I tend to not touch configure.in if a change is only
> required for a Makfile or two.

I hear that, but let's do this right.

> > 
> > ::: embedding/android/geckoview_example/Makefile.in
> > @@ +49,5 @@
> > >  assets/libxul.so: $(DIST)/geckoview_library/geckoview_assets.zip FORCE
> > >  	$(UNZIP) -o $(DIST)/geckoview_library/geckoview_assets.zip
> > >  
> > > +build.xml: AndroidManifest.xml
> > > +	mv AndroidManifest.xml AndroidManifest.xml.save
> > 
> > $(MV)?
> there is no MV defined (at least on my system). This surprised me as well.

Wat.  Live and learn.  $(CP)?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> > No reason other than I tend to not touch configure.in if a change is only
> > required for a Makfile or two.
> 
> I hear that, but let's do this right.
Can you provide the equivalent of "ANDROID_TARGET_SDK= $(lastword $(subst -, ,$(notdir $(ANDROID_SDK))))" in configure-ese?

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> (In reply to Nick Alexander :nalexander from comment #5)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> > > No reason other than I tend to not touch configure.in if a change is only
> > > required for a Makfile or two.
> > 
> > I hear that, but let's do this right.
> Can you provide the equivalent of "ANDROID_TARGET_SDK= $(lastword $(subst -,
> ,$(notdir $(ANDROID_SDK))))" in configure-ese?

I know very little about autoconf and m4, but based on build/autoconf/toolchain.m4, namely

GCC_MAJOR_VERSION=`echo ${GCC_VERSION} | $AWK -F\. '{ print <<$>>1 }'`

it looks like the standard is just to run shell and awk.  So perhaps

ANDROID_TARGET_SDK=`echo $(ANDROID_SDK) | $AWK -F\- '{ print $NF }'`

I see also that we might be doing better already:

http://mxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4#273

But I know that glandium is a better guide here :)

Comment 8

[Nick Alexander :nalexander [he/him]]

> http://mxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4#273

So this is extracting the given level from the given platform, which is useful; but this ticket should be tracking setting minSdkVersion and targetSdkVersion in the tree (android.m4), and then substituting them into the relevant files.  So I'm confused as to why you need to extract at all.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

There's already some logic around setting a min SDK level, which looks to be confused and (obviously) not used for the AndroidManifests. Let's tackle that in a seperate bug and focus on the target SDK here.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: target_android_sdk.patch

robocop and (most of) the sutagents don't define target SDKs now. The one that does sets it to 8. I'd rather let a-team know that this exists now, and let them decide whether they want to start using it or not, rather than risk some subtle behavioral change in our unit tests.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8396886 [details] [diff] [review]
target_android_sdk.patch

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

I have some small comments, but I'm going to defer to glandium, 'cuz it looks like this already exists for b2g (ANDROID_VERSION) and we might want to unify.  (See http://mxr.mozilla.org/mozilla-central/source/configure.in#201, which is conditioned on Gonk.)

glandium, what we want to do is set android:targetSdkVersion in AndroidManifest.xml files based on the Android platform version Fennec is being built against.  It looks like this is implemented, more or less, for b2g/gonk.  Is it easy to generalize that little bit of configuration for b2g and Fennec, or is it better to proceed as blassey has written here?  If the latter, could you look at android.m4 in this patch?  It looks reasonable to me, but I don't understand much of the configure system.  Thanks.

::: embedding/android/geckoview_example/Makefile.in
@@ +1,4 @@
> +PP_TARGETS = properties manifest
> +
> +manifest = AndroidManifest.xml.in
> +manifest_PATH = .

This is the default.  From rules.mk:

1500 # - <C>_PATH names the directory in which to place the preprocessed output
1501 #   files. We create this directory if it does not already exist. Setting
1502 #   this variable is optional; if unset, we install the files in $(CURDIR).

@@ +46,5 @@
>  
>  assets/libxul.so: $(DIST)/geckoview_library/geckoview_assets.zip FORCE
>  	$(UNZIP) -o $(DIST)/geckoview_library/geckoview_assets.zip
>  
> +build.xml: AndroidManifest.xml

To be safe, make this $(CURDIR)/AndroidManifest.xml.  I added a note about this issue elsewhere; if I'm wrong due to a VPATH interaction I'm not understanding, apologies.

And use $(manifest_deps).  (Or don't define it.)

::: mobile/android/geckoview_library/Makefile.in
@@ +14,3 @@
>  
>  properties = local.properties.in
>  properties_PATH = .

ditto: re: default, throughout.

@@ +39,4 @@
>  
>  _ABS_DIST = $(abspath $(DIST))
>  
> +package: $(properties_deps)  AndroidManifest.xml project.properties FORCE

nit: remove extra space.

And use $(properties_deps) $(manifest_deps).  Also, I think you may run into a situation that I don't fully understand re: VPATH.  I have found that the output of PP_TARGETS (like AndroidManifest.xml) doesn't get the correct deps unless I depend on $(CURDIR)/AndroidManifest.xml.

So I would make project_deps := $(CURDIR)/project.properties, etc.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8396886 [details] [diff] [review]
target_android_sdk.patch

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

There's too much that /could/ be shared with b2g but isn't to request that this particular bit should be.

::: build/autoconf/android.m4
@@ +276,4 @@
>              AC_MSG_ERROR([Unexpected error: no AndroidVersion.ApiLevel field has been found in source.properties.])
>          fi
>  
> +	AC_DEFINE_UNQUOTED(ANDROID_TARGET_SDK,$ANDROID_TARGET_SDK)

AC_DEFINE(ANDROID_TARGET_SDK) should be enough.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e6cccd1fb990
https://hg.mozilla.org/mozilla-central/rev/07993ac505b3

Comment 14

[Nick Alexander :nalexander [he/him]]

Upstreamed at

https://github.com/mozilla-services/android-sync/commit/6b5e198083a33983e5d0e8cde052020a13b03d06