Final set of VM scripts - integrated with changes from Zelidrag to create a test image if specified.

Final set of VM scripts - integrated with changes from Zelidrag to create a test image if specified.
Integrated comments from http://codereview.chromium.org/2604001/show
[the git repository that commit was on was deleted and replaced, hence this new commit]

[ericli, 2010-06-21 20:38:19]

http://codereview.chromium.org/2803015/diff/1/2
File fixup_image_for_qemu.py (right):

http://codereview.chromium.org/2803015/diff/1/2#newcode26
fixup_image_for_qemu.py:26: import os.path
usually in python, import os and call os.path.funct() inside code.

http://codereview.chromium.org/2803015/diff/1/2#newcode44
fixup_image_for_qemu.py:44: MOUSE_SECTION_IDENTIFIERS += ['Identifier "Mouse']
do you want to add an ending double quote after Mouse or this is intentional?

http://codereview.chromium.org/2803015/diff/1/2#newcode81
fixup_image_for_qemu.py:81: for ident in MOUSE_SECTION_IDENTIFIERS:
this section is very hard to understand. And its fragile to the original xorg
conf file.
Why not generate a patch file, source control it and just apply a patch here?

http://codereview.chromium.org/2803015/diff/1/3
File image_to_vm.sh (right):

http://codereview.chromium.org/2803015/diff/1/3#newcode99
image_to_vm.sh:99: # Check for instqall shim.
typerr: instqall?

http://codereview.chromium.org/2803015/diff/1/3#newcode124
image_to_vm.sh:124: FLAGS_make_vmx=${FLAGS_FALSE}
quote

http://codereview.chromium.org/2803015/diff/1/3#newcode146
image_to_vm.sh:146: dd if="${FLAGS_from}/chromiumos_image.bin" of="${TEMP_PMBR}"
bs=512 count=1
what happen if you work on chromiumos_test_image.bin here?

http://codereview.chromium.org/2803015/diff/1/3#newcode163
image_to_vm.sh:163: sudo "${TEMP_MNT}"/postinst /dev/sda3
not sure the hard code of /dev/sda3 is a good idea here?

postinst script was modified inside fixup, could you make a copy of it inside
fixup and keep postinst unchanged?

http://codereview.chromium.org/2803015/diff/1/3#newcode227
image_to_vm.sh:227: if [[ ${FLAGS_make_vmx} = ${FLAGS_TRUE} ]]; then
quote the vars.

[rkc1, 2010-06-21 21:05:05]

Incorporated review changes.

http://codereview.chromium.org/2803015/diff/1/2
File fixup_image_for_qemu.py (right):

http://codereview.chromium.org/2803015/diff/1/2#newcode26
fixup_image_for_qemu.py:26: import os.path
On 2010/06/21 20:38:19, ericli wrote:
> usually in python, import os and call os.path.funct() inside code.

Done.

http://codereview.chromium.org/2803015/diff/1/2#newcode44
fixup_image_for_qemu.py:44: MOUSE_SECTION_IDENTIFIERS += ['Identifier "Mouse']
On 2010/06/21 20:38:19, ericli wrote:
> do you want to add an ending double quote after Mouse or this is intentional?

This is intentional - We want to be able to catch either Mouse, Mouse0, Mouse1,
etc.

http://codereview.chromium.org/2803015/diff/1/2#newcode81
fixup_image_for_qemu.py:81: for ident in MOUSE_SECTION_IDENTIFIERS:
On 2010/06/21 20:38:19, ericli wrote:
> this section is very hard to understand. And its fragile to the original xorg
> conf file.
> Why not generate a patch file, source control it and just apply a patch here?

I considered other approaches but the thing is, we actually want to remove all
existing mouse section identifiers (it doesn't really matter what they contain)
and add just one of our own. The original xorg.conf is required to follow the
sectionmarker/endsection format - otherwise it'll be an incorrect xorg.conf to
start with. So this seemed to be the most correct way.

http://codereview.chromium.org/2803015/diff/1/3
File image_to_vm.sh (right):

http://codereview.chromium.org/2803015/diff/1/3#newcode99
image_to_vm.sh:99: # Check for instqall shim.
On 2010/06/21 20:38:19, ericli wrote:
> typerr: instqall?

Done.

http://codereview.chromium.org/2803015/diff/1/3#newcode124
image_to_vm.sh:124: FLAGS_make_vmx=${FLAGS_FALSE}
On 2010/06/21 20:38:19, ericli wrote:
> quote

Done.

http://codereview.chromium.org/2803015/diff/1/3#newcode146
image_to_vm.sh:146: dd if="${FLAGS_from}/chromiumos_image.bin" of="${TEMP_PMBR}"
bs=512 count=1
On 2010/06/21 20:38:19, ericli wrote:
> what happen if you work on chromiumos_test_image.bin here?
Whoops! Fixed here and on 135.
Done.

http://codereview.chromium.org/2803015/diff/1/3#newcode163
image_to_vm.sh:163: sudo "${TEMP_MNT}"/postinst /dev/sda3
On 2010/06/21 20:38:19, ericli wrote:
> not sure the hard code of /dev/sda3 is a good idea here?
This code is fairly hard coded. If we were to fix the /sda3, we should also not
use the part_xxx numbers directly. Currently I am not aware of a better way.

> 
> postinst script was modified inside fixup, could you make a copy of it inside
> fixup and keep postinst unchanged?
postinst isn't ever called from anywhere else

http://codereview.chromium.org/2803015/diff/1/3#newcode227
image_to_vm.sh:227: if [[ ${FLAGS_make_vmx} = ${FLAGS_TRUE} ]]; then
On 2010/06/21 20:38:19, ericli wrote:
> quote the vars.

Done.

[ericli, 2010-06-21 21:24:40]

http://codereview.chromium.org/2803015/diff/1/3
File image_to_vm.sh (right):

http://codereview.chromium.org/2803015/diff/1/3#newcode163
image_to_vm.sh:163: sudo "${TEMP_MNT}"/postinst /dev/sda3
I am thinking about postinst will be used anywhere else, e.g. running live
update after this, but now post inst seems been contaminated by fixup.

My question is:
if every thing still working after image_to_vm?
Can I still run image_to_usb or image_to_live? since you changed postinst and
chromium_test_image.bin.


On 2010/06/21 21:05:06, rkc wrote:
> On 2010/06/21 20:38:19, ericli wrote:
> > not sure the hard code of /dev/sda3 is a good idea here?
> This code is fairly hard coded. If we were to fix the /sda3, we should also
not
> use the part_xxx numbers directly. Currently I am not aware of a better way.
> 
> > 
> > postinst script was modified inside fixup, could you make a copy of it
inside
> > fixup and keep postinst unchanged?
> postinst isn't ever called from anywhere else
>

http://codereview.chromium.org/2803015/diff/1/3#newcode163
image_to_vm.sh:163: sudo "${TEMP_MNT}"/postinst /dev/sda3
move /dev/sda3 into a FLAG, and make it as the default value.

Document how to change it if needed.


On 2010/06/21 21:05:06, rkc wrote:
> On 2010/06/21 20:38:19, ericli wrote:
> > not sure the hard code of /dev/sda3 is a good idea here?
> This code is fairly hard coded. If we were to fix the /sda3, we should also
not
> use the part_xxx numbers directly. Currently I am not aware of a better way.
> 
> > 
> > postinst script was modified inside fixup, could you make a copy of it
inside
> > fixup and keep postinst unchanged?
> postinst isn't ever called from anywhere else
>

[rkc1, 2010-06-22 12:08:40]

http://codereview.chromium.org/2803015/diff/1/3
File image_to_vm.sh (right):

http://codereview.chromium.org/2803015/diff/1/3#newcode163
image_to_vm.sh:163: sudo "${TEMP_MNT}"/postinst /dev/sda3
On 2010/06/21 21:24:41, ericli wrote:
> I am thinking about postinst will be used anywhere else, e.g. running live
> update after this, but now post inst seems been contaminated by fixup.
> 
> My question is:
> if every thing still working after image_to_vm?
> Can I still run image_to_usb or image_to_live? since you changed postinst and
> chromium_test_image.bin.
> 
> 
> On 2010/06/21 21:05:06, rkc wrote:
> > On 2010/06/21 20:38:19, ericli wrote:
> > > not sure the hard code of /dev/sda3 is a good idea here?
> > This code is fairly hard coded. If we were to fix the /sda3, we should also
> not
> > use the part_xxx numbers directly. Currently I am not aware of a better way.
> > 
> > > 
> > > postinst script was modified inside fixup, could you make a copy of it
> inside
> > > fixup and keep postinst unchanged?
> > postinst isn't ever called from anywhere else
> > 
> 
> 

So even if run from a live install; on a VM, the EFI processing is still
ignored.

Either way, I've created a copy, just in case.

Done.

http://codereview.chromium.org/2803015/diff/1/3#newcode163
image_to_vm.sh:163: sudo "${TEMP_MNT}"/postinst /dev/sda3
On 2010/06/21 21:24:41, ericli wrote:
> move /dev/sda3 into a FLAG, and make it as the default value.
> 
> Document how to change it if needed.
> 
> 
> On 2010/06/21 21:05:06, rkc wrote:
> > On 2010/06/21 20:38:19, ericli wrote:
> > > not sure the hard code of /dev/sda3 is a good idea here?
> > This code is fairly hard coded. If we were to fix the /sda3, we should also
> not
> > use the part_xxx numbers directly. Currently I am not aware of a better way.
> > 
> > > 
> > > postinst script was modified inside fixup, could you make a copy of it
> inside
> > > fixup and keep postinst unchanged?
> > postinst isn't ever called from anywhere else
> > 
> 
> 

Done.

[ericli, 2010-06-22 16:26:11]

http://codereview.chromium.org/2803015/diff/13001/14002
File image_to_vm.sh (right):

http://codereview.chromium.org/2803015/diff/13001/14002#newcode166
image_to_vm.sh:166: sudo "${TEMP_MNT}"/postinst_vm "${ROOTFS_PARTITION}"
I really like to see this to be a FLAG.

[sosa, 2010-06-22 16:58:08]

http://codereview.chromium.org/2803015/diff/6001/7002
File image_to_vm.sh (right):

http://codereview.chromium.org/2803015/diff/6001/7002#newcode3
image_to_vm.sh:3: # Copyright (c) 2009 The Chromium OS Authors. All rights
reserved.
2010

http://codereview.chromium.org/2803015/diff/6001/7002#newcode25
image_to_vm.sh:25: DEFINE_string board "${DEFAULT_BOARD}" \
Alphabetize Flags

http://codereview.chromium.org/2803015/diff/6001/7002#newcode35
image_to_vm.sh:35: 
Why are there so many categories?  Can you add comments when separating flags. 
Either that or put no spaces and alphabetize all.  Also if you do leave as
sections alphabetize per section.

http://codereview.chromium.org/2803015/diff/6001/7002#newcode69
image_to_vm.sh:69: error "--board is required."
Use die instead of error.

http://codereview.chromium.org/2803015/diff/6001/7002#newcode76
image_to_vm.sh:76: FLAGS_from="${IMAGES_DIR}/`ls -t $IMAGES_DIR | head -1`"
Should use $() instead of ``

http://codereview.chromium.org/2803015/diff/6001/7002#newcode177
image_to_vm.sh:177: dd if="${TEMP_ROOTFS}" of="${TEMP_IMG}" conv=notrunc bs=512
\
Why 512?  4M is much more efficient for all

http://codereview.chromium.org/2803015/diff/6001/7002#newcode198
image_to_vm.sh:198: echo invalid format: "${FLAGS_format}"
Use die from common.sh

http://codereview.chromium.org/2803015/diff/6001/7002#newcode209
image_to_vm.sh:209: # Generate the vmware config file
Can this be in a separate file?

[rkc1, 2010-06-23 16:18:37]

Review changes.

http://codereview.chromium.org/2803015/diff/6001/7002
File image_to_vm.sh (right):

http://codereview.chromium.org/2803015/diff/6001/7002#newcode3
image_to_vm.sh:3: # Copyright (c) 2009 The Chromium OS Authors. All rights
reserved.
On 2010/06/22 16:58:08, sosa wrote:
> 2010

Done.

http://codereview.chromium.org/2803015/diff/6001/7002#newcode25
image_to_vm.sh:25: DEFINE_string board "${DEFAULT_BOARD}" \
On 2010/06/22 16:58:08, sosa wrote:
> Alphabetize Flags

Done.

http://codereview.chromium.org/2803015/diff/6001/7002#newcode35
image_to_vm.sh:35: 
On 2010/06/22 16:58:08, sosa wrote:
> Why are there so many categories?  Can you add comments when separating flags.

> Either that or put no spaces and alphabetize all.  Also if you do leave as
> sections alphabetize per section.

Done.

http://codereview.chromium.org/2803015/diff/6001/7002#newcode69
image_to_vm.sh:69: error "--board is required."
On 2010/06/22 16:58:08, sosa wrote:
> Use die instead of error.

Done.

http://codereview.chromium.org/2803015/diff/6001/7002#newcode76
image_to_vm.sh:76: FLAGS_from="${IMAGES_DIR}/`ls -t $IMAGES_DIR | head -1`"
On 2010/06/22 16:58:08, sosa wrote:
> Should use $() instead of ``

Done.

http://codereview.chromium.org/2803015/diff/6001/7002#newcode177
image_to_vm.sh:177: dd if="${TEMP_ROOTFS}" of="${TEMP_IMG}" conv=notrunc bs=512
\
On 2010/06/22 16:58:08, sosa wrote:
> Why 512?  4M is much more efficient for all 

Not sure; the pack/unpack scripts all use 512 and using 4M causes dd to throw an
error (cannot seek, invalid parameter).

http://codereview.chromium.org/2803015/diff/6001/7002#newcode198
image_to_vm.sh:198: echo invalid format: "${FLAGS_format}"
On 2010/06/22 16:58:08, sosa wrote:
> Use die from common.sh

Done.

http://codereview.chromium.org/2803015/diff/6001/7002#newcode209
image_to_vm.sh:209: # Generate the vmware config file
On 2010/06/22 16:58:08, sosa wrote:
> Can this be in a separate file?

If this section gets larger, we can move it to a separate file; currently the
code is kinda small to dedicate a separate script to it - plus I doubt if it'll
grow much.

[deshantm, 2010-06-24 04:21:22]

execute bit needs to be set on image_to_vm.sh

[rkc1, 2010-06-28 12:36:59]

On 2010/06/24 04:21:22, deshantm wrote:
> execute bit needs to be set on image_to_vm.sh
It's been set on the latest patch.

[zel, 2010-07-01 20:05:35]

LGTM