Only search for the gpt tool if we need it.

Only search for the gpt tool if we need it.

[robotboy, 2010-04-21 19:09:05]

Second try, Please take a look.

[adlr, 2010-04-21 19:12:48]

I think some other scripts use GPT, too. Could you grep "GPT" and/or
"chromeos-common" to find them and fix them w/ this change.

A quick search shows:


(cros-chroot) adlr@adlr ~/trunk/src $ fgrep chromeos-common.sh scripts/*
platform/*/*
scripts/build_gpt.sh:. "$(dirname "$0")/chromeos-common.sh"
scripts/emit_gpt_scripts.sh:. "$(dirname "$0")/chromeos-common.sh"
scripts/image_to_usb.sh:. "$(dirname "$0")/chromeos-common.sh"
scripts/image_to_vmware.sh:. "$(dirname "$0")/chromeos-common.sh"
scripts/mod_image_for_test.sh:. "$(dirname "$0")/chromeos-common.sh"
platform/factory_installer/factory_install.sh:. "$(dirname
"$0")/chromeos-common.sh"
platform/init/clobber-state:. /usr/sbin/chromeos-common.sh
platform/installer/chromeos-install:. "$(dirname "$0")/chromeos-common.sh"
|| exit 1


-andrew

On Wed, Apr 21, 2010 at 12:09 PM, <robotboy@chromium.org> wrote:

> Reviewers: Bill Richardson, adlr, Yusuke Sato,
>
> Message:
> Second try, Please take a look.
>
> Description:
> Only search for the gpt tool if we need it.
>
> Please review this at http://codereview.chromium.org/1747005/show
>
> Affected files:
>  M src/platform/installer/chromeos-common.sh
>  M src/scripts/emit_gpt_scripts.sh
>
>
> Index: src/platform/installer/chromeos-common.sh
> diff --git a/src/platform/installer/chromeos-common.sh
> b/src/platform/installer/chromeos-common.sh
> index
>
d1da743b17bb321c45f1f382af17dc5104532206..347ec3ef2b752a433aeefde6734fe7ba7cfa0312
> 100644
> --- a/src/platform/installer/chromeos-common.sh
> +++ b/src/platform/installer/chromeos-common.sh
> @@ -81,20 +81,24 @@ rounddown() {
>   echo $num
>  }
>
> -
> -# We need to locate the gpt tool. It should already be installed in the
> build
> -# chroot, but some of these functions may be invoked outside the chroot
> (by
> +# Locate the gpt tool. It should already be installed in the build chroot,
> +# but some of these functions may be invoked outside the chroot (by
>  # image_to_usb or similar), so we need to find it.
> -GPT=$(which gpt 2>/dev/null) || /bin/true
> -if [ -z "$GPT" ]; then
> -  if [ -x "${DEFAULT_CHROOT_DIR:-}/usr/bin/gpt" ]; then
> -    GPT="${DEFAULT_CHROOT_DIR:-}/usr/bin/gpt"
> -  else
> -    echo "can't find gpt tool" 1>&2
> -    exit 1
> +GPT=""
> +
> +locate_gpt() {
> +  if [ -z "$GPT" ]; then
> +    GPT=$(which gpt 2>/dev/null) || /bin/true
> +    if [ -z "$GPT" ]; then
> +      if [ -x "${DEFAULT_CHROOT_DIR:-}/usr/bin/gpt" ]; then
> +        GPT="${DEFAULT_CHROOT_DIR:-}/usr/bin/gpt"
> +      else
> +        echo "can't find gpt tool" 1>&2
> +        exit 1
> +      fi
> +    fi
>   fi
> -fi
> -
> +}
>
>  # This installs a GPT into the specified device or file, using the given
>  # components. If the target is a block device or the FORCE_FULL arg is
> "true"
> @@ -191,6 +195,8 @@ install_gpt() {
>
>   local start_useful=$(roundup $(($start_future_11 + 1)))
>
> +  locate_gpt
> +
>   # What are we doing?
>   if [ -b "$outdev" -o "$force_full" = "true" ]; then
>     # Block device, need to be root.
> @@ -281,6 +287,7 @@ install_gpt() {
>   # Create the new GPT partitions. The order determines the partition
> number.
>   # Note that the partition label is in the GPT only. The filesystem label
> is
>   # what's used to populate /dev/disk/by-label/, and this is not that.
> +
>   $sudo $GPT create ${outdev}
>
>   $sudo $GPT add -b ${START_STATEFUL} -s ${NUM_STATEFUL_SECTORS} \
> @@ -346,6 +353,9 @@ _partinfo() {
>   local device=$1
>   local partnum=$2
>   local start size part x n
> +
> +  locate_gpt
> +
>   sudo $GPT -r -S show $device \
>     | grep 'GPT part -' \
>     | while read start size part x x x n x; do \
> Index: src/scripts/emit_gpt_scripts.sh
> diff --git a/src/scripts/emit_gpt_scripts.sh
> b/src/scripts/emit_gpt_scripts.sh
> index
>
825f511ed2cb4a2fb5d0e8b9965b087bd7f0adb3..b16e4b3a4285e8cc994b78521af4a9bbe8d243af
> 100755
> --- a/src/scripts/emit_gpt_scripts.sh
> +++ b/src/scripts/emit_gpt_scripts.sh
> @@ -23,6 +23,8 @@ fi
>  PACK="${DIR}/pack_partitions.sh"
>  UNPACK="${DIR}/unpack_partitions.sh"
>
> +locate_gpt
> +
>  TMP=$(mktemp)
>  sudo $GPT -r show -l "$IMAGE" > $TMP
>
>
>
>

[robotboy, 2010-04-21 19:25:14]

All of the locations that use the gpt tool and thus the GPT variable have been
changed to call locate_gpt.  This change allows us to use the scripts that
include chromeos-common.sh on machines that have no chroot.  For instance on a
machine that has just downloaded an image from the build bots.

[Bill Richardson, 2010-04-21 19:36:24]

LGTM

[sosa, 2010-04-27 22:12:24]

Sorry I just noticed this.

"which" is not on chromeos-base.  So memento_updater will break on
non-dev images.  Can we split the use of chromeos-common to specify
which or have a backup if which is not found?

On Wed, Apr 21, 2010 at 12:36 PM,  <wfrichar@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/1747005/show
>

[Bill Richardson, 2010-04-28 19:23:34]

Probably could use 'type gpt' instead of 'which gpt'.

[adlr, 2010-04-29 22:00:16]

If there isn't whitespace or escaped colons, etc, in $PATH, this is a
makeshift 'which'

PROG=emerge-x86-generic; for i in $(echo $PATH | tr : ' ' ); do if [ -x
"${i}/$PROG" ]; then echo "${i}/$PROG"; break; fi; done

On Thu, Apr 29, 2010 at 2:48 PM, Anton Staaf <robotboy@google.com> wrote:

>     I didn't write the initial code and am not certain of the semantics of
> type over which (it looks like type might be a little trickier to parse) and
> so am less inclined to make this change.  For instance, these two outputs
> make me scared that parsing the output of "type" will be fragile.
>
> $ type gpt
> gpt is /usr/bin/gpt
>
> $ which gpt
> /usr/bin/gpt
>
> $ type ls
> ls is aliased to `ls --color=auto'
>
> $ which ls
> /bin/ls
>
> -Anton
>
> On Wed, Apr 28, 2010 at 12:23 PM, <wfrichar@chromium.org> wrote:
>
>> Probably could use 'type gpt' instead of 'which gpt'.
>>
>>
>>
>> http://codereview.chromium.org/1747005/show
>>
>
>

[robotboy, 2010-04-29 22:22:34]

This should really be a separate code review issue.  I just noticed that at some
point this issue was un-closed.  The code is in and I didn't introduce the use
of which.  Can I close this issue and we can move this thread to a new CL?

Thanks,
     Anton

[Bill Richardson, 2010-04-29 22:27:32]

Yeah, close it and start a new one. I opened it again since the comment came
along after the commit.



On Thu, Apr 29, 2010 at 3:22 PM, <robotboy@chromium.org> wrote:

> This should really be a separate code review issue.  I just noticed that at
> some
> point this issue was un-closed.  The code is in and I didn't introduce the
> use
> of which.  Can I close this issue and we can move this thread to a new CL?
>
> Thanks,
>     Anton
>
>
>
> http://codereview.chromium.org/1747005/show
>