cros_workon: redefine the concept of a "workon" package list to depend on the board

cros_workon: redefine the concept of a "workon" package list to depend on the board

* Modified all workon listing functions to also look for keyword

* Added a fallback to list all workon ebuilds if keyword is not specified, which
is needed for cros_mark_all_as_stable, which does not differentiate between boards.

This, amongst other potential issues, resolves the case when it was possible to
start working on a package not keyworded for the given board, and making
build_packages fail unconditionally.

TEST=below
$ ./cros_workon list --all --board=x86-generic |wc -l
73
$ ./cros_workon list --all --host |wc -l
57
Looking at the lists rather than "|wc -l" looks correct
$ ./cros_mark_all_as_stable
^ Produces satisfactory result

BUG=6700
Change-Id: Ieee92a39febcef5fb95e59cf97b6e63281a7c750

[Mandeep Singh Baines, 2010-09-14 16:53:00]

http://codereview.chromium.org/3400001/diff/1/2
File cros_workon (right):

http://codereview.chromium.org/3400001/diff/1/2#newcode70
cros_workon:70: BOARD_KEYWORD="amd64" # FIXME: Bleh, hardcoded, but how to do
this better?
portageq envvar ARCH

http://codereview.chromium.org/3400001/diff/1/3
File lib/cros_workon_common.sh (right):

http://codereview.chromium.org/3400001/diff/1/3#newcode28
lib/cros_workon_common.sh:28: if [ -z "${keyword}" ]; then # show all
How about this:

if [ -z "${keyword}" ]; then
  keyword="ALL"
fi

http://codereview.chromium.org/3400001/diff/1/3#newcode31
lib/cros_workon_common.sh:31: fi; echo "${CROS_ALL_EBUILDS}"
Why not on a new line?

http://codereview.chromium.org/3400001/diff/1/3#newcode35
lib/cros_workon_common.sh:35: fi; eval echo \"\${CROS_${keyword}_EBUILDS}\"
Why not on a new line?

http://codereview.chromium.org/3400001/diff/1/3#newcode39
lib/cros_workon_common.sh:39: show_workon_ebuilds() {
Why is this being changed?

http://codereview.chromium.org/3400001/diff/1/3#newcode45
lib/cros_workon_common.sh:45: ) | sort | uniq
uniq is not necessary

[zbehan, 2010-09-14 18:05:45]

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

>
> http://codereview.chromium.org/3400001/diff/1/2
> File cros_workon (right):
>
> http://codereview.chromium.org/3400001/diff/1/2#newcode70
> cros_workon:70: BOARD_KEYWORD="amd64" # FIXME: Bleh, hardcoded, but how
> to do this better?
> portageq envvar ARCH
>

That's a good point. I created a crazy function in cros_common that gets me
the same info from ACCEPT_KEYWORDS, but this is probably significantly
easier. I'll just replace both this and 63 to portageq.

http://codereview.chromium.org/3400001/diff/1/3
> File lib/cros_workon_common.sh (right):
>
> http://codereview.chromium.org/3400001/diff/1/3#newcode28
> lib/cros_workon_common.sh:28: if [ -z "${keyword}" ]; then # show all
> How about this:
>
> if [ -z "${keyword}" ]; then
>  keyword="ALL"
> fi
>

I'd like to make this simpler as much as you do, but
$(find_keyword_workon_ebuilds ALL) won't work. ALL needs to be ".*".

Could fix up find_keyword_workon_ebuilds to recognise ALL, but that seems
weird.

http://codereview.chromium.org/3400001/diff/1/3#newcode31
> lib/cros_workon_common.sh:31: fi; echo "${CROS_ALL_EBUILDS}"
> Why not on a new line?
>
> http://codereview.chromium.org/3400001/diff/1/3#newcode35
> lib/cros_workon_common.sh:35: fi; eval echo
> \"\${CROS_${keyword}_EBUILDS}\"
> Why not on a new line?
>

And now, specially for msb, on a newline! I really just tried to make the
code slightly shorter.

http://codereview.chromium.org/3400001/diff/1/3#newcode39
> lib/cros_workon_common.sh:39: show_workon_ebuilds() {
> Why is this being changed?
>

I realised that the original sucked, and did really weird things, didn't
output things on split lines yet parsed them through sort, and it all
manifested when i added the ${1} to show_workon_ebuilds_files. Now that I'm
looking over it again, I can put it outside of the ( ) block, because of the
for cycle, but the changed code is generally more resilient.

http://codereview.chromium.org/3400001/diff/1/3#newcode45
> lib/cros_workon_common.sh:45: ) | sort | uniq
> uniq is not necessary


Changed into sort -u.

[Mandeep Singh Baines, 2010-09-14 18:51:52]

http://codereview.chromium.org/3400001/diff/7001/8002
File lib/cros_workon_common.sh (right):

http://codereview.chromium.org/3400001/diff/7001/8002#newcode27
lib/cros_workon_common.sh:27: keyword=$1
How about remove the caching and implement as:

if [ -z "${keyword}" ]; then
  keyword=".*"
fi
find_keyword_workon_ebuilds "${keyword}"

[Mandeep Singh Baines, 2010-09-14 19:00:58]

Zdenek Behan (zbehan@chromium.org) wrote:
> On Tue, Sep 14, 2010 at 9:53 AM, <msb@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/3400001/diff/1/2
> > File cros_workon (right):
> >
> > http://codereview.chromium.org/3400001/diff/1/2#newcode70
> > cros_workon:70: BOARD_KEYWORD="amd64" # FIXME: Bleh, hardcoded, but how
> > to do this better?
> > portageq envvar ARCH
> >
> 
> That's a good point. I created a crazy function in cros_common that gets me
> the same info from ACCEPT_KEYWORDS, but this is probably significantly
> easier. I'll just replace both this and 63 to portageq.
> 
> http://codereview.chromium.org/3400001/diff/1/3
> > File lib/cros_workon_common.sh (right):
> >
> > http://codereview.chromium.org/3400001/diff/1/3#newcode28
> > lib/cros_workon_common.sh:28: if [ -z "${keyword}" ]; then # show all
> > How about this:
> >
> > if [ -z "${keyword}" ]; then
> >  keyword="ALL"
> > fi
> >
> 
> I'd like to make this simpler as much as you do, but
> $(find_keyword_workon_ebuilds ALL) won't work. ALL needs to be ".*".
> 
> Could fix up find_keyword_workon_ebuilds to recognise ALL, but that seems
> weird.
> 
> http://codereview.chromium.org/3400001/diff/1/3#newcode31
> > lib/cros_workon_common.sh:31: fi; echo "${CROS_ALL_EBUILDS}"
> > Why not on a new line?
> >
> > http://codereview.chromium.org/3400001/diff/1/3#newcode35
> > lib/cros_workon_common.sh:35: fi; eval echo
> > \"\${CROS_${keyword}_EBUILDS}\"
> > Why not on a new line?
> >
> 
> And now, specially for msb, on a newline! I really just tried to make the
> code slightly shorter.
> 
> http://codereview.chromium.org/3400001/diff/1/3#newcode39
> > lib/cros_workon_common.sh:39: show_workon_ebuilds() {
> > Why is this being changed?
> >
> 
> I realised that the original sucked, and did really weird things, didn't
> output things on split lines yet parsed them through sort, and it all

Isn't the output of show_workon_ebuilds_files one entry per line?

> manifested when i added the ${1} to show_workon_ebuilds_files. Now that I'm
> looking over it again, I can put it outside of the ( ) block, because of the
> for cycle, but the changed code is generally more resilient.
> 
> http://codereview.chromium.org/3400001/diff/1/3#newcode45
> > lib/cros_workon_common.sh:45: ) | sort | uniq
> > uniq is not necessary
> 
> 
> Changed into sort -u.

Just sort would have been fine. Uniqueness was already guaranteed by
show_workon_ebuilds_files.

[zbehan, 2010-09-14 20:02:00]

On Tue, Sep 14, 2010 at 11:54 AM, Mandeep Singh Baines <msb@chromium.org>wrote:

> Zdenek Behan (zbehan@chromium.org) wrote:
> > On Tue, Sep 14, 2010 at 9:53 AM, <msb@chromium.org> wrote:
> >
> > >
> > > http://codereview.chromium.org/3400001/diff/1/2
> > > File cros_workon (right):
> > >
> > > http://codereview.chromium.org/3400001/diff/1/2#newcode70
> > > cros_workon:70: BOARD_KEYWORD="amd64" # FIXME: Bleh, hardcoded, but how
> > > to do this better?
> > > portageq envvar ARCH
> > >
> >
> > That's a good point. I created a crazy function in cros_common that gets
> me
> > the same info from ACCEPT_KEYWORDS, but this is probably significantly
> > easier. I'll just replace both this and 63 to portageq.
> >
> > http://codereview.chromium.org/3400001/diff/1/3
> > > File lib/cros_workon_common.sh (right):
> > >
> > > http://codereview.chromium.org/3400001/diff/1/3#newcode28
> > > lib/cros_workon_common.sh:28: if [ -z "${keyword}" ]; then # show all
> > > How about this:
> > >
> > > if [ -z "${keyword}" ]; then
> > >  keyword="ALL"
> > > fi
> > >
> >
> > I'd like to make this simpler as much as you do, but
> > $(find_keyword_workon_ebuilds ALL) won't work. ALL needs to be ".*".
> >
> > Could fix up find_keyword_workon_ebuilds to recognise ALL, but that seems
> > weird.
> >
> > http://codereview.chromium.org/3400001/diff/1/3#newcode31
> > > lib/cros_workon_common.sh:31: fi; echo "${CROS_ALL_EBUILDS}"
> > > Why not on a new line?
> > >
> > > http://codereview.chromium.org/3400001/diff/1/3#newcode35
> > > lib/cros_workon_common.sh:35: fi; eval echo
> > > \"\${CROS_${keyword}_EBUILDS}\"
> > > Why not on a new line?
> > >
> >
> > And now, specially for msb, on a newline! I really just tried to make the
> > code slightly shorter.
> >
> > http://codereview.chromium.org/3400001/diff/1/3#newcode39
> > > lib/cros_workon_common.sh:39: show_workon_ebuilds() {
> > > Why is this being changed?
> > >
> >
> > I realised that the original sucked, and did really weird things, didn't
> > output things on split lines yet parsed them through sort, and it all
>
> Isn't the output of show_workon_ebuilds_files one entry per line?
>

Hmm, actually it is now. I think in one of my development stages it wasn't.


> > manifested when i added the ${1} to show_workon_ebuilds_files. Now that
> I'm
> > looking over it again, I can put it outside of the ( ) block, because of
> the
> > for cycle, but the changed code is generally more resilient.
> >
> > http://codereview.chromium.org/3400001/diff/1/3#newcode45
> > > lib/cros_workon_common.sh:45: ) | sort | uniq
> > > uniq is not necessary
> >
> >
> > Changed into sort -u.
>
> Just sort would have been fine. Uniqueness was already guaranteed by
> show_workon_ebuilds_files.
>

No it wasn't. The uniq was originally in find_workon_ebuilds(), but would be
now utterly ineffective, because it gives a list of paths to ebuilds, rather
than list of ebuilds. That means if one ebuild was in two different
overlays, it would end up being twice in the list. That's why i moved the
uniqueness to the end of the pipeline.

Currently, there's no workon ebuilds that are duplicate in two overlays, so
you're right in a sense, but sort -u should stay here.

[Mandeep Singh Baines, 2010-09-14 20:08:58]

On 2010/09/14 20:02:00, zbehan wrote:
> On Tue, Sep 14, 2010 at 11:54 AM, Mandeep Singh Baines
<msb@chromium.org>wrote:
> 
> > Zdenek Behan (mailto:zbehan@chromium.org) wrote:
> > > On Tue, Sep 14, 2010 at 9:53 AM, <mailto:msb@chromium.org> wrote:
> > >
> > > >
> > > > http://codereview.chromium.org/3400001/diff/1/2
> > > > File cros_workon (right):
> > > >
> > > > http://codereview.chromium.org/3400001/diff/1/2#newcode70
> > > > cros_workon:70: BOARD_KEYWORD="amd64" # FIXME: Bleh, hardcoded, but how
> > > > to do this better?
> > > > portageq envvar ARCH
> > > >
> > >
> > > That's a good point. I created a crazy function in cros_common that gets
> > me
> > > the same info from ACCEPT_KEYWORDS, but this is probably significantly
> > > easier. I'll just replace both this and 63 to portageq.
> > >
> > > http://codereview.chromium.org/3400001/diff/1/3
> > > > File lib/cros_workon_common.sh (right):
> > > >
> > > > http://codereview.chromium.org/3400001/diff/1/3#newcode28
> > > > lib/cros_workon_common.sh:28: if [ -z "${keyword}" ]; then # show all
> > > > How about this:
> > > >
> > > > if [ -z "${keyword}" ]; then
> > > >  keyword="ALL"
> > > > fi
> > > >
> > >
> > > I'd like to make this simpler as much as you do, but
> > > $(find_keyword_workon_ebuilds ALL) won't work. ALL needs to be ".*".
> > >
> > > Could fix up find_keyword_workon_ebuilds to recognise ALL, but that seems
> > > weird.
> > >
> > > http://codereview.chromium.org/3400001/diff/1/3#newcode31
> > > > lib/cros_workon_common.sh:31: fi; echo "${CROS_ALL_EBUILDS}"
> > > > Why not on a new line?
> > > >
> > > > http://codereview.chromium.org/3400001/diff/1/3#newcode35
> > > > lib/cros_workon_common.sh:35: fi; eval echo
> > > > \"\${CROS_${keyword}_EBUILDS}\"
> > > > Why not on a new line?
> > > >
> > >
> > > And now, specially for msb, on a newline! I really just tried to make the
> > > code slightly shorter.
> > >
> > > http://codereview.chromium.org/3400001/diff/1/3#newcode39
> > > > lib/cros_workon_common.sh:39: show_workon_ebuilds() {
> > > > Why is this being changed?
> > > >
> > >
> > > I realised that the original sucked, and did really weird things, didn't
> > > output things on split lines yet parsed them through sort, and it all
> >
> > Isn't the output of show_workon_ebuilds_files one entry per line?
> >
> 
> Hmm, actually it is now. I think in one of my development stages it wasn't.
> 
> 
> > > manifested when i added the ${1} to show_workon_ebuilds_files. Now that
> > I'm
> > > looking over it again, I can put it outside of the ( ) block, because of
> > the
> > > for cycle, but the changed code is generally more resilient.
> > >
> > > http://codereview.chromium.org/3400001/diff/1/3#newcode45
> > > > lib/cros_workon_common.sh:45: ) | sort | uniq
> > > > uniq is not necessary
> > >
> > >
> > > Changed into sort -u.
> >
> > Just sort would have been fine. Uniqueness was already guaranteed by
> > show_workon_ebuilds_files.
> >
> 
> No it wasn't. The uniq was originally in find_workon_ebuilds(), but would be
> now utterly ineffective, because it gives a list of paths to ebuilds, rather
> than list of ebuilds. That means if one ebuild was in two different
> overlays, it would end up being twice in the list. That's why i moved the
> uniqueness to the end of the pipeline.
> 
> Currently, there's no workon ebuilds that are duplicate in two overlays, so
> you're right in a sense, but sort -u should stay here.
> 

Good point. Didn't consider multiple overlays.

[zbehan, 2010-09-14 20:51:40]

Does that mean lgtm? :)

On Tue, Sep 14, 2010 at 1:08 PM, <msb@chromium.org> wrote:

> On 2010/09/14 20:02:00, zbehan wrote:
>
>> On Tue, Sep 14, 2010 at 11:54 AM, Mandeep Singh Baines
>>
> <msb@chromium.org>wrote:
>
>  > Zdenek Behan (mailto:zbehan@chromium.org) wrote:
>>
>> > > On Tue, Sep 14, 2010 at 9:53 AM, <mailto:msb@chromium.org> wrote:
>> > >
>> > > >
>> > > > http://codereview.chromium.org/3400001/diff/1/2
>> > > > File cros_workon (right):
>> > > >
>> > > > http://codereview.chromium.org/3400001/diff/1/2#newcode70
>> > > > cros_workon:70: BOARD_KEYWORD="amd64" # FIXME: Bleh, hardcoded, but
>> how
>> > > > to do this better?
>> > > > portageq envvar ARCH
>> > > >
>> > >
>> > > That's a good point. I created a crazy function in cros_common that
>> gets
>> > me
>> > > the same info from ACCEPT_KEYWORDS, but this is probably significantly
>> > > easier. I'll just replace both this and 63 to portageq.
>> > >
>> > > http://codereview.chromium.org/3400001/diff/1/3
>> > > > File lib/cros_workon_common.sh (right):
>> > > >
>> > > > http://codereview.chromium.org/3400001/diff/1/3#newcode28
>> > > > lib/cros_workon_common.sh:28: if [ -z "${keyword}" ]; then # show
>> all
>> > > > How about this:
>> > > >
>> > > > if [ -z "${keyword}" ]; then
>> > > >  keyword="ALL"
>> > > > fi
>> > > >
>> > >
>> > > I'd like to make this simpler as much as you do, but
>> > > $(find_keyword_workon_ebuilds ALL) won't work. ALL needs to be ".*".
>> > >
>> > > Could fix up find_keyword_workon_ebuilds to recognise ALL, but that
>> seems
>> > > weird.
>> > >
>> > > http://codereview.chromium.org/3400001/diff/1/3#newcode31
>> > > > lib/cros_workon_common.sh:31: fi; echo "${CROS_ALL_EBUILDS}"
>> > > > Why not on a new line?
>> > > >
>> > > > http://codereview.chromium.org/3400001/diff/1/3#newcode35
>> > > > lib/cros_workon_common.sh:35: fi; eval echo
>> > > > \"\${CROS_${keyword}_EBUILDS}\"
>> > > > Why not on a new line?
>> > > >
>> > >
>> > > And now, specially for msb, on a newline! I really just tried to make
>> the
>> > > code slightly shorter.
>> > >
>> > > http://codereview.chromium.org/3400001/diff/1/3#newcode39
>> > > > lib/cros_workon_common.sh:39: show_workon_ebuilds() {
>> > > > Why is this being changed?
>> > > >
>> > >
>> > > I realised that the original sucked, and did really weird things,
>> didn't
>> > > output things on split lines yet parsed them through sort, and it all
>> >
>> > Isn't the output of show_workon_ebuilds_files one entry per line?
>> >
>>
>
>  Hmm, actually it is now. I think in one of my development stages it
>> wasn't.
>>
>
>
>  > > manifested when i added the ${1} to show_workon_ebuilds_files. Now
>> that
>> > I'm
>> > > looking over it again, I can put it outside of the ( ) block, because
>> of
>> > the
>> > > for cycle, but the changed code is generally more resilient.
>> > >
>> > > http://codereview.chromium.org/3400001/diff/1/3#newcode45
>> > > > lib/cros_workon_common.sh:45: ) | sort | uniq
>> > > > uniq is not necessary
>> > >
>> > >
>> > > Changed into sort -u.
>> >
>> > Just sort would have been fine. Uniqueness was already guaranteed by
>> > show_workon_ebuilds_files.
>> >
>>
>
>  No it wasn't. The uniq was originally in find_workon_ebuilds(), but would
>> be
>> now utterly ineffective, because it gives a list of paths to ebuilds,
>> rather
>> than list of ebuilds. That means if one ebuild was in two different
>> overlays, it would end up being twice in the list. That's why i moved the
>> uniqueness to the end of the pipeline.
>>
>
>  Currently, there's no workon ebuilds that are duplicate in two overlays,
>> so
>> you're right in a sense, but sort -u should stay here.
>>
>
>
> Good point. Didn't consider multiple overlays.
>
>
> http://codereview.chromium.org/3400001/show
>

[Mandeep Singh Baines, 2010-09-14 20:52:15]

http://codereview.chromium.org/3400001/diff/13001/14002
File lib/cros_workon_common.sh (right):

http://codereview.chromium.org/3400001/diff/13001/14002#newcode43
lib/cros_workon_common.sh:43: echo $(show_workon_ebuilds_files $1) | \
Why the echo?

[zbehan, 2010-09-14 20:59:49]

On Tue, Sep 14, 2010 at 1:52 PM, <msb@chromium.org> wrote:

>
> http://codereview.chromium.org/3400001/diff/13001/14002
>
> File lib/cros_workon_common.sh (right):
>
> http://codereview.chromium.org/3400001/diff/13001/14002#newcode43
> lib/cros_workon_common.sh:43: echo $(show_workon_ebuilds_files $1) | \
> Why the echo?


Ugh, another artifact. Will fix. Thanks!

[zbehan, 2010-09-14 22:29:19]

Well, meh.

It seems that it stopped working mysteriously.

The caching function is destroying the multiline-ness. I've checked it, and
doing the find is blazing fast when running from caches, so i'll just kill
the caching function as unnecessary and do things the normal way. This is
prone to breaking anyway, if someone makes live changes to ebuilds inside
their script and then re-runs show_workon_ebuilds to see the new result,
they'll get the old result.

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

>
>
> On Tue, Sep 14, 2010 at 1:52 PM, <msb@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/3400001/diff/13001/14002
>>
>> File lib/cros_workon_common.sh (right):
>>
>> http://codereview.chromium.org/3400001/diff/13001/14002#newcode43
>> lib/cros_workon_common.sh:43: echo $(show_workon_ebuilds_files $1) | \
>> Why the echo?
>
>
> Ugh, another artifact. Will fix. Thanks!
>

[Mandeep Singh Baines, 2010-09-14 23:05:24]

http://codereview.chromium.org/3400001/diff/5003/23002
File lib/cros_workon_common.sh (right):

http://codereview.chromium.org/3400001/diff/5003/23002#newcode32
lib/cros_workon_common.sh:32: keyword="ALL"
With the new cleanup, you should be able to:

keyword=".*"

here and remove the if from find_keyword_workon_ebuilds.

[zbehan, 2010-09-14 23:54:01]

Or better yet, because the regexp "KEYWORDS=".*${keyword}.*" works even if
$keyword is empty, which is exactly what i'm passing to it in case i want
all.

I guess this converts the "feature to print all" into "sideeffect of not
filtering any".

On Tue, Sep 14, 2010 at 4:05 PM, <msb@chromium.org> wrote:

>
> http://codereview.chromium.org/3400001/diff/5003/23002
>
> File lib/cros_workon_common.sh (right):
>
> http://codereview.chromium.org/3400001/diff/5003/23002#newcode32
> lib/cros_workon_common.sh:32: keyword="ALL"
> With the new cleanup, you should be able to:
>
> keyword=".*"
>
> here and remove the if from find_keyword_workon_ebuilds.
>
>
> http://codereview.chromium.org/3400001/show
>

[Mandeep Singh Baines, 2010-09-15 18:14:25]

LGTM