Closed Bug 863445 Opened 11 years ago Closed 11 years ago

Make build/mobile/robocop/Makefile.in build robocop.apk

Categories

(Testing :: Mochitest, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla24

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(3 files)

My goal is to reduce the turn-around time for developers to update and run mochitest-robotium tests.

At the moment, robocop.apk is built by `make package` (toolkit/mozapps/installer/packager.mk) and placed directly into $(DIST).  This means that the main Fennec APK must be re-packaged after changes to Robocop itself or the test files in mobile/android/base/tests.  To make matters worse, `make install` doesn't install robocop.apk, so `make package install` is still not enough.

I propose to move the packaging of robocop.apk into build/mobile/robocop/Makefile.in.  This will let savvy developers build and deploy Robocop and its tests faster, at least when there have been no Fennec changes.

This must be balanced by the cost of having to build Fennec separately when there are Fennec changes, but I believe this already required developer intervention.  In addition, there may be places in the build infrastructure (outside of m-c, that is) that expect robocop.apk to be in $OBJDIR/dist.  Feedback gladly welcomed.
It has been a while since I did the building of robocop.apk and while taking a few minutes to look through the makefiles, they are noticeably different than when I originally did it all.

One thing to consider here is we have to sign the robocop.apk with the same key as fennec.apk.  This uses a developer key locally, but for our official builds (i.e. all builds done for automation and via buildbot), we have a key that is only defined during the 'make package' step.  This is why we create the robocop.apk during 'make package':
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#326

We depend on a few files to be in the dist folder:
* fennec.apk
* robocop.apk
* fennec_ids.txt (generated map of fennec  view ids so robocop can use them)

Because fennec_ids.txt is tied directly to fennec.apk, we need to ensure that whatever fennec.apk is being used also references the exact fennec_ids.txt reference file.  All harnesses (mochitest and talos) which depend on robocop need to know where this fennec_ids.txt file is located at and use it to push to the device.

I am not familiar with 'make install', but that sounds like a great opportunity to make things smoother.  Do all developers want to install robocop at the same time they install fennec?
Bug 855306 is a bit different, but slightly related.

Be aware that 'make mochitest-robotium' installs robocop.apk when using the ADB devicemanager; it does not when using SUT. That has always seemed odd to me, but hasn't caused me any grief.

We should keep in mind that not everyone runs Robocop and some even build with --disable-tests.
See Also: → 855306
(In reply to Joel Maher (:jmaher) from comment #1)
> It has been a while since I did the building of robocop.apk and while taking
> a few minutes to look through the makefiles, they are noticeably different
> than when I originally did it all.

Thanks for the great info.  Comments and further questions inline.
 
> One thing to consider here is we have to sign the robocop.apk with the same
> key as fennec.apk.  This uses a developer key locally, but for our official
> builds (i.e. all builds done for automation and via buildbot), we have a key
> that is only defined during the 'make package' step.  This is why we create
> the robocop.apk during 'make package':
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/
> packager.mk#326

I knew there was a reason, I just couldn't figure out what it was.  Thanks for this valuable context.

> We depend on a few files to be in the dist folder:
> * fennec.apk
> * robocop.apk
> * fennec_ids.txt (generated map of fennec  view ids so robocop can use them)
> 
> Because fennec_ids.txt is tied directly to fennec.apk, we need to ensure
> that whatever fennec.apk is being used also references the exact
> fennec_ids.txt reference file.  All harnesses (mochitest and talos) which
> depend on robocop need to know where this fennec_ids.txt file is located at
> and use it to push to the device.

I think it makes sense to move the logic creating fennec_ids.txt into mobile/android/base/Makefile.in so that it happens at the same time as building fennec.ap_.  These two targets should always agree.

> I am not familiar with 'make install', but that sounds like a great
> opportunity to make things smoother.  Do all developers want to install
> robocop at the same time they install fennec?

No, I don't think so: I personally don't want `make install` to install robocop.apk on Android because most of the time I don't want to wait for the push.

Back to improving this flow for local developers.  Some of the things I see that make sense for buildbot but not for local devs:

* `make mochitest-robotium` always pushes robocop.apk to device but doesn't build it
* `make mochitest-robotium` always rebuilds fennec_ids.txt, but doesn't make sure that it agrees with fennec.apk
* `make -C build/mobile/robocop` builds robocop.ap_ but doesn't package robocop.apk

I propose:

1. generate fennec_ids.txt in mobile/android/base/Makefile.in, so it stays in sync with fennec.ap_ (and hopefully fennec.apk).  It seems reasonable to move parse_ids.py into mobile/android/base to support this, and perhaps call it `make_fennec_ids.py` or similar.
2. continue installing fennec_ids.txt into $(DIST), but use INSTALL_TARGETS so that it is symlinked if possible
3. remove fennec_ids.txt generation from `make mochitest-robotium`

I don't think that's very controversial.  The following is a little more controversial:

4. make build/mobile/robocop/Makefile.in generate both robocop.apk and robocop-unsigned-unaligned.apk (in $OBJDIR/build/mobile/robocop)
5. make packager.mk sign and align $OBJDIR/build/mobile/robocop/robocop-unsigned-unaligned.apk, writing to $(DIST)/robocop.apk as usual

Devs can use `make -C $OBJDIR/build/mobile/robocop` and actually get robocop.apk out of it; buildbot still signs $(DIST)/robocop.apk with the correct keys.  We pay in two ways: there are now two robocop.apk's in $OBJDIR, taking up space; and the two robocop.apk's are subtly different on buildbot machines.  I think there is some precedent here: Mac OS X builds dump outputs into $(DIST)/bin which are then (partially) duplicated into $(DIST)/XXX.App.
 
Thoughts?
> 4. make build/mobile/robocop/Makefile.in generate both robocop.apk and
> robocop-unsigned-unaligned.apk (in $OBJDIR/build/mobile/robocop)
> 5. make packager.mk sign and align
> $OBJDIR/build/mobile/robocop/robocop-unsigned-unaligned.apk, writing to
> $(DIST)/robocop.apk as usual

Urgh, this won't work: `make mochitest-robotium` needs to install a robocop.apk that has been signed with the same key as the on-device Fennec, so on buildbot builders it has to install a robocop.apk produced by `make package`.

The only way forward I can see is to teach `make package` to be smart enough to not re-package fennec-*.apk unless it needs to, and this seems like a fools errand given how packager.mk is written.
joey: what do you think about these changes?  They certainly make it more pleasant to develop Robocop mochitests locally, and I think they are in keeping with our goal to improve dependency tracking.
Attachment #741383 - Flags: review?(joey)
Attachment #741384 - Flags: review?(joey)
Attachment #741385 - Flags: review?(joey)
(In reply to Nick Alexander :nalexander from comment #7)
> Created attachment 741385 [details] [diff] [review]
> Patch against s-c, part 3

I replaced |tools:: robocop.apk| with |libs:: robocop.apk| because it seemed unnecessary to complicate things by using multiple stages.  Let me know if there's more to this.
joey: review ping?
Flags: needinfo?(joey)
Attachment #741383 - Flags: review?(joey) → review?(jmaher)
Attachment #741384 - Flags: review?(joey) → review?(jmaher)
Attachment #741385 - Flags: review?(joey) → review?(jmaher)
joey: I was speaking with jmaher and he offered to review these.
Flags: needinfo?(joey)
Comment on attachment 741383 [details] [diff] [review]
Patch against s-c, part 1

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

this patch is good.  we can safely move the creation of fennec_ids.txt.
Attachment #741383 - Flags: review?(jmaher) → review+
Comment on attachment 741384 [details] [diff] [review]
Patch against s-c, part 2

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

r- only because we need to retain --robocop.

::: testing/mochitest/runtestsremote.py
@@ +84,2 @@
>                      help = "name of the .ini file containing the list of tests to run")
> +        defaults["robocopIni"] = ""

We need to retain the --robocop argument and deprecate it.  Once the buildbot scripts are updated (would require this to ride the trains), we can remove --robocop and just --robocop-ini.
Attachment #741384 - Flags: review?(jmaher) → review-
Comment on attachment 741385 [details] [diff] [review]
Patch against s-c, part 3

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

::: testing/testsuite-targets.mk
@@ +116,5 @@
>      else \
>          $(RUN_MOCHITEST_ROBOTIUM); \
>      fi
>  
> +mochitest-robotium-debug-signed: DM_TRANS?=adb

I really don't like this long target name.  I would rather see something easier for developers to remember and work with.  Maybe 'robotium', 'robocop', 'fennec-robocop' ?
Attachment #741385 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 741384 [details] [diff] [review]
> Patch against s-c, part 2
> 
> Review of attachment 741384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- only because we need to retain --robocop.
> 
> ::: testing/mochitest/runtestsremote.py
> @@ +84,2 @@
> >                      help = "name of the .ini file containing the list of tests to run")
> > +        defaults["robocopIni"] = ""
> 
> We need to retain the --robocop argument and deprecate it.  Once the
> buildbot scripts are updated (would require this to ride the trains), we can
> remove --robocop and just --robocop-ini.

Good catch.  Is there an official deprecation strategy?  I see "[DEPRECATED- please use --XX]" in |runtests.py|.
> > +mochitest-robotium-debug-signed: DM_TRANS?=adb
> 
> I really don't like this long target name.  I would rather see something
> easier for developers to remember and work with.  Maybe 'robotium',
> 'robocop', 'fennec-robocop' ?

If I recall our conversation Friday correctly, these make targets are not used by the buildbots (they use explicit python commands).  If that is correct, I suggest that we make mochitest-robotium do the right thing for developers and use the APK in $OBJDIR/build/mobile/robocop, and we remove the less useful for developers option altogether.

This makes a good deal more sense for when I get around to implementing |mach mochitest-robitium|.

If we feel both signed and unsigned targets are useful, we could also try mochitest-robotium-signed, or signed-mochitest-robotium.

I think this ticket also could use a README entry in build/mobile/robocop and mobile/android/base/tests/; I'll work on that when I address your feedback.

Thanks for review, Joel!
correct, the buildsystem doesn't use make targets, just a raw python commandline.  We should make the developer experience as smooth as possible and I can help ensure that the automation continues to run as it is.  a README is a great idea!

please add a DEPRECATED message to the cli flags, we have some examples in mochitest/runtests.py
Nick: let's get these tiny review comments fixed and get this landed!

Joel, can we assume r+ if Nick keeps --robocop in part 2?
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Yes, if we keep the robocop cli flag from part 2 I would be fine with an r+.  Looking forward to seeing this landed.
jmaher: I added mobile/android/base/tests/README.rst, made `make mochitest-robocop` build the debug signed APK, and deprecated (with a message) `make mochitest-robotium`.  Let me know if any follow-up is needed.  I'll post to mobile-firefox-dev sometime soon about the improved flow.
Backed out for not having fennec_ids.txt in *tests.zip:

https://hg.mozilla.org/services/services-central/rev/c26aa2631a50
Limited try run to test restoring fennec_ids.txt:

https://tbpl.mozilla.org/?tree=Try&rev=8d0467984e63
Sigh: previous try re-instated erroneously deleted hunk, but didn't update it:

https://tbpl.mozilla.org/?tree=Try&rev=aad3900ebdaf
This is causing failures on m-c Android Nightlies, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=22896775&tree=Mozilla-Central#error1

{
07:12:30     INFO - Running command: make package-tests AB_CD=multi in /builds/slave/m-cen-and-a6-ntly-000000000000/./build/obj-firefox
07:12:30     INFO -  rm -rf ./dist/test-package-stage
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ./dist/test-package-stage
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ./dist/test-package-stage/bin
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ./dist/test-package-stage/bin/components
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ./dist/test-package-stage/certs
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ./dist/test-package-stage/jetpack
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ./dist/test-package-stage/peptest
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ./dist/test-package-stage/mozbase
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ./dist/test-package-stage/modules
07:12:31     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall ./build/mobile/sutagent/android/sutAgentAndroid.apk ./dist/test-package-stage/bin
07:12:32     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall ./build/mobile/sutagent/android/watcher/Watcher.apk ./dist/test-package-stage/bin
07:12:32     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall ./build/mobile/sutagent/android/fencp/FenCP.apk ./dist/test-package-stage/bin
07:12:32     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall ./build/mobile/sutagent/android/ffxcp/FfxCP.apk ./dist/test-package-stage/bin
07:12:32     INFO -  make -C ./testing/mochitest stage-package
07:12:32     INFO -  make[1]: Entering directory `/builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/testing/mochitest'
07:12:32     INFO -  make[1]: Leaving directory `/builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/testing/mochitest'
07:12:32     INFO -  make[1]: Entering directory `/builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/testing/mochitest'
07:12:32     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ../../dist/test-package-stage/mochitest/content/
07:12:32     INFO -  cp -RL ../../_tests/testing/mochitest/browser ../../dist/test-package-stage/mochitest/content/
07:12:32     INFO -  cp -RL ../../_tests/testing/mochitest/chrome ../../dist/test-package-stage/mochitest/content/
07:12:33     INFO -  cp -RL ../../_tests/testing/mochitest/a11y ../../dist/test-package-stage/mochitest/content/
07:12:33     INFO -  (cd ../../dist/test-package-stage/mochitest && zip -rq tests.jar content/)
07:12:33     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ../../dist/test-package-stage/mochitest && /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ../../dist/test-package-stage/bin/plugins && /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall -D ../../dist/plugins
07:12:33     INFO -  (cd ../../_tests/testing/mochitest/ && tar -chf - *) | (cd ../../dist/test-package-stage/mochitest && tar -xf -)
07:12:33     INFO -  xpcshell
07:12:33     INFO -  ssltunnel
07:12:33     INFO -  certutil
07:12:33     INFO -  pk12util
07:12:33     INFO -  fix_stack_using_bpsyms.py
07:12:33     INFO -  fix-linux-stack.pl
07:12:33     INFO -  test_necko.xpt
07:12:33     INFO -  (cd /builds/slave/m-cen-and-a6-ntly-000000000000/build/build/pgo/certs && tar -chf - *) | (cd ../../dist/test-package-stage/certs && tar -xf -)
07:12:33     INFO -  libnptest.so
07:12:33     INFO -  libnpsecondtest.so
07:12:33     INFO -  make[1]: Leaving directory `/builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/testing/mochitest'
07:12:33     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall ./dist/fennec_ids.txt ./dist/test-package-stage/mochitest
07:12:33     INFO -  /builds/slave/m-cen-and-a6-ntly-000000000000/build/obj-firefox/config/nsinstall: cannot access ./dist/fennec_ids.txt: No such file or directory
07:12:33     INFO -  make: *** [stage-mochitest] Error 1
07:12:33    ERROR - Return code: 2
07:12:33    FATAL - Halting on failure while running make package-tests AB_CD=multi
07:12:33    FATAL - Exiting 2
}
Also, it looks like the bugs merged by https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7130e5134a6e were never marked using m-cMerge.
(In reply to Ed Morley [:edmorley UTC+1] from comment #26)
> Also, it looks like the bugs merged by
> https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7130e5134a6e
> were never marked using m-cMerge.

Done. (This was Nick's first merge.)
(In reply to Ed Morley [:edmorley UTC+1] from comment #26)
> Also, it looks like the bugs merged by
> https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7130e5134a6e
> were never marked using m-cMerge.

Yes, my apologies -- I didn't know what the right automated tool was, so I was planning to do it this morning.  Shan't happen again.
Joel: can I get your input on this?  I saw something similar (but not identical) on s-c, when I didn't update the |stage-mochitest| target correctly in testing/testsuite-targets.mk.  After the patches, it should read:

stage-mochitest: make-stage-dir
	$(MAKE) -C $(DEPTH)/testing/mochitest stage-package
ifeq ($(MOZ_BUILD_APP),mobile/android)
	$(NSINSTALL) $(DIST)/fennec_ids.txt $(PKG_STAGE)/mochitest
endif

Is it possible that $(DIST) is incorrect?  Am I being bitten by a language repack that is not keeping a copy of $(DIST)/fennec_ids.txt across repacks?
Flags: needinfo?(jmaher)
(In reply to Nick Alexander :nalexander from comment #29)
> Yes, my apologies -- I didn't know what the right automated tool was, so I
> was planning to do it this morning.  Shan't happen again.

No problem - I wasn't criticising, more just wanting to make sure it didn't get forgotten :-)
I don't see anything immediately wrong, but I will say when we did language repacks I had trouble with the original robocop packaging.  I recall we don't build with tests, or something specific that we needed for robocop and that ended up causing the robocop.apk creation to fail as it was always called.
Flags: needinfo?(jmaher)
The issue with building Nightly is with language repack not keeping a copy of $(DIST)/fennec_ids.txt: step one is to 

07:08:35     INFO - #####
07:08:35     INFO - ##### Running package-multi step.
07:08:35     INFO - #####
07:08:35     INFO - Running command: rm -rfv dist/fennec* in /builds/slave/m-cen-and-ntly-0000000000000
...
07:08:35     INFO -  removed `dist/fennec_ids.txt'

So poor old fennec_ids.txt just happens to be unfortunately named.

There are a few ways around this: including renaming fennec_ids.txt, making the package-multi not delete fennec_ids.txt, or having the packager copy fennec_ids.txt to $(DIST) each package.  Renaming touches code in several unnecessary places; I'm loathe to touch package-multi for a trivial reason; so I'm going to try always installing fennec_ids.txt.
Releng kindly provided me with a build slave to test this, and the simple workaround for the above is to always install fennec_ids.txt to $(DIST) when we |make package|.

I'm going to reland with this one-line change.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: