Closed Bug 1365060 Opened 7 years ago Closed 7 years ago

Move MOZ_NATIVES_DEVICES and MOZ_INSTALL_TRACKING to moz.configure

Categories

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

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: nalexander, Assigned: nalexander, NeedInfo)

References

Details

(Whiteboard: [LP_M1])

Attachments

(1 file)

Bug 1365060 - Move MOZ_{NATIVE_DEVICES, INSTALL_TRACKING} to moz.configure.
59 bytes, text/x-review-board-request
chmanchester
: review+
Details 
Bug 1351585 wants to add a MOZ_ANDROID_MMA flag.  The MMA (Mobile Marketing Automation) frameworks all depend on GPS and GCM (Google Play Services and Google Cloud Messaging), so they should depend on the existing flags that are proxies for the relevant Google library support: Move MOZ_NATIVES_DEVICES and MOZ_INSTALL_TRACKING.  Those flags are currently set in old-configure.in and mobile/android/confvars.sh.  This ticket tracks moving them to mobile/android/moz.configure, so we can add the new MOZ_ANDROID_MMA flag without increasing the amount of future work required to modernize old-configure.in.

It's important that developers be able to _set_ the value of these flags locally during development.  Sadly, `project_flag` doesn't "just work": see https://bugzilla.mozilla.org/show_bug.cgi?id=1363357.

This ticket is the "correct" way to prepare the ground for Bug 1351585, but it seems to be a mountain of work for a molehill of gain, and that ticket is on a very tight timeline, so we may just have to bloat old-configure.in and leave it for another day.
Blocks: 1365089
Comment on attachment 8867900 [details]
Bug 1365060 - Move MOZ_{NATIVE_DEVICES, INSTALL_TRACKING} to moz.configure.

https://reviewboard.mozilla.org/r/139434/#review142800

::: mobile/android/confvars.sh
(Diff revision 3)
> -# Enable second screen using native Android libraries.
> -MOZ_NATIVE_DEVICES=1

This becomes settable as a result of this patch. If that's intentional please mention it in the commit message. If not let's set it directly in `mobile/android/moz.configure`, and maybe file a follow up to simplify some of this on the basis of that.

::: mobile/android/moz.configure:33
(Diff revision 3)
> +
> +# Enable install tracking SDK if we have Google Play support; MOZ_NATIVE_DEVICES
> +# is a proxy flag for that support.
> +@depends(milestone, 'MOZ_NATIVE_DEVICES', '--help')
> +def install_tracking_default(milestone, native_devices, help):
> +    return bool(milestone.is_release_or_beta and native_devices)

Something in `moz.configure` should probably be doing this conversion for us. I'm pretty surprised by the behavior given that `PositiveOptionValue` defines `__nonzero__`.

::: toolkit/content/moz.build:19
(Diff revision 3)
>  if CONFIG['OS_TARGET'] == 'Android':
>      DEFINES['ANDROID_PACKAGE_NAME'] = CONFIG['ANDROID_PACKAGE_NAME']
>  
> +if CONFIG['MOZ_INSTALL_TRACKING']:
> +    DEFINES['MOZ_INSTALL_TRACKING'] = 1
> +

This looks like it might belong in a separate patch.
Attachment #8867900 - Flags: review?(cmanchester) → review+
(In reply to Chris Manchester (:chmanchester) from comment #4)
> Comment on attachment 8867900 [details]
> Bug 1365060 - Move MOZ_{NATIVE_DEVICES, INSTALL_TRACKING} to moz.configure.
> 
> https://reviewboard.mozilla.org/r/139434/#review142800
> 
> ::: mobile/android/confvars.sh
> (Diff revision 3)
> > -# Enable second screen using native Android libraries.
> > -MOZ_NATIVE_DEVICES=1
> 
> This becomes settable as a result of this patch. If that's intentional
> please mention it in the commit message. If not let's set it directly in
> `mobile/android/moz.configure`, and maybe file a follow up to simplify some
> of this on the basis of that.

It's already in the commit message:

This intentionally allows to set MOZ_INSTALL_TRACKING without
reference to the milestone being release or beta.  That is, we
separate the default value (which depends on release or beta) from the
value specified, making life easier for developers.

> ::: mobile/android/moz.configure:33
> (Diff revision 3)
> > +
> > +# Enable install tracking SDK if we have Google Play support; MOZ_NATIVE_DEVICES
> > +# is a proxy flag for that support.
> > +@depends(milestone, 'MOZ_NATIVE_DEVICES', '--help')
> > +def install_tracking_default(milestone, native_devices, help):
> > +    return bool(milestone.is_release_or_beta and native_devices)
> 
> Something in `moz.configure` should probably be doing this conversion for
> us. I'm pretty surprised by the behavior given that `PositiveOptionValue`
> defines `__nonzero__`.

What fails is a type check at http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/python/mozbuild/mozbuild/configure/options.py#154, which only makes sense if things are evaluated.  Perhaps this needs to grow to handle the various *Value types, or needs to try to cast to bool?  I don't really understand the fancy @depends/deferred evaluation model so I can't say if this isn't sensible.

> ::: toolkit/content/moz.build:19
> (Diff revision 3)
> >  if CONFIG['OS_TARGET'] == 'Android':
> >      DEFINES['ANDROID_PACKAGE_NAME'] = CONFIG['ANDROID_PACKAGE_NAME']
> >  
> > +if CONFIG['MOZ_INSTALL_TRACKING']:
> > +    DEFINES['MOZ_INSTALL_TRACKING'] = 1
> > +
> 
> This looks like it might belong in a separate patch.

I don't understand how it worked before -- I can only imagine we were getting a DEFINE out of the old environment handling.  In any case, this is used at https://dxr.mozilla.org/mozilla-central/rev/0f4df67c5f162e00d6f52825badf468aefbfba19/toolkit/content/license.html#70.

Thanks for all your help getting this working, Chris!
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23c76f63807b
Move MOZ_{NATIVE_DEVICES, INSTALL_TRACKING} to moz.configure. r=chmanchester
Aryx: I'm asking you since you're an active sheriff these days.  The code I've modified is only enabled for release and beta, so my try build -- while I think it's correct -- could inadvertently disable a critical piece when it makes it to beta.  Can you help me "try" beta, so that I can download the APK, disassemble it, and ensure I haven't made an error?

Thanks!
Flags: needinfo?(aryx.bugmail)
(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Chris Manchester (:chmanchester) from comment #4)
> > Comment on attachment 8867900 [details]
> > Bug 1365060 - Move MOZ_{NATIVE_DEVICES, INSTALL_TRACKING} to moz.configure.
> > 
> > https://reviewboard.mozilla.org/r/139434/#review142800
> > 
> > ::: mobile/android/confvars.sh
> > (Diff revision 3)
> > > -# Enable second screen using native Android libraries.
> > > -MOZ_NATIVE_DEVICES=1
> > 
> > This becomes settable as a result of this patch. If that's intentional
> > please mention it in the commit message. If not let's set it directly in
> > `mobile/android/moz.configure`, and maybe file a follow up to simplify some
> > of this on the basis of that.
> 
> It's already in the commit message:
> 
> This intentionally allows to set MOZ_INSTALL_TRACKING without
> reference to the milestone being release or beta.  That is, we
> separate the default value (which depends on release or beta) from the
> value specified, making life easier for developers.

The commit message doesn't mention MOZ_NATIVE_DEVICES, which is settable as a result of this patch.
(In reply to Chris Manchester (:chmanchester) from comment #8)
> (In reply to Nick Alexander :nalexander from comment #5)
> > (In reply to Chris Manchester (:chmanchester) from comment #4)
> > > Comment on attachment 8867900 [details]
> > > Bug 1365060 - Move MOZ_{NATIVE_DEVICES, INSTALL_TRACKING} to moz.configure.
> > > 
> > > https://reviewboard.mozilla.org/r/139434/#review142800
> > > 
> > > ::: mobile/android/confvars.sh
> > > (Diff revision 3)
> > > > -# Enable second screen using native Android libraries.
> > > > -MOZ_NATIVE_DEVICES=1
> > > 
> > > This becomes settable as a result of this patch. If that's intentional
> > > please mention it in the commit message. If not let's set it directly in
> > > `mobile/android/moz.configure`, and maybe file a follow up to simplify some
> > > of this on the basis of that.
> > 
> > It's already in the commit message:
> > 
> > This intentionally allows to set MOZ_INSTALL_TRACKING without
> > reference to the milestone being release or beta.  That is, we
> > separate the default value (which depends on release or beta) from the
> > value specified, making life easier for developers.
> 
> The commit message doesn't mention MOZ_NATIVE_DEVICES, which is settable as
> a result of this patch.

You are correct.  I've pulled the trigger and I don't think it's worth backing out and relanding to address it, but feel free to do so yourself if you think it's valuable.  Otherwise, this conversation will have to suffice.  I apologize for skimming over the difference when processing your feedback.
https://hg.mozilla.org/mozilla-central/rev/23c76f63807b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Whiteboard: [LP_M2]
Whiteboard: [LP_M2] → [LP_M1]
Assignee: nobody → nalexander
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: