Merge lp:~zyga/linaro-image-tools/hacking into lp:linaro-image-tools/11.11

Proposed by Zygmunt Krynicki
Status: Rejected
Rejected by: Loïc Minier
Proposed branch: lp:~zyga/linaro-image-tools/hacking
Merge into: lp:linaro-image-tools/11.11
Diff against target: 77 lines (+15/-8)
2 files modified
linaro_media_create/check_device.py (+9/-6)
linaro_media_create/partitions.py (+6/-2)
To merge this branch: bzr merge lp:~zyga/linaro-image-tools/hacking
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) Needs Fixing
Review via email: mp+49615@code.launchpad.net

Description of the change

Display the selected device more clearly

To post a comment you must log in.
Revision history for this message
Guilherme Salgado (salgado) wrote :

Hi Zygmunt,

Thanks for your change, but I think what we really want here is to list
the selected device and all its partitions at the top of the list or
maybe even on a separate table, with proper headings instead of just "I
see...".

However, I think your changes can be an improvement, and it should be OK
to land just them if you don't have the time to do (or don't feel like
doing) the other changes. I have just a few suggestions.

First, the length of the new column is a bit too small so it feels like
instead of having two columns (Selected, and Device), we have just one
(Selected Device).

 review needsfixing

On Mon, 2011-02-14 at 12:10 +0000, Zygmunt Krynicki wrote:
[...]
> === modified file 'linaro_media_create/check_device.py'
> --- linaro_media_create/check_device.py 2011-01-28 19:50:48 +0000
> +++ linaro_media_create/check_device.py 2011-02-14 12:09:46 +0000
> @@ -65,10 +65,10 @@
> return True
>
>
> -def _print_devices():
> +def _print_devices(hilight_device=None):
> """Print disk devices found on the system."""
> bus, udisks = _get_system_bus_and_udisks_iface()
> - print '%-16s %-16s %s' % ('Device', 'Mount point', 'Size')
> + print '%-8s %-16s %-16s %s' % ('Selected', 'Device', 'Mount point', 'Size')
> devices = udisks.get_dbus_method('EnumerateDevices')()
> for path in devices:
> device = bus.get_object("org.freedesktop.UDisks", path)
> @@ -81,11 +81,13 @@
>
> if _get_dbus_property('DeviceIsPartition', device, path):
> part_size = _get_dbus_property('partition-size', device, path)
> - print '%-16s %-16s %dMB' % (
> + print '%-8s %-16s %-16s %dMB' % (
> + "------->" if hilight_device == device_file else "",

This is never going to print the marker as we're dealing with partitions
here ('DeviceIsPartition').

I'd also prefer if you used regular if/else blocks instead of the short
form here. Maybe you can even move it before the "if
_get_dbus_property()" block so that we avoid duplicating it in the else
block below?

> device_file, mount_point, part_size / 1024**2)
> else:
> device_size = _get_dbus_property('device-size', device, path)
> - print '%-16s %-16s %dMB' % (
> + print '%-8s %-16s %-16s %dMB' % (
> + "------->" if hilight_device == device_file else "",
> device_file, mount_point, device_size / 1024**2)
>
>
> @@ -121,7 +123,7 @@
> """
> if _does_device_exist(device):
> print '\nI see...'
> - _print_devices()
> + _print_devices(hilight_device=device)
> if _select_device(device):
> _ensure_device_partitions_not_mounted(device)
> return True
>

--
Guilherme Salgado <https://launchpad.net/~salgado>

review: Needs Fixing
lp:~zyga/linaro-image-tools/hacking updated
288. By Loïc Minier

Merge lp:~lool/linaro-image-tools/4-mib-align; this reworks the way we create
partitions and will align the rootfs partition on 4 MiB boundaries by default.
It adds an --align-boot-part linaro-media-create flag to also align the boot
partition in case your x-loader is recent enough to support this.

289. By Loïc Minier

Merge lp:~doanac/linaro-image-tools/overo-video; enable Overo on 1024x768.

290. By Loïc Minier

Merge lp:~lool/linaro-image-tools/efikamx; adds EfikaMX support.

291. By Guilherme Salgado

l-m-c now stores the original boot script (before we feed it to mkimage) in the boot partition as boot.txt

292. By Guilherme Salgado

Saving of the plain boot script (boot.txt) on the boot partition was failing with a permission error; this fixes it.

Revision history for this message
Loïc Minier (lool) wrote :

Instead of merging sleep()s, I'd rather we find out what step does not complete synchronously and add a polling loop to test whether it has completed afterwards.

lp:~zyga/linaro-image-tools/hacking updated
293. By Loïc Minier

Merge lp:~linaro-landing-team-freescale/linaro-image-tools/mx53-loco; adds
support for i.MX53 LOCO board.

294. By Loïc Minier

Merge lp:~lool/linaro-image-tools/share-omap; set kernel_suffix = 'linaro-omap'
in OmapConfig instead of children classes.

295. By James Westby

Add support for the smdkv310. Thanks Angus.

296. By Guilherme Salgado

Change cmd_runner.run() to always pass -E to sudo

298. By Zygmunt Krynicki

Increase the delay after sync to 3 seconds.

This fixes building images on my system due to race condition between udisks and sfdisk

299. By Zygmunt Krynicki

Add sleep after partitioning the medium

300. By Zygmunt Krynicki

Add a hack that makes it possible to run batch jobs

Unmerged revisions

300. By Zygmunt Krynicki

Add a hack that makes it possible to run batch jobs

299. By Zygmunt Krynicki

Add sleep after partitioning the medium

298. By Zygmunt Krynicki

Increase the delay after sync to 3 seconds.

This fixes building images on my system due to race condition between udisks and sfdisk

297. By Zygmunt Krynicki

Hilight selected device so that it's easier to read the partition/device list

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'linaro_media_create/check_device.py'
--- linaro_media_create/check_device.py 2011-01-28 19:50:48 +0000
+++ linaro_media_create/check_device.py 2011-03-10 12:13:49 +0000
@@ -65,10 +65,10 @@
65 return True65 return True
6666
6767
68def _print_devices():68def _print_devices(hilight_device=None):
69 """Print disk devices found on the system."""69 """Print disk devices found on the system."""
70 bus, udisks = _get_system_bus_and_udisks_iface()70 bus, udisks = _get_system_bus_and_udisks_iface()
71 print '%-16s %-16s %s' % ('Device', 'Mount point', 'Size')71 print '%-8s %-16s %-16s %s' % ('Selected', 'Device', 'Mount point', 'Size')
72 devices = udisks.get_dbus_method('EnumerateDevices')()72 devices = udisks.get_dbus_method('EnumerateDevices')()
73 for path in devices:73 for path in devices:
74 device = bus.get_object("org.freedesktop.UDisks", path)74 device = bus.get_object("org.freedesktop.UDisks", path)
@@ -81,11 +81,13 @@
81 81
82 if _get_dbus_property('DeviceIsPartition', device, path):82 if _get_dbus_property('DeviceIsPartition', device, path):
83 part_size = _get_dbus_property('partition-size', device, path)83 part_size = _get_dbus_property('partition-size', device, path)
84 print '%-16s %-16s %dMB' % (84 print '%-8s %-16s %-16s %dMB' % (
85 "------->" if hilight_device == device_file else "",
85 device_file, mount_point, part_size / 1024**2)86 device_file, mount_point, part_size / 1024**2)
86 else:87 else:
87 device_size = _get_dbus_property('device-size', device, path)88 device_size = _get_dbus_property('device-size', device, path)
88 print '%-16s %-16s %dMB' % (89 print '%-8s %-16s %-16s %dMB' % (
90 "------->" if hilight_device == device_file else "",
89 device_file, mount_point, device_size / 1024**2)91 device_file, mount_point, device_size / 1024**2)
9092
9193
@@ -121,8 +123,9 @@
121 """123 """
122 if _does_device_exist(device):124 if _does_device_exist(device):
123 print '\nI see...'125 print '\nI see...'
124 _print_devices()126 _print_devices(hilight_device=device)
125 if _select_device(device):127 import os
128 if os.getenv("LINARO_MEDIA_CREATE_NOT_INTERACTIVE") == "yes" or _select_device(device):
126 _ensure_device_partitions_not_mounted(device)129 _ensure_device_partitions_not_mounted(device)
127 return True130 return True
128 else:131 else:
129132
=== modified file 'linaro_media_create/partitions.py'
--- linaro_media_create/partitions.py 2011-02-28 19:40:43 +0000
+++ linaro_media_create/partitions.py 2011-03-10 12:13:49 +0000
@@ -307,7 +307,7 @@
307307
308308
309def create_partitions(board_config, media, heads, sectors, cylinders=None,309def create_partitions(board_config, media, heads, sectors, cylinders=None,
310 should_align_boot_part=False):310 should_align_boot_part=False, sleep_duration=5):
311 """Partition the given media according to the board requirements.311 """Partition the given media according to the board requirements.
312312
313 :param board_config: A BoardConfig class.313 :param board_config: A BoardConfig class.
@@ -325,6 +325,8 @@
325 proc = cmd_runner.run(325 proc = cmd_runner.run(
326 ['parted', '-s', media.path, 'mklabel', 'msdos'], as_root=True)326 ['parted', '-s', media.path, 'mklabel', 'msdos'], as_root=True)
327 proc.wait()327 proc.wait()
328 print "Waiting for %d seconds after wiping partition table" % sleep_duration
329 time.sleep(sleep_duration)
328330
329 sfdisk_cmd = board_config.get_sfdisk_cmd(331 sfdisk_cmd = board_config.get_sfdisk_cmd(
330 should_align_boot_part=should_align_boot_part)332 should_align_boot_part=should_align_boot_part)
@@ -336,7 +338,9 @@
336 # errors because the disk is not partitioned then we should revisit this.338 # errors because the disk is not partitioned then we should revisit this.
337 # XXX: This sleep can probably die now; need to do more tests before doing339 # XXX: This sleep can probably die now; need to do more tests before doing
338 # so, though.340 # so, though.
339 time.sleep(1)341
342 print "Waiting for %d seconds after partitioning" % sleep_duration
343 time.sleep(sleep_duration)
340344
341345
342class Media(object):346class Media(object):

Subscribers

People subscribed via source and target branches