cros_mark_all_as_stable: implement tracking of eclass changes

cros_mark_all_as_stable: implement tracking of eclass changes

Change-Id: I4cf6f81aa9ce461c551c43fe456b936da70e8a58

BUG=
TEST=

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=868b777

[zbehan, 2010-09-14 04:59:07]

There's a few things being wrong with this, some hardcoded things that I did
not care about yet. I'm looking for early feedback, possibly discussing in
person how could this be made pushable.

On Mon, Sep 13, 2010 at 9:56 PM, <zbehan@chromium.org> wrote:

> Reviewers: anush, Mandeep Singh Baines, sosa,
>
> Description:
> cros_mark_all_as_stable: implement tracking of eclass changes
>
> Change-Id: I4cf6f81aa9ce461c551c43fe456b936da70e8a58
>
> BUG=
> TEST=
>
> Please review this at http://codereview.chromium.org/3426001/show
>
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
>
> Affected files:
>  M cros_mark_all_as_stable
>
>
> Index: cros_mark_all_as_stable
> diff --git a/cros_mark_all_as_stable b/cros_mark_all_as_stable
> index
>
e85bb0f94291a826fef9db8d3ad3a01242c4c526..40d0b6657f25be96890593589eef4759c8bbc719
> 100755
> --- a/cros_mark_all_as_stable
> +++ b/cros_mark_all_as_stable
> @@ -53,6 +53,69 @@ function package_is_blacklisted() {
>   expr "${1}" : "^\(${blist_regex/\\|/}\)$" &> /dev/null && return 0 ||
> return 1
>  }
>
> +function eclass_affected_ebuilds() {
> +  # Fixme: Hardcoded
> +
>  CHROMIUMOS_OVERLAY='/home/zbehan/trunk/src/third_party/chromiumos-overlay/'
> +
> +  info "Tracking eclass changes"
> +  pushd "${CHROMIUMOS_OVERLAY}" 1> /dev/null
> +
> +  # Look at the last time chrome-bot commited anything.
> +  last_bot_commit=$(
> +    git log --author=chrome-bot -1|head -n 1|cut -f2 -d' '
> +  )
> +  info "Last bot commit is: ${last_bot_commit}"
> +
> +  # List of eclasses touched in all commits since that commit.
> +  eclass_touched=$(
> +    git log --name-only ${last_bot_commit}..HEAD|grep
> "^eclass\/.*\.eclass" | \
> +      sed -e 's,eclass/\(.*\)\.eclass,\1,' | \
> +      sort | uniq
> +  )
> +
> +  # Iteratively add all eclasses that inherit the current list, until
> +  # the first iteration that will not add anything.
> +  count_last=0
> +  count_new=$(echo $eclass_touched |wc -w)
> +  while [ "${count_last}" != "${count_new}" ]; do
> +    count_last=${count_new}
> +
> +    # regexp to search for eclass inheritance
> +    searchregexp="$(for i in ${eclass_touched}; do echo -n "\|${i}";
> done)"
> +    searchregexp="inherit.*\(${searchregexp/|/}\).*"
> +
> +    # Iterate the current list of eclasses and add immediate dependencies.
> +    eclass_touched="$(
> +      find "${CHROMIUMOS_OVERLAY}/eclass/" -name '*.eclass' | \
> +        xargs grep -l "${searchregexp}" | \
> +        sed -e "s,${CHROMIUMOS_OVERLAY}/eclass/\(.*\)\.eclass,\1,"
> +    ) ${eclass_touched}"
> +
> +    # IFS-normalize
> +    eclass_touched=$(
> +      for i in ${eclass_touched}; do echo "${i}"; done | \
> +        sort|uniq
> +    )
> +    count_new=$(echo $eclass_touched |wc -w)
> +  done
> +
> +  info "Eclasses changed: ${eclass_touched}"
> +
> +  # Look which ebuilds are affected.
> +  searchregexp="$(for i in ${eclass_touched}; do echo -n "\|${i}"; done)"
> +  searchregexp="inherit.*\(${searchregexp/|/}\).*"
> +  ebuilds_affected=$(
> +      find "${CHROMIUMOS_OVERLAY}/" -name '*9999.ebuild' | \
> +        xargs grep -l "cros-workon" | \
> +        xargs grep -l "${searchregexp}" | \
> +        sed -e "s,${CHROMIUMOS_OVERLAY}/\(.*\)/.*-9999.ebuild,\1,"
> +  )
> +
> +  echo "${ebuilds_affected}"
> +
> +  popd 1> /dev/null
> +}
> +
>  # For each package, compares the head commit id to the commit id in the
> ebuild.
>  # If they do not match, add the package and its commit id into
> ${PACKAGE_LIST}
>  # and ${COMMIT_ID_LIST}
> @@ -92,6 +155,8 @@ for package in ${PACKAGES}; do
>   fi
>  done
>
> +PACKAGE_LIST="${PACKAGE_LIST} $(eclass_affected_ebuilds)"
> +
>  if [ -n "${PACKAGE_LIST}" ] ; then
>   info "Candidate package list ${PACKAGE_LIST}"
>   info "With commit id list ${COMMIT_ID_LIST}"
>
>
>

[Mandeep Singh Baines, 2010-09-14 16:31:06]

http://codereview.chromium.org/3426001/diff/1/2
File cros_mark_all_as_stable (right):

http://codereview.chromium.org/3426001/diff/1/2#newcode58
cros_mark_all_as_stable:58:
CHROMIUMOS_OVERLAY='/home/zbehan/trunk/src/third_party/chromiumos-overlay/'
Don't forget to fix this.

http://codereview.chromium.org/3426001/diff/1/2#newcode71
cros_mark_all_as_stable:71: git log --name-only ${last_bot_commit}..HEAD|grep
"^eclass\/.*\.eclass" | \
git diff --name-only ${last_bot_commit}

http://codereview.chromium.org/3426001/diff/1/2#newcode73
cros_mark_all_as_stable:73: sort | uniq
sort -u

http://codereview.chromium.org/3426001/diff/1/2#newcode79
cros_mark_all_as_stable:79: count_new=$(echo $eclass_touched |wc -w)
Instead of count_last and count_new. Why not store the previous eclass_touched
as eclass_touched_prev and do a string compare. Be sure to canonicalize
eclass_touched to be cr delimited.

http://codereview.chromium.org/3426001/diff/1/2#newcode89
cros_mark_all_as_stable:89: find "${CHROMIUMOS_OVERLAY}/eclass/" -name
'*.eclass' | \
You already pushd ${CHROMIUMOS_OVERLAY} so "find eclass" will work just as well.

http://codereview.chromium.org/3426001/diff/1/2#newcode91
cros_mark_all_as_stable:91: sed -e
"s,${CHROMIUMOS_OVERLAY}/eclass/\(.*\)\.eclass,\1,"
This regex is duplicated in two places.

http://codereview.chromium.org/3426001/diff/1/2#newcode96
cros_mark_all_as_stable:96: for i in ${eclass_touched}; do echo "${i}"; done | \
echo "${eclass_touched}" | tr ' ' '\n' | sort -u

http://codereview.chromium.org/3426001/diff/1/2#newcode102
cros_mark_all_as_stable:102: info "Eclasses changed: ${eclass_touched}"
Be sure that info doesn't affect the output of this function.

http://codereview.chromium.org/3426001/diff/1/2#newcode158
cros_mark_all_as_stable:158: PACKAGE_LIST="${PACKAGE_LIST}
$(eclass_affected_ebuilds)"
Are dupes OK here?

[sosa, 2010-09-14 16:47:18]

I'm not sure this is the right place for this.  Consider moving this logic (in
python) to cros_mark_as_stable because this script will soon only be called if
Force Build is pressed (i.e. revisions == None).

My thinking is that we should add an extra channel to cros_mark_as_stable that
detects if chromiumos_overlay has changed.  If it has, then go through checks
about eclass's, etc.

[zbehan, 2010-09-14 17:15:51]

Well, I was looking adding into at least something at all, because currently
the use of eclasses to remove code duplication which should be generally
encouraged, breaks the uprevving mechanism, which is unfortunate.

It also gave me a million ideas about what could be done about other
overlays, which currently get zero uprev'ving love.

On Tue, Sep 14, 2010 at 9:47 AM, <sosa@chromium.org> wrote:

> I'm not sure this is the right place for this.  Consider moving this logic
> (in
> python) to cros_mark_as_stable because this script will soon only be called
> if
> Force Build is pressed (i.e. revisions == None).
>
> My thinking is that we should add an extra channel to cros_mark_as_stable
> that
> detects if chromiumos_overlay has changed.  If it has, then go through
> checks
> about eclass's, etc.
>
>
> http://codereview.chromium.org/3426001/show
>

[zbehan, 2010-09-14 23:52:20]

On Tue, Sep 14, 2010 at 9:31 AM, <msb@chromium.org> wrote:

>
> http://codereview.chromium.org/3426001/diff/1/2
> File cros_mark_all_as_stable (right):
>
> http://codereview.chromium.org/3426001/diff/1/2#newcode58
> cros_mark_all_as_stable:58:
>
> CHROMIUMOS_OVERLAY='/home/zbehan/trunk/src/third_party/chromiumos-overlay/'
> Don't forget to fix this.
>

Fixed to $HOME (I don't know a better way). This will not work for sudo $0,
but I've checked and cros_mark_as_stable figures this out the same way, so
at least they'll both fail together.


> http://codereview.chromium.org/3426001/diff/1/2#newcode71
> cros_mark_all_as_stable:71: git log --name-only
>
> ${last_bot_commit}..HEAD|grep "^eclass\/.*\.eclass" | \
>
git diff --name-only ${last_bot_commit}
>

git log <commit> - gives log from <commit> to the start of repo
git log <commit>..HEAD - gives log from HEAD to <commit>+1

Also I'm thinking about a fallback if the above commit does not exist (ie.
chrome-bot never commited). Is there any shorthand for "first commit ever"
as a default value to last_bot_commit?

http://codereview.chromium.org/3426001/diff/1/2#newcode73
> cros_mark_all_as_stable:73: sort | uniq
> sort -u
>
> http://codereview.chromium.org/3426001/diff/1/2#newcode79
> cros_mark_all_as_stable:79: count_new=$(echo $eclass_touched |wc -w)
> Instead of count_last and count_new. Why not store the previous
> eclass_touched as eclass_touched_prev and do a string compare. Be sure
> to canonicalize eclass_touched to be cr delimited.
>

Fixed

http://codereview.chromium.org/3426001/diff/1/2#newcode89
> cros_mark_all_as_stable:89: find "${CHROMIUMOS_OVERLAY}/eclass/" -name
> '*.eclass' | \
> You already pushd ${CHROMIUMOS_OVERLAY} so "find eclass" will work just
> as well.
>

Fixed.


> http://codereview.chromium.org/3426001/diff/1/2#newcode91
> cros_mark_all_as_stable:91: sed -e
>
> "s,${CHROMIUMOS_OVERLAY}/eclass/\(.*\)\.eclass,\1,"
> This regex is duplicated in two places.
>

After I fix the above comment, unfortunately, yes. But putting the sed
expression into a variable means having to worry about double-escaping.

http://codereview.chromium.org/3426001/diff/1/2#newcode96
> cros_mark_all_as_stable:96: for i in ${eclass_touched}; do echo "${i}";
> done | \
> echo "${eclass_touched}" | tr ' ' '\n' | sort -u
>

Nice, but will not get rid of multiple whitespace. That is, if you have two
sets exactly identical except for one of them having two spaces in one place
and the other having one space in the same place, the first one will end up
having an empty line. for will IFS-separate the input, take each word once,
and put it out.

I separated this into a nice function so that it's less ugly now.


> http://codereview.chromium.org/3426001/diff/1/2#newcode102
> cros_mark_all_as_stable:102: info "Eclasses changed: ${eclass_touched}"
> Be sure that info doesn't affect the output of this function.
>

Info & co. has been recently changed to spew to stderr.

http://codereview.chromium.org/3426001/diff/1/2#newcode158
> cros_mark_all_as_stable:158: PACKAGE_LIST="${PACKAGE_LIST}
> $(eclass_affected_ebuilds)"
> Are dupes OK here?


Probably not, thanks! I've parsed that through the normalizing function i
created above and tr.

[Mandeep Singh Baines, 2010-09-15 17:54:19]

http://codereview.chromium.org/3426001/diff/8001/4002
File cros_mark_all_as_stable (right):

http://codereview.chromium.org/3426001/diff/8001/4002#newcode73
cros_mark_all_as_stable:73: eclass_touched="$(ifs_normalize $(
The quotes are unnecessary.

http://codereview.chromium.org/3426001/diff/8001/4002#newcode91
cros_mark_all_as_stable:91: eclass_touched="$(ifs_normalize $(
The quotes are unnecessary.

http://codereview.chromium.org/3426001/diff/8001/4002#newcode155
cros_mark_all_as_stable:155: ifs_normalize ${PACKAGE_LIST}
$(eclass_affected_ebuilds)|tr '\n' ' '
Might be simpler as:

PACKAGE_LIST="${PACKAGE_LIST} $(eclass_affected_ebuilds)"

And then do the tr at the bottom of eclass_affected_ebuilds function:

echo"${ebuilds_affected}" | tr '\n' ' '

[Mandeep Singh Baines, 2010-09-15 17:55:07]

Zdenek Behan (zbehan@chromium.org) wrote:
> On Tue, Sep 14, 2010 at 9:31 AM, <msb@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/3426001/diff/1/2
> > File cros_mark_all_as_stable (right):
> >
> > http://codereview.chromium.org/3426001/diff/1/2#newcode58
> > cros_mark_all_as_stable:58:
> >
> > CHROMIUMOS_OVERLAY='/home/zbehan/trunk/src/third_party/chromiumos-overlay/'
> > Don't forget to fix this.
> >
> 
> Fixed to $HOME (I don't know a better way). This will not work for sudo $0,
> but I've checked and cros_mark_as_stable figures this out the same way, so
> at least they'll both fail together.
> 

common.sh defines SRC_ROOT.


> 
> > http://codereview.chromium.org/3426001/diff/1/2#newcode71
> > cros_mark_all_as_stable:71: git log --name-only
> >
> > ${last_bot_commit}..HEAD|grep "^eclass\/.*\.eclass" | \
> >
> git diff --name-only ${last_bot_commit}
> >
> 
> git log <commit> - gives log from <commit> to the start of repo
> git log <commit>..HEAD - gives log from HEAD to <commit>+1

Wouldn't git diff --name-only ${last_bot_commit} be simpler since
its only lists the affected files and not the logs.

> 
> Also I'm thinking about a fallback if the above commit does not exist (ie.
> chrome-bot never commited). Is there any shorthand for "first commit ever"
> as a default value to last_bot_commit?
> 
> http://codereview.chromium.org/3426001/diff/1/2#newcode73
> > cros_mark_all_as_stable:73: sort | uniq
> > sort -u
> >
> > http://codereview.chromium.org/3426001/diff/1/2#newcode79
> > cros_mark_all_as_stable:79: count_new=$(echo $eclass_touched |wc -w)
> > Instead of count_last and count_new. Why not store the previous
> > eclass_touched as eclass_touched_prev and do a string compare. Be sure
> > to canonicalize eclass_touched to be cr delimited.
> >
> 
> Fixed
> 
> http://codereview.chromium.org/3426001/diff/1/2#newcode89
> > cros_mark_all_as_stable:89: find "${CHROMIUMOS_OVERLAY}/eclass/" -name
> > '*.eclass' | \
> > You already pushd ${CHROMIUMOS_OVERLAY} so "find eclass" will work just
> > as well.
> >
> 
> Fixed.
> 
> 
> > http://codereview.chromium.org/3426001/diff/1/2#newcode91
> > cros_mark_all_as_stable:91: sed -e
> >
> > "s,${CHROMIUMOS_OVERLAY}/eclass/\(.*\)\.eclass,\1,"
> > This regex is duplicated in two places.
> >
> 
> After I fix the above comment, unfortunately, yes. But putting the sed
> expression into a variable means having to worry about double-escaping.
> 
> http://codereview.chromium.org/3426001/diff/1/2#newcode96
> > cros_mark_all_as_stable:96: for i in ${eclass_touched}; do echo "${i}";
> > done | \
> > echo "${eclass_touched}" | tr ' ' '\n' | sort -u
> >
> 
> Nice, but will not get rid of multiple whitespace. That is, if you have two
> sets exactly identical except for one of them having two spaces in one place
> and the other having one space in the same place, the first one will end up
> having an empty line. for will IFS-separate the input, take each word once,
> and put it out.
> 
> I separated this into a nice function so that it's less ugly now.
> 
> 
> > http://codereview.chromium.org/3426001/diff/1/2#newcode102
> > cros_mark_all_as_stable:102: info "Eclasses changed: ${eclass_touched}"
> > Be sure that info doesn't affect the output of this function.
> >
> 
> Info & co. has been recently changed to spew to stderr.
> 
> http://codereview.chromium.org/3426001/diff/1/2#newcode158
> > cros_mark_all_as_stable:158: PACKAGE_LIST="${PACKAGE_LIST}
> > $(eclass_affected_ebuilds)"
> > Are dupes OK here?
> 
> 
> Probably not, thanks! I've parsed that through the normalizing function i
> created above and tr.

[zbehan, 2010-09-15 18:17:04]

On Wed, Sep 15, 2010 at 10:54 AM, <msb@chromium.org> wrote:

>
> http://codereview.chromium.org/3426001/diff/8001/4002
>
> File cros_mark_all_as_stable (right):
>
> http://codereview.chromium.org/3426001/diff/8001/4002#newcode73
> cros_mark_all_as_stable:73: eclass_touched="$(ifs_normalize $(
> The quotes are unnecessary.
>
> http://codereview.chromium.org/3426001/diff/8001/4002#newcode91
> cros_mark_all_as_stable:91: eclass_touched="$(ifs_normalize $(
> The quotes are unnecessary.
>

Fixed both.


> http://codereview.chromium.org/3426001/diff/8001/4002#newcode155
> cros_mark_all_as_stable:155: ifs_normalize ${PACKAGE_LIST}
> $(eclass_affected_ebuilds)|tr '\n' ' '
> Might be simpler as:
>
>
> PACKAGE_LIST="${PACKAGE_LIST} $(eclass_affected_ebuilds)"
>
> And then do the tr at the bottom of eclass_affected_ebuilds function:
>
> echo"${ebuilds_affected}" | tr '\n' ' '


This actually does something else, i parse things through ifs_normalize,
which kills duplicates and outputs a list. And then convert the list to
lines. But you're absolutely right that I can do it differently, I pulled
the tr into ifs_normalize, which has a side-effect of eclass-list not being
newline delimited, rather space delimited, and breaks nothing.

Thanks!

[zbehan, 2010-09-15 18:25:02]

On Wed, Sep 15, 2010 at 10:54 AM, Mandeep Singh Baines <msb@chromium.org>wrote:

> Zdenek Behan (zbehan@chromium.org) wrote:
> > On Tue, Sep 14, 2010 at 9:31 AM, <msb@chromium.org> wrote:
> >
> > >
> > > http://codereview.chromium.org/3426001/diff/1/2
> > > File cros_mark_all_as_stable (right):
> > >
> > > http://codereview.chromium.org/3426001/diff/1/2#newcode58
> > > cros_mark_all_as_stable:58:
> > >
> > >
> CHROMIUMOS_OVERLAY='/home/zbehan/trunk/src/third_party/chromiumos-overlay/'
> > > Don't forget to fix this.
> > >
> >
> > Fixed to $HOME (I don't know a better way). This will not work for sudo
> $0,
> > but I've checked and cros_mark_as_stable figures this out the same way,
> so
> > at least they'll both fail together.
> >
>
> common.sh defines SRC_ROOT.
>

Heh. It does:
--
# Note that 'realpath' is equivalent to 'readlink -f'.
TOP_SCRIPT_DIR=`readlink -f $TOP_SCRIPT_DIR`
GCLIENT_ROOT=`readlink -f $GCLIENT_ROOT`

# Other directories should always be pathed down from GCLIENT_ROOT.
SRC_ROOT="$GCLIENT_ROOT/src"
SRC_INTERNAL="$GCLIENT_ROOT/src-internal"
SCRIPTS_DIR="$SRC_ROOT/scripts"
--

Say, we didn't grep all scripts for GCLIENT_ROOT during conversion, did we?
:P

>
> > > http://codereview.chromium.org/3426001/diff/1/2#newcode71
> > > cros_mark_all_as_stable:71: git log --name-only
> > >
> > > ${last_bot_commit}..HEAD|grep "^eclass\/.*\.eclass" | \
> > >
> > git diff --name-only ${last_bot_commit}
> > >
> >
> > git log <commit> - gives log from <commit> to the start of repo
> > git log <commit>..HEAD - gives log from HEAD to <commit>+1
>
> Wouldn't git diff --name-only ${last_bot_commit} be simpler since
> its only lists the affected files and not the logs.
>

OH! You said git diff, not git log. That explains stuff, you learn new
things about git every day.

>
> > Also I'm thinking about a fallback if the above commit does not exist
> (ie.
> > chrome-bot never commited). Is there any shorthand for "first commit
> ever"
> > as a default value to last_bot_commit?
> >
> > http://codereview.chromium.org/3426001/diff/1/2#newcode73
> > > cros_mark_all_as_stable:73: sort | uniq
> > > sort -u
> > >
> > > http://codereview.chromium.org/3426001/diff/1/2#newcode79
> > > cros_mark_all_as_stable:79: count_new=$(echo $eclass_touched |wc -w)
> > > Instead of count_last and count_new. Why not store the previous
> > > eclass_touched as eclass_touched_prev and do a string compare. Be sure
> > > to canonicalize eclass_touched to be cr delimited.
> > >
> >
> > Fixed
> >
> > http://codereview.chromium.org/3426001/diff/1/2#newcode89
> > > cros_mark_all_as_stable:89: find "${CHROMIUMOS_OVERLAY}/eclass/" -name
> > > '*.eclass' | \
> > > You already pushd ${CHROMIUMOS_OVERLAY} so "find eclass" will work just
> > > as well.
> > >
> >
> > Fixed.
> >
> >
> > > http://codereview.chromium.org/3426001/diff/1/2#newcode91
> > > cros_mark_all_as_stable:91: sed -e
> > >
> > > "s,${CHROMIUMOS_OVERLAY}/eclass/\(.*\)\.eclass,\1,"
> > > This regex is duplicated in two places.
> > >
> >
> > After I fix the above comment, unfortunately, yes. But putting the sed
> > expression into a variable means having to worry about double-escaping.
> >
> > http://codereview.chromium.org/3426001/diff/1/2#newcode96
> > > cros_mark_all_as_stable:96: for i in ${eclass_touched}; do echo "${i}";
> > > done | \
> > > echo "${eclass_touched}" | tr ' ' '\n' | sort -u
> > >
> >
> > Nice, but will not get rid of multiple whitespace. That is, if you have
> two
> > sets exactly identical except for one of them having two spaces in one
> place
> > and the other having one space in the same place, the first one will end
> up
> > having an empty line. for will IFS-separate the input, take each word
> once,
> > and put it out.
> >
> > I separated this into a nice function so that it's less ugly now.
> >
> >
> > > http://codereview.chromium.org/3426001/diff/1/2#newcode102
> > > cros_mark_all_as_stable:102: info "Eclasses changed: ${eclass_touched}"
> > > Be sure that info doesn't affect the output of this function.
> > >
> >
> > Info & co. has been recently changed to spew to stderr.
> >
> > http://codereview.chromium.org/3426001/diff/1/2#newcode158
> > > cros_mark_all_as_stable:158: PACKAGE_LIST="${PACKAGE_LIST}
> > > $(eclass_affected_ebuilds)"
> > > Are dupes OK here?
> >
> >
> > Probably not, thanks! I've parsed that through the normalizing function i
> > created above and tr.
>

[Mandeep Singh Baines, 2010-09-15 18:30:52]

LGTM