Use ANDROID_TARGET_SDK and MOZ_ANDROID_MIN_SDK_VERSION consistently in Gradle and AndroidManifest.xml.in; add MOZ_ANDROID_COMPILE_SDK_VERSION

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

In general, we can use

targetSdkVersion mozconfig.substs.ANDROID_TARGET_SDK
minSdkVersion mozconfig.substs.MOZ_ANDROID_MIN_SDK_VERSION

in build.gradle without issue.  We should add MOZ_ANDROID_COMPILE_SDK_VERSION (for Gradle's compileSdkVersion) and have it be a parameter to MOZ_ANDROID_SDK in old-configure.in (around https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/old-configure.in#2347) to make this easier to migrate forward.

This isn't required for Bug 1352015, but it makes it more pleasant to experiment with.
Comment on attachment 8884030 [details]
Bug 1352602 - Part 2: Use build system SDK versions in Gradle configurations.

https://reviewboard.mozilla.org/r/154986/#review160660

I think this should be followed by "Drop api 15 support" bug.
Attachment #8884030 - Flags: review?(max) → review+
Blocks: 1316462
Comment on attachment 8884029 [details]
Bug 1352602 - Part 1: Don't AC_DEFINE ANDROID_TARGET_SDK, MOZ_ANDROID_{MIN,MAX}_SDK_VERSION.

https://reviewboard.mozilla.org/r/154984/#review160958

::: mobile/android/base/generate_build_config.py:71
(Diff revision 1)
>          if CONFIG[var]:
>              DEFINES[var] = CONFIG[var]
>  
>      for var in ('ANDROID_CPU_ARCH',
>                  'ANDROID_PACKAGE_NAME',
> +                'ANDROID_TARGET_SDK',

Should we also put this variable in the loop above, so if it were ever absent the pre-processor would treat that as a fatal error? It probably doesn't matter, but the distinction isn't really clear to me as is.

::: mobile/android/tests/browser/robocop/AndroidManifest.xml.in:15
(Diff revision 1)
>  
>      <uses-sdk android:minSdkVersion="@MOZ_ANDROID_MIN_SDK_VERSION@"
>  #ifdef MOZ_ANDROID_MAX_SDK_VERSION
>                android:maxSdkVersion="@MOZ_ANDROID_MAX_SDK_VERSION@"
>  #endif
> -              android:targetSdkVersion="23"/>
> +              android:targetSdkVersion="@ANDROID_TARGET_SDK@"/>

Please also mention these various places we're replacing a hard-coded value with a variable in the commit message.
Attachment #8884029 - Flags: review?(cmanchester) → review+
Comment on attachment 8884030 [details]
Bug 1352602 - Part 2: Use build system SDK versions in Gradle configurations.

https://reviewboard.mozilla.org/r/154986/#review160962

::: commit-message-b20e4:1
(Diff revision 1)
> +Bug 1352602 - Part 2: Use moz.configure SDK versions in Gradle configurations. r=chmanchester,maliu

Shouldn't this be "Use mozbuild SDK versions..."? moz.configure isn't really involved right now.

::: build.gradle:20
(Diff revision 1)
> +        compileSdkVersion = tryInt(mozconfig.substs.ANDROID_COMPILE_SDK_VERSION)
> +        buildToolsVersion = tryInt(mozconfig.substs.ANDROID_BUILD_TOOLS_VERSION)
> +        targetSdkVersion = tryInt(mozconfig.substs.ANDROID_TARGET_SDK)
> +        minSdkVersion = tryInt(mozconfig.substs.MOZ_ANDROID_MIN_SDK_VERSION)

Don't we know ahead of time which of these will be integers? Will this really do the right thing if we happen to have strings or integers?

::: build/autoconf/android.m4:236
(Diff revision 1)
>  
>  ])
>  
>  dnl Configure an Android SDK.
> -dnl Arg 1: target SDK version, like 23.
> -dnl Arg 2: list of build-tools versions, like "23.0.3 23.0.1".
> +dnl Arg 1: compile SDK version, like 23.
> +dnl Arg 2: target SDK version, like 23.

Are we ever going to have ANDROID_COMPILE_SDK_VERSION != ANDROID_TARGET_SDK? Can we default ANDROID_COMPILE_SDK_VERSION to ANDROID_TARGET_SDK for our purposes here?

::: old-configure.in:2305
(Diff revision 1)
>      case "$MOZ_BUILD_APP" in
>      mobile/android)
> -        MOZ_ANDROID_SDK(23, 23.0.3 23.0.1, 25.3.2 25.3.1)
> +        dnl `compileSdkVersion ANDROID_COMPILE_SDK_VERSION`
> +        dnl `targetSdkVersion ANDROID_TARGET_SDK`
> +        dnl `buildToolsVersion ANDROID_BUILD_TOOLS_VERSION` (list).
> +        dnl Lint version (list).

This comment isn't very clear. I'm not sure it's necessary.
Attachment #8884030 - Flags: review?(cmanchester)
Comment on attachment 8884030 [details]
Bug 1352602 - Part 2: Use build system SDK versions in Gradle configurations.

https://reviewboard.mozilla.org/r/154986/#review160962

> Don't we know ahead of time which of these will be integers? Will this really do the right thing if we happen to have strings or integers?

There's definitely weirdness around pre-releases: Google will have `targetSdkVersion "O"` and and `compileSdkVersion "android-O"` or similar, and I really didn't care enough to work through the cases and try to get it all correct.  If you feel strongly, I can do so.

> Are we ever going to have ANDROID_COMPILE_SDK_VERSION != ANDROID_TARGET_SDK? Can we default ANDROID_COMPILE_SDK_VERSION to ANDROID_TARGET_SDK for our purposes here?

Yes, we want to move these two independently.  In general, we want both to move to track current as quickly as possible, but there are build system changes that sometimes require holding back the `compileSdkVersion`, and Android handles certain code paths differently depending on `targetSdkVersion` so we can't always bump them together.  Basically, these are different in Gradle for a reason.

> This comment isn't very clear. I'm not sure it's necessary.

I'll drop it.
Comment on attachment 8884030 [details]
Bug 1352602 - Part 2: Use build system SDK versions in Gradle configurations.

https://reviewboard.mozilla.org/r/154986/#review160962

> There's definitely weirdness around pre-releases: Google will have `targetSdkVersion "O"` and and `compileSdkVersion "android-O"` or similar, and I really didn't care enough to work through the cases and try to get it all correct.  If you feel strongly, I can do so.

It's probably not that important, but this does seem bizarre because it's hard to imagine how these values are used. If compileSdkVersion as provided might be a string or an integer, why is the conversion necessary in cases it happens to be an integer? Would passing whatever string we get from configure instead of converting cause a problem?
Comment on attachment 8884030 [details]
Bug 1352602 - Part 2: Use build system SDK versions in Gradle configurations.

https://reviewboard.mozilla.org/r/154986/#review160962

> Shouldn't this be "Use mozbuild SDK versions..."? moz.configure isn't really involved right now.

I'll just make this "build system SDK versions", since I don't care to distinguish moz.configure from old-configure.in, and I think the build system just passes them through.

> It's probably not that important, but this does seem bizarre because it's hard to imagine how these values are used. If compileSdkVersion as provided might be a string or an integer, why is the conversion necessary in cases it happens to be an integer? Would passing whatever string we get from configure instead of converting cause a problem?

Yes, the Android-Gradle build plugin definitely expects ints for some fields -- except when it doesn't, like with pre-releases.  I'm going to keep it permissive; when we get bitten, we'll fix it.
Comment on attachment 8884029 [details]
Bug 1352602 - Part 1: Don't AC_DEFINE ANDROID_TARGET_SDK, MOZ_ANDROID_{MIN,MAX}_SDK_VERSION.

https://reviewboard.mozilla.org/r/154984/#review160958

> Should we also put this variable in the loop above, so if it were ever absent the pre-processor would treat that as a fatal error? It probably doesn't matter, but the distinction isn't really clear to me as is.

That loop is the set of variables that cause errors if they're missing, 'cuz the `CONFIG['ANDROID_TARGET_SDK']` will fail with a `KeyError`.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d14a94ba9fc7
Part 1: Don't AC_DEFINE ANDROID_TARGET_SDK, MOZ_ANDROID_{MIN,MAX}_SDK_VERSION. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/a419686f707e
Part 2: Use build system SDK versions in Gradle configurations. r=maliu
https://hg.mozilla.org/mozilla-central/rev/d14a94ba9fc7
https://hg.mozilla.org/mozilla-central/rev/a419686f707e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.