upgrade_chroot: create an upgrade script

upgrade_chroot: create an upgrade script

Change-Id: Ief6360b636884b02bdf74baeb6c4103eb180d2b7

BUG=6151
TEST=run it

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=358b738

[davidjames, 2010-09-03 02:41:39]

LGTM

http://codereview.chromium.org/3331011/diff/1/3
File cros_upgrade_chroot (right):

http://codereview.chromium.org/3331011/diff/1/3#newcode19
cros_upgrade_chroot:19: echo -e "${V_BOLD_GREEN}** ${V_NORMAL}${*}"
No need for {} around one character variables (e.g. $*) :)
I practically couldn't understand what this means

http://codereview.chromium.org/3331011/diff/1/3#newcode26
cros_upgrade_chroot:26: announce "Nothing significant happens here, it only
means you have been assimilated."
80 chars. Comma splice. (Just separate the two sentences with a period rather
than a comma.)

[kliegs, 2010-09-03 15:24:43]

What is the goal of this file? I thought make_chroot would update your chroot if
needed?

And while I suspect those comments in function_upgrade_1 may be placeholders I'd
prefer to see real information in their place.  Announcing messages that don't
tell the user everything just add more noise preventing useful messages from
being read.  

As for the outputs, common.sh already has info, warrning and error.  Is there a
need for a new type announce over those?  If so it should be moved into
common.sh and the reasons for its use over the other types made clear.

Similarly the direct output you have from 'echo' in the main body should be
turned into appropriate info, error, ... calls.

[Mandeep Singh Baines, 2010-09-03 17:06:25]

Independent changes should go into independent CLs. Could you please
put the common.sh stuff were into a separate CL.

zbehan@chromium.org (zbehan@chromium.org) wrote:
> Reviewers: davidjames, anush, Mandeep Singh Baines,
> 
> Description:
> upgrade_chroot: create an upgrade script
> 
> Change-Id: Ief6360b636884b02bdf74baeb6c4103eb180d2b7
> 
> BUG=
> TEST=
> 
> Please review this at http://codereview.chromium.org/3331011/show
> 
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
> 
> Affected files:
>   M common.sh
>   A cros_upgrade_chroot
> 
> 
> Index: common.sh
> diff --git a/common.sh b/common.sh
> index
9e55a9a1b04923d8ab469a5485a53156ec7c5c61..8af47fa6d86fdcde1a27ceedc6f9507d4d5cf0d8
> 100644
> --- a/common.sh
> +++ b/common.sh
> @@ -313,11 +313,14 @@ function check_flags_only_and_allow_null_arg {
>    return $do_shift
>  }
> 
> +V_NORMAL="\e[0m"
>  V_RED="\e[31m"
>  V_YELLOW="\e[33m"
> -V_BOLD_GREEN="\e[1;32m"
> -V_BOLD_RED="\e[1;31m"
> -V_BOLD_YELLOW="\e[1;33m"
> +V_BOLD="\e[1m"
> +V_BOLD_GREEN="${V_BOLD}\e[32m"
> +V_BOLD_RED="${V_BOLD}\e[31m"
> +V_BOLD_YELLOW="${V_BOLD}\e[33m"
> +V_BOLD_CYAN="${V_BOLD}\e[36m"
> 
>  function info {
>    echo -e >&2 "${V_BOLD_GREEN}INFO   : $1${V_VIDOFF}"
> Index: cros_upgrade_chroot
> diff --git a/cros_upgrade_chroot b/cros_upgrade_chroot
> new file mode 100755
> index
0000000000000000000000000000000000000000..f2cb3bd9c339f4ccb3f75b9d46a3207b576ea69a
> --- /dev/null
> +++ b/cros_upgrade_chroot
> @@ -0,0 +1,46 @@
> +#!/bin/bash
> +
> +# Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
> +# Use of this source code is governed by a BSD-style license that can be
> +# found in the LICENSE file.
> +
> +# Load common constants.  This should be the first executable line.
> +# The path to common.sh should be relative to your script's location.
> +. "$(dirname "$0")/common.sh"
> +
> +# Script must be run inside the chroot
> +restart_in_chroot_if_needed $*
> +
> +VERSION_FILE=~/.version
> +
> +#########################  Utility functions  ########################
> +
> +function announce() {
> +  echo -e "${V_BOLD_GREEN}** ${V_NORMAL}${*}"
> +}
> +
> +######################################################################
> +
> +function upgrade_1() {
> +  announce "You have just been upgraded to chroot version 1!
> Congratulations!"
> +  announce "Nothing significant happens here, it only means you
> have been assimilated."
> +}
> +
> +LATEST_VERSION=1
> +
> +# default goes here
> +if ! [ -f "${VERSION_FILE}" ]; then
> +  echo "Warning: chroot of unknown version, assuming 0"
> +  echo "0" > "${VERSION_FILE}"
> +fi
> +
> +CHROOT_VERSION=$(cat ${VERSION_FILE})
> +
> +if [ "${LATEST_VERSION}" -gt "${CHROOT_VERSION}" ]; then
> +  echo "Outdated chroot found"
> +  for n in $(seq "$(expr ${CHROOT_VERSION} + 1)" "${LATEST_VERSION}"); do
> +    echo -e "${V_BOLD_CYAN}Rollup to version ${V_RED}${n}${V_NORMAL}"
> +    eval upgrade_${n}
> +  done
> +  echo "${LATEST_VERSION}" > "${VERSION_FILE}"
> +fi
> 
> 

[zbehan, 2010-09-03 18:30:07]

On Fri, Sep 3, 2010 at 8:24 AM, <kliegs@chromium.org> wrote:

> What is the goal of this file? I thought make_chroot would update your
> chroot if
> needed?
>

You're missing on the personal discussions we had about this, and probably
didn't see the bug. Make chroot requires an explicit call, and is almost
never done by the user, or even easy to determine when it should be done.
This script tries to solve a different problem, when an explicit cleanup
action in the chroot is required, like, removing one link. Look in your
inbox for the ACTION REQUIRED emails.

This script should be automatically called in all the places which user
calls explicitly (like, enter_chroot, build_packages, ...), and perform an
explicit action on demand of someone who did something incompatible in the
chroot.

It should print out a warning that something is going to be done, and likely
ask for confirmation to do it.

And while I suspect those comments in function_upgrade_1 may be placeholders
> I'd
> prefer to see real information in their place.  Announcing messages that
> don't
> tell the user everything just add more noise preventing useful messages
> from
> being read.
>

They're not just "placeholders", they're an info that the system is in
place, but don't do anything useful. The "default" version will have to be
changed later to the LATEST, because that's where all freshly created
chroots will start.


> As for the outputs, common.sh already has info, warrning and error.  Is
> there a
> need for a new type announce over those?  If so it should be moved into
> common.sh and the reasons for its use over the other types made clear.
>

I just didn't like the color-bleeding scheme of those, and i created my
own.

Similarly the direct output you have from 'echo' in the main body should be
> turned into appropriate info, error, ... calls.


This script is really just a proposal that is not going to be used anywhere
yet. It is up for both improvements, and subsequent changes.

[kliegs, 2010-09-03 19:16:42]

Sorry for missing the bug - didn't see it referenced in the CR.  I'll work
on finding more spies to prowl the MV corridors for me however.  Downside of
being remote :(

Rest of my stuff inline below...

On Fri, Sep 3, 2010 at 2:29 PM, Zdenek Behan <zbehan@chromium.org> wrote:

> On Fri, Sep 3, 2010 at 8:24 AM, <kliegs@chromium.org> wrote:
>
>> What is the goal of this file? I thought make_chroot would update your
>> chroot if
>> needed?
>>
>
> You're missing on the personal discussions we had about this, and probably
> didn't see the bug. Make chroot requires an explicit call, and is almost
> never done by the user, or even easy to determine when it should be done.
> This script tries to solve a different problem, when an explicit cleanup
> action in the chroot is required, like, removing one link. Look in your
> inbox for the ACTION REQUIRED emails.
>
> This script should be automatically called in all the places which user
> calls explicitly (like, enter_chroot, build_packages, ...), and perform an
> explicit action on demand of someone who did something incompatible in the
> chroot.
>
> It should print out a warning that something is going to be done, and
> likely ask for confirmation to do it.
>

Any plans on a mechanism for non-linear changes?  I'm worried this script
might become a contention point if its done in a "upgrade_1, upgrade_2, ...
upgrade_latest" way.  I've seen multiple ACTION_REQUIRED (well - think I
have, I could be wrong) emails that seem to overlap.  If one had to get
rolled back or got applied in a different order that might cause trouble for
this script?  Is there a way to do something similar to runhooks in gclient?
 Or maybe this code should just go into the pkg_setup part of the ebuilds?
 It feels to me like this is a better spot - each package can control any
updating it needs to do on its own and not rely on a central script.  Unless
this script gets setup in a modular fashion with hooks or call backs I'm
worried it would become a bottleneck and patching nightmare (where people
either rush their changes in to "beat the conflict" or spend too much time
needing to retest after patches).  Unless I misunderstand and this script is
only really intended for a specific package and not for any consumers that
might need it.


>
> And while I suspect those comments in function_upgrade_1 may be
>> placeholders I'd
>> prefer to see real information in their place.  Announcing messages that
>> don't
>> tell the user everything just add more noise preventing useful messages
>> from
>> being read.
>>
>
> They're not just "placeholders", they're an info that the system is in
> place, but don't do anything useful. The "default" version will have to be
> changed later to the LATEST, because that's where all freshly created
> chroots will start.
>

 Having a "nothing significant happened here" output adds no value at all to
the end user and causes people to miss messages that actual matter. If a
script is going to output information it should be relevant.   You already
have a "upgraded happened" message above it, a second one serves no further
purpose.



> As for the outputs, common.sh already has info, warrning and error.  Is
>> there a
>> need for a new type announce over those?  If so it should be moved into
>> common.sh and the reasons for its use over the other types made clear.
>>
>
> I just didn't like the color-bleeding scheme of those, and i created my
> own.
>

Then you should fix the color-bleeding.  Adding additional ways to do the
same thing leads to lots of confusion and heartache down the road.


>  Similarly the direct output you have from 'echo' in the main body should
>> be
>> turned into appropriate info, error, ... calls.
>
>
> This script is really just a proposal that is not going to be used anywhere
> yet. It is up for both improvements, and subsequent changes.
>

As a proposal it should still show the correct implementation.  Having the
prototype use the wrong invocations causes people to follow the prototype
and perpetuate the improper mechanisms.

[zbehan, 2010-09-03 19:41:15]

On Fri, Sep 3, 2010 at 12:16 PM, Jonathan Kliegman <kliegs@chromium.org>wrote:

> Sorry for missing the bug - didn't see it referenced in the CR.  I'll work
> on finding more spies to prowl the MV corridors for me however.  Downside of
> being remote :(
>
> Rest of my stuff inline below...
>
> On Fri, Sep 3, 2010 at 2:29 PM, Zdenek Behan <zbehan@chromium.org> wrote:
>
>> On Fri, Sep 3, 2010 at 8:24 AM, <kliegs@chromium.org> wrote:
>>
>>> What is the goal of this file? I thought make_chroot would update your
>>> chroot if
>>> needed?
>>>
>>
>> You're missing on the personal discussions we had about this, and probably
>> didn't see the bug. Make chroot requires an explicit call, and is almost
>> never done by the user, or even easy to determine when it should be done.
>> This script tries to solve a different problem, when an explicit cleanup
>> action in the chroot is required, like, removing one link. Look in your
>> inbox for the ACTION REQUIRED emails.
>>
>> This script should be automatically called in all the places which user
>> calls explicitly (like, enter_chroot, build_packages, ...), and perform an
>> explicit action on demand of someone who did something incompatible in the
>> chroot.
>>
>> It should print out a warning that something is going to be done, and
>> likely ask for confirmation to do it.
>>
>
> Any plans on a mechanism for non-linear changes?  I'm worried this script
> might become a contention point if its done in a "upgrade_1, upgrade_2, ...
> upgrade_latest" way.  I've seen multiple ACTION_REQUIRED (well - think I
> have, I could be wrong) emails that seem to overlap.  If one had to get
> rolled back or got applied in a different order that might cause trouble for
> this script?  Is there a way to do something similar to runhooks in gclient?
>  Or maybe this code should just go into the pkg_setup part of the ebuilds?
>  It feels to me like this is a better spot - each package can control any
> updating it needs to do on its own and not rely on a central script.  Unless
> this script gets setup in a modular fashion with hooks or call backs I'm
> worried it would become a bottleneck and patching nightmare (where people
> either rush their changes in to "beat the conflict" or spend too much time
> needing to retest after patches).  Unless I misunderstand and this script is
> only really intended for a specific package and not for any consumers that
> might need it.
>

Nope, but creators of upgrade functions are more than welcome to present the
user with a choice from one or more variants.


> And while I suspect those comments in function_upgrade_1 may be
>>> placeholders I'd
>>> prefer to see real information in their place.  Announcing messages that
>>> don't
>>> tell the user everything just add more noise preventing useful messages
>>> from
>>> being read.
>>>
>>
>> They're not just "placeholders", they're an info that the system is in
>> place, but don't do anything useful. The "default" version will have to be
>> changed later to the LATEST, because that's where all freshly created
>> chroots will start.
>>
>
>  Having a "nothing significant happened here" output adds no value at all
> to the end user and causes people to miss messages that actual matter. If a
> script is going to output information it should be relevant.   You already
> have a "upgraded happened" message above it, a second one serves no further
> purpose.
>

That's why this message is going to be displayed at most once, to indicate
that the initialization of the version file was done.


> As for the outputs, common.sh already has info, warrning and error.  Is
>>> there a
>>> need for a new type announce over those?  If so it should be moved into
>>> common.sh and the reasons for its use over the other types made clear.
>>>
>>
>> I just didn't like the color-bleeding scheme of those, and i created my
>> own.
>>
>
> Then you should fix the color-bleeding.  Adding additional ways to do the
> same thing leads to lots of confusion and heartache down the road.
>

This is not the _same_ thing. It's a different type of message printout.
Changing the already existing info messages will be strongly frowned upon,
because they're used in just about every script out there.

I can move the announce function into common.sh.


>  Similarly the direct output you have from 'echo' in the main body should
>>> be
>>> turned into appropriate info, error, ... calls.
>>
>>
>> This script is really just a proposal that is not going to be used
>> anywhere yet. It is up for both improvements, and subsequent changes.
>>
>
> As a proposal it should still show the correct implementation.  Having the
> prototype use the wrong invocations causes people to follow the prototype
> and perpetuate the improper mechanisms.
>

It's not using the "wrong" invocations. Just not being entirely perfect
about wrapping each echo in a function, because I don't need all echoes to
be special. In other scripts, the info/error/blah messages are not used
exclusively either, iirc.

[sosa, 2010-09-03 19:45:30]

Side question:

Shouldn't make_chroot set a version when it creates a chroot?

On Fri, Sep 3, 2010 at 12:41 PM, Zdenek Behan <zbehan@chromium.org> wrote:
> On Fri, Sep 3, 2010 at 12:16 PM, Jonathan Kliegman <kliegs@chromium.org>
> wrote:
>>
>> Sorry for missing the bug - didn't see it referenced in the CR.  I'll work
>> on finding more spies to prowl the MV corridors for me however.  Downside of
>> being remote :(
>> Rest of my stuff inline below...
>>
>> On Fri, Sep 3, 2010 at 2:29 PM, Zdenek Behan <zbehan@chromium.org> wrote:
>>>
>>> On Fri, Sep 3, 2010 at 8:24 AM, <kliegs@chromium.org> wrote:
>>>>
>>>> What is the goal of this file? I thought make_chroot would update your
>>>> chroot if
>>>> needed?
>>>
>>> You're missing on the personal discussions we had about this, and
>>> probably didn't see the bug. Make chroot requires an explicit call, and is
>>> almost never done by the user, or even easy to determine when it should be
>>> done. This script tries to solve a different problem, when an explicit
>>> cleanup action in the chroot is required, like, removing one link. Look in
>>> your inbox for the ACTION REQUIRED emails.
>>> This script should be automatically called in all the places which user
>>> calls explicitly (like, enter_chroot, build_packages, ...), and perform an
>>> explicit action on demand of someone who did something incompatible in the
>>> chroot.
>>> It should print out a warning that something is going to be done, and
>>> likely ask for confirmation to do it.
>>
>> Any plans on a mechanism for non-linear changes?  I'm worried this script
>> might become a contention point if its done in a "upgrade_1, upgrade_2, ...
>> upgrade_latest" way.  I've seen multiple ACTION_REQUIRED (well - think I
>> have, I could be wrong) emails that seem to overlap.  If one had to get
>> rolled back or got applied in a different order that might cause trouble for
>> this script?  Is there a way to do something similar to runhooks in gclient?
>>  Or maybe this code should just go into the pkg_setup part of the ebuilds?
>>  It feels to me like this is a better spot - each package can control any
>> updating it needs to do on its own and not rely on a central script.  Unless
>> this script gets setup in a modular fashion with hooks or call backs I'm
>> worried it would become a bottleneck and patching nightmare (where people
>> either rush their changes in to "beat the conflict" or spend too much time
>> needing to retest after patches).  Unless I misunderstand and this script is
>> only really intended for a specific package and not for any consumers that
>> might need it.
>
> Nope, but creators of upgrade functions are more than welcome to present the
> user with a choice from one or more variants.
>
>>>>
>>>> And while I suspect those comments in function_upgrade_1 may be
>>>> placeholders I'd
>>>> prefer to see real information in their place.  Announcing messages that
>>>> don't
>>>> tell the user everything just add more noise preventing useful messages
>>>> from
>>>> being read.
>>>
>>> They're not just "placeholders", they're an info that the system is in
>>> place, but don't do anything useful. The "default" version will have to be
>>> changed later to the LATEST, because that's where all freshly created
>>> chroots will start.
>>
>>
>>  Having a "nothing significant happened here" output adds no value at all
>> to the end user and causes people to miss messages that actual matter. If a
>> script is going to output information it should be relevant.   You already
>> have a "upgraded happened" message above it, a second one serves no further
>> purpose.
>
> That's why this message is going to be displayed at most once, to indicate
> that the initialization of the version file was done.
>
>>>>
>>>> As for the outputs, common.sh already has info, warrning and error.  Is
>>>> there a
>>>> need for a new type announce over those?  If so it should be moved into
>>>> common.sh and the reasons for its use over the other types made clear.
>>>
>>>
>>> I just didn't like the color-bleeding scheme of those, and i created my
>>> own.
>>
>> Then you should fix the color-bleeding.  Adding additional ways to do the
>> same thing leads to lots of confusion and heartache down the road.
>
> This is not the _same_ thing. It's a different type of message printout.
> Changing the already existing info messages will be strongly frowned upon,
> because they're used in just about every script out there.
> I can move the announce function into common.sh.
>
>>>>
>>>> Similarly the direct output you have from 'echo' in the main body should
>>>> be
>>>> turned into appropriate info, error, ... calls.
>>>
>>> This script is really just a proposal that is not going to be used
>>> anywhere yet. It is up for both improvements, and subsequent changes.
>>
>> As a proposal it should still show the correct implementation.  Having the
>> prototype use the wrong invocations causes people to follow the prototype
>> and perpetuate the improper mechanisms.
>
> It's not using the "wrong" invocations. Just not being entirely perfect
> about wrapping each echo in a function, because I don't need all echoes to
> be special. In other scripts, the info/error/blah messages are not used
> exclusively either, iirc.

[zbehan, 2010-09-03 19:57:03]

The idea was more that default version should be set to latest, and
make_chroot should run this.

On Fri, Sep 3, 2010 at 12:45 PM, Chris Sosa <sosa@chromium.org> wrote:

> Side question:
>
> Shouldn't make_chroot set a version when it creates a chroot?
>
> On Fri, Sep 3, 2010 at 12:41 PM, Zdenek Behan <zbehan@chromium.org> wrote:
> > On Fri, Sep 3, 2010 at 12:16 PM, Jonathan Kliegman <kliegs@chromium.org>
> > wrote:
> >>
> >> Sorry for missing the bug - didn't see it referenced in the CR.  I'll
> work
> >> on finding more spies to prowl the MV corridors for me however.
>  Downside of
> >> being remote :(
> >> Rest of my stuff inline below...
> >>
> >> On Fri, Sep 3, 2010 at 2:29 PM, Zdenek Behan <zbehan@chromium.org>
> wrote:
> >>>
> >>> On Fri, Sep 3, 2010 at 8:24 AM, <kliegs@chromium.org> wrote:
> >>>>
> >>>> What is the goal of this file? I thought make_chroot would update your
> >>>> chroot if
> >>>> needed?
> >>>
> >>> You're missing on the personal discussions we had about this, and
> >>> probably didn't see the bug. Make chroot requires an explicit call, and
> is
> >>> almost never done by the user, or even easy to determine when it should
> be
> >>> done. This script tries to solve a different problem, when an explicit
> >>> cleanup action in the chroot is required, like, removing one link. Look
> in
> >>> your inbox for the ACTION REQUIRED emails.
> >>> This script should be automatically called in all the places which user
> >>> calls explicitly (like, enter_chroot, build_packages, ...), and perform
> an
> >>> explicit action on demand of someone who did something incompatible in
> the
> >>> chroot.
> >>> It should print out a warning that something is going to be done, and
> >>> likely ask for confirmation to do it.
> >>
> >> Any plans on a mechanism for non-linear changes?  I'm worried this
> script
> >> might become a contention point if its done in a "upgrade_1, upgrade_2,
> ...
> >> upgrade_latest" way.  I've seen multiple ACTION_REQUIRED (well - think I
> >> have, I could be wrong) emails that seem to overlap.  If one had to get
> >> rolled back or got applied in a different order that might cause trouble
> for
> >> this script?  Is there a way to do something similar to runhooks in
> gclient?
> >>  Or maybe this code should just go into the pkg_setup part of the
> ebuilds?
> >>  It feels to me like this is a better spot - each package can control
> any
> >> updating it needs to do on its own and not rely on a central script.
>  Unless
> >> this script gets setup in a modular fashion with hooks or call backs I'm
> >> worried it would become a bottleneck and patching nightmare (where
> people
> >> either rush their changes in to "beat the conflict" or spend too much
> time
> >> needing to retest after patches).  Unless I misunderstand and this
> script is
> >> only really intended for a specific package and not for any consumers
> that
> >> might need it.
> >
> > Nope, but creators of upgrade functions are more than welcome to present
> the
> > user with a choice from one or more variants.
> >
> >>>>
> >>>> And while I suspect those comments in function_upgrade_1 may be
> >>>> placeholders I'd
> >>>> prefer to see real information in their place.  Announcing messages
> that
> >>>> don't
> >>>> tell the user everything just add more noise preventing useful
> messages
> >>>> from
> >>>> being read.
> >>>
> >>> They're not just "placeholders", they're an info that the system is in
> >>> place, but don't do anything useful. The "default" version will have to
> be
> >>> changed later to the LATEST, because that's where all freshly created
> >>> chroots will start.
> >>
> >>
> >>  Having a "nothing significant happened here" output adds no value at
> all
> >> to the end user and causes people to miss messages that actual matter.
> If a
> >> script is going to output information it should be relevant.   You
> already
> >> have a "upgraded happened" message above it, a second one serves no
> further
> >> purpose.
> >
> > That's why this message is going to be displayed at most once, to
> indicate
> > that the initialization of the version file was done.
> >
> >>>>
> >>>> As for the outputs, common.sh already has info, warrning and error.
>  Is
> >>>> there a
> >>>> need for a new type announce over those?  If so it should be moved
> into
> >>>> common.sh and the reasons for its use over the other types made clear.
> >>>
> >>>
> >>> I just didn't like the color-bleeding scheme of those, and i created my
> >>> own.
> >>
> >> Then you should fix the color-bleeding.  Adding additional ways to do
> the
> >> same thing leads to lots of confusion and heartache down the road.
> >
> > This is not the _same_ thing. It's a different type of message printout.
> > Changing the already existing info messages will be strongly frowned
> upon,
> > because they're used in just about every script out there.
> > I can move the announce function into common.sh.
> >
> >>>>
> >>>> Similarly the direct output you have from 'echo' in the main body
> should
> >>>> be
> >>>> turned into appropriate info, error, ... calls.
> >>>
> >>> This script is really just a proposal that is not going to be used
> >>> anywhere yet. It is up for both improvements, and subsequent changes.
> >>
> >> As a proposal it should still show the correct implementation.  Having
> the
> >> prototype use the wrong invocations causes people to follow the
> prototype
> >> and perpetuate the improper mechanisms.
> >
> > It's not using the "wrong" invocations. Just not being entirely perfect
> > about wrapping each echo in a function, because I don't need all echoes
> to
> > be special. In other scripts, the info/error/blah messages are not used
> > exclusively either, iirc.
>

[zbehan, 2010-09-14 01:16:33]

Sorry, this got a bit stale and sour, uploading new version, where I got rid
of misc color stuff and common.sh changes, because those seemed to be the
biggest pain.

Also, default version: After thinking about this, this script should
probably have a --init-latest option, which checks for version file and sets
to latest if it doesn't exist. That would be used in make_chroot, everywhere
else, the script would be called as-is, where it will default to 0, and run
all upgrade functions on first run. This would ensure that new chroots get
no upgrades, except for those that happened after chroot creation, and old
chroots get everything since 0.

On Fri, Sep 3, 2010 at 12:56 PM, Zdenek Behan <zbehan@chromium.org> wrote:

> The idea was more that default version should be set to latest, and
> make_chroot should run this.
>
>
> On Fri, Sep 3, 2010 at 12:45 PM, Chris Sosa <sosa@chromium.org> wrote:
>
>> Side question:
>>
>> Shouldn't make_chroot set a version when it creates a chroot?
>>
>> On Fri, Sep 3, 2010 at 12:41 PM, Zdenek Behan <zbehan@chromium.org>
>> wrote:
>> > On Fri, Sep 3, 2010 at 12:16 PM, Jonathan Kliegman <kliegs@chromium.org
>> >
>> > wrote:
>> >>
>> >> Sorry for missing the bug - didn't see it referenced in the CR.  I'll
>> work
>> >> on finding more spies to prowl the MV corridors for me however.
>>  Downside of
>> >> being remote :(
>> >> Rest of my stuff inline below...
>> >>
>> >> On Fri, Sep 3, 2010 at 2:29 PM, Zdenek Behan <zbehan@chromium.org>
>> wrote:
>> >>>
>> >>> On Fri, Sep 3, 2010 at 8:24 AM, <kliegs@chromium.org> wrote:
>> >>>>
>> >>>> What is the goal of this file? I thought make_chroot would update
>> your
>> >>>> chroot if
>> >>>> needed?
>> >>>
>> >>> You're missing on the personal discussions we had about this, and
>> >>> probably didn't see the bug. Make chroot requires an explicit call,
>> and is
>> >>> almost never done by the user, or even easy to determine when it
>> should be
>> >>> done. This script tries to solve a different problem, when an explicit
>> >>> cleanup action in the chroot is required, like, removing one link.
>> Look in
>> >>> your inbox for the ACTION REQUIRED emails.
>> >>> This script should be automatically called in all the places which
>> user
>> >>> calls explicitly (like, enter_chroot, build_packages, ...), and
>> perform an
>> >>> explicit action on demand of someone who did something incompatible in
>> the
>> >>> chroot.
>> >>> It should print out a warning that something is going to be done, and
>> >>> likely ask for confirmation to do it.
>> >>
>> >> Any plans on a mechanism for non-linear changes?  I'm worried this
>> script
>> >> might become a contention point if its done in a "upgrade_1, upgrade_2,
>> ...
>> >> upgrade_latest" way.  I've seen multiple ACTION_REQUIRED (well - think
>> I
>> >> have, I could be wrong) emails that seem to overlap.  If one had to get
>> >> rolled back or got applied in a different order that might cause
>> trouble for
>> >> this script?  Is there a way to do something similar to runhooks in
>> gclient?
>> >>  Or maybe this code should just go into the pkg_setup part of the
>> ebuilds?
>> >>  It feels to me like this is a better spot - each package can control
>> any
>> >> updating it needs to do on its own and not rely on a central script.
>>  Unless
>> >> this script gets setup in a modular fashion with hooks or call backs
>> I'm
>> >> worried it would become a bottleneck and patching nightmare (where
>> people
>> >> either rush their changes in to "beat the conflict" or spend too much
>> time
>> >> needing to retest after patches).  Unless I misunderstand and this
>> script is
>> >> only really intended for a specific package and not for any consumers
>> that
>> >> might need it.
>> >
>> > Nope, but creators of upgrade functions are more than welcome to present
>> the
>> > user with a choice from one or more variants.
>> >
>> >>>>
>> >>>> And while I suspect those comments in function_upgrade_1 may be
>> >>>> placeholders I'd
>> >>>> prefer to see real information in their place.  Announcing messages
>> that
>> >>>> don't
>> >>>> tell the user everything just add more noise preventing useful
>> messages
>> >>>> from
>> >>>> being read.
>> >>>
>> >>> They're not just "placeholders", they're an info that the system is in
>> >>> place, but don't do anything useful. The "default" version will have
>> to be
>> >>> changed later to the LATEST, because that's where all freshly created
>> >>> chroots will start.
>> >>
>> >>
>> >>  Having a "nothing significant happened here" output adds no value at
>> all
>> >> to the end user and causes people to miss messages that actual matter.
>> If a
>> >> script is going to output information it should be relevant.   You
>> already
>> >> have a "upgraded happened" message above it, a second one serves no
>> further
>> >> purpose.
>> >
>> > That's why this message is going to be displayed at most once, to
>> indicate
>> > that the initialization of the version file was done.
>> >
>> >>>>
>> >>>> As for the outputs, common.sh already has info, warrning and error.
>>  Is
>> >>>> there a
>> >>>> need for a new type announce over those?  If so it should be moved
>> into
>> >>>> common.sh and the reasons for its use over the other types made
>> clear.
>> >>>
>> >>>
>> >>> I just didn't like the color-bleeding scheme of those, and i created
>> my
>> >>> own.
>> >>
>> >> Then you should fix the color-bleeding.  Adding additional ways to do
>> the
>> >> same thing leads to lots of confusion and heartache down the road.
>> >
>> > This is not the _same_ thing. It's a different type of message printout.
>> > Changing the already existing info messages will be strongly frowned
>> upon,
>> > because they're used in just about every script out there.
>> > I can move the announce function into common.sh.
>> >
>> >>>>
>> >>>> Similarly the direct output you have from 'echo' in the main body
>> should
>> >>>> be
>> >>>> turned into appropriate info, error, ... calls.
>> >>>
>> >>> This script is really just a proposal that is not going to be used
>> >>> anywhere yet. It is up for both improvements, and subsequent changes.
>> >>
>> >> As a proposal it should still show the correct implementation.  Having
>> the
>> >> prototype use the wrong invocations causes people to follow the
>> prototype
>> >> and perpetuate the improper mechanisms.
>> >
>> > It's not using the "wrong" invocations. Just not being entirely perfect
>> > about wrapping each echo in a function, because I don't need all echoes
>> to
>> > be special. In other scripts, the info/error/blah messages are not used
>> > exclusively either, iirc.
>>
>
>

[kliegs, 2010-09-14 14:46:50]

Can you please include the BUG you referenced earlier in the comments?

Do you have any thoughts/plans on how to implement a rollback if something goes
wrong in a future update function?  Perhaps a "reset to version 0 and start
over" clobber-type function?  	

You mentioned this script is just a proposal.  Is this CL going to actually be
submitted then?  Or just used as a point for gathering comments and reviews on a
future script to be written?  I'm curious to see how the other scripts
(specifically make_chroot) interact with this.

Finally is it worth adding a function to common.sh to get the chroot version?  
I can see a case where an upcoming change to the chroot is announced with a
"When Chroot version 4 is announced your scripts should do X".  To allow other
scripts or packages to be written to support feature X (and support testing)
they could have conditionals with "if version >= X do y else do z".  This way
other changes could be implemented while version 4 is being developed and tested
and activate immediately on its release.

http://codereview.chromium.org/3331011/diff/15001/16001
File cros_upgrade_chroot (right):

http://codereview.chromium.org/3331011/diff/15001/16001#newcode20
cros_upgrade_chroot:20: info "Nothing significant happens here, you've only been
assimilated."
Would still like to see this line removed.  The build system is very verbose so
extraneous lines just cause us to miss important information elsewhere.  I know
its info but since we don't really have a 'normal' log level info acts in its
place.

I know its not seen more than once per new build but it adds no value to the
user.  Output should always add value.  In addition when we have upgrade_1 to
upgrade_20 seeing this line mixed in with 20 other upgrade functions worth of
output just makes the user less likely to see anything important.

http://codereview.chromium.org/3331011/diff/15001/16001#newcode27
cros_upgrade_chroot:27: echo "Warning: chroot of unknown version, assuming 0"
If this is a warning the warning function should be used

http://codereview.chromium.org/3331011/diff/15001/16001#newcode33
cros_upgrade_chroot:33: if [ "${LATEST_VERSION}" -gt "${CHROOT_VERSION}" ]; then
I'd like to see some error checking around this.  Namely to make sure
VERSION_FILE contains a number.  If the user runs out of disk space its possible
for VERSION_FILE to be empty which would fail this check.

Perhaps a simple check after reading VERSION_FILE such that if its not a number
it gets set to 0 with a warning (similar to how if the file is missing its set
to 0

http://codereview.chromium.org/3331011/diff/15001/16001#newcode34
cros_upgrade_chroot:34: echo "Outdated chroot found"
Shouldn't this use warn? or info? instead of echo?

http://codereview.chromium.org/3331011/diff/15001/16001#newcode39
cros_upgrade_chroot:39: echo "${LATEST_VERSION}" > "${VERSION_FILE}"
If an upgrade_${n} function fails I think this will still be called. Running
with 'set -e' and making sure the upgrade functions properly error can prevent a
chroot from being set to the wrong versoin if a step fails.  

Making the upgrade functions explicitly return success or failure could be
useful in this case.  Setting VERSION_FILE to LATEST_VERSION incorrectly would
be a very bad and confusing things for users.

[zbehan, 2010-09-15 01:25:59]

On Tue, Sep 14, 2010 at 7:46 AM, <kliegs@chromium.org> wrote:

> Can you please include the BUG you referenced earlier in the comments?


Done.

6151 <detail?id=6151>

Do you have any thoughts/plans on how to implement a rollback if something
> goes
> wrong in a future update function?  Perhaps a "reset to version 0 and start
> over" clobber-type function?
>

The function of this is really intended to be for simple easilly scriptable
actions, and replacement for those ACTION REQUIRED emails. Invisible hand
that reaches to other people's chroots.

It should be functions that are damn near impossible to fail. Ex: chmod this
file if it exists; create this symlink if it doesn't exist; run sed on this
configuration file if it exists

The intent is to avoid currently required manual steps, not to create more.
Once you introduce things like rollback, you have to start being extremely
careful about everything. If this fails, should I increase the version
number? If i can't increase the version number, what do i do?

IMHO there's always going to be failure cases where some manual steps will
have to be taken, we want to minimize that to as few cases as possible, and
automate the simple actions.

You mentioned this script is just a proposal.  Is this CL going to actually
> be
> submitted then?  Or just used as a point for gathering comments and reviews
> on a
> future script to be written?  I'm curious to see how the other scripts
> (specifically make_chroot) interact with this.
>

Unless there will be strong objections, I'm all for submitting this bare
script without the actual integration, and do the Integration into other
scripts in other CL.

The interaction should be as simple as possible and in as many places as
possible. The point is for a noop call to be entirely invisible, and the
script to be called just about anywhere, in as many commands as possible, so
that people simply can't avoid getting the changes that someone decides that
they need.

Finally is it worth adding a function to common.sh to get the chroot
> version?
> I can see a case where an upcoming change to the chroot is announced with a
> "When Chroot version 4 is announced your scripts should do X".  To allow
> other
> scripts or packages to be written to support feature X (and support
> testing)
> they could have conditionals with "if version >= X do y else do z".  This
> way
> other changes could be implemented while version 4 is being developed and
> tested
> and activate immediately on its release.
>

Announcement is probably something that we want to avoid. Changes should be
transparent. Each rollup change should provide short description of what it
did, for easier debugging and less confusion of users, but people who don't
care can just skip the message and shouldn't have to worry about it.


> http://codereview.chromium.org/3331011/diff/15001/16001
>
> File cros_upgrade_chroot (right):
>
> http://codereview.chromium.org/3331011/diff/15001/16001#newcode20
> cros_upgrade_chroot:20: info "Nothing significant happens here, you've
> only been assimilated."
> Would still like to see this line removed.  The build system is very
> verbose so extraneous lines just cause us to miss important information
> elsewhere.  I know its info but since we don't really have a 'normal'
> log level info acts in its place.
>
> I know its not seen more than once per new build but it adds no value to
> the user.  Output should always add value.  In addition when we have
> upgrade_1 to upgrade_20 seeing this line mixed in with 20 other upgrade
> functions worth of output just makes the user less likely to see
> anything important.
>

It adds the value of users noticing that there's something new going on.

http://codereview.chromium.org/3331011/diff/15001/16001#newcode27
> cros_upgrade_chroot:27: echo "Warning: chroot of unknown version,
> assuming 0"
> If this is a warning the warning function should be used
>

Agreed.

http://codereview.chromium.org/3331011/diff/15001/16001#newcode33
> cros_upgrade_chroot:33: if [ "${LATEST_VERSION}" -gt "${CHROOT_VERSION}"
> ]; then
> I'd like to see some error checking around this.  Namely to make sure
> VERSION_FILE contains a number.  If the user runs out of disk space its
> possible for VERSION_FILE to be empty which would fail this check.
>

Will fix.

Perhaps a simple check after reading VERSION_FILE such that if its not a
> number it gets set to 0 with a warning (similar to how if the file is
> missing its set to 0
>
> http://codereview.chromium.org/3331011/diff/15001/16001#newcode34
> cros_upgrade_chroot:34: echo "Outdated chroot found"
> Shouldn't this use warn? or info? instead of echo?
>

I'm not a big fan of those centralised echo functions. I can use one but I
didn't find anything for a simple "print this, without any coloring".

http://codereview.chromium.org/3331011/diff/15001/16001#newcode39
> cros_upgrade_chroot:39: echo "${LATEST_VERSION}" > "${VERSION_FILE}"
> If an upgrade_${n} function fails I think this will still be called.
> Running with 'set -e' and making sure the upgrade functions properly
> error can prevent a chroot from being set to the wrong versoin if a step
> fails.
>
> Making the upgrade functions explicitly return success or failure could
> be useful in this case.  Setting VERSION_FILE to LATEST_VERSION
> incorrectly would be a very bad and confusing things for users.


That is just replacing emails in your inbox saying "You broke my chroot,
what do I do" with emails saying "Your upgrade function didn't unbreak my
chroot, what do I do".
While I don't think that we should have extensive belief in the fact that
people will not make mistakes in the upgrade functions, this should not
become an action-response mechanism, and people shouldn't be adding stuff
like 'action || error "If this fails for you, I owe you a beer"'.

I think in general, you're really imagining the solution to be something
bigger than it intends to be, I'm not trying to overengineer the problem,
just looking for a simple solution for simple things that take too many
people too much time, every day.

[kliegs, 2010-09-15 16:50:19]

On Tue, Sep 14, 2010 at 9:25 PM, Zdenek Behan <zbehan@chromium.org> wrote:

> On Tue, Sep 14, 2010 at 7:46 AM, <kliegs@chromium.org> wrote:
>
>> Do you have any thoughts/plans on how to implement a rollback if something
>> goes
>
> wrong in a future update function?  Perhaps a "reset to version 0 and start
>> over" clobber-type function?
>>
>
> The function of this is really intended to be for simple easilly scriptable
> actions, and replacement for those ACTION REQUIRED emails. Invisible hand
> that reaches to other people's chroots.
>
> It should be functions that are damn near impossible to fail. Ex: chmod
> this file if it exists; create this symlink if it doesn't exist; run sed on
> this configuration file if it exists
>
> The intent is to avoid currently required manual steps, not to create more.
> Once you introduce things like rollback, you have to start being extremely
> careful about everything. If this fails, should I increase the version
> number? If i can't increase the version number, what do i do?
>
> IMHO there's always going to be failure cases where some manual steps will
> have to be taken, we want to minimize that to as few cases as possible, and
> automate the simple actions.
>
I'm not arguing we'd never need manual steps.  Just that we should make it
obvious when they are needed and have the framework support it.

And just because its "damn near impossible" doesn't mean it won't happen and
isn't an excuse to not check the results.

However the case here I'm more concerned with is we release version 10.
 Shortly afterwards we realize that version 10 has a bug in it that wasn't
caught or anticipated.  We should be able to revert version 10.  That's what
I mean by rollback - that we're able to undo a bad upgrade.


>
>  You mentioned this script is just a proposal.  Is this CL going to
>> actually be
>> submitted then?  Or just used as a point for gathering comments and
>> reviews on a
>> future script to be written?  I'm curious to see how the other scripts
>> (specifically make_chroot) interact with this.
>>
>
> Unless there will be strong objections, I'm all for submitting this bare
> script without the actual integration, and do the Integration into other
> scripts in other CL.
>
I don't object to it being released with integration. But I wanted to
understand how the integration would occur.  Is there a reason though to
release it without integrating?


>  Finally is it worth adding a function to common.sh to get the chroot
>> version?
>> I can see a case where an upcoming change to the chroot is announced with
>> a
>> "When Chroot version 4 is announced your scripts should do X".  To allow
>> other
>> scripts or packages to be written to support feature X (and support
>> testing)
>> they could have conditionals with "if version >= X do y else do z".  This
>> way
>> other changes could be implemented while version 4 is being developed and
>> tested
>> and activate immediately on its release.
>>
>
> Announcement is probably something that we want to avoid. Changes should be
> transparent. Each rollup change should provide short description of what it
> did, for easier debugging and less confusion of users, but people who don't
> care can just skip the message and shouldn't have to worry about it.
>

I think my example might have been too broad.  I'm not looking at a global
case for all developers being aware or impacted, but more a specific one
within the infrastructure teams.  Lets say I'm working on a change to a
config file that this script will update.  You are working on a change that
would take advantage of my change if it exists, but needs to function
without it since you're releasing ahead of mine.  If you build your code
with the conditional (such as if CHROOT_VERSION > 3 CONFIG_OPTS=-a else
CONFIG_OPTS=-b) you can release once and not need to release a new version
once I submit my change.


>
>
>> http://codereview.chromium.org/3331011/diff/15001/16001
>>
>> File cros_upgrade_chroot (right):
>>
>> http://codereview.chromium.org/3331011/diff/15001/16001#newcode20
>> cros_upgrade_chroot:20: info "Nothing significant happens here, you've
>> only been assimilated."
>> Would still like to see this line removed.
>>
>
> It adds the value of users noticing that there's something new going on.
>
I'm just arguing that you only need one line of output. The second line adds
nothing - do it all in one line please.

http://codereview.chromium.org/3331011/diff/15001/16001#newcode39
>> cros_upgrade_chroot:39: echo "${LATEST_VERSION}" > "${VERSION_FILE}"
>> If an upgrade_${n} function fails I think this will still be called.
>> Running with 'set -e' and making sure the upgrade functions properly
>> error can prevent a chroot from being set to the wrong versoin if a step
>> fails.
>>
>> Making the upgrade functions explicitly return success or failure could
>> be useful in this case.  Setting VERSION_FILE to LATEST_VERSION
>> incorrectly would be a very bad and confusing things for users.
>
>
> That is just replacing emails in your inbox saying "You broke my chroot,
> what do I do" with emails saying "Your upgrade function didn't unbreak my
> chroot, what do I do".
> While I don't think that we should have extensive belief in the fact that
> people will not make mistakes in the upgrade functions, this should not
> become an action-response mechanism, and people shouldn't be adding stuff
> like 'action || error "If this fails for you, I owe you a beer"'.
>
> I think in general, you're really imagining the solution to be something
> bigger than it intends to be, I'm not trying to overengineer the problem,
> just looking for a simple solution for simple things that take too many
> people too much time, every day.
>

I can see this mechanism supporting more complex upgrade functions over time
(although code reviewers can help keep that in check).  Even if there is no
recoure other than "rebuild from scratch", "ask on IRC" or "manually edit
file foo" we should at least check that what was attempted did work and
react accordingly.  Setting the version when we aren't actually at that
version will cause a lot more pain if something does go wrong than failing
to set it.  It also means if something goes wrong we can abort the script so
the user knows something is broken rather than continuing blindly and seeing
errors later on that appear unrelated.    Picture a case where we fail to
add a flag to a config file.  The script continues until 20 packages later
it fails to build.  Tracking this back to the config file failure earlier
(unless there's a blatantly obvious message like "you need to add -x to
/etc/somefile") can be difficult.  Especially as I can see going to irc
saying "xxx didn't build".  And someone asking "What chroot version are
you?" and me responding with the result of cat ${VERSION_FILE} which would
be wrong.

[zbehan, 2010-09-15 17:21:45]

On Wed, Sep 15, 2010 at 9:49 AM, Jonathan Kliegman <kliegs@chromium.org>wrote:

>
> On Tue, Sep 14, 2010 at 9:25 PM, Zdenek Behan <zbehan@chromium.org> wrote:
>
>> On Tue, Sep 14, 2010 at 7:46 AM, <kliegs@chromium.org> wrote:
>>
>>> Do you have any thoughts/plans on how to implement a rollback if
>>> something goes
>>
>> wrong in a future update function?  Perhaps a "reset to version 0 and
>>> start
>>> over" clobber-type function?
>>>
>>
>> The function of this is really intended to be for simple easilly
>> scriptable actions, and replacement for those ACTION REQUIRED emails.
>> Invisible hand that reaches to other people's chroots.
>>
>> It should be functions that are damn near impossible to fail. Ex: chmod
>> this file if it exists; create this symlink if it doesn't exist; run sed on
>> this configuration file if it exists
>>
>> The intent is to avoid currently required manual steps, not to create
>> more. Once you introduce things like rollback, you have to start being
>> extremely careful about everything. If this fails, should I increase the
>> version number? If i can't increase the version number, what do i do?
>>
>> IMHO there's always going to be failure cases where some manual steps will
>> have to be taken, we want to minimize that to as few cases as possible, and
>> automate the simple actions.
>>
> I'm not arguing we'd never need manual steps.  Just that we should make it
> obvious when they are needed and have the framework support it.
>
> And just because its "damn near impossible" doesn't mean it won't happen
> and isn't an excuse to not check the results.
>
> However the case here I'm more concerned with is we release version 10.
>  Shortly afterwards we realize that version 10 has a bug in it that wasn't
> caught or anticipated.  We should be able to revert version 10.  That's what
> I mean by rollback - that we're able to undo a bad upgrade.
>

I think that rollback of 10 should be done by upgrade to 11, by the person
who broke it in 10. Automatic rollback is going to either make people who
write upgrades also write rollback, which is hopefully never going to get
used, and therefore be even less tested than the upgrade, or place some
heavy restrictions on what can be contained in the upgrade function.


>
>>  You mentioned this script is just a proposal.  Is this CL going to
>>> actually be
>>> submitted then?  Or just used as a point for gathering comments and
>>> reviews on a
>>> future script to be written?  I'm curious to see how the other scripts
>>> (specifically make_chroot) interact with this.
>>>
>>
>> Unless there will be strong objections, I'm all for submitting this bare
>> script without the actual integration, and do the Integration into other
>> scripts in other CL.
>>
> I don't object to it being released with integration. But I wanted to
> understand how the integration would occur.  Is there a reason though to
> release it without integrating?
>

There's an advantage: Short period of running manually by just the build
team and coming up with improvements that should be in version 0.


>  Finally is it worth adding a function to common.sh to get the chroot
>>> version?
>>> I can see a case where an upcoming change to the chroot is announced with
>>> a
>>> "When Chroot version 4 is announced your scripts should do X".  To allow
>>> other
>>> scripts or packages to be written to support feature X (and support
>>> testing)
>>> they could have conditionals with "if version >= X do y else do z".  This
>>> way
>>> other changes could be implemented while version 4 is being developed and
>>> tested
>>> and activate immediately on its release.
>>>
>>
>> Announcement is probably something that we want to avoid. Changes should
>> be transparent. Each rollup change should provide short description of what
>> it did, for easier debugging and less confusion of users, but people who
>> don't care can just skip the message and shouldn't have to worry about it.
>>
>
> I think my example might have been too broad.  I'm not looking at a global
> case for all developers being aware or impacted, but more a specific one
> within the infrastructure teams.  Lets say I'm working on a change to a
> config file that this script will update.  You are working on a change that
> would take advantage of my change if it exists, but needs to function
> without it since you're releasing ahead of mine.  If you build your code
> with the conditional (such as if CHROOT_VERSION > 3 CONFIG_OPTS=-a else
> CONFIG_OPTS=-b) you can release once and not need to release a new version
> once I submit my change.
>

I can imagine the use case for this, but I can also imagine how people would
never cleanup these and scripts would be littered with artifacts like this.
Also, we assume that a chroot is always going to get upgraded to the last
version, no matter what, so making a code that checks for certain version
has very little purpose, unless you write it for some future unreleased
version.


>
>>
>>> http://codereview.chromium.org/3331011/diff/15001/16001
>>>
>>> File cros_upgrade_chroot (right):
>>>
>>> http://codereview.chromium.org/3331011/diff/15001/16001#newcode20
>>> cros_upgrade_chroot:20: info "Nothing significant happens here, you've
>>> only been assimilated."
>>> Would still like to see this line removed.
>>>
>>
>> It adds the value of users noticing that there's something new going on.
>>
> I'm just arguing that you only need one line of output. The second line
> adds nothing - do it all in one line please.
>

OK

http://codereview.chromium.org/3331011/diff/15001/16001#newcode39
>>> cros_upgrade_chroot:39: echo "${LATEST_VERSION}" > "${VERSION_FILE}"
>>> If an upgrade_${n} function fails I think this will still be called.
>>> Running with 'set -e' and making sure the upgrade functions properly
>>> error can prevent a chroot from being set to the wrong versoin if a step
>>> fails.
>>>
>>> Making the upgrade functions explicitly return success or failure could
>>> be useful in this case.  Setting VERSION_FILE to LATEST_VERSION
>>> incorrectly would be a very bad and confusing things for users.
>>
>>
>> That is just replacing emails in your inbox saying "You broke my chroot,
>> what do I do" with emails saying "Your upgrade function didn't unbreak my
>> chroot, what do I do".
>> While I don't think that we should have extensive belief in the fact that
>> people will not make mistakes in the upgrade functions, this should not
>> become an action-response mechanism, and people shouldn't be adding stuff
>> like 'action || error "If this fails for you, I owe you a beer"'.
>>
>> I think in general, you're really imagining the solution to be something
>> bigger than it intends to be, I'm not trying to overengineer the problem,
>> just looking for a simple solution for simple things that take too many
>> people too much time, every day.
>>
>
> I can see this mechanism supporting more complex upgrade functions over
> time (although code reviewers can help keep that in check).  Even if there
> is no recoure other than "rebuild from scratch", "ask on IRC" or "manually
> edit file foo" we should at least check that what was attempted did work and
> react accordingly.  Setting the version when we aren't actually at that
> version will cause a lot more pain if something does go wrong than failing
> to set it.  It also means if something goes wrong we can abort the script so
> the user knows something is broken rather than continuing blindly and seeing
> errors later on that appear unrelated.    Picture a case where we fail to
> add a flag to a config file.  The script continues until 20 packages later
> it fails to build.  Tracking this back to the config file failure earlier
> (unless there's a blatantly obvious message like "you need to add -x to
> /etc/somefile") can be difficult.  Especially as I can see going to irc
> saying "xxx didn't build".  And someone asking "What chroot version are
> you?" and me responding with the result of cat ${VERSION_FILE} which would
> be wrong.


So in your concept, a version is more like a sequence of assertions. If
everything was upgraded to version 5, upgrade steps 1 2 3 4 5 all ran and
returned success, and you are therefore guaranteed to have fe. files X Y and
Z.

I was really trying to aproach it more trivially to avoid this problem,
because then you have to start deciding what to do if the upgrade failed.
Maybe allow the user to fix it and upgrade --continue? Maybe just ask the
user to recreate chroot? Automatically "retry" the last failed upgrade? That
would possibly impose restrictions on what people can place in the upgrade
functions. This is a significantly more complex problem than pushing trivial
actions to someone's chroot, though I'm not entirely disagreeing that we
want to go that way, i just think it's significantly more work than the
current approach, if you want to do it right. And I think that's exactly
what your concern is, too.

And this is another reason why I'd push it without integration. So that
other people can come up with a CL and we can discuss specific approaches to
these improvements you feel are necessary, but were originally not planned.

[kliegs, 2010-09-15 17:49:52]

On Wed, Sep 15, 2010 at 1:21 PM, Zdenek Behan <zbehan@chromium.org> wrote:

> On Wed, Sep 15, 2010 at 9:49 AM, Jonathan Kliegman <kliegs@chromium.org>wrote:
>
>>
>> On Tue, Sep 14, 2010 at 9:25 PM, Zdenek Behan <zbehan@chromium.org>wrote:
>>
>>>  On Tue, Sep 14, 2010 at 7:46 AM, <kliegs@chromium.org> wrote:
>>>
>>>> Do you have any thoughts/plans on how to implement a rollback if
>>>> something goes
>>>
>>> wrong in a future update function?  Perhaps a "reset to version 0 and
>>>> start
>>>> over" clobber-type function?
>>>>
>>>
>>> The function of this is really intended to be for simple easilly
>>> scriptable actions, and replacement for those ACTION REQUIRED emails.
>>> Invisible hand that reaches to other people's chroots.
>>>
>>> It should be functions that are damn near impossible to fail. Ex: chmod
>>> this file if it exists; create this symlink if it doesn't exist; run sed on
>>> this configuration file if it exists
>>>
>>> The intent is to avoid currently required manual steps, not to create
>>> more. Once you introduce things like rollback, you have to start being
>>> extremely careful about everything. If this fails, should I increase the
>>> version number? If i can't increase the version number, what do i do?
>>>
>>> IMHO there's always going to be failure cases where some manual steps
>>> will have to be taken, we want to minimize that to as few cases as possible,
>>> and automate the simple actions.
>>>
>> I'm not arguing we'd never need manual steps.  Just that we should make it
>> obvious when they are needed and have the framework support it.
>>
>> And just because its "damn near impossible" doesn't mean it won't happen
>> and isn't an excuse to not check the results.
>>
>> However the case here I'm more concerned with is we release version 10.
>>  Shortly afterwards we realize that version 10 has a bug in it that wasn't
>> caught or anticipated.  We should be able to revert version 10.  That's what
>> I mean by rollback - that we're able to undo a bad upgrade.
>>
>
> I think that rollback of 10 should be done by upgrade to 11, by the person
> who broke it in 10. Automatic rollback is going to either make people who
> write upgrades also write rollback, which is hopefully never going to get
> used, and therefore be even less tested than the upgrade, or place some
> heavy restrictions on what can be contained in the upgrade function.
>

I'm not thrilled with this but can live it. I just don't like seeing code
released without a clear path for a rollback (in most of the actual source
code commits a revert handles the rollback.  A script like this has
secondary effects which a code revert won't fix)


>
>
>>
>>>  You mentioned this script is just a proposal.  Is this CL going to
>>>> actually be
>>>> submitted then?  Or just used as a point for gathering comments and
>>>> reviews on a
>>>> future script to be written?  I'm curious to see how the other scripts
>>>> (specifically make_chroot) interact with this.
>>>>
>>>
>>> Unless there will be strong objections, I'm all for submitting this bare
>>> script without the actual integration, and do the Integration into other
>>> scripts in other CL.
>>>
>> I don't object to it being released with integration. But I wanted to
>> understand how the integration would occur.  Is there a reason though to
>> release it without integrating?
>>
>
> There's an advantage: Short period of running manually by just the build
> team and coming up with improvements that should be in version 0.
>

Good point.  I'd still be interested in seeing what the actual
implementation looks like but distributing this unlinked allows for wider
testing which is good.


>
>
>>   Finally is it worth adding a function to common.sh to get the chroot
>>>> version?
>>>> I can see a case where an upcoming change to the chroot is announced
>>>> with a
>>>> "When Chroot version 4 is announced your scripts should do X".  To allow
>>>> other
>>>> scripts or packages to be written to support feature X (and support
>>>> testing)
>>>> they could have conditionals with "if version >= X do y else do z".
>>>>  This way
>>>> other changes could be implemented while version 4 is being developed
>>>> and tested
>>>> and activate immediately on its release.
>>>>
>>>
>>> Announcement is probably something that we want to avoid. Changes should
>>> be transparent. Each rollup change should provide short description of what
>>> it did, for easier debugging and less confusion of users, but people who
>>> don't care can just skip the message and shouldn't have to worry about it.
>>>
>>
>> I think my example might have been too broad.  I'm not looking at a global
>> case for all developers being aware or impacted, but more a specific one
>> within the infrastructure teams.  Lets say I'm working on a change to a
>> config file that this script will update.  You are working on a change that
>> would take advantage of my change if it exists, but needs to function
>> without it since you're releasing ahead of mine.  If you build your code
>> with the conditional (such as if CHROOT_VERSION > 3 CONFIG_OPTS=-a else
>> CONFIG_OPTS=-b) you can release once and not need to release a new version
>> once I submit my change.
>>
>
> I can imagine the use case for this, but I can also imagine how people
> would never cleanup these and scripts would be littered with artifacts like
> this. Also, we assume that a chroot is always going to get upgraded to the
> last version, no matter what, so making a code that checks for certain
> version has very little purpose, unless you write it for some future
> unreleased version.
>
>
The last bit "unless you write it for some future unreleased version" is
what I was anticipating.  If an upcoming change takes two weeks to release
this lets people plan ahead.  It does have the risk you mentioned of
potentially convoluted scripts.  I guess I'd rather see the functionality
available from the beginning even if it isn't used (since it should only
take a few lines to implement this).  You have the function to determine the
version already so this just involves moving to a common include file.


>  http://codereview.chromium.org/3331011/diff/15001/16001#newcode39
>>>> cros_upgrade_chroot:39: echo "${LATEST_VERSION}" > "${VERSION_FILE}"
>>>> If an upgrade_${n} function fails I think this will still be called.
>>>> Running with 'set -e' and making sure the upgrade functions properly
>>>> error can prevent a chroot from being set to the wrong versoin if a step
>>>> fails.
>>>>
>>>> Making the upgrade functions explicitly return success or failure could
>>>> be useful in this case.  Setting VERSION_FILE to LATEST_VERSION
>>>> incorrectly would be a very bad and confusing things for users.
>>>
>>>
>>> That is just replacing emails in your inbox saying "You broke my chroot,
>>> what do I do" with emails saying "Your upgrade function didn't unbreak my
>>> chroot, what do I do".
>>> While I don't think that we should have extensive belief in the fact that
>>> people will not make mistakes in the upgrade functions, this should not
>>> become an action-response mechanism, and people shouldn't be adding stuff
>>> like 'action || error "If this fails for you, I owe you a beer"'.
>>>
>>> I think in general, you're really imagining the solution to be something
>>> bigger than it intends to be, I'm not trying to overengineer the problem,
>>> just looking for a simple solution for simple things that take too many
>>> people too much time, every day.
>>>
>>
>> I can see this mechanism supporting more complex upgrade functions over
>> time (although code reviewers can help keep that in check).  Even if there
>> is no recoure other than "rebuild from scratch", "ask on IRC" or "manually
>> edit file foo" we should at least check that what was attempted did work and
>> react accordingly.  Setting the version when we aren't actually at that
>> version will cause a lot more pain if something does go wrong than failing
>> to set it.  It also means if something goes wrong we can abort the script so
>> the user knows something is broken rather than continuing blindly and seeing
>> errors later on that appear unrelated.    Picture a case where we fail to
>> add a flag to a config file.  The script continues until 20 packages later
>> it fails to build.  Tracking this back to the config file failure earlier
>> (unless there's a blatantly obvious message like "you need to add -x to
>> /etc/somefile") can be difficult.  Especially as I can see going to irc
>> saying "xxx didn't build".  And someone asking "What chroot version are
>> you?" and me responding with the result of cat ${VERSION_FILE} which would
>> be wrong.
>
>
> So in your concept, a version is more like a sequence of assertions. If
> everything was upgraded to version 5, upgrade steps 1 2 3 4 5 all ran and
> returned success, and you are therefore guaranteed to have fe. files X Y and
> Z.
>
> I was really trying to aproach it more trivially to avoid this problem,
> because then you have to start deciding what to do if the upgrade failed.
> Maybe allow the user to fix it and upgrade --continue? Maybe just ask the
> user to recreate chroot? Automatically "retry" the last failed upgrade? That
> would possibly impose restrictions on what people can place in the upgrade
> functions. This is a significantly more complex problem than pushing trivial
> actions to someone's chroot, though I'm not entirely disagreeing that we
> want to go that way, i just think it's significantly more work than the
> current approach, if you want to do it right. And I think that's exactly
> what your concern is, too.
>
> And this is another reason why I'd push it without integration. So that
> other people can come up with a CL and we can discuss specific approaches to
> these improvements you feel are necessary, but were originally not planned.
>

If things do end up breaking down horribly I'm ok with the script giving up
and saying "We're sorry but your chroot has become corrupted.  These steps
failed. You need to rebuild your (chroot|/build) and these steps do that".
 Or if its a case where there are some failures we anticipate but can give
manual directions we could do "You can try doing xxxx (either directions or
link to them) and rerunning the script.  This may resolve the issue  If not
try yyy".  Including a way to report bugs on this script (paste this output
into bug xxx) or something could help get a patch in to the script to
prevent this from happening to others.  Even if there's no way to resolve a
problem, we should still abort and say "something is really bad" rather than
blithely continuing.

I think you summed up my argument there in the end pretty well: "it's
significantly more work than the current approach, if you want to do it
right".  This script has the potential to become central and creates entries
that are outside source control.  I'd rather we take an extra couple of
weeks to get it right as the long term savings are significant (forcing a
chroot rebuild is a 2 hour hit / developer impacted.  If taking a little
extra time on this script prevents even 4 rebuilds impacting 20 people each
that's 2 man-weeks worth of time we've saved).


Something else I've thought of which changes the scope.  This isn't really a
comment on the CL itself but an alternate mechanism.  Instead of having the
functions in the file itself, maybe allow sourcing of a conf.d type
directory.  I'm not sure if this really gains us anything other than
allowing us to separate CL's related to improving the framework vs. the
content of the changes (a change to the script would involve one type of
review and test, a change to the directory a different one).

[kliegs, 2010-09-17 15:46:49]

Thanks very much for moving the code in this direction.  I think it sets up a
much stronger framework and allows some of the future functionality you
mentioned (--force, --skip) to be added in easily afterwards.    I know you've
got the one bug you wrote for me re: testing, but you had a lot of other
thoughts about useful additions. Are those documented somewhere?  I want to make
sure they don't get lost.

http://codereview.chromium.org/3331011/diff/27001/28001
File cros_upgrade_chroot (right):

http://codereview.chromium.org/3331011/diff/27001/28001#newcode22
cros_upgrade_chroot:22: LATEST_VERSION=$(
I'd like to see latest_version defined by a config parameter.  This allows
distribution of a file for testing in different environments (or a "you have
this problem I think this will fix it. test it and i'll push live).

http://codereview.chromium.org/3331011/diff/27001/28001#newcode34
cros_upgrade_chroot:34: if ! expr match "${CHROOT_VERSION}" '^[0-9]*$' &>
/dev/null; then
Should this be [0-9]+ to cover the empty line?

An empty line might be worth turning into '0' always but I'm not sure. I'm fine
with any bogus value being thrown out as fatal to simplify and it can be
extended later if its a common problem (don't expect it to be unless users
constantly have disks space errors)

http://codereview.chromium.org/3331011/diff/27001/28001#newcode44
cros_upgrade_chroot:44: # Deprecation check; Deprecation can be done by removing
old upgrade
Not sure I fully understand this.  Is this meant to force a new make_chroot
--force to be run which will automatically start at a version > 0 in the case
that changes have become too compatible?

http://codereview.chromium.org/3331011/diff/27001/28001#newcode48
cros_upgrade_chroot:48: error "Fatal: Upgrade ${n} doesn't exist. Your chroot is
too old!"
If my assumption above (forcing a fresh chroot image) then changing this error
message to something informative like:
Upgrade ${n} doesn't exist.  This usually indicates that your chroot is 
too out of date to patch.  Exiting the chroot and running ./make_chroot --force
should fix this. 

I'd probably want to spend a few more minutes brain storming on if there are
scenarios where there's a gap in the ordering and you wouldn't want to run a
fresh make_chroot (other than 'rm update.d/$(rand)' equivalents).  But an
informative "how to fix" message here seems appropriate as this is either the
result of a bad sync (likelihood of this?) or will never resolve itself
automatically.

http://codereview.chromium.org/3331011/diff/27001/28001#newcode62
cros_upgrade_chroot:62: if ! source ${n}_*; then
Would eval make more sense?  I believe it protects against exit calls

http://codereview.chromium.org/3331011/diff/27001/28001#newcode63
cros_upgrade_chroot:63: error "Fatal: failed to upgrade!"
It's probably worth repeating the version/script that failed again here.  Just
in case the script spews too much out its nice to have the final error line
indicate the actual file that caused the problems.
Printing out $? may be useful as well

[zbehan, 2010-09-24 03:18:58]

On Fri, Sep 17, 2010 at 8:46 AM, <kliegs@chromium.org> wrote:

> Thanks very much for moving the code in this direction.  I think it sets up
> a
> much stronger framework and allows some of the future functionality you
> mentioned (--force, --skip) to be added in easily afterwards.    I know
> you've
> got the one bug you wrote for me re: testing, but you had a lot of other
> thoughts about useful additions. Are those documented somewhere?  I want to
> make
> sure they don't get lost.
>

I'm not sure which you mean.


> http://codereview.chromium.org/3331011/diff/27001/28001
>
> File cros_upgrade_chroot (right):
>
> http://codereview.chromium.org/3331011/diff/27001/28001#newcode22
> cros_upgrade_chroot:22: LATEST_VERSION=$(
> I'd like to see latest_version defined by a config parameter.  This
> allows distribution of a file for testing in different environments (or
> a "you have this problem I think this will fix it. test it and i'll push
> live).
>

Added.

http://codereview.chromium.org/3331011/diff/27001/28001#newcode34
> cros_upgrade_chroot:34: if ! expr match "${CHROOT_VERSION}" '^[0-9]*$'
> &> /dev/null; then
> Should this be [0-9]+ to cover the empty line?
>

I took a different approach; [ $var -gt 0 ] &> /dev/null dies if $var is NaN

An empty line might be worth turning into '0' always but I'm not sure.
> I'm fine with any bogus value being thrown out as fatal to simplify and
> it can be extended later if its a common problem (don't expect it to be
> unless users constantly have disks space errors)
>

Sounds overcomplicated to me, and details can be resolved later when it's
put into "production".

http://codereview.chromium.org/3331011/diff/27001/28001#newcode44
> cros_upgrade_chroot:44: # Deprecation check; Deprecation can be done by
> removing old upgrade
> Not sure I fully understand this.  Is this meant to force a new
> make_chroot --force to be run which will automatically start at a
> version > 0 in the case that changes have become too compatible?
>

It's supposed to give us the possibility to simply delete some of the super
old upgrades and "deprecate" chroots this old.

http://codereview.chromium.org/3331011/diff/27001/28001#newcode48
> cros_upgrade_chroot:48: error "Fatal: Upgrade ${n} doesn't exist. Your
> chroot is too old!"
> If my assumption above (forcing a fresh chroot image) then changing this
> error message to something informative like:
> Upgrade ${n} doesn't exist.  This usually indicates that your chroot is
> too out of date to patch.  Exiting the chroot and running ./make_chroot
> --force
> should fix this.
>
> I'd probably want to spend a few more minutes brain storming on if there
> are scenarios where there's a gap in the ordering and you wouldn't want
> to run a fresh make_chroot (other than 'rm update.d/$(rand)'
> equivalents).  But an informative "how to fix" message here seems
> appropriate as this is either the result of a bad sync (likelihood of
> this?) or will never resolve itself automatically.
>

I honestly think you're overcomplicating this CL. There is no need for any
scenario more complicated than "fix a symlink" at the moment. When such
scenarios appear, I'm all for making support for them. After they appear.

http://codereview.chromium.org/3331011/diff/27001/28001#newcode62
> cros_upgrade_chroot:62: if ! source ${n}_*; then

Would eval make more sense?  I believe it protects against exit calls
>

Eval does one level of expansion before executing the command as is. I'm not
seeing why it should protect against exit calls, and I'm also pretty sure it
doesn't.

http://codereview.chromium.org/3331011/diff/27001/28001#newcode63
> cros_upgrade_chroot:63: error "Fatal: failed to upgrade!"
> It's probably worth repeating the version/script that failed again here.
>  Just in case the script spews too much out its nice to have the final
> error line indicate the actual file that caused the problems.
> Printing out $? may be useful as well


Fixed.

[kliegs, 2010-09-24 14:51:05]

http://codereview.chromium.org/3331011/diff/33001/34001
File cros_upgrade_chroot (right):

http://codereview.chromium.org/3331011/diff/33001/34001#newcode33
cros_upgrade_chroot:33: if ! [ "${CHROOT_VERSION}" -ge "0" ] &> /dev/null; then
In another CL I was just told this form &> isn't posix compliant.  I personally
like this form better (code looks cleaner!) but we should be consistent between
scripts

[kliegs, 2010-09-24 14:51:46]

On Thu, Sep 23, 2010 at 11:18 PM, Zdenek Behan <zbehan@chromium.org> wrote:

> On Fri, Sep 17, 2010 at 8:46 AM, <kliegs@chromium.org> wrote:
>
>> Thanks very much for moving the code in this direction.  I think it sets
>> up a
>> much stronger framework and allows some of the future functionality you
>> mentioned (--force, --skip) to be added in easily afterwards.    I know
>> you've
>> got the one bug you wrote for me re: testing, but you had a lot of other
>> thoughts about useful additions. Are those documented somewhere?  I want
>> to make
>> sure they don't get lost.
>>
>
> I'm not sure which you mean.
>

You'd mentioned wanting --force and --skip commands but it looks like there
isn't time to implement them before your internship ends.  I just wanted to
make sure those enhancements (and others you might have had) were listed
somewhere so the next person to work on them can get a headstart.

> I'd like to see latest_version defined by a config parameter.  This
>> allows distribution of a file for testing in different environments (or
>> a "you have this problem I think this will fix it. test it and i'll push
>> live).
>>
>
> Added.
>

Sorry - I think I was unclear on what I meant by a config paramater.  What
you added is good but I'd like to see a default value contained in the
script.  My goal was that installing a new file wouldn't automatically force
all scripts to update it.  You could install a file, distribute for test and
then bump the config value up so everyone else gets it.  Adding a flag for
testing/overriding makes sense (especially for testers) but it feels like
this value should have a default (if I remember right this is intended to be
called from multiple files so they each shouldn't need logic to determine
the correct version).

> An empty line might be worth turning into '0' always but I'm not sure.
>> I'm fine with any bogus value being thrown out as fatal to simplify and
>> it can be extended later if its a common problem (don't expect it to be
>> unless users constantly have disks space errors)
>>
>
> Sounds overcomplicated to me, and details can be resolved later when it's
> put into "production".
>

I think we agree on that.  If there are reports that this file is
consistently getting corrupted its probably better to know anyways as that
usually means something else is wrong.


>
> http://codereview.chromium.org/3331011/diff/27001/28001#newcode44
>> cros_upgrade_chroot:44: # Deprecation check; Deprecation can be done by
>> removing old upgrade
>> Not sure I fully understand this.  Is this meant to force a new
>> make_chroot --force to be run which will automatically start at a
>> version > 0 in the case that changes have become too compatible?
>>
>
> It's supposed to give us the possibility to simply delete some of the super
> old upgrades and "deprecate" chroots this old.
>
> http://codereview.chromium.org/3331011/diff/27001/28001#newcode48
>> cros_upgrade_chroot:48: error "Fatal: Upgrade ${n} doesn't exist. Your
>> chroot is too old!"
>> If my assumption above (forcing a fresh chroot image) then changing this
>> error message to something informative like:
>> Upgrade ${n} doesn't exist.  This usually indicates that your chroot is
>> too out of date to patch.  Exiting the chroot and running ./make_chroot
>> --force
>> should fix this.
>>
>> I'd probably want to spend a few more minutes brain storming on if there
>> are scenarios where there's a gap in the ordering and you wouldn't want
>> to run a fresh make_chroot (other than 'rm update.d/$(rand)'
>> equivalents).  But an informative "how to fix" message here seems
>> appropriate as this is either the result of a bad sync (likelihood of
>> this?) or will never resolve itself automatically.
>>
>
> I honestly think you're overcomplicating this CL. There is no need for any
> scenario more complicated than "fix a symlink" at the moment. When such
> scenarios appear, I'm all for making support for them. After they appear.
>

I think you may have missed my concern/request.  If you've got logic in
there to detect an old/deprecated chroot then there should be a command on
how to fix it (even if its just "run this to clobber).   Giving a fatal
error with no obvious way to fix it will just frustrate people.  You can add
in "ask on irc" or "email the list" or "give up and start over".  There
doesn't need to be code to automatically fix this, just a friendlier error
message that helps the user.

 I also wanted to make sure you had a plan for how you would end up in this
state and what the flow would look like (make_chroot setting the
VERSION_FILE on create?).  It doesn't need to be implemented, but it should
be thought out.




>
> http://codereview.chromium.org/3331011/diff/27001/28001#newcode62
>> cros_upgrade_chroot:62: if ! source ${n}_*; then
>
> Would eval make more sense?  I believe it protects against exit calls
>>
>
> Eval does one level of expansion before executing the command as is. I'm
> not seeing why it should protect against exit calls, and I'm also pretty
> sure it doesn't.
>
You mentioned a concern with having to wrap the target files.  Eval does
protect against this:
file 1:
  #!/bin/bash
  eval foo.sh
  echo done
foo.sh:
  #!/bin/bash
  echo hi
  exit 1

Output:
hi
done

Downside is eval tosses out local variables so a tradeoff.  It makes things
safer (scripts can't accidentally cause the main loop to exit) but more care
needs to be taken with environment variables.

[zbehan, 2010-09-24 17:35:20]

On Fri, Sep 24, 2010 at 7:51 AM, Jonathan Kliegman <kliegs@chromium.org>wrote:

> On Thu, Sep 23, 2010 at 11:18 PM, Zdenek Behan <zbehan@chromium.org>wrote:
>
>> On Fri, Sep 17, 2010 at 8:46 AM, <kliegs@chromium.org> wrote:
>>
>>> Thanks very much for moving the code in this direction.  I think it sets
>>> up a
>>> much stronger framework and allows some of the future functionality you
>>> mentioned (--force, --skip) to be added in easily afterwards.    I know
>>> you've
>>> got the one bug you wrote for me re: testing, but you had a lot of other
>>> thoughts about useful additions. Are those documented somewhere?  I want
>>> to make
>>> sure they don't get lost.
>>>
>>
>> I'm not sure which you mean.
>>
>
> You'd mentioned wanting --force and --skip commands but it looks like there
> isn't time to implement them before your internship ends.  I just wanted to
> make sure those enhancements (and others you might have had) were listed
> somewhere so the next person to work on them can get a headstart.
>

Yes, I can put those in really easilly. In fact i was just about to, but I
had to catch my shuttle yesterday.


>  I'd like to see latest_version defined by a config parameter.  This
>>> allows distribution of a file for testing in different environments (or
>>> a "you have this problem I think this will fix it. test it and i'll push
>>> live).
>>>
>>
>> Added.
>>
>
> Sorry - I think I was unclear on what I meant by a config paramater.  What
> you added is good but I'd like to see a default value contained in the
> script.  My goal was that installing a new file wouldn't automatically force
> all scripts to update it.  You could install a file, distribute for test and
> then bump the config value up so everyone else gets it.  Adding a flag for
> testing/overriding makes sense (especially for testers) but it feels like
> this value should have a default (if I remember right this is intended to be
> called from multiple files so they each shouldn't need logic to determine
> the correct version).
>

I disagree. "staging" version files seem unnecessarily complicated, and I
can't imagine a use for that.


>  An empty line might be worth turning into '0' always but I'm not sure.
>>> I'm fine with any bogus value being thrown out as fatal to simplify and
>>> it can be extended later if its a common problem (don't expect it to be
>>> unless users constantly have disks space errors)
>>>
>>
>> Sounds overcomplicated to me, and details can be resolved later when it's
>> put into "production".
>>
>
> I think we agree on that.  If there are reports that this file is
> consistently getting corrupted its probably better to know anyways as that
> usually means something else is wrong.
>
>
>>
>> http://codereview.chromium.org/3331011/diff/27001/28001#newcode44
>>> cros_upgrade_chroot:44: # Deprecation check; Deprecation can be done by
>>> removing old upgrade
>>> Not sure I fully understand this.  Is this meant to force a new
>>> make_chroot --force to be run which will automatically start at a
>>> version > 0 in the case that changes have become too compatible?
>>>
>>
>> It's supposed to give us the possibility to simply delete some of the
>> super old upgrades and "deprecate" chroots this old.
>>
>> http://codereview.chromium.org/3331011/diff/27001/28001#newcode48
>>> cros_upgrade_chroot:48: error "Fatal: Upgrade ${n} doesn't exist. Your
>>> chroot is too old!"
>>> If my assumption above (forcing a fresh chroot image) then changing this
>>> error message to something informative like:
>>> Upgrade ${n} doesn't exist.  This usually indicates that your chroot is
>>> too out of date to patch.  Exiting the chroot and running ./make_chroot
>>> --force
>>> should fix this.
>>>
>>> I'd probably want to spend a few more minutes brain storming on if there
>>> are scenarios where there's a gap in the ordering and you wouldn't want
>>> to run a fresh make_chroot (other than 'rm update.d/$(rand)'
>>> equivalents).  But an informative "how to fix" message here seems
>>> appropriate as this is either the result of a bad sync (likelihood of
>>> this?) or will never resolve itself automatically.
>>>
>>
>> I honestly think you're overcomplicating this CL. There is no need for any
>> scenario more complicated than "fix a symlink" at the moment. When such
>> scenarios appear, I'm all for making support for them. After they appear.
>>
>
> I think you may have missed my concern/request.  If you've got logic in
> there to detect an old/deprecated chroot then there should be a command on
> how to fix it (even if its just "run this to clobber).   Giving a fatal
> error with no obvious way to fix it will just frustrate people.  You can add
> in "ask on irc" or "email the list" or "give up and start over".  There
> doesn't need to be code to automatically fix this, just a friendlier error
> message that helps the user.
>
>  I also wanted to make sure you had a plan for how you would end up in this
> state and what the flow would look like (make_chroot setting the
> VERSION_FILE on create?).  It doesn't need to be implemented, but it should
> be thought out.
>

I'm pretty sure I understood what you mean, but don't like that you're
trying to transform this functionality into something else, just because you
believe you have better plans. Gap between CURRENT and FIRST_KNOWN means
that we have discontinued a very old chroot (noone says that such an action
will be ever done or necessary), and the obvious fix is to start over. And
frankly, if someone makes a "gap" in a normal sequence of versions, I don't
know what better action to take than to die. And why would anyone even want
to do that? Versions should be added, not retrospectively modified.

This also gives us the ability to pull in changes that "kill the chroot".
You just make a gap, bump up the default version, and this tells everyone
that they have to recreate their chroot, in case the change is so massive
that there's no way to convert. And that only after exhausting all the
possible options on how to upgrade. Without sending a single email to
anyone. (which is the purpose of this script)

http://codereview.chromium.org/3331011/diff/27001/28001#newcode62
>>> cros_upgrade_chroot:62: if ! source ${n}_*; then
>>
>> Would eval make more sense?  I believe it protects against exit calls
>>>
>>
>> Eval does one level of expansion before executing the command as is. I'm
>> not seeing why it should protect against exit calls, and I'm also pretty
>> sure it doesn't.
>>
> You mentioned a concern with having to wrap the target files.  Eval does
> protect against this:
> file 1:
>   #!/bin/bash
>   eval foo.sh
>   echo done
> foo.sh:
>   #!/bin/bash
>   echo hi
>   exit 1
>
> Output:
> hi
> done
>
> Downside is eval tosses out local variables so a tradeoff.  It makes things
> safer (scripts can't accidentally cause the main loop to exit) but more care
> needs to be taken with environment variables.
>

What you did here is "eval the line containing foo.sh", which does
absolutely nothing, and then executes foo.sh. It's exactly equivalent to
just "foo.sh". If they're two separate scripts, running return/exit inside
the second one will not terminate the parent shell. I'm doing a "source
foo.sh" exactly for the reason that it preserves all variables, functions,
etc., but a return/exit function will kill the top-level script, because the
sourced script is not executed in a separate process, rather executed in the
same context. For return to start working, the scripts have to be wrapped in
a code block, as I already did.

[kliegs, 2010-09-24 18:50:19]

On Fri, Sep 24, 2010 at 1:35 PM, Zdenek Behan <zbehan@chromium.org> wrote:

> On Fri, Sep 24, 2010 at 7:51 AM, Jonathan Kliegman <kliegs@chromium.org>wrote:
>
>> Sorry - I think I was unclear on what I meant by a config paramater.  What
>> you added is good but I'd like to see a default value contained in the
>> script.  My goal was that installing a new file wouldn't automatically force
>> all scripts to update it.  You could install a file, distribute for test and
>> then bump the config value up so everyone else gets it.  Adding a flag for
>> testing/overriding makes sense (especially for testers) but it feels like
>> this value should have a default (if I remember right this is intended to be
>> called from multiple files so they each shouldn't need logic to determine
>> the correct version).
>>
>
> I disagree. "staging" version files seem unnecessarily complicated, and I
> can't imagine a use for that.
>

Staging files allows for increased testing and canaries.  We've seen
previous chroot upgrades corrupt the chroot so the ability to do a staged or
staggered rollout could be helpful. Yes - this could also be mimicked with
'git cl patch ...' patterns but this mechanism lets us do more testing for
less overall work (One time 10 minute cose to implement in now vs. 1 min /
tester / patch).


>  http://codereview.chromium.org/3331011/diff/27001/28001#newcode44
>
>>  cros_upgrade_chroot:44: # Deprecation check; Deprecation can be done by
>>>> removing old upgrade
>>>> Not sure I fully understand this.  Is this meant to force a new
>>>> make_chroot --force to be run which will automatically start at a
>>>> version > 0 in the case that changes have become too compatible?
>>>>
>>>
>>> It's supposed to give us the possibility to simply delete some of the
>>> super old upgrades and "deprecate" chroots this old.
>>>
>>> http://codereview.chromium.org/3331011/diff/27001/28001#newcode48
>>>> cros_upgrade_chroot:48: error "Fatal: Upgrade ${n} doesn't exist. Your
>>>> chroot is too old!"
>>>> If my assumption above (forcing a fresh chroot image) then changing this
>>>> error message to something informative like:
>>>> Upgrade ${n} doesn't exist.  This usually indicates that your chroot is
>>>> too out of date to patch.  Exiting the chroot and running ./make_chroot
>>>> --force
>>>> should fix this.
>>>>
>>>> I'd probably want to spend a few more minutes brain storming on if there
>>>> are scenarios where there's a gap in the ordering and you wouldn't want
>>>> to run a fresh make_chroot (other than 'rm update.d/$(rand)'
>>>> equivalents).  But an informative "how to fix" message here seems
>>>> appropriate as this is either the result of a bad sync (likelihood of
>>>> this?) or will never resolve itself automatically.
>>>>
>>>
>>> I honestly think you're overcomplicating this CL. There is no need for
>>> any scenario more complicated than "fix a symlink" at the moment. When such
>>> scenarios appear, I'm all for making support for them. After they appear.
>>>
>>
>> I think you may have missed my concern/request.  If you've got logic in
>> there to detect an old/deprecated chroot then there should be a command on
>> how to fix it (even if its just "run this to clobber).   Giving a fatal
>> error with no obvious way to fix it will just frustrate people.  You can add
>> in "ask on irc" or "email the list" or "give up and start over".  There
>> doesn't need to be code to automatically fix this, just a friendlier error
>> message that helps the user.
>>
>>  I also wanted to make sure you had a plan for how you would end up in
>> this state and what the flow would look like (make_chroot setting the
>> VERSION_FILE on create?).  It doesn't need to be implemented, but it should
>> be thought out.
>>
>
> I'm pretty sure I understood what you mean, but don't like that you're
> trying to transform this functionality into something else, just because you
> believe you have better plans. Gap between CURRENT and FIRST_KNOWN means
> that we have discontinued a very old chroot (noone says that such an action
> will be ever done or necessary), and the obvious fix is to start over. And
> frankly, if someone makes a "gap" in a normal sequence of versions, I don't
> know what better action to take than to die. And why would anyone even want
> to do that? Versions should be added, not retrospectively modified.
>
> This also gives us the ability to pull in changes that "kill the chroot".
> You just make a gap, bump up the default version, and this tells everyone
> that they have to recreate their chroot, in case the change is so massive
> that there's no way to convert. And that only after exhausting all the
> possible options on how to upgrade. Without sending a single email to
> anyone. (which is the purpose of this script)
>
>
I'm agreeing that dying is fine. And the ability to kill the chroot if
necessary is fine.

What I'm asking for is that the error message actually tell the user what do
do (i.e. rebuild chroot).  If in every case where we get this error and a
user asks what do do and we respond with "rebuild chroot". Then just put
"rebuild chroot" into the error message with the actual command line to do
so.  This way the user can fix the problem and they don't need to contact
everyone.


>
> What you did here is "eval the line containing foo.sh", which does
> absolutely nothing, and then executes foo.sh. It's exactly equivalent to
> just "foo.sh". If they're two separate scripts, running return/exit inside
> the second one will not terminate the parent shell. I'm doing a "source
> foo.sh" exactly for the reason that it preserves all variables, functions,
> etc., but a return/exit function will kill the top-level script, because the
> sourced script is not executed in a separate process, rather executed in the
> same context. For return to start working, the scripts have to be wrapped in
> a code block, as I already did.


Yah.  Guess somewhere I got used to using eval when calling shellscripts.
Bad habit I must have picked up.  Like you said its a tradeoff - I tend to
lean more towards not allowing a third party script to not crash the caller.
 But I can see the argument for pre-processing the variables in a central
place.  Passing the variables defined in this script should be easy enough
(just do export ...) but included files like common.sh wouldn't work.
 Although if the reviewers are on top of things for the individual scripts
this shouldn't be a problem (and likewise if the test script is written
correctly to cover this case it can be caught there).    I'm fine with it
staying how it is.


Lastly any thoughts on &> /dev/null vs  > /dev/null 2>&1?  I was told in
another CL the first method isn't portable (not posix compliant) and it
makes sense to me that we should keep our implementations consistent between
scripts.

[Mandeep Singh Baines, 2010-09-24 20:51:45]

On 2010/09/24 18:50:19, kliegs wrote:
> On Fri, Sep 24, 2010 at 1:35 PM, Zdenek Behan <mailto:zbehan@chromium.org>
wrote:
> 
> > On Fri, Sep 24, 2010 at 7:51 AM, Jonathan Kliegman
<kliegs@chromium.org>wrote:
> >
> >> Sorry - I think I was unclear on what I meant by a config paramater.  What
> >> you added is good but I'd like to see a default value contained in the
> >> script.  My goal was that installing a new file wouldn't automatically
force
> >> all scripts to update it.  You could install a file, distribute for test
and
> >> then bump the config value up so everyone else gets it.  Adding a flag for
> >> testing/overriding makes sense (especially for testers) but it feels like
> >> this value should have a default (if I remember right this is intended to
be
> >> called from multiple files so they each shouldn't need logic to determine
> >> the correct version).
> >>
> >
> > I disagree. "staging" version files seem unnecessarily complicated, and I
> > can't imagine a use for that.
> >
> 
> Staging files allows for increased testing and canaries.  We've seen
> previous chroot upgrades corrupt the chroot so the ability to do a staged or
> staggered rollout could be helpful. Yes - this could also be mimicked with
> 'git cl patch ...' patterns but this mechanism lets us do more testing for
> less overall work (One time 10 minute cose to implement in now vs. 1 min /
> tester / patch).
> 

I'm worried that this would add more complexity that may or may not be needed.
Every new feature increases the state space we need to support: we'd now need to
handle a bunch more cases where developer applied a "staged" update.

Can we defer this till its absolutely needed? For this feature, not implementing
now, does not preclude it being added later. There is no "point of no return" on
this particular feature.

> 
> >  http://codereview.chromium.org/3331011/diff/27001/28001#newcode44
> >
> >>  cros_upgrade_chroot:44: # Deprecation check; Deprecation can be done by
> >>>> removing old upgrade
> >>>> Not sure I fully understand this.  Is this meant to force a new
> >>>> make_chroot --force to be run which will automatically start at a
> >>>> version > 0 in the case that changes have become too compatible?
> >>>>
> >>>
> >>> It's supposed to give us the possibility to simply delete some of the
> >>> super old upgrades and "deprecate" chroots this old.
> >>>
> >>> http://codereview.chromium.org/3331011/diff/27001/28001#newcode48
> >>>> cros_upgrade_chroot:48: error "Fatal: Upgrade ${n} doesn't exist. Your
> >>>> chroot is too old!"
> >>>> If my assumption above (forcing a fresh chroot image) then changing this
> >>>> error message to something informative like:
> >>>> Upgrade ${n} doesn't exist.  This usually indicates that your chroot is
> >>>> too out of date to patch.  Exiting the chroot and running ./make_chroot
> >>>> --force
> >>>> should fix this.
> >>>>
> >>>> I'd probably want to spend a few more minutes brain storming on if there
> >>>> are scenarios where there's a gap in the ordering and you wouldn't want
> >>>> to run a fresh make_chroot (other than 'rm update.d/$(rand)'
> >>>> equivalents).  But an informative "how to fix" message here seems
> >>>> appropriate as this is either the result of a bad sync (likelihood of
> >>>> this?) or will never resolve itself automatically.
> >>>>
> >>>
> >>> I honestly think you're overcomplicating this CL. There is no need for
> >>> any scenario more complicated than "fix a symlink" at the moment. When
such
> >>> scenarios appear, I'm all for making support for them. After they appear.
> >>>
> >>
> >> I think you may have missed my concern/request.  If you've got logic in
> >> there to detect an old/deprecated chroot then there should be a command on
> >> how to fix it (even if its just "run this to clobber).   Giving a fatal
> >> error with no obvious way to fix it will just frustrate people.  You can
add
> >> in "ask on irc" or "email the list" or "give up and start over".  There
> >> doesn't need to be code to automatically fix this, just a friendlier error
> >> message that helps the user.
> >>
> >>  I also wanted to make sure you had a plan for how you would end up in
> >> this state and what the flow would look like (make_chroot setting the
> >> VERSION_FILE on create?).  It doesn't need to be implemented, but it should
> >> be thought out.
> >>
> >
> > I'm pretty sure I understood what you mean, but don't like that you're
> > trying to transform this functionality into something else, just because you
> > believe you have better plans. Gap between CURRENT and FIRST_KNOWN means
> > that we have discontinued a very old chroot (noone says that such an action
> > will be ever done or necessary), and the obvious fix is to start over. And
> > frankly, if someone makes a "gap" in a normal sequence of versions, I don't
> > know what better action to take than to die. And why would anyone even want
> > to do that? Versions should be added, not retrospectively modified.
> >
> > This also gives us the ability to pull in changes that "kill the chroot".
> > You just make a gap, bump up the default version, and this tells everyone
> > that they have to recreate their chroot, in case the change is so massive
> > that there's no way to convert. And that only after exhausting all the
> > possible options on how to upgrade. Without sending a single email to
> > anyone. (which is the purpose of this script)
> >
> >
> I'm agreeing that dying is fine. And the ability to kill the chroot if
> necessary is fine.
> 
> What I'm asking for is that the error message actually tell the user what do
> do (i.e. rebuild chroot).  If in every case where we get this error and a
> user asks what do do and we respond with "rebuild chroot". Then just put
> "rebuild chroot" into the error message with the actual command line to do
> so.  This way the user can fix the problem and they don't need to contact
> everyone.
> 
> 
> >
> > What you did here is "eval the line containing foo.sh", which does
> > absolutely nothing, and then executes foo.sh. It's exactly equivalent to
> > just "foo.sh". If they're two separate scripts, running return/exit inside
> > the second one will not terminate the parent shell. I'm doing a "source
> > foo.sh" exactly for the reason that it preserves all variables, functions,
> > etc., but a return/exit function will kill the top-level script, because the
> > sourced script is not executed in a separate process, rather executed in the
> > same context. For return to start working, the scripts have to be wrapped in
> > a code block, as I already did.
> 
> 
> Yah.  Guess somewhere I got used to using eval when calling shellscripts.
> Bad habit I must have picked up.  Like you said its a tradeoff - I tend to
> lean more towards not allowing a third party script to not crash the caller.
>  But I can see the argument for pre-processing the variables in a central
> place.  Passing the variables defined in this script should be easy enough
> (just do export ...) but included files like common.sh wouldn't work.
>  Although if the reviewers are on top of things for the individual scripts
> this shouldn't be a problem (and likewise if the test script is written
> correctly to cover this case it can be caught there).    I'm fine with it
> staying how it is.
> 
> 
> Lastly any thoughts on &> /dev/null vs  > /dev/null 2>&1?  I was told in
> another CL the first method isn't portable (not posix compliant) and it
> makes sense to me that we should keep our implementations consistent between
> scripts.
>

[kliegs, 2010-09-24 22:24:42]

On Fri, Sep 24, 2010 at 4:51 PM, <msb@chromium.org> wrote:

> On 2010/09/24 18:50:19, kliegs wrote:
>
>> On Fri, Sep 24, 2010 at 1:35 PM, Zdenek Behan <mailto:zbehan@chromium.org
>> >
>>
> wrote:
>
>  > On Fri, Sep 24, 2010 at 7:51 AM, Jonathan Kliegman
>>
> <kliegs@chromium.org>wrote:
>
>> >
>> >> Sorry - I think I was unclear on what I meant by a config paramater.
>>  What
>> >> you added is good but I'd like to see a default value contained in the
>> >> script.  My goal was that installing a new file wouldn't automatically
>>
> force
>
>> >> all scripts to update it.  You could install a file, distribute for
>> test
>>
> and
>
>> >> then bump the config value up so everyone else gets it.  Adding a flag
>> for
>> >> testing/overriding makes sense (especially for testers) but it feels
>> like
>> >> this value should have a default (if I remember right this is intended
>> to
>>
> be
>
>> >> called from multiple files so they each shouldn't need logic to
>> determine
>> >> the correct version).
>> >>
>> >
>> > I disagree. "staging" version files seem unnecessarily complicated, and
>> I
>> > can't imagine a use for that.
>> >
>>
>
>  Staging files allows for increased testing and canaries.  We've seen
>> previous chroot upgrades corrupt the chroot so the ability to do a staged
>> or
>> staggered rollout could be helpful. Yes - this could also be mimicked with
>> 'git cl patch ...' patterns but this mechanism lets us do more testing for
>> less overall work (One time 10 minute cose to implement in now vs. 1 min /
>> tester / patch).
>>
>
>
> I'm worried that this would add more complexity that may or may not be
> needed.
> Every new feature increases the state space we need to support: we'd now
> need to
> handle a bunch more cases where developer applied a "staged" update.
>
> Can we defer this till its absolutely needed? For this feature, not
> implementing
> now, does not preclude it being added later. There is no "point of no
> return" on
> this particular feature


I guess I saw the staged updates as being used by people who know what
they're doing and willing to test so the resultant configuration wouldn't
need to be supported.

In terms of things I want to really fight over this isn't one so if you feel
strongly it should go out without it I won't object.  After first seeing
this script I imagined it as a define at the top of the file stating the
expected version.  Basically just replace the whole file system lookup
section with
LATEST_VERSION=1
instead of the complicated ls|grep|cut section.
And this leaves room in the future for more complicated logic if we ever do
need canaries.  And allows someone testing to easily tweak the file (or add
a flag to override it later).


>
>
>  >  http://codereview.chromium.org/3331011/diff/27001/28001#newcode44
>> >
>> >>  cros_upgrade_chroot:44: # Deprecation check; Deprecation can be done
>> by
>> >>>> removing old upgrade
>> >>>> Not sure I fully understand this.  Is this meant to force a new
>> >>>> make_chroot --force to be run which will automatically start at a
>> >>>> version > 0 in the case that changes have become too compatible?
>> >>>>
>> >>>
>> >>> It's supposed to give us the possibility to simply delete some of the
>> >>> super old upgrades and "deprecate" chroots this old.
>> >>>
>> >>> http://codereview.chromium.org/3331011/diff/27001/28001#newcode48
>> >>>> cros_upgrade_chroot:48: error "Fatal: Upgrade ${n} doesn't exist.
>> Your
>> >>>> chroot is too old!"
>> >>>> If my assumption above (forcing a fresh chroot image) then changing
>> this
>> >>>> error message to something informative like:
>> >>>> Upgrade ${n} doesn't exist.  This usually indicates that your chroot
>> is
>> >>>> too out of date to patch.  Exiting the chroot and running
>> ./make_chroot
>> >>>> --force
>> >>>> should fix this.
>> >>>>
>> >>>> I'd probably want to spend a few more minutes brain storming on if
>> there
>> >>>> are scenarios where there's a gap in the ordering and you wouldn't
>> want
>> >>>> to run a fresh make_chroot (other than 'rm update.d/$(rand)'
>> >>>> equivalents).  But an informative "how to fix" message here seems
>> >>>> appropriate as this is either the result of a bad sync (likelihood of
>> >>>> this?) or will never resolve itself automatically.
>> >>>>
>> >>>
>> >>> I honestly think you're overcomplicating this CL. There is no need for
>> >>> any scenario more complicated than "fix a symlink" at the moment. When
>>
> such
>
>> >>> scenarios appear, I'm all for making support for them. After they
>> appear.
>> >>>
>> >>
>> >> I think you may have missed my concern/request.  If you've got logic in
>> >> there to detect an old/deprecated chroot then there should be a command
>> on
>> >> how to fix it (even if its just "run this to clobber).   Giving a fatal
>> >> error with no obvious way to fix it will just frustrate people.  You
>> can
>>
> add
>
>> >> in "ask on irc" or "email the list" or "give up and start over".  There
>> >> doesn't need to be code to automatically fix this, just a friendlier
>> error
>> >> message that helps the user.
>> >>
>> >>  I also wanted to make sure you had a plan for how you would end up in
>> >> this state and what the flow would look like (make_chroot setting the
>> >> VERSION_FILE on create?).  It doesn't need to be implemented, but it
>> should
>> >> be thought out.
>> >>
>> >
>> > I'm pretty sure I understood what you mean, but don't like that you're
>> > trying to transform this functionality into something else, just because
>> you
>> > believe you have better plans. Gap between CURRENT and FIRST_KNOWN means
>> > that we have discontinued a very old chroot (noone says that such an
>> action
>> > will be ever done or necessary), and the obvious fix is to start over.
>> And
>> > frankly, if someone makes a "gap" in a normal sequence of versions, I
>> don't
>> > know what better action to take than to die. And why would anyone even
>> want
>> > to do that? Versions should be added, not retrospectively modified.
>> >
>> > This also gives us the ability to pull in changes that "kill the
>> chroot".
>> > You just make a gap, bump up the default version, and this tells
>> everyone
>> > that they have to recreate their chroot, in case the change is so
>> massive
>> > that there's no way to convert. And that only after exhausting all the
>> > possible options on how to upgrade. Without sending a single email to
>> > anyone. (which is the purpose of this script)
>> >
>> >
>> I'm agreeing that dying is fine. And the ability to kill the chroot if
>> necessary is fine.
>>
>
>  What I'm asking for is that the error message actually tell the user what
>> do
>> do (i.e. rebuild chroot).  If in every case where we get this error and a
>> user asks what do do and we respond with "rebuild chroot". Then just put
>> "rebuild chroot" into the error message with the actual command line to do
>> so.  This way the user can fix the problem and they don't need to contact
>> everyone.
>>
>
>
>  >
>> > What you did here is "eval the line containing foo.sh", which does
>> > absolutely nothing, and then executes foo.sh. It's exactly equivalent to
>> > just "foo.sh". If they're two separate scripts, running return/exit
>> inside
>> > the second one will not terminate the parent shell. I'm doing a "source
>> > foo.sh" exactly for the reason that it preserves all variables,
>> functions,
>> > etc., but a return/exit function will kill the top-level script, because
>> the
>> > sourced script is not executed in a separate process, rather executed in
>> the
>> > same context. For return to start working, the scripts have to be
>> wrapped in
>> > a code block, as I already did.
>>
>
>
>  Yah.  Guess somewhere I got used to using eval when calling shellscripts.
>> Bad habit I must have picked up.  Like you said its a tradeoff - I tend to
>> lean more towards not allowing a third party script to not crash the
>> caller.
>>  But I can see the argument for pre-processing the variables in a central
>> place.  Passing the variables defined in this script should be easy enough
>> (just do export ...) but included files like common.sh wouldn't work.
>>  Although if the reviewers are on top of things for the individual scripts
>> this shouldn't be a problem (and likewise if the test script is written
>> correctly to cover this case it can be caught there).    I'm fine with it
>> staying how it is.
>>
>
>
>  Lastly any thoughts on &> /dev/null vs  > /dev/null 2>&1?  I was told in
>> another CL the first method isn't portable (not posix compliant) and it
>> makes sense to me that we should keep our implementations consistent
>> between
>> scripts.
>>
>
>
>
>
> http://codereview.chromium.org/3331011/show
>

[kliegs, 2010-09-24 22:31:49]

http://codereview.chromium.org/3331011/diff/41001/32002
File cros_upgrade_chroot (right):

http://codereview.chromium.org/3331011/diff/41001/32002#newcode79
cros_upgrade_chroot:79: error "Your chroot is too old, you need to re-create
it!"
Why not just put the command here?  Save people the need to tab to the website
to reread the docs since (hopefully) chroot won't be run often.  Unless you're
worried that the command listed here may be obsolete by the time its needed?  

This isn't really a major point or something that should necessarily block this
CL.  But I've always viewed tools like this as making things as easy as possible
for the consumers of the script and giving explicit instructions here (even if
its not a common or expected case) makes their life easier.  Sure all these
changes could come later, but maintenance never happens as much as we'd like and
if the effort is similar better to do it right in the initial version.

[zbehan, 2010-09-24 22:59:50]

I'm not sure. I think the command may be changing in the near future, for
example with the new cros_chroot script, and even the syntax may vary. I
don't want to be the one documenting that if you change the command or
anything about it, you also change this place.

Also, what kind of developer doesn't remember how to create their chroot?
It's not something you do every day, I agree, but it's something that you do
as a first thing, and should be able to do it again.

On Fri, Sep 24, 2010 at 3:31 PM, <kliegs@chromium.org> wrote:

>
> http://codereview.chromium.org/3331011/diff/41001/32002
>
> File cros_upgrade_chroot (right):
>
> http://codereview.chromium.org/3331011/diff/41001/32002#newcode79
> cros_upgrade_chroot:79: error "Your chroot is too old, you need to
> re-create it!"
> Why not just put the command here?  Save people the need to tab to the
> website to reread the docs since (hopefully) chroot won't be run often.
> Unless you're worried that the command listed here may be obsolete by
> the time its needed?
>
> This isn't really a major point or something that should necessarily
> block this CL.  But I've always viewed tools like this as making things
> as easy as possible for the consumers of the script and giving explicit
> instructions here (even if its not a common or expected case) makes
> their life easier.  Sure all these changes could come later, but
> maintenance never happens as much as we'd like and if the effort is
> similar better to do it right in the initial version.
>
>
> http://codereview.chromium.org/3331011/show
>

[Mandeep Singh Baines, 2010-09-24 23:21:35]

LGTM.

I don't see any problems/bugs with zbehan's approach.

Let's push this since zbehan is leaving. Enhancements can be done in follow up
CLs.