Merge lp:~a-j-buxton/phablet-tools/download-only into lp:phablet-tools

[Alistair Buxton]

Adds an option to only download image files and skip device flashing.

Adds an option to only download image files and skip device flashing. If device is connected it will be autodetected as normal, but free space check is not done. If no device is connected, -d can be used to select a target device as normal.

[Ricardo Salveti (rsalveti)]

24 + '--download',

Would you mind using --download-only instead? This is just to make it easier for people when only reading the arg list.

37 - device = validate_device(adb, args.device)
38 + if args.download:
39 + device = detect_device(adb, args.device)
40 + else:
41 + device = validate_device(adb, args.device)

I'd suggest to always call detect_device and remove it from validate_device. Also move validate_device a bit bellow in the code, to call it only in case we're actually flashing the device (bellow line 59, for example).

review: Needs Fixing

[Alistair Buxton (a-j-buxton)]

I would agree with all of that. I also don't like how detect and validate exit(-1). They should raise an exception or something instead, then the caller can provide context sensitive help.

[Ricardo Salveti (rsalveti)]

> I would agree with all of that. I also don't like how detect and validate
> exit(-1). They should raise an exception or something instead, then the caller
> can provide context sensitive help.

+1, we can improve that at another MR if you prefer.

[lp:~a-j-buxton/phablet-tools/download-only]

65. By Alistair Buxton on 2013-03-11

Download only v2:
 * Rename option to '--download-only'.
 * Remove some unneccessary logic.
 * Split detection and validation.
 * Add missing decorator to validate_device.

[Ricardo Salveti (rsalveti)]

71 + device = validate_device(adb, device)

We already know the device as we're calling detect_device for all use cases, so just call validate_device without checking for the returned value.

Also, please remove the "return device" from the validate_device function.

review: Needs Fixing

[lp:~a-j-buxton/phablet-tools/download-only]

66. By Alistair Buxton on 2013-03-11

Don't return device from validate_device as it cannot modify it.

67. By Alistair Buxton on 2013-03-11

Really get rid of device parameter this time.

68. By Alistair Buxton on 2013-03-11

Remove it from the method call too.

[Ricardo Salveti (rsalveti)]

Looks good, thanks!

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:68
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~a-j-buxton/phablet-tools/download-only/+merge/152599/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/phablet-tools-ci/14/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-tools-ci/./build=pbuilder,distribution=quantal,flavor=i386/14/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/phablet-tools-ci/14//rebuild/?

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:68
http://jenkins.qa.ubuntu.com/job/phablet-tools-ci/15/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-tools-ci/./build=pbuilder,distribution=quantal,flavor=i386/15/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/phablet-tools-ci/15//rebuild/?

review: Approve (continuous-integration)

=== modified file 'phablet-flash'
--- phablet-flash	2013-02-26 16:04:10 +0000
+++ phablet-flash	2013-03-11 20:40:26 +0000
@@ -49,13 +49,21 @@
                         help='''Device serial. Use when more than
                                 one device is connected.''',
                        )
-    parser.add_argument('-b',
-                        '--bootstrap',
-                        help='''Bootstrap the target device, this only
-                                works on Nexus devices or devices that
-                                use fastboot and are unlocked.''',
-                        action='store_true',
-                       )
+    group = parser.add_mutually_exclusive_group()
+    group.add_argument('-b',
+                       '--bootstrap',
+                       help='''Bootstrap the target device, this only
+                               works on Nexus devices or devices that
+                               use fastboot and are unlocked.''',
+                       action='store_true',
+                      )
+    group.add_argument('-D',
+                       '--download-only',
+                       help='''Download image only, but do not flash device.
+                               Use -d to override target device autodetection
+                               if target device is not connected.''',
+                       action='store_true',
+                      )
     parser.add_argument('-r',
                         '--revision',
                         required=False,
@@ -266,12 +274,11 @@
     return jenkins_data
 
 
-def validate_device(adb, device_param):
+@adb_errors
+def validate_device(adb):
     '''
     Validates if the image would be installable on the target
-    returns device
     '''
-    device = detect_device(adb, device_param)
     df = adb.shell('df').split('\n')
     try:
         free_data = map(str.split, 
@@ -284,7 +291,6 @@
     else:
         log.error('Not enough space in /data, found %s', free_data)
         exit(1)
-    return device
 
 
 def main(args):
@@ -295,7 +301,7 @@
         adb = AndroidBridge(args.serial)
     else:
         adb = AndroidBridge()
-    device = validate_device(adb, args.device)
+    device = detect_device(adb, args.device)
     # Determine uri to download from
     if not args.uri:
         args.uri = settings.download_uri
@@ -333,16 +339,18 @@
     except subprocess.CalledProcessError:
         log.error('Error while downloading, ensure connection')
         exit(1)
-    if args.bootstrap:
-        push_for_autodeploy(adb, download_mgr.files[settings.ubuntu_image])
-        bootstrap(adb, download_mgr.files[recovery_img],
-                  download_mgr.files[device_img],
-                  download_mgr.files[boot_img],)
-    else:
-        recovery_file = create_recovery_file(adb,
-            download_mgr.files[device_img],
-            download_mgr.files[settings.ubuntu_image])
-        deploy_recovery_image(adb, download_mgr.files, recovery_file)
+    if not args.download_only:
+        validate_device(adb)
+        if args.bootstrap:
+            push_for_autodeploy(adb, download_mgr.files[settings.ubuntu_image])
+            bootstrap(adb, download_mgr.files[recovery_img],
+                      download_mgr.files[device_img],
+                      download_mgr.files[boot_img],)
+        else:
+            recovery_file = create_recovery_file(adb,
+                download_mgr.files[device_img],
+                download_mgr.files[settings.ubuntu_image])
+            deploy_recovery_image(adb, download_mgr.files, recovery_file)
 
 
 if __name__ == "__main__":