Closed Bug 1203800 Opened 4 years ago Closed 4 years ago

Allow for specifying the ANDROID_VERSION_CODE to use

Categories

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

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: toonetown, Assigned: toonetown)

References

Details

Attachments

(2 files)

The changes that were made in #1137898 cause a build failure when building for a min-sdk of greater than 11.  The description states:

The bit labelled 'g' is 1 if the build is targetting Android API 11+ and 0 otherwise

However, the check on line 110 of that file is:

elif min_sdk == 11:

Which causes the ValueError to be raised.  This check should probably be ">=" instead of "==".
Simple patch which just changes == to >=.  Flagging nalexander for review, since he's the one who did the initial code.
Attachment #8659655 - Flags: review?(nalexander)
Assignee: nobody → nathan
(In reply to Nathan Toone from comment #0)
> The changes that were made in #1137898 cause a build failure when building
> for a min-sdk of greater than 11.  The description states:

Sorry - I prefixed with a pound sign, so it didn't recognize bug 1137898 in my first comment
(In reply to Nathan Toone from comment #0)
> The changes that were made in #1137898 cause a build failure when building
> for a min-sdk of greater than 11.  The description states:
> 
> The bit labelled 'g' is 1 if the build is targetting Android API 11+ and 0
> otherwise
> 
> However, the check on line 110 of that file is:
> 
> elif min_sdk == 11:
> 
> Which causes the ValueError to be raised.  This check should probably be
> ">=" instead of "==".

Nathan -- help me understand more about what you're doing.  This code is intentionally rejecting min_sdk values it doesn't recognize, since changing the SDK requirements changes the APK splits, and those need to update the android:versionCode computations.  I'm loathe to relax this.

If I understand your situation, you are building a single custom APK with a different min_sdk.  So you have no APK split at all (and no architecture split?), and could keep all the low-order bits free.

This ticket suggests that the versionCode computation somehow needs to be configured further in the mozconfig.  I guess we could add a flag to the Python script saying "do something else with the low order bits" for your builds.

You are not building with MOZILLA_OFFICIAL, right?  I'd be okay relaxing the script outside of MOZILLA_OFFICIAL.
Depends on: 1137898
Flags: needinfo?(nathan)
Yes - I am doing a custom build, but I only want to support (for example) API 15 and higher.  However, I get a failure when trying to build with this current comparison.  I am actually doing an architecture split, though.  But not an API split.

No - I'm not building with MOZILLA_OFFICIAL.  I agree, it would probably be best to just do the relaxation of checking outside of MOZILLA_OFFICIAL.
Flags: needinfo?(nathan)
(In reply to Nathan Toone from comment #5)
> Yes - I am doing a custom build, but I only want to support (for example)
> API 15 and higher.  However, I get a failure when trying to build with this
> current comparison.  I am actually doing an architecture split, though.  But
> not an API split.
> 
> No - I'm not building with MOZILLA_OFFICIAL.  I agree, it would probably be
> best to just do the relaxation of checking outside of MOZILLA_OFFICIAL.

Do you even want rolling build versions based on the build ID?  Mozilla does that for automation reasons, and some history.  Most mainstream Android applications pin the version number and manually bump it as appropriate.  Would it work for you to manually bump?

If you look at https://dxr.mozilla.org/mozilla-central/source/mobile/android/confvars.sh, there's a MOZ_APP_VERSION=43.0a1 or whatever.  We could let you set ANDROID_VERSION_CODE in mozconfig or branding, and then fallback to our Mozilla-specific computation?
Flags: needinfo?(nathan)
That is exactly what I would like to do (set ANDROID_VERSION_CODE myself).  I was actually trying to dig in and figure out how to just use whatever got generated, but if I could specify it myself, that is even better.
Flags: needinfo?(nathan)
(In reply to Nathan Toone from comment #7)
> That is exactly what I would like to do (set ANDROID_VERSION_CODE myself). 
> I was actually trying to dig in and figure out how to just use whatever got
> generated, but if I could specify it myself, that is even better.

I think we add a new MOZ_APP_ANDROID_VERSION_CODE variable around https://dxr.mozilla.org/mozilla-central/source/configure.in#8710, and AC_SUBST it like MOZ_APP_VERSION.  Comment it like, "android:versionCode to pack into any Android App produced.  If not set, falls back to a Mozilla-specific value derived from the build ID."

Then, around https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in?from=mobile%2Fandroid%2Fbase%2FMakefile.in#7, make ANDROID_VERSION_CODE conditional on the new MOZ_APP_ANDROID_VERSION_CODE.

Be aware that you will need to handle the architecture yourself manually, although you must be doing so in your mozconfig.  I realize now that it's awkward to handle architecture in mozconfig and versionCode in branding.  Perhaps you can set that new variable in mozconfig, using `export` or `mk_add_options`?  I don't understand exactly how those work.

glandium: is this approach reasonable?
Flags: needinfo?(nathan)
glandium: is this approach reasonable?
Flags: needinfo?(mh+mozilla)
Sounds good to me - I'll start working up a patch and submit it back here (I changed the title of the bug to better reflect what we will do)
Flags: needinfo?(nathan)
Summary: Cannot build with min-sdk > 11 → Allow for specifying the ANDROID_VERSION_CODE to use
Bug 1203800 - Allow for specifying the ANDROID_VERSION_CODE to use; r?nalexander

This patch allows you to set MOZ_APP_ANDROID_VERSION_CODE in a branding's configure.sh to specify the exact android:versionCode to use in the final (main) APK.  It does *not* modify the android:versionCode used in any other APKs.
Attachment #8660166 - Flags: review?(nalexander)
Attachment #8659655 - Flags: review?(nalexander)
Comment on attachment 8660166 [details]
MozReview Request: Bug 1203800 - Allow for specifying the ANDROID_VERSION_CODE to use; r?nalexander

Bug 1203800 - Allow for specifying the ANDROID_VERSION_CODE to use; r?nalexander

This patch allows you to set MOZ_APP_ANDROID_VERSION_CODE in a branding's configure.sh to specify the exact android:versionCode to use in the final (main) APK.  It does *not* modify the android:versionCode used in any other APKs.
Comment on attachment 8660166 [details]
MozReview Request: Bug 1203800 - Allow for specifying the ANDROID_VERSION_CODE to use; r?nalexander

https://reviewboard.mozilla.org/r/19087/#review17017

If it works for you, it works for me.
Attachment #8660166 - Flags: review?(nalexander) → review+
wfm
Flags: needinfo?(mh+mozilla)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87f8052b2b4a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
https://reviewboard.mozilla.org/r/19087/#review36639

Approved and already release in version 43
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 43 → mozilla43
You need to log in before you can comment on or make changes to this bug.