Move MOZ_NATIVES_DEVICES and MOZ_INSTALL_TRACKING to moz.configure

RESOLVED FIXED in Firefox 55
(NeedInfo from)

Status

()

enhancement
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: nalexander, Assigned: nalexander, NeedInfo)

Tracking

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

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [LP_M1])

Attachments

(1 attachment)

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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1365089

Comment 4

2 years ago
mozreview-review
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!

Comment 6

2 years ago
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!
(Assignee)

Updated

2 years ago
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.

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23c76f63807b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 years ago
Whiteboard: [LP_M2]

Updated

2 years ago
Whiteboard: [LP_M2] → [LP_M1]
Assignee: nobody → nalexander
You need to log in before you can comment on or make changes to this bug.