Closed Bug 1010587 Opened 7 years ago Closed 6 years ago

set android:debuggable="true" #ifdef NIGHTLY_BUILD and MOZ_DEBUG

Categories

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

32 Branch
ARM
Android
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(2 files, 2 obsolete files)

It'd be very helpful for debugging purposes to be able to install a build of Nightly with android:debuggable="true".

At the same time, nalexander notes that we don't want to distribute such builds to users, as the flag indicates that the app has no expectation of its data being private, and Nightly users would expect their browser's data to be private.

So here's a patch that sets android:debuggable="true" for MOZILLA_OFFICIAL builds only if both NIGHTLY_BUILD and MOZ_DEBUG are defined, which restricts the setting to builds like this one produced by tinderbox:

https://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-central-android-debug/latest/

And hopefully also the debug builds produced by this tryserver run in progress:

https://tbpl.mozilla.org/?tree=Try&rev=3b496d25cdad
Attachment #8422788 - Flags: review?(nalexander)
Comment on attachment 8422788 [details] [diff] [review]
patch v1: set android:debuggable="true" #ifdef NIGHTLY_BUILD and MOZ_DEBUG

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

If it's green on try, and if it does what you want locally, it's good for me.  mfinkle was on board, too.
Attachment #8422788 - Flags: review?(nalexander) → review+
Thanks for the quick review!  I'll confirm try results (including both debug and non-debug builds) before landing and post to m-f-d afterward about how I intend to use it to access the data of an existing Nightly installation on a device that isn't rooted (per <http://blog.shvetsov.com/2013/02/access-android-app-data-without-root.html>).
Comment on attachment 8422788 [details] [diff] [review]
patch v1: set android:debuggable="true" #ifdef NIGHTLY_BUILD and MOZ_DEBUG

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

::: mobile/android/base/AndroidManifest.xml.in
@@ +76,5 @@
>                   android:name="org.mozilla.gecko.GeckoApplication"
>                   android:hardwareAccelerated="true"
>  #ifdef MOZILLA_OFFICIAL
> +#ifdef NIGHTLY_BUILD
> +#ifdef MOZ_DEBUG

You know, while Preprocessor.py is not very advanced, it does support #if defined(FOO) && defined(BAR)
(In reply to Mike Hommey [:glandium] from comment #3)
> You know, while Preprocessor.py is not very advanced, it does support #if
> defined(FOO) && defined(BAR)

Thanks, that's handy!  It makes this conditional more elegantly expressible.

https://tbpl.mozilla.org/?tree=Try&rev=e19197dbea1c
Attachment #8422788 - Attachment is obsolete: true
Attachment #8422813 - Flags: review?(nalexander)
Comment on attachment 8422813 [details] [diff] [review]
patch v2: more elegantly express conditional

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

Based on the #mobile backscroll, this is correct :)
Attachment #8422813 - Flags: review?(nalexander) → review+
For the benefit of future archaeologists: the preprocessor does not yet support arbitrary parentheses, so this cannot be parenthesized for clarity.
The patch didn't work, as android:debuggable="true" for all the builds, including the optimized ones.  Digging into it…
Hmm, ok, this is a bug in Fennec's Makefile (or possibly a change to the preprocessor).

Fennec's Makefile unconditionally adds MOZ_DEBUG to its list of DEFINES:

  DEFINES += \
  …
    -DMOZ_DEBUG=$(MOZ_DEBUG) \
    $(NULL)

  - http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in?rev=a2a09fc0c39d#43

Which gets passed to the preprocessor via a -D flag that defines MOZ_DEBUG without specifying its value:

  python -m mozbuild.action.preprocessor … -DMOZ_DEBUG= …

Which causes the preprocessor's handleD function to set the context property MOZ_DEBUG to an empty string:

  def handleD(option, opt, value, parser):
      vals = value.split('=', 1)
      …
      self.context[vals[0]] = vals[1]

  - http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/preprocessor.py?rev=fe4eba8fc092#488

Which causes the preprocessor's *defined* lambda to evaluate defined(MOZ_DEBUG) to true:

  'defined': lambda tok: tok.value in context,

  - http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/preprocessor.py?rev=fe4eba8fc092#209

So MOZ_DEBUG is defined, as far as the preprocessor is concerned, for both the debug and optimized builds.  And |!defined(MOZILLA_OFFICIAL) || defined(NIGHTLY_BUILD) && defined(MOZ_DEBUG)| evaluates to True for them.

The fix is for Fennec's Makefile to add MOZ_DEBUG to DEFINES only if it's actually defined (like browser/installer/Makefile.in does).

Or possibly to make the preprocessor treat such declarations as undefined, although that seems like a stretch, given that the metavar for the -D flag is "VAR[=VAL]", so I would be more inclined to make the preprocessor raise an exception on a case like "-DVAR=", which doesn't match the declared syntax.

In any case, here's the patch with the fix to the Makefile and another try:

https://tbpl.mozilla.org/?tree=Try&rev=b7de30e9695a
Attachment #8422813 - Attachment is obsolete: true
Attachment #8423332 - Flags: review?(nalexander)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #8)
> In any case, here's the patch with the fix to the Makefile and another try:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b7de30e9695a

It worked!  The debug build has android:debuggable="true", while the three opt builds have android:debuggable="false".
This is the same behaviour as the C preprocessor. If you want -DFOO= to work as if there is no -D, just #if FOO instead of #if defined(FOO)
Comment on attachment 8423332 [details] [diff] [review]
patch v3: add MOZ_DEBUG to DEFINES only if actually defined

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

As the comment says, let's do this in moz.build, like we do for MOZILLA_OFFICIAL.  That will get rid of the -DMOZ_DEBUG= that you were seeing.

This will change a consumer: Assert.java, via DEBUG_BUILD, but in the right way.  Will need a try build, though.  No need for re-review if the try is green.

::: mobile/android/base/Makefile.in
@@ +43,3 @@
>    $(NULL)
>  
> +ifdef MOZ_DEBUG

I think MOZ_DEBUG should be added to the list at

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#516

rather than handled in this Makefile.in.
Attachment #8423332 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #11)
> As the comment says, let's do this in moz.build, like we do for
> MOZILLA_OFFICIAL.  That will get rid of the -DMOZ_DEBUG= that you were
> seeing.

Sounds good, here's the patch.


> This will change a consumer: Assert.java, via DEBUG_BUILD, but in the right
> way.  Will need a try build, though.  No need for re-review if the try is
> green.

Here's the try run:

https://tbpl.mozilla.org/?tree=Try&rev=9558a06768b3
https://hg.mozilla.org/mozilla-central/rev/ecf17dd38106
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Depends on: 1025103
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 32 → mozilla32
You need to log in before you can comment on or make changes to this bug.