cros_workon: introduce a very simple listall command

cros_workon: introduce a very simple listall command

TBR: sosa, msb - I already got the LGTM, then made a silly whitespace change!

[brian d, 2010-07-13 16:38:01]

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

http://codereview.chromium.org/2909009/diff/1/2#newcode48
cros_workon:48: [ "${WORKON_CMD}" != "listall" ] && \
Not sure why this is necessary. src/scripts/.default_board can be configured
using 'setup_board'. The current listall functionality isn't using board
specific directories either.

[zbehan, 2010-07-13 18:25:40]

On Tue, Jul 13, 2010 at 9:38 AM, <bdaugher@chromium.org> wrote:

>
> http://codereview.chromium.org/2909009/diff/1/2
> File cros_workon (right):
>
> http://codereview.chromium.org/2909009/diff/1/2#newcode48
> cros_workon:48: [ "${WORKON_CMD}" != "listall" ] && \
> Not sure why this is necessary. src/scripts/.default_board can be
> configured using 'setup_board'. The current listall functionality isn't
> using board specific directories either.


I originally thought I'll put in just a hack using a hardcoded path, but I
thought of a really easy way to fix this. I'll put it on top of the current
CL.

[Mandeep Singh Baines, 2010-07-13 22:44:29]

LGTM. Thank:)

[sosa, 2010-07-13 22:53:46]

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

http://codereview.chromium.org/2909009/diff/1/2#newcode45
cros_workon:45: [ -n "${FLAGS_board}" ] && [ "${FLAGS_host}" = ${FLAGS_TRUE} ]
&& \
Quick question.  Using this logic, you can never use --host if you have a
default board setup. Do you want this to be mutually exclusive or maybe just
have host override board?

http://codereview.chromium.org/2909009/diff/1/2#newcode138
cros_workon:138: # FIXME: This should probably import the appropriate board
make.conf
Switch to TODO format

http://codereview.chromium.org/2909009/diff/1/2#newcode140
cros_workon:140: CROS_OVERLAYS=~/trunk/src/third_party/chromiumos-overlay/
I've avoided using ~.  Use /home/${USER} instead.

http://codereview.chromium.org/2909009/diff/1/2#newcode144
cros_workon:144: pushd ${overlay} &> /dev/null
Why stderr as well?  Don't you want to know if something goes wrong with the
pushd?

http://codereview.chromium.org/2909009/diff/1/2#newcode147
cros_workon:147: popd &> /dev/null
Same comment

[zbehan, 2010-07-13 23:29:38]

On Tue, Jul 13, 2010 at 3:53 PM, <sosa@chromium.org> wrote:

>
> http://codereview.chromium.org/2909009/diff/1/2
> File cros_workon (right):
>
> http://codereview.chromium.org/2909009/diff/1/2#newcode45
> cros_workon:45: [ -n "${FLAGS_board}" ] && [ "${FLAGS_host}" =
> ${FLAGS_TRUE} ] && \
> Quick question.  Using this logic, you can never use --host if you have
> a default board setup. Do you want this to be mutually exclusive or
> maybe just have host override board?
>

FLAGS_board defaults to DEFAULT_BOARD. --host can override board, but that
will probably make the invocation slightly confusing for people who never
open the insides of the script and don't bother reading -h. Mutual
exclusivity is a clear and simple rule that is easilly enforced and has no
implicit behaviour.


> http://codereview.chromium.org/2909009/diff/1/2#newcode138
> cros_workon:138: # FIXME: This should probably import the appropriate
> board make.conf
> Switch to TODO format
>
> http://codereview.chromium.org/2909009/diff/1/2#newcode140
> cros_workon:140:
>
> CROS_OVERLAYS=~/trunk/src/third_party/chromiumos-overlay/
> I've avoided using ~.  Use /home/${USER} instead.
>

You're looking at an old revision of the patch.

http://codereview.chromium.org/2909009/diff/1/2#newcode144
> cros_workon:144: pushd ${overlay} &> /dev/null
> Why stderr as well?  Don't you want to know if something goes wrong with
> the pushd?
>

To be honest, I don't. In the new revision, I'm looking at directories that
the build specified as PORTDIR_OVERLAY, and if those don't exist, there'll
be a lot of warnings in more places. I can add in an additional check if
${overlay} exists and is executable and readable by me, but if that
condition is met, pushd shouldn't fail.

I'll silence just 2, it's just something I didn't think is important.

http://codereview.chromium.org/2909009/diff/1/2#newcode147
> cros_workon:147: popd &> /dev/null
> Same comment

[sosa, 2010-07-13 23:31:46]

SGTM. LGTM

On Tue, Jul 13, 2010 at 4:29 PM, Zdenek Behan <zbehan@chromium.org> wrote:
> On Tue, Jul 13, 2010 at 3:53 PM, <sosa@chromium.org> wrote:
>>
>> http://codereview.chromium.org/2909009/diff/1/2
>> File cros_workon (right):
>>
>> http://codereview.chromium.org/2909009/diff/1/2#newcode45
>> cros_workon:45: [ -n "${FLAGS_board}" ] && [ "${FLAGS_host}" =
>> ${FLAGS_TRUE} ] && \
>> Quick question.  Using this logic, you can never use --host if you have
>> a default board setup. Do you want this to be mutually exclusive or
>> maybe just have host override board?
>
> FLAGS_board defaults to DEFAULT_BOARD. --host can override board, but that
> will probably make the invocation slightly confusing for people who never
> open the insides of the script and don't bother reading -h. Mutual
> exclusivity is a clear and simple rule that is easilly enforced and has no
> implicit behaviour.
>
>>
>> http://codereview.chromium.org/2909009/diff/1/2#newcode138
>> cros_workon:138: # FIXME: This should probably import the appropriate
>> board make.conf
>> Switch to TODO format
>>
>> http://codereview.chromium.org/2909009/diff/1/2#newcode140
>> cros_workon:140:
>> CROS_OVERLAYS=~/trunk/src/third_party/chromiumos-overlay/
>> I've avoided using ~.  Use /home/${USER} instead.
>
>
> You're looking at an old revision of the patch.
>>
>> http://codereview.chromium.org/2909009/diff/1/2#newcode144
>> cros_workon:144: pushd ${overlay} &> /dev/null
>> Why stderr as well?  Don't you want to know if something goes wrong with
>> the pushd?
>
> To be honest, I don't. In the new revision, I'm looking at directories that
> the build specified as PORTDIR_OVERLAY, and if those don't exist, there'll
> be a lot of warnings in more places. I can add in an additional check if
> ${overlay} exists and is executable and readable by me, but if that
> condition is met, pushd shouldn't fail.
> I'll silence just 2, it's just something I didn't think is important.
>>
>> http://codereview.chromium.org/2909009/diff/1/2#newcode147
>> cros_workon:147: popd &> /dev/null
>> Same comment

[sosa, 2010-07-13 23:41:06]

LGTM agains