Open Bug 1093218 Opened 6 years ago Updated 1 year ago

Build, sign, and upload a debug (non-Proguarded) version of Fennec

Categories

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

All
Android
defect

Tracking

(Not tracked)

People

(Reporter: nalexander, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now, we build, sign, and upload exactly one APK, which has had Proguard run on the Java code.  We have @RobocopTarget (and other markers) to prevent Proguard removing things that we reference only in test code.  As we push to test more of Fennec with instrumentation tests that access the Java class hierarchy directly, this gets more onerous.

This ticket tracks building, signing, and uploading a debug (non-Proguarded) version of Fennec for testing only.

I think it will be simplest to keep the package name the same (newer versions of Gradle build debug and release APKs with different package names, IIRC).

I think the simplest thing will be to produce two classes.dex files in mobile/android/base, to extract some of the existing packaging code from packager.mk into either a helper Makefile or a stand-alone Python script, and then to package up the debug version of Fennec as part of |make package-tests|.  We already produce signed instrumentation testing APKs in package-tests; this APK would be for testing only.  This has at least the following advantages:

1) uploading additional APKs using UPLOAD_EXTRA_FILES causes the buildbots problems
2) consumers will not accidentally fetch a debug APK from the build outputs directory
3) the testing scripts (mozharness, test runners) can install the debug APK without needing to bump the buildbots.
glandium: do you have a comment on where things get built (classes.dex) or my analysis of the pros and cons of signing and packaging as part of |make package-tests|?
Flags: needinfo?(mh+mozilla)
Instead of just not running Proguard at all, you might prefer to run Proguard but have it not preserve Robocop annotations (notice I wrote a utility for doing that on the proguard config generation ticket :P )

The advantage of that approach is that it reduces false-positive test results. Not all proguard-induced failures are caused by stuff that was used only for testing being deleted. If you forget to annotate a JNITarget (something which is a core part of the app that Proguard needs to preserve), under your scheme, no tests will fail.
That seems like it could be very bad.

It seems like what we really want is a way to Proguard as much as we can while still preserving the tests in the debug build (so more of the things that might go wrong do go wrong in the test build). It is for this reason that multiple otherwise-identical classes of keep annotation exist (I thought ahead :P)

In either case, this stops Proguard wins from being inversely proportional to test coverage, so I'm a big fan. I just don't think "turn it off" is the best way to do this.
(In reply to Chris Kitching [:ckitching] from comment #2)
> Instead of just not running Proguard at all, you might prefer to run
> Proguard but have it not preserve Robocop annotations (notice I wrote a
> utility for doing that on the proguard config generation ticket :P )

I don't want to annotate stuff for tests at all.  This is useless friction that stops us writing tests!

> The advantage of that approach is that it reduces false-positive test
> results. Not all proguard-induced failures are caused by stuff that was used
> only for testing being deleted. If you forget to annotate a JNITarget
> (something which is a core part of the app that Proguard needs to preserve),
> under your scheme, no tests will fail.
> That seems like it could be very bad.

This is a valid point, but this already applies to third party libraries (and lots of other things) that require us to modify the PG configuration.  There's just no way to statically guarantee the PG config is correct.  I submit that missing JNITargets, etc, will be noticed at the app/integration testing level and not at the instrumentation testing level.  My intention is not to run the debug APK for said integration tests (Mochitests and reftests).

> It seems like what we really want is a way to Proguard as much as we can
> while still preserving the tests in the debug build (so more of the things
> that might go wrong do go wrong in the test build). It is for this reason
> that multiple otherwise-identical classes of keep annotation exist (I
> thought ahead :P)

Again, I don't want to annotate anything just for testing, so I don't think this is the right goal.

> In either case, this stops Proguard wins from being inversely proportional
> to test coverage, so I'm a big fan. I just don't think "turn it off" is the
> best way to do this.

I do, because that's what every other build system does: debug has no PG, release has a PG configuration.  But I'm not wedded to this perspective.
Ah yes, I do apologise, I'm being dense. :P

(In reply to Nick Alexander :nalexander from comment #3)
> (In reply to Chris Kitching [:ckitching] from comment #2)
> > Instead of just not running Proguard at all, you might prefer to run
> > Proguard but have it not preserve Robocop annotations (notice I wrote a
> > utility for doing that on the proguard config generation ticket :P )
> 
> I don't want to annotate stuff for tests at all.  This is useless friction
> that stops us writing tests!

Yes, quite right. *facepalm*

One another alternative you might like to think about: for testing, run Proguard with both the Robocop and the Fennec classes given as program classes. For Fennec, run Proguard just with the Fennec classes.

What will happen then is Proguard will "see" the calls from Robocop into Fennec as they're now part of its input program, and will avoid deleting them. No more annotations needed, and you get some of the benefits I enumerated before.

That might not be worth it. (particularly as, as test coverage approaches 100%, the difference between this proposal and just not running Proguard at all tends towards zero :P )



Anyway, dooo it.
(In reply to Chris Kitching [:ckitching] from comment #4)
> Ah yes, I do apologise, I'm being dense. :P
> 
> (In reply to Nick Alexander :nalexander from comment #3)
> > (In reply to Chris Kitching [:ckitching] from comment #2)
> > > Instead of just not running Proguard at all, you might prefer to run
> > > Proguard but have it not preserve Robocop annotations (notice I wrote a
> > > utility for doing that on the proguard config generation ticket :P )
> > 
> > I don't want to annotate stuff for tests at all.  This is useless friction
> > that stops us writing tests!
> 
> Yes, quite right. *facepalm*
> 
> One another alternative you might like to think about: for testing, run
> Proguard with both the Robocop and the Fennec classes given as program
> classes. For Fennec, run Proguard just with the Fennec classes.
> 
> What will happen then is Proguard will "see" the calls from Robocop into
> Fennec as they're now part of its input program, and will avoid deleting
> them. No more annotations needed, and you get some of the benefits I
> enumerated before.
> 
> That might not be worth it. (particularly as, as test coverage approaches
> 100%, the difference between this proposal and just not running Proguard at
> all tends towards zero :P )

Huh, this is a good idea.  I thought to myself, how can you determine the configuration this produces automatically and didn't think of this.  I still think not PGing is good 'cuz it's slow locally, but it might be worth doing this in automation.
Some considerations:

This is only intended for use during testing, so rather than always
upload at build time I pack the APK into the tests package.  This avoids
Bug 919627 which found that uploading extra APKs only works for certain
names.

Making this test only also skirts needing to have unpackage and package
work for the regular and debug APKs.

It also means that I don't have to install two classes.dex files into
$(DIST), or do some light magic to rename a file as its packed into a
ZIP archive.

This incurs the penalty of DEXing the Proguarded and non-Proguarded JARs
when building mobile/android/base.  In the future, I hope to avoid this.
Attachment #8521981 - Flags: review?(mh+mozilla)
Argh, I forgot that package-tests runs *before* package.  I'll do the pack and sign in package-tests; or perhaps, glandium, you think we should *always* build and upload a debug APK?
Comment on attachment 8521981 [details] [diff] [review]
Build and sign a non-Proguarded version of Fennec.

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

::: mobile/android/base/Makefile.in
@@ +126,5 @@
> +
> +debug/classes.dex: $(ALL_JARS) $(extra_dex_jars)
> +	$(REPORT_BUILD)
> +	$(NSINSTALL) -D ${@D}
> +	$(DX) --dex --output=$@ $(ALL_JARS) $(extra_dex_jars)

You could use $^ instead of $(ALL_JARS) $(extra_dex_jars)

::: toolkit/mozapps/installer/packager.mk
@@ -445,5 @@
>      $(ZIP) -0 $(_ABS_DIST)/gecko.ap_ $(OMNIJAR_DIR)$(OMNIJAR_NAME)) && \
> -  rm -f $(_ABS_DIST)/gecko.apk && \
> -  cp $(_ABS_DIST)/gecko.ap_ $(_ABS_DIST)/gecko.apk && \
> -  $(ZIP) -j0 $(_ABS_DIST)/gecko.apk $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/classes.dex && \
> -  cp $(_ABS_DIST)/gecko.apk $(_ABS_DIST)/gecko-unsigned-unaligned.apk && \

You're effectively removing gecko-unsigned-unaligned.apk, which has a side effect on http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#331
Attachment #8521981 - Flags: review?(mh+mozilla) → feedback-
Some considerations:

This is only intended for use during testing, so rather than always
upload at build time I pack the APK into the tests package.  This avoids
Bug 919627 which found that uploading extra APKs only works for certain
names.

Making this test only also skirts needing to have unpackage and package
work for the regular and debug APKs.

It also means that I don't have to install two classes.dex files into
$(DIST), or do some light magic to rename a file as its packed into a
ZIP archive.

This incurs the penalty of DEXing the Proguarded and non-Proguarded JARs
when building mobile/android/base.  In the future, I hope to avoid this.

glandium: this one always uploads rather than trying to pack the debug
APK into the tests package.  I added some commenting around
gecko-unsigned-unaligned.apk.  I'm not a big fan of $^ in Make but if
you feel strongly I'll do it.

Try is green:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fc37a09efee8

And the APK is used because automation picks up the wrong APK.  FFS:

12:38:18     INFO -                 "category": null, 
12:38:18     INFO -                 "files": [
12:38:18     INFO -                     {
12:38:18     INFO -                         "url": null, 
12:38:18     INFO -                         "name": "http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nalexander@mozilla.com-fc37a09efee8/try-android/fennec-36.0a1.en-US.android-arm-debug.apk"
12:38:18     INFO -                     }, 
12:38:18     INFO -                     {
12:38:18     INFO -                         "url": null, 
12:38:18     INFO -                         "name": "http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nalexander@mozilla.com-fc37a09efee8/try-android/fennec-36.0a1.en-US.android-arm.tests.zip"
12:38:18     INFO -                     }

I'll find a way to name the debug APK so that it's just uploaded and
not used.
Attachment #8521981 - Attachment is obsolete: true
Attachment #8523166 - Flags: review?(mh+mozilla)
I spoke too soon: on 2.3, we hit the dreaded LinearAlloc bound with the debug APK.  I believe the APK itself is fine, it's just too large without PG stripping things.  I'll have to figure out how to work around this :(

11-14 12:41:44.954 D/installd(   38): DexInv: --- BEGIN '/data/app/org.mozilla.fennec-1.apk' ---
11-14 12:41:45.294 D/dalvikvm(  356): creating instr width table
11-14 12:41:46.674 D/dalvikvm(  356): Note: class Lcom/google/android/gms/games/internal/IGamesService$Stub; has 191 unimplemented (abstract) methods
11-14 12:41:47.334 D/dalvikvm(  356): DexOpt: couldn't find field Landroid/app/Notification;.category
11-14 12:41:47.334 D/dalvikvm(  356): DexOpt: couldn't find field Landroid/app/Notification;.actions
11-14 12:41:47.434 D/dalvikvm(  356): DexOpt: couldn't find static field
11-14 12:41:47.444 D/dalvikvm(  356): DexOpt: couldn't find field Landroid/graphics/BitmapFactory$Options;.inMutable
11-14 12:41:48.454 E/dalvikvm(  356): LinearAlloc exceeded capacity (5242880), last=364
11-14 12:41:48.454 E/dalvikvm(  356): VM aborting
Flags: needinfo?(mh+mozilla)
Comment on attachment 8523166 [details] [diff] [review]
Build and sign a non-Proguarded version of Fennec.

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

::: toolkit/mozapps/installer/packager.mk
@@ +459,5 @@
>      $(ZIP) -0 $(_ABS_DIST)/gecko.ap_ $(OMNIJAR_DIR)$(OMNIJAR_NAME)) && \
> +  $(call PACK_AND_RELEASE_SIGN_APK,$(_ABS_DIST)/gecko.ap_,$(GECKO_CLASSES_DEX_PATH),$(PACKAGE)) && \
> +  $(INNER_GECKO_DEBUG_PACKAGE) && \
> +  cp $(PACKAGE) gecko-unsigned-unaligned.apk && \
> +  rm $(_ABS_DIST)/gecko.ap_ && \

$(RM)
Attachment #8523166 - Flags: review?(mh+mozilla) → review+
See Also: → 1208120
Blocks: 1260241
No longer blocks: 1260241
Depends on: 1260241
Nick, still relevant?
Component: Build & Test → Build Config & IDE Support
Flags: needinfo?(nalexander)
Product: Android Background Services → Firefox for Android
(In reply to Michael Comella (:mcomella) from comment #27)
> Nick, still relevant?

Sadly, yes.  We do pretty unorthodox things here.  I think GeckoView will do orthodox things for the equivalent testing, at which point we might give up on this.  But until then, we might as well keep the history.
Flags: needinfo?(nalexander)
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: -- → P5
Product: Firefox for Android → Firefox Build System
You need to log in before you can comment on or make changes to this bug.