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|?

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.

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

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.

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

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)

Nick, still relevant?

(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.

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195 Needinfo :susheel if you think this bug should be re-triaged.