Closed Bug 1147918 Opened 5 years ago Closed 4 years ago

Run autophone tests locally using mach

Categories

(Testing :: Autophone, defect)

x86
macOS
defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: snorp, Assigned: gbrown)

References

Details

Attachments

(2 files, 13 obsolete files)

7.39 KB, patch
Details | Diff | Splinter Review
29.31 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
It would be very useful if we could run the autophone performance tests via mach.
Blocks: 1036773
adding :gbrown in here as this might be a fun work item for him!  We would need to resolve the issues with device specifics (rooting, permissions, sdcard, etc.).  I assume if there were no device issues, we could automatically clone the autophone repository and then run the tests specified.

I am not sure about how to actually configure autophone- right now there are a lot of specific .ini files that are needed for editing.  We can avoid uploading to phonedash and treeherder.  We would also need to fire up a webserver and point to the data we care about.  Another thing is actually getting the build and test packages downloaded for a specific revision (ideally from the objdir).

It sounds like if we really want this to happen, we should sort out the pre-requisites.
Emulator support will be required. I attempted to get emulator support and ran into a number of issues with starting the emulator, tracking the adb processes etc. That will be the thorniest issue I think.

.ini files aren't *required*, they are just a convenience. Everything should be configurable via command line options. If not, that would be a bug.
Depends on: 1078645
Have we considered moving autophone in-tree? The mach command could git clone autophone into .mozbuild, or something like that, but it would be faster/cleaner/better to just operate on in-tree code, like most mach commands.
I've never considered it but wouldn't be completely opposed. With that said,...

I'm not sure what the approval process would look like to move it into the tree though.

I also wouldn't want to have to pull the full tree on the host server.

Would we still be able to keep the production configurations in tree?

I'm a bit concerned about having to live with the check in rules and restrictions for mozilla-central when making changes especially with regard to production configuration changes.

Finally, I've kept local copies of adb.py and adb_android.py in autophone separate from the versions in mozdevice to prevent outside requirements from causing autophone pain. If we did move it into the tree, we should use the mozdevice versions. In that event, I would be much more stringent in accepting changes to adb*.py.

What is the trade off between the pain you save and the pain you cause with the move?
Assignee: nobody → gbrown
Attached patch wip (obsolete) — Splinter Review
This is still really rough, but it's getting interesting!
Attached patch add 'mach autophone' command (obsolete) — Splinter Review
I'd appreciate some feedback when you have a minute. 

Does the patch work for you? Are the prompts clear? Do we need to configure more (tests, treeherder)?


gbrown@mozpad:~/src$ ./mach autophone
 0:00.09 This mach command is experimental. Report problems in bug 1147918.
 0:00.09 autophone directory not found.
Enter location of new autophone directory: [/home/gbrown/autophone.1]
 0:02.67 Cloning autophone repository to '/home/gbrown/autophone.1'...
 0:11.55 Installing required modules...
 0:32.74 You must configure at least one Android device before running autophone.
Configure devices now? (Y/n) 
Connect your test device(s) and press Enter
 0:40.66 Added 'emulator-5554' to device configuration.
 0:40.66 Launching autophone...
2016-02-19 16:31:37,473|8623|MainThread|console|INFO|Starting server on port 28001.
2016-02-19 16:31:37,473|8623|MainThread|console|INFO|Starting build-cache server on port 28008.
2016-02-19 16:31:37,653|8623|MainThread|console|INFO|Starting autophone.
2016-02-19 16:31:37,656|8623|MainThread|console|INFO|Loading tests.
2016-02-19 16:31:37,661|8623|MainThread|console|INFO|Initializing devices.
2016-02-19 16:31:37,661|8623|MainThread|console|INFO|Initializing device name=device-1, serialno=emulator-5554
2016-02-19 16:31:40,882|8623|MainThread|console|INFO|Autophone started.
 0:45.67 Type 'autophone-help' for help, 'autophone-shutdown' to shutdown and quit.
autophone command? autophone-status
Hello? Yes this is Autophone.
state: RUNNING
phone device-1 (emulator-5554):
  state RUNNING
  debug level 3
  no build loaded
  last update 0:00:09 ago:
    IDLE
  IDLE for 0:00:09
ok
autophone command? autophone-shutdown
Hello? Yes this is Autophone.
ok
2016-02-19 16:31:56,622|8623|Thread-2|console|INFO|Shutting down Autophone...
2016-02-19 16:31:56,626|8623|MainThread|console|INFO|Worker device-1 shutdown
2016-02-19 16:31:57,124|8623|MainThread|console|INFO|AutoPhone terminated.
2016-02-19 16:31:57,124|8623|MainThread|console|INFO|Shutting down build-cache server...
2016-02-19 16:31:57,603|8623|MainThread|console|INFO|Done.
gbrown@mozpad:~/src$
Attachment #8716523 - Attachment is obsolete: true
Attachment #8721536 - Flags: feedback?(jmaher)
Attachment #8721536 - Flags: feedback?(bob)
Comment on attachment 8721536 [details] [diff] [review]
add 'mach autophone' command

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

a lot of nits- lets see how this works out and I assume there will be a lot of papercuts as various developers start using this.

::: mobile/android/mach_commands.py
@@ +203,5 @@
> +    def loadConfig(self):
> +        import json
> +        if os.path.exists(self.CONFIG_FILE):
> +            with open(self.CONFIG_FILE, 'r') as f:
> +                self.config = json.load(f)

should we have a try/catch here?

@@ +247,5 @@
> +        if not self.config['requirements-installed']:
> +            dir = self.config['base-dir']
> +            self.log(logging.INFO, "autophone", {},
> +                "Installing required modules...")
> +            self.runProcess(['virtualenv', 'env'], cwd=dir)

this assumes virtualenv is available, I recall in the past (last 12 months) this hasn't been the case for a handful of developers trying to get talos running.

@@ +248,5 @@
> +            dir = self.config['base-dir']
> +            self.log(logging.INFO, "autophone", {},
> +                "Installing required modules...")
> +            self.runProcess(['virtualenv', 'env'], cwd=dir)
> +            self.runProcess(['./env/bin/pip', 'install', '-r', 'requirements.txt'], cwd=dir)

likewise, we assume pip is installed.  Also are these requirements installed in the virtualenv?  Do we activate the virtualenv anywhere?

@@ +265,5 @@
> +            response = raw_input(
> +               "Configure devices now? (Y/n) ").strip()
> +            if response.lower().startswith('y') or response == '':
> +                response = raw_input(
> +                    "Connect your test device(s) and press Enter")

should we say:
"Connect your test device(s) with usb and press Enter"

@@ +277,5 @@
> +                try:
> +                    with open(os.path.join(self.config['base-dir'], 'devices.ini'), 'w') as f:
> +                        for device in dm.devices():
> +                            serial = device[0]
> +                            f.write("[device-%d]\nserialno=%s\n" % (device_index, serial))

should we ask before adding the device?  They might have a test device and a personal device- we should ensure we at least make it known.

@@ +297,5 @@
> +
> +    def configure(self):
> +        if not self.configureDevices():
> +            return False
> +        # configure treeherder, etc?

do we need to comment out treeherder and pulse credentials/urls, or is this done by default?

@@ +317,5 @@
> +        dir = self.config['base-dir']
> +        time.sleep(5)
> +        if self.thread.isAlive():
> +            self.log(logging.INFO, "autophone", {},
> +                "Type 'autophone-help' for help, 'autophone-shutdown' to shutdown and quit.")

to make this userfriendly, we should fix autophone to not be 'autophone-shutdown', but instead 'shutdown', likewise remove autophone- from all commands.
Attachment #8721536 - Flags: feedback?(jmaher) → feedback+
(In reply to Joel Maher (:jmaher) from comment #8)
> ::: mobile/android/mach_commands.py
> @@ +203,5 @@
> > +    def loadConfig(self):
> > +        import json
> > +        if os.path.exists(self.CONFIG_FILE):
> > +            with open(self.CONFIG_FILE, 'r') as f:
> > +                self.config = json.load(f)
> 
> should we have a try/catch here?

Yes. Now I catch in loadConfig and saveConfig, report the error, but treat it as non-fatal.
 
> @@ +247,5 @@
> > +        if not self.config['requirements-installed']:
> > +            dir = self.config['base-dir']
> > +            self.log(logging.INFO, "autophone", {},
> > +                "Installing required modules...")
> > +            self.runProcess(['virtualenv', 'env'], cwd=dir)
> 
> this assumes virtualenv is available, I recall in the past (last 12 months)
> this hasn't been the case for a handful of developers trying to get talos
> running.

I had a look at what other projects do and discovered that mach has VirtualenvManager -- I'll try to use that.
 
> @@ +265,5 @@
> > +            response = raw_input(
> > +               "Configure devices now? (Y/n) ").strip()
> > +            if response.lower().startswith('y') or response == '':
> > +                response = raw_input(
> > +                    "Connect your test device(s) and press Enter")
> 
> should we say:
> "Connect your test device(s) with usb and press Enter"

Let's make it "Connect your Android test device(s) with usb and press Enter".

> @@ +277,5 @@
> > +                try:
> > +                    with open(os.path.join(self.config['base-dir'], 'devices.ini'), 'w') as f:
> > +                        for device in dm.devices():
> > +                            serial = device[0]
> > +                            f.write("[device-%d]\nserialno=%s\n" % (device_index, serial))
> 
> should we ask before adding the device?  They might have a test device and a
> personal device- we should ensure we at least make it known.

I was trying to avoid the need for per-device prompts with "Connect your Android test device(s) with usb and press Enter" + the status messages "Added '%s' to device configuration." -- not enough?

> @@ +297,5 @@
> > +
> > +    def configure(self):
> > +        if not self.configureDevices():
> > +            return False
> > +        # configure treeherder, etc?
> 
> do we need to comment out treeherder and pulse credentials/urls, or is this
> done by default?

I don't know what to do about treeherder and pulse yet.

> @@ +317,5 @@
> > +        dir = self.config['base-dir']
> > +        time.sleep(5)
> > +        if self.thread.isAlive():
> > +            self.log(logging.INFO, "autophone", {},
> > +                "Type 'autophone-help' for help, 'autophone-shutdown' to shutdown and quit.")
> 
> to make this userfriendly, we should fix autophone to not be
> 'autophone-shutdown', but instead 'shutdown', likewise remove autophone-
> from all commands.

I think we used 'autophone-shutdown' because there is also a 'device-shutdown' command (and other autophone/device command pairs). 'shutdown' is more intuitive to an autophone newbie, but 'autophone-shutdown' leaves no room for misinterpretation. I don't have a strong preference. :bc?
lets stick with no prompts for each device and see if the end users are fine with it!
Comment on attachment 8721536 [details] [diff] [review]
add 'mach autophone' command

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

I don't see where we:

1. Specify the command line arguments/config file to autophone.py.
2. Specify the tests/test manifest?
3. Specify/Configure/Start the webserver?

::: mobile/android/mach_commands.py
@@ +190,5 @@
> +        self.saveConfig()
> +        self.interactive()
> +        return 0
> +
> +    def getDefaultBaseDir(self):

We will create a new autophone-<n> directory for each invocation?

@@ +254,5 @@
> +        return keep_going
> +
> +    def configureDevices(self):
> +        import datetime
> +        from mozdevice import DeviceManagerADB, DMError

adb.py not appropriate?

@@ +256,5 @@
> +    def configureDevices(self):
> +        import datetime
> +        from mozdevice import DeviceManagerADB, DMError
> +        keep_going = True
> +        device_ini = os.path.join(self.config['base-dir'], 'devices.ini')

I'm a little concerned about hardcoded config file names especially those that aren't ignored by git. Any developer*.ini file is ignored. Though, I guess --devices=devices.ini by default. nevermind.

@@ +277,5 @@
> +                try:
> +                    with open(os.path.join(self.config['base-dir'], 'devices.ini'), 'w') as f:
> +                        for device in dm.devices():
> +                            serial = device[0]
> +                            f.write("[device-%d]\nserialno=%s\n" % (device_index, serial))

How do we check and notify that the device must be rooted?

@@ +297,5 @@
> +
> +    def configure(self):
> +        if not self.configureDevices():
> +            return False
> +        # configure treeherder, etc?

The developer's shouldn't be concerned about Treeherder or Pulse.

@@ +309,5 @@
> +        self.thread.start()
> +
> +    def runAutophone(self):
> +        dir = self.config['base-dir']
> +        self.runProcess(['./env/bin/python', 'autophone.py'], cwd=dir, dump=True)

Where are you either setting up the command line parameters or the autophone config file to govern the autophone process?

@@ +317,5 @@
> +        dir = self.config['base-dir']
> +        time.sleep(5)
> +        if self.thread.isAlive():
> +            self.log(logging.INFO, "autophone", {},
> +                "Type 'autophone-help' for help, 'autophone-shutdown' to shutdown and quit.")

If they are only accessing the commands and help via this interface, there is no need to change autophone's command names. I don't think we need expose the full set of commands to developers. Simply map the developer's response from 'shutdown' to autophone-shutdown and 'stop' to autophone-stop with explanations of the difference. What other commands do you think they will be interested in using other than triggering jobs?

@@ +328,5 @@
> +                    "Quitting...")
> +                response = "autophone-shutdown"
> +            if response == "autophone-shutdown":
> +                quitting = True
> +            self.runProcess(['./ap.sh', response], cwd=dir, dump=True)

Do we care about Windows support? Autophone has been developed with the idea that it is a server that will be running on Linux or a minimum of OS X. This is particular is definitely not Windows. We should alert users that Windows is not supported.
Attachment #8721536 - Flags: feedback?(bob)
(In reply to Bob Clary [:bc:] from comment #11)
> I don't see where we:
> 
> 1. Specify the command line arguments/config file to autophone.py.
> 2. Specify the tests/test manifest?
> 3. Specify/Configure/Start the webserver?

Sorry, I should have managed expectations better: I hadn't attempted any of that yet. This version was just about pulling autophone and getting it running.

> > +    def getDefaultBaseDir(self):
> 
> We will create a new autophone-<n> directory for each invocation?

No. Only the first time; next invocation re-uses autophone-<n> and doesn't download anything. (I'm also adding a --clean option to delete the old directory and start over.)
 
> @@ +254,5 @@
> > +        return keep_going
> > +
> > +    def configureDevices(self):
> > +        import datetime
> > +        from mozdevice import DeviceManagerADB, DMError
> 
> adb.py not appropriate?

We usually use mozdevice in the android mach commands. We won't use it much ... shouldn't get into trouble.

> @@ +256,5 @@
> > +    def configureDevices(self):
> > +        import datetime
> > +        from mozdevice import DeviceManagerADB, DMError
> > +        keep_going = True
> > +        device_ini = os.path.join(self.config['base-dir'], 'devices.ini')
> 
> I'm a little concerned about hardcoded config file names especially those
> that aren't ignored by git. Any developer*.ini file is ignored. Though, I
> guess --devices=devices.ini by default. nevermind.

Yeah, I'm trying to get away with using that default.

> How do we check and notify that the device must be rooted?

Not attempted yet. 

> The developer's shouldn't be concerned about Treeherder or Pulse.

That sounds right.
 
> Where are you either setting up the command line parameters or the autophone
> config file to govern the autophone process?

Coming in the next version.
 
> @@ +317,5 @@
> > +        dir = self.config['base-dir']
> > +        time.sleep(5)
> > +        if self.thread.isAlive():
> > +            self.log(logging.INFO, "autophone", {},
> > +                "Type 'autophone-help' for help, 'autophone-shutdown' to shutdown and quit.")
> 
> If they are only accessing the commands and help via this interface, there
> is no need to change autophone's command names. I don't think we need expose
> the full set of commands to developers. Simply map the developer's response
> from 'shutdown' to autophone-shutdown and 'stop' to autophone-stop with
> explanations of the difference. What other commands do you think they will
> be interested in using other than triggering jobs?

autophone-status comes to mind. Probably not much else. It's easy to give access to all the ap.sh commands though.
 
> Do we care about Windows support? Autophone has been developed with the idea
> that it is a server that will be running on Linux or a minimum of OS X. This
> is particular is definitely not Windows. We should alert users that Windows
> is not supported.

Agreed - coming soon!
(In reply to Geoff Brown [:gbrown] from comment #9)
> > this assumes virtualenv is available, I recall in the past (last 12 months)
> > this hasn't been the case for a handful of developers trying to get talos
> > running.
> 
> I had a look at what other projects do and discovered that mach has
> VirtualenvManager -- I'll try to use that.

Some mach commands use self.virtualenv_manager. That's not suitable for us because that stores the venv in the objdir - so a clobber or a change to MOZCONFIG can cause us to lose the autophone required packages. Some mach commands use self.virtualenv_manager and re-install their requirements every time (seems wasteful) or re-install if they cannot import a well-known package (seems awkward for autophone). I'll try to leverage VirtualenvManager but create the venv in the autophone directory.
Attached patch add 'mach autophone' command (obsolete) — Splinter Review
Now includes elementary test selection and trigger_runs support.
Attachment #8721536 - Attachment is obsolete: true
Attachment #8724285 - Attachment is obsolete: true
Attachment #8729670 - Attachment is obsolete: true
I want 'mach autophone' to offer a menu of test choices - s1/s2, robocop, tp4m, etc. I want that menu to be dynamic, in that I want to be able to add new choices easily, and naturally from the autophone perspective. Initially I thought I would just enumerate <autophone>/tests/*.ini, but that contains production-*.ini files not suitable for mach and some other ini files of questionable value from the mach perspective.

In this patch, I add a comment to select ini files (and added some new manifests) and include "@mach@" in the comment. ini files with such a comment are used to construct a menu of test choices for mach. With this patch applied, 'mach autophone' offers:

Select a test to run:
1. Robocop - testAdobeFlash
2. Smoketest
3. Talos tp4m
4. Talos tsvg
5. s1/s2 Throbber Start/Stop
Test to run? (1-5) 

(I selected smoketest because it is simple and basic and the others because these are the ones currently in production.)
Attachment #8730866 - Flags: review?(bob)
Attachment #8730772 - Attachment is obsolete: true
We'll probably need someone else to actually review mach stuff.

(In reply to Geoff Brown [:gbrown] from comment #17)
 
> (I selected smoketest because it is simple and basic and the others because
> these are the ones currently in production.)

I bet autophone-mochitest-dom-media "Mochitest DOM Media" will be popular as well.

Since you are adding new manifests for use with mach, do you think we should prefix their names with mach- ?

(In reply to Geoff Brown [:gbrown] from comment #18)
> Created attachment 8730956 [details] [diff] [review]
> wip - add 'mach autophone' command

It would be nice to have a trailing space after the prompt's default in:
Enter location of new autophone directory: [/home/bclary/autophone]/tmp/autophone

The prompt for IP address of webserver...

IP address of webserver [192.168.1.50] 192.168.1.50:1800

This actually conflates the address of the webserver with the ipaddr of the host running autophone. We need both and need to keep them separate.

(In reply to Geoff Brown [:gbrown] from comment #18)

Note the ipaddr is actually an IP address, but webserver_url can be a address:port combination. If I enter 192.168.1.50:1800 for the webserver, trigger will fail to contact the running Autophone instance to actually download a build. Actually, I'm having problems with triggering a build.
Attachment #8730866 - Flags: review?(bob)
As discussed yesterday, this version adds @mach@ to existing manifests and adds new manifests so that it is possible to select practically every test for which there is an existing config.

With the latest (coming soon) mach patch, this yields test selection like:

Select a test to run:
1. Talos - tcheck
2. Talos - tp4m
3. Talos - tsvg
4. crashtests - all
5. js-reftests - all
6. mochitests - all
7. mochitests - dom browser element
8. mochitests - dom media
9. mochitests - media
10. mochitests - skia
11. mochitests - toolkit widgets
12. reftests - all
13. reftests - ogg video
14. reftests - webm video
15. robocop - all
16. robocop - testAdobeFlash
17. s1/s2 Throbber Start/Stop - all
18. s1/s2 Throbber Start/Stop - blank - local
19. s1/s2 Throbber Start/Stop - blank - remote
20. s1/s2 Throbber Start/Stop - nytimes - local
21. s1/s2 Throbber Start/Stop - nytimes - remote
22. s1/s2 Throbber Start/Stop - nytimes - settings
23. s1/s2 Throbber Start/Stop - twitter - local
24. s1/s2 Throbber Start/Stop - twitter - remote
25. smoketest
26. webappstartup
Test(s) to run? (1-26, or enter path to test manifest) 

"@webserver@" is present in Talos and remote s1/s2 manifests to enable optional webserver support (also coming soon).

> do you think we should prefix their names with mach- ?

ftr, we decided against that yesterday.

I noticed that some existing manifests are named xxx-manifest.ini, while others are simply xxx.ini; I used the xxx.ini form for new manifests, but am happy to rename if there is some convention.
Attachment #8730866 - Attachment is obsolete: true
Attachment #8731800 - Flags: review?(bob)
Comment on attachment 8731800 [details] [diff] [review]
add @mach@ comments to test manifests

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

> I noticed that some existing manifests are named xxx-manifest.ini, while others
> are simply xxx.ini; I used the xxx.ini form for new manifests, but am happy to
> rename if there is some convention.

The original file naming dates back to ctalbert I think. I've never cared for it but never took the time to fix it.

I like your new scheme. Would you mind doing the git mv dance and rename all of the manifests in tests/ (including the production manifests) to follow your new scheme. I think it will be worth it to be consistent here going forward.

You will need to update the production-autophone-{1,2,3}.ini files to point to the new names for the production manifests and some related edits in USAGE.md as well.

::: tests/manifest.ini
@@ -1,2 @@
>  [s1s2test.py]
>  config = ../configs/s1s2-settings.ini

This test actually includes both local and remote so if we keep it, it should have the webserver. It is a hold over from before we split out configs for local and remote.
 
I think we can drop this manifest and the corresponding config file configs/s1s2-settings.ini since we get the same results by using the local and remote configs separately. We will need to update the USAGE.md document as it references this file.

::: tests/s1s2-nytimes-settings.ini
@@ +1,3 @@
> +# @mach@ s1/s2 Throbber Start/Stop - nytimes - settings
> +[s1s2test.py]
> +config = ../configs/s1s2-nytimes-settings.ini

This test actually includes both local and remote so if we keep it, it should have the webserver. It is a hold over from before we split out configs for local and remote.

I think we can drop this manifest and the corresponding config file configs/s1s2-nytimes-settings.ini since we get the same results by using the local and remote configs separately.
Attachment #8731800 - Flags: review?(bob) → review+
OK, I deleted manifest.ini, s1s2-settings.ini, s1s2-nytimes-settings.ini and renamed various manifests, and updated USAGE.md and production ini's accordingly. That's a lot to keep track of...I hope it is all consistent!

https://github.com/mozilla/autophone/commit/62b517b2072b7160f0b71afc3a2f0a628b6737d1
Attached patch add 'mach autophone' command (obsolete) — Splinter Review
Attachment #8730956 - Attachment is obsolete: true
Removed unittests-manifest.ini too, just to make things simpler:

https://github.com/mozilla/autophone/commit/29808f06c26b4ad03c5f5348e7039a58cae84912
(In reply to Geoff Brown [:gbrown] from comment #22)
> 
> https://github.com/mozilla/autophone/commit/
> 62b517b2072b7160f0b71afc3a2f0a628b6737d1

(In reply to Geoff Brown [:gbrown] from comment #24)
> 
> https://github.com/mozilla/autophone/commit/
> 29808f06c26b4ad03c5f5348e7039a58cae84912

deployed 2016-03-19 05:30 PT
Attached patch add 'mach autophone' command (obsolete) — Splinter Review
This works great for me -- does it work for you? Apply this patch, run 'mach autophone', and see if you can run your own autophone test!

Tell me:
 - Is anything confusing?
 - Were you able to run tests?
 - Is any basic functionality missing?

No need to look at the patch at this stage -- I'm more interested in your experience trying to run tests.
Attachment #8732402 - Attachment is obsolete: true
Attachment #8732676 - Flags: feedback?(snorp)
Attachment #8732676 - Flags: feedback?(jmaher)
Attachment #8732676 - Flags: feedback?(bob)
Comment on attachment 8732676 [details] [diff] [review]
add 'mach autophone' command

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

Overall this is looking pretty good. We have some work to do though.

should detect if i have a build environment set up properly first.
finding out after attempting to select devices
via the BuildEnvironmentNotFoundException: config.status not available. Run configure. and tracback stack is not friendly.

IP address of interface to use for phone callbacks
    What does this mean?

Test(s) to run? (1-24, or path to test manifest) 7, 8, 9, 10, 13, 14, 17
    multiple test selection doesn't work. it appears to only take the last

18. s1/s2 Throbber Start/Stop - blank - remote
Test(s) to run? (1-24, or path to test manifest) 18
    this attempted to run local tests off of the device instead
    of a web server.

    Doesn't allow me to specify web server url.

['Ncat: Connection refused.']
   Would be nice if these weren't displayed

Nice that it waits for autophone to start though before prompting.

 1:06.89 You may optionally specify a repository name like 'mozilla-central' or 'try'.
Enter repo name [] mozilla-central
   mozilla-central is the default?

You may optionally specify a test name.
    Not sure how this is used when selecting multiple tests.
    Having the user enter this may cause issues if the name doesn't match
    and expected value.

    What is it used for?

Might want to tell the user if they want to change tests they need to quit and start again. Or make that an explicit command.
Attachment #8732676 - Flags: feedback?(bob)
Comment on attachment 8732676 [details] [diff] [review]
add 'mach autophone' command

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

I think bc gave a lot of great things, I have a few things to add.  This patch is quite large, I suspect it will be easy to miss small details in review- we should just budget time for small iterations on this once we feel it is decent.

::: mobile/android/mach_commands.py
@@ +227,5 @@
> +                if os.path.exists(self.CONFIG_FILE):
> +                    os.remove(self.CONFIG_FILE)
> +                if os.path.exists(dir):
> +                    shutil.rmtree(dir)
> +        else:

this should be:
if os.path.exists(self.CONFIG_FILE):
   ...
else:
   already clean

@@ +293,5 @@
> +            self.log(logging.ERROR, "autophone", {},
> +                "Unable to clone autophone directory.")
> +            if not verbose:
> +                self.log(logging.ERROR, "autophone", {},
> +                    "Try re-running this command with --verbose to get more info.")

will verbose give us more info here?

@@ +491,5 @@
> +    def configureIP(self):
> +        import socket
> +        # take a guess at the IP -- this won't always get the "right" IP
> +        s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
> +        s.connect(('8.8.8.8', 0))

why 8.8.8.8?  not 127.0.0.1?

@@ +566,5 @@
> +        import time
> +        dir = self.config['base-dir']
> +        for seconds in [5, 5, 3, 3, 1, 1, 1, 1]:
> +            time.sleep(seconds)
> +            if self.runProcess(['./ap.sh', 'autophone-status'], cwd=dir, dump=False):

what does this test for us?  do we check the output of autophone-status?
Attachment #8732676 - Flags: feedback?(jmaher) → feedback+
(In reply to Bob Clary [:bc:] from comment #27)
> should detect if i have a build environment set up properly first.
> finding out after attempting to select devices
> via the BuildEnvironmentNotFoundException: config.status not available. Run
> configure. and tracback stack is not friendly.

Oh, that's interesting -- glad you found it. It turns out we only need a mach "build environment" to look up the location of adb. I'll change this to prompt for the adb path if a build environment is not available and adb is not otherwise found.
 
> IP address of interface to use for phone callbacks
>     What does this mean?

I'm setting the autophone.py --ipaddr option.  "IP address of interface to use for phone callbacks" is the help text from that option! What should I say instead?

> Test(s) to run? (1-24, or path to test manifest) 7, 8, 9, 10, 13, 14, 17
>     multiple test selection doesn't work. it appears to only take the last

Sigh. This is used to set the --test-path option. Multiple selections will result in "--test-path <manifest-1> --test-path <manifest-2>" ... but now I see that autophone.py only allows for one test manifest. Can autophone.py be updated to allow multiple --test-path options and multiple manifests? If that's non-trivial, perhaps we should abandon the idea of multiple test selection, or spin it off to a separate bug.

> 18. s1/s2 Throbber Start/Stop - blank - remote
> Test(s) to run? (1-24, or path to test manifest) 18
>     this attempted to run local tests off of the device instead
>     of a web server.
> 
>     Doesn't allow me to specify web server url.

Can you check on that? I cannot reproduce. "18" prompts me for a web server url and runs the correct test on device.

Test(s) to run? (1-24, or path to test manifest) 18
IP address of interface to use for phone callbacks [192.168.0.81] 
 0:08.01 Some of your selected tests require a webserver.
Start a webserver now? [Y/n] 
Webserver address? [192.168.0.81:8100] 
 0:20.08 Launching webserver...
 0:20.08 Launching autophone...

> ['Ncat: Connection refused.']
>    Would be nice if these weren't displayed

I'm trying to sort that out. I don't see it myself.
 
>  1:06.89 You may optionally specify a repository name like 'mozilla-central'
> or 'try'.
> Enter repo name [] mozilla-central
>    mozilla-central is the default?

The default is "": no --repo option is passed to trigger_runs.py. Would an explicit "mozilla-central" default be better?
 
> You may optionally specify a test name.
>     Not sure how this is used when selecting multiple tests.
>     Having the user enter this may cause issues if the name doesn't match
>     and expected value.
> 
>     What is it used for?

If entered, this sets trigger_runs.py's --test option. I think, now that we have all the one-test-per-manifest manifests, this is not very useful. I'd like to remove this option (from mach).

> Might want to tell the user if they want to change tests they need to quit
> and start again. Or make that an explicit command.

I'll add some text to the initial interactive prompt.
(In reply to Joel Maher (:jmaher) from comment #28)
> ::: mobile/android/mach_commands.py
> @@ +227,5 @@
> > +                if os.path.exists(self.CONFIG_FILE):
> > +                    os.remove(self.CONFIG_FILE)
> > +                if os.path.exists(dir):
> > +                    shutil.rmtree(dir)
> > +        else:
> 
> this should be:
> if os.path.exists(self.CONFIG_FILE):
>    ...
> else:
>    already clean
> 
> @@ +293,5 @@
> > +            self.log(logging.ERROR, "autophone", {},
> > +                "Unable to clone autophone directory.")
> > +            if not verbose:
> > +                self.log(logging.ERROR, "autophone", {},
> > +                    "Try re-running this command with --verbose to get more info.")
> 
> will verbose give us more info here?

Usually, yes: --verbose will show the 'git clone' command and all of its output.

> @@ +491,5 @@
> > +    def configureIP(self):
> > +        import socket
> > +        # take a guess at the IP -- this won't always get the "right" IP
> > +        s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
> > +        s.connect(('8.8.8.8', 0))
> 
> why 8.8.8.8?  not 127.0.0.1?

I'm trying to come up with an IP address to offer as a quick default choice for the --ipaddr argument to autophone.py. With the possibility of multiple network interfaces (wifi, ethernet, etc), I think it's  impossible to determine the right IP address, but I would like to offer a choice that might be right.

Substituting '127.0.0.1' above gives me back an IP address of '127.0.0.1', rather than the real IP of the interface: I don't think that will usually be useful.

More discussion at http://stackoverflow.com/questions/166506/finding-local-ip-addresses-using-pythons-stdlib.

> @@ +566,5 @@
> > +        import time
> > +        dir = self.config['base-dir']
> > +        for seconds in [5, 5, 3, 3, 1, 1, 1, 1]:
> > +            time.sleep(seconds)
> > +            if self.runProcess(['./ap.sh', 'autophone-status'], cwd=dir, dump=False):
> 
> what does this test for us?  do we check the output of autophone-status?

We're basically waiting for autophone to start.

After all the prompts, the mach command launches autophone.py in a separate thread and dumps autophone's output to stdout (so you see, for instance, "Autophone started", from autophone.py). We go through this autophone-status wait, and then prompt for interactive commands ("autophone command? "). The wait is there so that the prompt for interactive commands doesn't get mixed up with autophone's startup output. I'm relying on the exit code of ap.sh rather than trying to parse the output -- I think I'm comfortable with that.
(In reply to Geoff Brown [:gbrown] from comment #29)
> (In reply to Bob Clary [:bc:] from comment #27)
> > should detect if i have a build environment set up properly first.
> > finding out after attempting to select devices
> > via the BuildEnvironmentNotFoundException: config.status not available. Run
> > configure. and tracback stack is not friendly.
> 
> Oh, that's interesting -- glad you found it. It turns out we only need a
> mach "build environment" to look up the location of adb. I'll change this to
> prompt for the adb path if a build environment is not available and adb is
> not otherwise found.
>  

I just had to set MOZCONFIG. It didn't seem related to adb.

> > IP address of interface to use for phone callbacks
> >     What does this mean?
> 
> I'm setting the autophone.py --ipaddr option.  "IP address of interface to
> use for phone callbacks" is the help text from that option! What should I
> say instead?
> 

Heh. I forgot about this wording. I guess it is a hold over from the earliest days of Autophone. Let's just leave it as is for now.

> > Test(s) to run? (1-24, or path to test manifest) 7, 8, 9, 10, 13, 14, 17
> >     multiple test selection doesn't work. it appears to only take the last
> 
> Sigh. This is used to set the --test-path option. Multiple selections will
> result in "--test-path <manifest-1> --test-path <manifest-2>" ... but now I
> see that autophone.py only allows for one test manifest. Can autophone.py be
> updated to allow multiple --test-path options and multiple manifests? If
> that's non-trivial, perhaps we should abandon the idea of multiple test
> selection, or spin it off to a separate bug.
> 

Right. I missed the manifest vs test issue completely. We could either go with a single test manifest or cat together all of the manifests into a temporary and used that.

> > 18. s1/s2 Throbber Start/Stop - blank - remote
> > Test(s) to run? (1-24, or path to test manifest) 18
> >     this attempted to run local tests off of the device instead
> >     of a web server.
> > 
> >     Doesn't allow me to specify web server url.
> 
> Can you check on that? I cannot reproduce. "18" prompts me for a web server
> url and runs the correct test on device.
> 

I'll try again after the meeting.

> Test(s) to run? (1-24, or path to test manifest) 18
> IP address of interface to use for phone callbacks [192.168.0.81] 
>  0:08.01 Some of your selected tests require a webserver.
> Start a webserver now? [Y/n] 
> Webserver address? [192.168.0.81:8100] 
>  0:20.08 Launching webserver...
>  0:20.08 Launching autophone...
> 
> > ['Ncat: Connection refused.']
> >    Would be nice if these weren't displayed
> 
> I'm trying to sort that out. I don't see it myself.

I didn't look at the patch, but maybe capture stdout/stderr from the nc process?

>  
> >  1:06.89 You may optionally specify a repository name like 'mozilla-central'
> > or 'try'.
> > Enter repo name [] mozilla-central
> >    mozilla-central is the default?
> 
> The default is "": no --repo option is passed to trigger_runs.py. Would an
> explicit "mozilla-central" default be better?
>  

Well, autophone and trigger both default to mozilla-central if no repo is specified. Perhaps a message that mozilla-central is the default?

> > You may optionally specify a test name.
> >     Not sure how this is used when selecting multiple tests.
> >     Having the user enter this may cause issues if the name doesn't match
> >     and expected value.
> > 
> >     What is it used for?
> 
> If entered, this sets trigger_runs.py's --test option. I think, now that we
> have all the one-test-per-manifest manifests, this is not very useful. I'd
> like to remove this option (from mach).
> 

Yeah, if we only have one manifest and one test per manifest, this doesn't add anything. I would say that if we do allow multiple manifests to be catted together I would still remove it and just run all of the tests specified in the manifests.

> > Might want to tell the user if they want to change tests they need to quit
> > and start again. Or make that an explicit command.
> 
> I'll add some text to the initial interactive prompt.

I'll get back to you about the web server stuff.
Starting the web server worked:

Start a webserver now? [Y/n] y
Webserver address? [192.168.1.50:8100] 

But saying no:

Start a webserver now? [Y/n] n

It didn't prompt me for the web server url
...including prompting for webserver-url when webserver start has been declined (thanks :bc!).
This is looking rightous. I'm doing 18 atm and so far so good.

One more comment.

Connecting to autophone server...
autophone-triggerjobs {"test_names": [], "build": "http://ftp.mozilla.org/pub/mobile/nightly/2016/03/2016-03-22-03-04-17-mozilla-central-android-api-15/fennec-48.0a1.multi.android-arm.apk", "devices": []}
- ok
autophone-triggerjobs {"test_names": [], "build": "http://ftp.mozilla.org/pub/mobile/nightly/2016/03/2016-03-22-03-04-17-mozilla-central-android-x86/fennec-48.0a1.multi.android-i386.apk", "devices": []}
- ok
exit
-
autophone command? 

They may not realize what is going on while it downloads the build or understand how to check. Maybe a comment that it should be downloading the build and will begin testing in a few minutes. A prompt to do autophone-status might be helpful for the user as well.

Looking Good Eugune!

Do we have anyone on a mac we can get to test? I'm concerned about the utility path and the dynamically loaded library issues I've seen in the past.
Good idea. Here's an attempt at making this more foolproof:

 0:39.57 Launching autophone...
2016-03-22 14:42:18,894|14465|MainThread|console|INFO|Starting server on port 28001.
2016-03-22 14:42:18,895|14465|MainThread|console|INFO|Starting build-cache server on port 28008.
2016-03-22 14:42:19,004|14465|MainThread|console|INFO|Starting autophone.
2016-03-22 14:42:19,006|14465|MainThread|console|INFO|Loading tests.
2016-03-22 14:42:19,033|14465|MainThread|console|INFO|Initializing devices.
2016-03-22 14:42:19,033|14465|MainThread|console|INFO|Initializing device name=device-1, serialno=01498B300600B008
2016-03-22 14:42:21,168|14465|MainThread|console|INFO|Autophone started.
 0:45.62 Use 'trigger' to select builds to test using the current test manifest.
 0:45.62 Type 'trigger', 'help', 'quit', or an autophone command.
autophone command? trigger
 0:52.32 Tests will be run against a build or collection of builds, selected by:
1. The latest build
2. Build URL
3. Build ID
4. Date/date-time range              
Build selection type? (1-4) 1
 0:57.91 You may optionally specify a repository name like 'mozilla-inbound' or 'try'.
 0:57.91 If not specified, 'mozilla-central' is assumed.
Enter repo name: 
 1:02.30 Triggering...Tests will run once builds have been downloaded.
 1:02.30 Use 'autophone-status' to check progress.
Connecting to autophone server...
autophone-triggerjobs {"test_names": [], "build": "http://ftp.mozilla.org/pub/mobile/nightly/2016/03/2016-03-22-03-04-17-mozilla-central-android-api-15/fennec-48.0a1.multi.android-arm.apk", "devices": []}
- ok
autophone-triggerjobs {"test_names": [], "build": "http://ftp.mozilla.org/pub/mobile/nightly/2016/03/2016-03-22-03-04-17-mozilla-central-android-x86/fennec-48.0a1.multi.android-i386.apk", "devices": []}
- ok
exit
-
autophone command?
I incorrectly set the utility path to the fennec dist bin which failed to start the xpcshell etc. The only way to recover was to delete the autophone directory and start over. It might be nice to include the prompt to set the utility path even for an existing setup. We could default it to the previous value but allow it to be changed rather than forcing a removal and complete setup again.
The unittests in particular are designed to have their results submitted to treeherder and don't make the test results available in the logs when run without treeherder. The test logs are available in the ~/autophone/builds/.../tests directory however. We should file follow up bugs on the unittests and the other tests including talos to make sure we present the test results to the user in an obvious, easily discovered fashion when run via mach.
The unittests-defaults.ini created by mach autophone has the old timeout value:

# Created by 'mach autophone'
[runtests]
xre_path = /mozilla/builds/inbound/mozilla/firefox-debug/dist/bin
utility_path = /mozilla/builds/inbound/mozilla/firefox-debug/dist/bin
console_level = DEBUG
log_level = DEBUG
time_out = 2400
(In reply to Bob Clary [:bc:] from comment #37)
> I incorrectly set the utility path to the fennec dist bin which failed to
> start the xpcshell etc. The only way to recover was to delete the autophone
> directory and start over. It might be nice to include the prompt to set the
> utility path even for an existing setup. We could default it to the previous
> value but allow it to be changed rather than forcing a removal and complete
> setup again.

I think most mobile devs will have host utils set up via mach and therefore will get the right directory without prompting or anything.

When we need to query for a directory, we should check that xpcshell is there -- updated in the next version of the patch.

And then, I agree, we need a way to offer a way to update an existing unit test configuration -- updated in the next version of the patch. (But I don't want the complexity of trying to update the xre_path in an existing file; I'm only offering the option of scrapping the existing configuration and starting over.)
Attachment #8732676 - Attachment is obsolete: true
Attachment #8733190 - Attachment is obsolete: true
Attachment #8732676 - Flags: feedback?(snorp)
Attachment #8733942 - Flags: feedback?(snorp)
(In reply to Bob Clary [:bc:] from comment #39)
> The unittests-defaults.ini created by mach autophone has the old timeout

Changed to 300 in the latest patch. Also updated in unittest-defaults.ini.example:

https://github.com/mozilla/autophone/commit/7043d33ed86132c64b90fd5f357cb0c31b170e06
Blocks: 1259128
Attached patch add 'mach autophone' command (obsolete) — Splinter Review
Cleaned up and re-organized: I moved the bulk of the code into a new file in mozrunner, similar to 'mach android-emulator'.
Attachment #8733942 - Attachment is obsolete: true
Attachment #8733942 - Flags: feedback?(snorp)
Attachment #8734425 - Flags: review?(bob)
Attachment #8734425 - Flags: feedback?(snorp)
Comment on attachment 8734425 [details] [diff] [review]
add 'mach autophone' command

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

Pretty much alright with me with a minimum of the error fixes. We can talk about some of the suggestions and I'd like to look at the updated version and to test it as well.

::: testing/mozbase/mozrunner/mozrunner/devices/autophone.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import datetime

datetime unused.

@@ +19,5 @@
> +import BaseHTTPServer
> +import SimpleHTTPServer
> +
> +from mozbuild.virtualenv import VirtualenvManager
> +from mozdevice import DeviceManagerADB, DMError

DMError unused.

@@ +52,5 @@
> +            response = raw_input(
> +                "Proceed with deletion? (y/N) ").strip()
> +            if response.lower().startswith('y'):
> +                os.remove(self.CONFIG_FILE)
> +                shutil.rmtree(dir)

I'm a little concerned about this.

@@ +57,5 @@
> +        else:
> +            self.build_obj.log(logging.INFO, "autophone", {},
> +                "Already clean -- nothing to do!")
> +
> +    def get_default_install_dir(self):

I'm not clear as to the usefulness of this check. It will find the first of autophone, autophone-1, ...

What is the utility of leaving the previous autophone directories?

@@ +58,5 @@
> +            self.build_obj.log(logging.INFO, "autophone", {},
> +                "Already clean -- nothing to do!")
> +
> +    def get_default_install_dir(self):
> +        default = os.path.join(os.path.expanduser('~'), 'autophone')

If we are concerned about trampling on an existing autophone directory in the user's home directory, perhaps using a different directory name such as mach-autophone would be better?

@@ +113,5 @@
> +            "Unable to find an existing autophone directory. Let's setup a new one...")
> +        response = raw_input(
> +            "Enter location of new autophone directory: [%s] " % dir).strip()
> +        if response != '':
> +            dir = response

I would prefer some limitation on the value of dir so that when we do shutil.rmtree(dir) elsewhere there is _no_ chance of deleting something we might regret. Perhaps requiring the directory name at least begin with autophone or mach-autophone?

@@ +126,5 @@
> +        if not os.path.exists(os.path.join(dir, '.git')):
> +            # git not installed? File permission problem? github not available?
> +            self.build_obj.log(logging.ERROR, "autophone", {},
> +                "Unable to clone autophone directory.")
> +            if not verbose:

self.verbose ?

@@ +175,5 @@
> +                "Connect your rooted Android test device(s) with usb and press Enter ")
> +            adb_path = 'adb'
> +            try:
> +                if os.path.exists(self.substs["ADB"]):
> +                    adb_path = self.substs["ADB"]

Where is self.substs determined? Isn't this self.build_obj.substs ?

@@ +176,5 @@
> +            adb_path = 'adb'
> +            try:
> +                if os.path.exists(self.substs["ADB"]):
> +                    adb_path = self.substs["ADB"]
> +            except:

Perhaps if self.verbose is True, we could log the exception?

@@ +200,5 @@
> +                            keep_going = True
> +                        else:
> +                            self.build_obj.log(logging.WARNING, "autophone", {},
> +                                "Device '%s' is not rooted - skipping" % serial)
> +            except:

Perhaps if self.verbose is True, we could log the exception?

@@ +267,5 @@
> +                            "'%s' unrecognized: Enter a number between 1 and %d!" % (response, highest))
> +            self.autophone_options.extend(['--test-path', path])
> +        else:
> +            # Provide a simple backup for the unusual case where test manifests
> +            # cannot be found.

When do you think this branch might be useful? Wouldn't this be an example of a condition where we would want this to return False?

@@ +296,5 @@
> +xre_path = %s
> +utility_path = %s
> +console_level = DEBUG
> +log_level = DEBUG
> +time_out = 300 """ % (xre_path, xre_path))

Again, not thrilled by escapes used to continue lines, but I can live with it. Trailing space after the 300 though.

@@ +410,5 @@
> +               self.configure_unittests() and \
> +               self.configure_tests() and \
> +               self.configure_ip() and \
> +               self.configure_webserver() and \
> +               self.configure_other()

I'm not thrilled with line continuations, but can live with it.

@@ +433,5 @@
> +        self.build_obj.log(logging.INFO, "autophone", {},
> +            "Launching autophone...")
> +        self.thread = threading.Thread(target=self.run_autophone)
> +        self.thread.start()
> +        # wait for startup

I'm a little concerned that if the start up takes longer than 20 seconds, the user might not be aware there is a problem.

Perhaps a flag that could be set when autophone-status succeeds and if it is not set when the 20 seconds completes, we output a message that Autophone is taking longer than expected to start.

@@ +510,5 @@
> +        while (int(response) < 1 or int(response) > highest):
> +            response = raw_input(
> +                "Build selection type? (1-%d) " % highest).strip()
> +            try:
> +                validate = int(response)

validate is unused, but could be re-used below instead of the repeated int(response) calls.

Elsewhere you have used choice for the name of a similar variable.

@@ +540,5 @@
> +            "Enter repo name: ").strip()
> +        if len(repo) > 0:
> +            options.extend(["--repo=%s" % repo])
> +            if repo != "mozilla-central":
> +                options.extend(["--build-location=tinderbox"])

This forces mozilla-central to be from http://ftp.mozilla.org/pub/mobile/nightly/ and does not allow the selection of mozilla-central builds from http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/.

If repo is mozilla-central or mozilla-aurora, we should prompt for either nightly or tinderbox locations.

@@ +565,5 @@
> +        class AutoHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
> +            # A simple request handler with logging suppressed and rooted
> +            # at the autophone directory.
> +            dir = None
> +            def translate_path(self, path):

Is all of this necessary now?

https://bugs.python.org/issue17324 was fixed 2013-09-13 and https://www.python.org/downloads/release/python-276/ was released 2013-11-10 so I would expect this is fixed in Python 2.7.6 and later. Even though the Linux build prerequisites lists 2.7.3 (2012-04-09) as a minimum Python, I wonder how often someone will attempt this with a Python 2.7.{3..5} ?

If we do need this, please link to the original issue and source and comment that it is a work around for a bug that was fixed in Python 2.7.6 and can be removed when the minimum build reaches 2.7.6.
Attachment #8734425 - Flags: review?(bob) → review+
(In reply to Bob Clary [:bc:] from comment #44)
> datetime unused.
> DMError unused.

Oops. Fixed now.

> @@ +52,5 @@
> > +            response = raw_input(
> > +                "Proceed with deletion? (y/N) ").strip()
> > +            if response.lower().startswith('y'):
> > +                os.remove(self.CONFIG_FILE)
> > +                shutil.rmtree(dir)
> 
> I'm a little concerned about this.
> 
> @@ +57,5 @@
> > +        else:
> > +            self.build_obj.log(logging.INFO, "autophone", {},
> > +                "Already clean -- nothing to do!")
> > +
> > +    def get_default_install_dir(self):
> 
> I'm not clear as to the usefulness of this check. It will find the first of
> autophone, autophone-1, ...
> 
> What is the utility of leaving the previous autophone directories?
> 
> If we are concerned about trampling on an existing autophone directory in
> the user's home directory, perhaps using a different directory name such as
> mach-autophone would be better?

Early in the development of this patch, I had an ~/autophone directory that I didn't want mach autophone to mess with, and the earlier patch was being more aggressive about proceeding with a directory without prompting. Today, I'm not as concerned because 'mach autophone' prompts before doing anything with the directory, and what it does is generally not destructive (--clean excepted). I like 'mach-autophone' -- I've switched to that, and removed the complicated search for a new directory.
 
> @@ +113,5 @@
> > +            "Unable to find an existing autophone directory. Let's setup a new one...")
> > +        response = raw_input(
> > +            "Enter location of new autophone directory: [%s] " % dir).strip()
> > +        if response != '':
> > +            dir = response
> 
> I would prefer some limitation on the value of dir so that when we do
> shutil.rmtree(dir) elsewhere there is _no_ chance of deleting something we
> might regret. Perhaps requiring the directory name at least begin with
> autophone or mach-autophone?

We only shutil.rmtree() when --clean has been specified and the user has responded 'y' to a prompt which includes the directory name. The directory name will be picked up from the configuration file, so it's likely a directory that was created by 'mach autophone'. As well, the default name will now be '~/mach-autophone'. I think that's enough precautions.
 
> @@ +126,5 @@
> > +        if not os.path.exists(os.path.join(dir, '.git')):
> > +            # git not installed? File permission problem? github not available?
> > +            self.build_obj.log(logging.ERROR, "autophone", {},
> > +                "Unable to clone autophone directory.")
> > +            if not verbose:
> 
> self.verbose ?

Yes - thanks!

> @@ +175,5 @@
> > +                "Connect your rooted Android test device(s) with usb and press Enter ")
> > +            adb_path = 'adb'
> > +            try:
> > +                if os.path.exists(self.substs["ADB"]):
> > +                    adb_path = self.substs["ADB"]
> 
> Where is self.substs determined? Isn't this self.build_obj.substs ?

Yes, self.build_obj.substs. (Too many global substitutions in that version of the patch!)

> @@ +176,5 @@
> > +            adb_path = 'adb'
> > +            try:
> > +                if os.path.exists(self.substs["ADB"]):
> > +                    adb_path = self.substs["ADB"]
> > +            except:
> 
> Perhaps if self.verbose is True, we could log the exception?

Agreed - done.
 
> @@ +200,5 @@
> > +                            keep_going = True
> > +                        else:
> > +                            self.build_obj.log(logging.WARNING, "autophone", {},
> > +                                "Device '%s' is not rooted - skipping" % serial)
> > +            except:
> 
> Perhaps if self.verbose is True, we could log the exception?

Yes. I added a couple more cases like this too.
 
> @@ +267,5 @@
> > +                            "'%s' unrecognized: Enter a number between 1 and %d!" % (response, highest))
> > +            self.autophone_options.extend(['--test-path', path])
> > +        else:
> > +            # Provide a simple backup for the unusual case where test manifests
> > +            # cannot be found.
> 
> When do you think this branch might be useful? Wouldn't this be an example
> of a condition where we would want this to return False?

I think the most likely scenario is that someone is running 'mach autophone' against a directory that they have cloned themselves, and that directory is out of date (prior to my @mach@ changes), or significantly modified. It's unusual/unlikely, but it's also easy enough to work around, so I'd prefer to offer this escape hatch rather than fail.

> @@ +433,5 @@
> > +        self.build_obj.log(logging.INFO, "autophone", {},
> > +            "Launching autophone...")
> > +        self.thread = threading.Thread(target=self.run_autophone)
> > +        self.thread.start()
> > +        # wait for startup
> 
> I'm a little concerned that if the start up takes longer than 20 seconds,
> the user might not be aware there is a problem.
> 
> Perhaps a flag that could be set when autophone-status succeeds and if it is
> not set when the 20 seconds completes, we output a message that Autophone is
> taking longer than expected to start.

Done.
 
> @@ +510,5 @@
> > +        while (int(response) < 1 or int(response) > highest):
> > +            response = raw_input(
> > +                "Build selection type? (1-%d) " % highest).strip()
> > +            try:
> > +                validate = int(response)
> 
> validate is unused, but could be re-used below instead of the repeated
> int(response) calls.
> 
> Elsewhere you have used choice for the name of a similar variable.

Thanks - I re-wrote this.

> @@ +540,5 @@
> > +            "Enter repo name: ").strip()
> > +        if len(repo) > 0:
> > +            options.extend(["--repo=%s" % repo])
> > +            if repo != "mozilla-central":
> > +                options.extend(["--build-location=tinderbox"])
> 
> This forces mozilla-central to be from
> http://ftp.mozilla.org/pub/mobile/nightly/ and does not allow the selection
> of mozilla-central builds from
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/.
> 
> If repo is mozilla-central or mozilla-aurora, we should prompt for either
> nightly or tinderbox locations.

Done.
 
> @@ +565,5 @@
> > +        class AutoHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
> > +            # A simple request handler with logging suppressed and rooted
> > +            # at the autophone directory.
> > +            dir = None
> > +            def translate_path(self, path):
> 
> Is all of this necessary now?

I think it is not necessary - simplified now.
Attachment #8734425 - Attachment is obsolete: true
Attachment #8734425 - Flags: feedback?(snorp)
Attachment #8735496 - Flags: review+
(In reply to Geoff Brown [:gbrown] from comment #45)
>  
> > @@ +565,5 @@
> > > +        class AutoHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
> > > +            # A simple request handler with logging suppressed and rooted
> > > +            # at the autophone directory.
> > > +            dir = None
> > > +            def translate_path(self, path):
> > 
> > Is all of this necessary now?
> 
> I think it is not necessary - simplified now.

Actually I meant all of the translate_path stuff, not just the ending slash. If we have Python >= 2.7.6 do we need the over-riddent translate_path at all?
translate_path normally serves everything relative to the process' current working directory. My version serves relative to the autophone directory. (I previously tried changing directory to the autophone directory and ran into some subtle trouble...can't quite remember now.)
...but it turns out changing directory is sufficient: now updated to remove translate_path.
Attachment #8735496 - Attachment is obsolete: true
Attachment #8735578 - Flags: review+
Blocks: 1260547
No longer depends on: 1078645
https://hg.mozilla.org/mozilla-central/rev/91dd7f2ddda6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.