Closed
Bug 863445
Opened 11 years ago
Closed 11 years ago
Make build/mobile/robocop/Makefile.in build robocop.apk
Categories
(Testing :: Mochitest, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla24
People
(Reporter: nalexander, Assigned: nalexander)
References
Details
Attachments
(3 files)
11.38 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
9.77 KB,
patch
|
jmaher
:
review-
|
Details | Diff | Splinter Review |
5.85 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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?
Comment 2•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
(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?
Assignee | ||
Comment 4•11 years ago
|
||
> 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.
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #741384 -
Flags: review?(joey)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #741385 -
Flags: review?(joey)
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Attachment #741383 -
Flags: review?(joey) → review?(jmaher)
Assignee | ||
Updated•11 years ago
|
Attachment #741384 -
Flags: review?(joey) → review?(jmaher)
Assignee | ||
Updated•11 years ago
|
Attachment #741385 -
Flags: review?(joey) → review?(jmaher)
Assignee | ||
Comment 10•11 years ago
|
||
joey: I was speaking with jmaher and he offered to review these.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(joey)
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
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 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
(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|.
Assignee | ||
Comment 15•11 years ago
|
||
> > +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!
Comment 16•11 years ago
|
||
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
Comment 17•11 years ago
|
||
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
Comment 18•11 years ago
|
||
Yes, if we keep the robocop cli flag from part 2 I would be fine with an r+. Looking forward to seeing this landed.
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/services/services-central/rev/e7739a472f09 https://hg.mozilla.org/services/services-central/rev/3fd142236a0f https://hg.mozilla.org/services/services-central/rev/382ec69cdfb1
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla23
Assignee | ||
Comment 20•11 years ago
|
||
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.
Assignee | ||
Comment 21•11 years ago
|
||
Backed out for not having fennec_ids.txt in *tests.zip: https://hg.mozilla.org/services/services-central/rev/c26aa2631a50
Assignee | ||
Comment 22•11 years ago
|
||
Limited try run to test restoring fennec_ids.txt: https://tbpl.mozilla.org/?tree=Try&rev=8d0467984e63
Assignee | ||
Comment 23•11 years ago
|
||
Sigh: previous try re-instated erroneously deleted hunk, but didn't update it: https://tbpl.mozilla.org/?tree=Try&rev=aad3900ebdaf
Assignee | ||
Comment 24•11 years ago
|
||
https://hg.mozilla.org/services/services-central/rev/2d5c2f191920 https://hg.mozilla.org/services/services-central/rev/611c81436abe https://hg.mozilla.org/services/services-central/rev/dac51812454a
Comment 25•11 years ago
|
||
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 }
Comment 26•11 years ago
|
||
Also, it looks like the bugs merged by https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7130e5134a6e were never marked using m-cMerge.
Comment 27•11 years ago
|
||
Backed out: remote: https://hg.mozilla.org/mozilla-central/rev/6d8a73f77af3 remote: https://hg.mozilla.org/mozilla-central/rev/8739db2431d0 remote: https://hg.mozilla.org/mozilla-central/rev/2e6cfac714f6
Whiteboard: [fixed in services] → [fixed in services][leave open]
Comment 28•11 years ago
|
||
(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.)
Assignee | ||
Comment 29•11 years ago
|
||
(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.
Assignee | ||
Comment 30•11 years ago
|
||
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)
Comment 31•11 years ago
|
||
(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 :-)
Comment 32•11 years ago
|
||
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)
Assignee | ||
Comment 33•11 years ago
|
||
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.
Assignee | ||
Comment 34•11 years ago
|
||
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.
Assignee | ||
Comment 35•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0013a2c30e78 https://hg.mozilla.org/integration/mozilla-inbound/rev/b7272530bd7c https://hg.mozilla.org/integration/mozilla-inbound/rev/ea47a388215d
Whiteboard: [fixed in services][leave open]
Target Milestone: mozilla23 → mozilla24
Comment 36•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0013a2c30e78 https://hg.mozilla.org/mozilla-central/rev/b7272530bd7c https://hg.mozilla.org/mozilla-central/rev/ea47a388215d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•