build_image: remove --dev_install flag

build_image: remove --dev_install flag

The dev install shim is deprecated. This CL removes --dev_install
support from build_image.

This is part of a larger rewrite of build_image to allow it to be the
single script from which all image types are generated.

BUG=chromium-os:12899
TEST=manual

Change-Id: Ic3a49f0d476d198f81001a05c75f24f16bc640c0

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=96d116d

[Vince Laviano, 2011-03-15 21:12:22]

This CL stems from an email discussion with wad@ and jrbarnette@ with the
subject "What's the status of the dev install shim?"  The key quote from Will is
"we can kill the dev shim idea in any sense of the name."  So, that's what this
CL does.

I'm particularly interested in feedback regarding whether any sort of PSA or
documentation update is required here.  I didn't find any references to the dev
install shim in our new Chromium OS Developer Guide.

[gauravsh, 2011-03-15 21:23:44]

I landed a change yesterday that removes dev shim support from the initramfs. I
don't believe anyone is using it, and the developer instructions for getting
their own image on the Cr-48 doesn't use that mode.

I don't believe a PSA is required. If you hear otherwise, let me know.

[Will Drewry, 2011-03-15 22:00:06]

Just to second that - no PSA needed.  This flag hasn't done anything
anyone could use for at least 4-5 months.

[anush, 2011-03-15 22:21:26]

LGTM

On Tue, Mar 15, 2011 at 2:12 PM, <vlaviano@chromium.org> wrote:

> Reviewers: anush, jrbarnette, Will Drewry,
>
> Message:
> This CL stems from an email discussion with wad@ and jrbarnette@ with the
> subject "What's the status of the dev install shim?"  The key quote from
> Will is
> "we can kill the dev shim idea in any sense of the name."  So, that's what
> this
> CL does.
>
> I'm particularly interested in feedback regarding whether any sort of PSA
> or
> documentation update is required here.  I didn't find any references to the
> dev
> install shim in our new Chromium OS Developer Guide.
>
> Description:
> build_image: remove --dev_install flag
>
> The dev install shim is deprecated. This CL removes --dev_install
> support from build_image.
>
> This is part of a larger rewrite of build_image to allow it to be the
> single script from which all image types are generated.
>
> BUG=chromium-os:12899
> TEST=manual
>
> Change-Id: Ic3a49f0d476d198f81001a05c75f24f16bc640c0
>
>
> Please review this at http://codereview.chromium.org/6694042/
>
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
>
> Affected files:
>  M build_image
>
>
> Index: build_image
> diff --git a/build_image b/build_image
> index
>
2f9a7e129997bf4146368b998ecdf2f101820101..6bb09987d61bad034fbb6f524d1c539193a6d617
> 100755
> --- a/build_image
> +++ b/build_image
> @@ -70,8 +70,6 @@ DEFINE_string to "" \
>  DEFINE_boolean factory_install ${FLAGS_FALSE} \
>   "Build a smaller image to overlay the factory install shim on; this
> argument \
>  is also required in image_to_usb."
> -DEFINE_boolean dev_install ${FLAGS_FALSE} \
> -  "Build a smaller image to overlay the dev recovery install shim on"
>  DEFINE_integer rootfs_partition_size 1024 \
>   "rootfs partition size in MiBs."
>  DEFINE_integer rootfs_size 850 \
> @@ -144,12 +142,7 @@ check_blacklist
>  # transition dust settles.
>  "${SCRIPTS_DIR}"/set_shared_user_password.sh --move_to_etc
>
> -# Verify user didn't specify incompatible flags for dev install shim
> -if [ "${FLAGS_factory_install}" -eq "${FLAGS_TRUE}" ] &&
> -   [ "${FLAGS_dev_install}" -eq "${FLAGS_TRUE}" ] ; then
> -  die "Incompatible flags: --factory_install and --dev_install cannot be \
> -both set to True. Please specify one or none."
> -fi
> +# TODO(vlaviano): validate flags
>
>  INSTALL_MASK=""
>  if [ "${FLAGS_installmask}" -eq "${FLAGS_TRUE}" ] ; then
> @@ -157,14 +150,13 @@ if [ "${FLAGS_installmask}" -eq "${FLAGS_TRUE}" ] ;
> then
>  fi
>
>  # Reduce the size of factory install shim.
> -if [ "${FLAGS_factory_install}" -eq "${FLAGS_TRUE}" ] ||
> -   [ "${FLAGS_dev_install}" -eq "${FLAGS_TRUE}" ] ; then
> -  # Disable --withdev flag when --*_install is set to True. Otherwise, the
> +if [ "${FLAGS_factory_install}" -eq "${FLAGS_TRUE}" ]; then
> +  # Disable --withdev flag when --factory_install is set to True.
> Otherwise, the
>   # dev image produced will be based on install shim, rather than a
> pristine
>   # image
>   if [ "${FLAGS_withdev}" -eq "${FLAGS_TRUE}" ]; then
> -    info "Incompatible flags: --withdev and --dev_install or
> --factory_install \
> -cannot be both set to True. Reset --withdev to False."
> +    info "Incompatible flags: --withdev and --factory_install cannot both
> be \
> +set to True. Resetting --withdev to False."
>     FLAGS_withdev=${FLAGS_FALSE}
>   fi
>
> @@ -228,9 +220,7 @@ PRISTINE_IMAGE_NAME=chromiumos_image.bin
>  if [ "${FLAGS_withdev}" -eq "${FLAGS_TRUE}" ]; then
>   PRISTINE_IMAGE_NAME=chromiumos_base_image.bin
>   DEVELOPER_IMAGE_NAME=chromiumos_image.bin
> -elif [ "${FLAGS_dev_install}" -eq "${FLAGS_TRUE}" ]; then
> -# Rename pristine images for install shims
> -  PRISTINE_IMAGE_NAME=dev_install_shim.bin
> +# Rename pristine image for factory install shim
>  elif [ "${FLAGS_factory_install}" -eq "${FLAGS_TRUE}" ]; then
>   PRISTINE_IMAGE_NAME=factory_install_shim.bin
>  fi
> @@ -487,10 +477,9 @@ update_dev_packages() {
>   fi
>
>   # Check that the image has been correctly created.  Only do it if not
> -  # building a factory install image and not a dev install shim, as the
> -  # INSTALL_MASK for it will make test_image fail.
> -  if [ ${FLAGS_factory_install} -eq ${FLAGS_FALSE} ] &&
> -     [ ${FLAGS_dev_install} -eq ${FLAGS_FALSE} ] ; then
> +  # building a factory install shim, as the INSTALL_MASK for it will make
> +  # test_image fail.
> +  if [ ${FLAGS_factory_install} -eq ${FLAGS_FALSE} ]; then
>     "${SCRIPTS_DIR}/test_image" \
>       --root="${ROOT_FS_DIR}" \
>       --target="${ARCH}"
> @@ -671,9 +660,8 @@ create_base_image() {
>     --install \
>     ${enable_rootfs_verification}
>
> -  # Don't test the factory install shim or the dev install shim
> -  if [ ${FLAGS_factory_install} -eq ${FLAGS_FALSE} ] &&
> -     [ ${FLAGS_dev_install} -eq ${FLAGS_FALSE} ]; then
> +  # Don't test the factory install shim
> +  if [ ${FLAGS_factory_install} -eq ${FLAGS_FALSE} ]; then
>     # Check that the image has been correctly created.
>     "${SCRIPTS_DIR}/test_image" \
>       --root="${ROOT_FS_DIR}" \
> @@ -762,8 +750,7 @@ else
>  fi
>
>  USE_DEV_KEYS=
> -if [ "${FLAGS_dev_install}" -eq "${FLAGS_TRUE}" ] || \
> -   [ "${FLAGS_factory_install}" -eq "${FLAGS_TRUE}" ]; then
> +if [ "${FLAGS_factory_install}" -eq "${FLAGS_TRUE}" ]; then
>   USE_DEV_KEYS="--use_dev_keys"
>  fi
>
> @@ -779,10 +766,10 @@ if [[ "${ARCH}" = "x86" ]] ||
>    [[ "${ARCH}" = "arm" &&
>       "${FLAGS_crosbug12352_arm_kernel_signing}" -eq "${FLAGS_TRUE}" ]];
> then
>   BOOT_FLAG=
> -  if [ "${FLAGS_dev_install}" -eq "${FLAGS_TRUE}" ] ||
> -     [ "${FLAGS_factory_install}" -eq "${FLAGS_TRUE}" ]; then
> +  if [ "${FLAGS_factory_install}" -eq "${FLAGS_TRUE}" ]; then
>     BOOT_FLAG="-b 1"  # BOOT_FLAG_DEVELOPER value defined in
> load_kernel_fw.h
> -    info "--dev_install set, pass BOOT_FLAG_DEVELOPER flag to
> load_kernel_test"
> +    info "--factory_install set, pass BOOT_FLAG_DEVELOPER flag to \
> +load_kernel_test"
>   fi
>
>   # Verify the final image.
>
>
>

[jrbarnette, 2011-03-16 01:01:45]

LGTM after addressing one comment nit.

http://codereview.chromium.org/6694042/diff/1/build_image
File build_image (right):

http://codereview.chromium.org/6694042/diff/1/build_image#newcode145
build_image:145: # TODO(vlaviano): validate flags
What's this about?  If the comment is actually relevant to the code you
want to push, I think it needs a bit more detail.

[Vince Laviano, 2011-03-16 01:21:15]

http://codereview.chromium.org/6694042/diff/1/build_image
File build_image (right):

http://codereview.chromium.org/6694042/diff/1/build_image#newcode145
build_image:145: # TODO(vlaviano): validate flags
On 2011/03/16 01:01:46, jrbarnette wrote:
> What's this about?  If the comment is actually relevant to the code you
> want to push, I think it needs a bit more detail.

The code that I deleted from here was examining a single set of mutually
incompatible flags (--factory_install and --dev_install) and ensuring that both
are not set. I added the comment to remind myself to perform a much more
comprehensive validation of flags as I rewrite build_image, since I will no
longer have this deleted code here as a reminder to do so.

[jrbarnette, 2011-03-16 22:11:02]

> > What's this about?  If the comment is actually relevant to the code you
> > want to push, I think it needs a bit more detail.
> 
> The code that I deleted from here was examining a single set of mutually
> incompatible flags (--factory_install and --dev_install) and ensuring that
both
> are not set. I added the comment to remind myself to perform a much more
> comprehensive validation of flags as I rewrite build_image, since I will no
> longer have this deleted code here as a reminder to do so.

Don't tell me here; explain it in the TODO.  :-)  I'm looking for a TODO
comment that explains the issue in sufficient detail that anyone reading it
can know how to resolve the TODO without needing to consult you, or
me, or this CL.

[Vince Laviano, 2011-03-16 22:46:59]

On 2011/03/16 22:11:02, jrbarnette wrote:
> > > What's this about?  If the comment is actually relevant to the code you
> > > want to push, I think it needs a bit more detail.
> > 
> > The code that I deleted from here was examining a single set of mutually
> > incompatible flags (--factory_install and --dev_install) and ensuring that
> both
> > are not set. I added the comment to remind myself to perform a much more
> > comprehensive validation of flags as I rewrite build_image, since I will no
> > longer have this deleted code here as a reminder to do so.
> 
> Don't tell me here; explain it in the TODO.  :-)  I'm looking for a TODO
> comment that explains the issue in sufficient detail that anyone reading it
> can know how to resolve the TODO without needing to consult you, or
> me, or this CL.

Done.

[jrbarnette, 2011-03-16 22:56:44]

On 2011/03/16 22:46:59, Vince Laviano wrote:
> On 2011/03/16 22:11:02, jrbarnette wrote:
> > > > What's this about?  If the comment is actually relevant to the code you
> > > > want to push, I think it needs a bit more detail.
> > > 
> > > The code that I deleted from here was examining a single set of mutually
> > > incompatible flags (--factory_install and --dev_install) and ensuring that
> > both
> > > are not set. I added the comment to remind myself to perform a much more
> > > comprehensive validation of flags as I rewrite build_image, since I will
no
> > > longer have this deleted code here as a reminder to do so.
> > 
> > Don't tell me here; explain it in the TODO.  :-)  I'm looking for a TODO
> > comment that explains the issue in sufficient detail that anyone reading it
> > can know how to resolve the TODO without needing to consult you, or
> > me, or this CL.
> 
> Done.

Thanks!  LGTM!

[Will Drewry, 2011-03-17 18:18:36]

LGTM

Thanks! I should've said so sooner!