Issue 6250170: Make enter_chroot not be chatty unless you use --verbose

dgarrett

9 years, 10 months ago (2011-02-04 22:17:24 UTC) #1

kliegs

http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh File enter_chroot.sh (right): http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode74 enter_chroot.sh:74: function v_info { Would it be better to just ...

9 years, 10 months ago (2011-02-04 22:46:50 UTC) #5

dgarrett

Just pushed before seeing your notes, wasn't blowing you off. On 2011/02/04 22:46:50, kliegs wrote: ...

9 years, 10 months ago (2011-02-04 23:02:53 UTC) #6

dgarrett

kliegs, keep in mind that in the build server / chromite world, it's rare to ...

9 years, 10 months ago (2011-02-04 23:57:33 UTC) #7

dgarrett

http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh File enter_chroot.sh (right): http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode74 enter_chroot.sh:74: function v_info { I just checked, and DEBUG doesn't ...

9 years, 10 months ago (2011-02-04 23:57:43 UTC) #8

kliegs

I do commend you for trying to do an output cleanup. Cleaning up trivial messages ...

9 years, 10 months ago (2011-02-05 07:55:54 UTC) #9

I do commend you for trying to do an output cleanup.  Cleaning up trivial
messages makes the real ones stick out much cleaner.

It feels to me like this could be better served by moving the messages that
matter to warning and leaving the suppressed ones at info (using a wrapper to
disable without the verbose flag) or at debug if you implement that (debug would
just be your v_info function renamed).  

Its a bit late since you've already submitted this and it sounds like busy work
now, but I'd rather see the changes done right and in a way that if someone were
to use the code in enter_chroot as a template for how to write a new shell
script elsewhere that they'd have a solid, cruft-free example to follow. 

This is honestly more a concern over future maintainability - a new engineer
coming in will likely be unsure about what should be info vs. v_info and waste
time working through it.  But almost any engineer who comes in and sees warning,
debug will already have context around those distinctions.

http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh
File enter_chroot.sh (right):

http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode74
enter_chroot.sh:74: function v_info {
Even if DEBUG doesn't exist I'd rather see you call your new function DEBUG than
VINFO.  VINFO has no meaning to anyone but people all understand what DEBUG
means.  Having the flag that enables it called verbose or similar is fine - a
--verbose or -v flag is a common mechanism for shell scripts.  

Creating a new level just strikes me as a really bad idea.  Every other project
I've been part of fights really hard to not add new log levels and there's a
reason for it.  I'd like to see us do the same and use the standard levels that
the industry is familiar with.  It should at least be done with some real
thought - breaking convention shouldn't be done lightly.

On 2011/02/04 23:57:43, dgarrett wrote:
> I just checked, and DEBUG doesn't seem to exist for shell scripts. There also
> doesn't seem to be any way to adjust which levels are displayed. I'd checked
for
> that before submitting this.
> 
> I think created a new 'level' for this script is the only way to go.
> 
> On 2011/02/04 22:46:50, kliegs wrote:
> > Would it be better to just shift the output levels used? So things that are
> info
> > but should stay get promoted to warning?
> > 
> > And things that should be suppressed are either left at INFO or moved to
> > DEBUG/NOTICE?
> > 
> > There are some (mostly) standard names for output messages (ERROR, WARN,
INFO,
> > DEBUG, NOTICE).  Adding a new one (V_INFO) seems unnecessary.
>

http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode199
enter_chroot.sh:199: v_info "Not mounting chrome source"
We're not in the chromite world yet.  Our entire team mounts chrome outside the
chroot and typos on the path are fairly common.  I know most teams ignore Chrome
and only use whatever the ebuild gets them but losing this warning is bad.

What I was suggesting wasn't to make it always say that.  But instead of
removing it fully, only remove it when -z is true. So it'd become

if [[ -z "$CHROME_ROOT" ]]; then
  v_info "Not mounting chrome source"
  sudo rm -rf "${FLAGS_chroot}${CHROME_ROOT_CONFIG}"
elif  [[ ! -d "${CHROME_ROOT}/src" ]]; then
  error "Not mounting chrome source"
else 
  v_info "Mounting chrome source at: $INNER_CHROME_ROOT"
   ...

On 2011/02/04 23:57:43, dgarrett wrote:
> Currently, this error is displayed every time you enter without valid Chrome
> source. That's just noise.
> 
> On 2011/02/04 22:46:50, kliegs wrote:
> > This can actually be an error if CHROME_ROOT is set but doesn't exist. If
> you're
> > going to suppress it in the case where CHROME_ROOT is not set you should add
> it
> > as an error for when it is
>

http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode285
enter_chroot.sh:285: v_info "tearing down env."
Its a very rare case for me.  I almost never see this unless something is wrong.
 Its also been instrumental in finding some problems with the build bots (will
they be running with verbose?)  How sure are you this is a common case?
On 2011/02/04 23:57:43, dgarrett wrote:
> It's an awfully normal case though.
> 
> On 2011/02/04 22:46:50, kliegs wrote:
> > This feels like a warning to me, not an error.  Depending on the situation
it
> > can often mean something is wrong and not just be an advisory info.
>

diandersAtChromium

FWIW, I agree w/ klieg's comments. I'd be happy to implement his suggestions if nobody ...

9 years, 10 months ago (2011-02-07 21:36:52 UTC) #10

FWIW, I agree w/ klieg's comments.  I'd be happy to implement his
suggestions if nobody else is planning on doing it.

Thanks!

-Doug

---

On Fri, Feb 4, 2011 at 11:55 PM, <kliegs@chromium.org> wrote:

> I do commend you for trying to do an output cleanup.  Cleaning up trivial
> messages makes the real ones stick out much cleaner.
>
> It feels to me like this could be better served by moving the messages that
> matter to warning and leaving the suppressed ones at info (using a wrapper
> to
> disable without the verbose flag) or at debug if you implement that (debug
> would
> just be your v_info function renamed).
>
> Its a bit late since you've already submitted this and it sounds like busy
> work
> now, but I'd rather see the changes done right and in a way that if someone
> were
> to use the code in enter_chroot as a template for how to write a new shell
> script elsewhere that they'd have a solid, cruft-free example to follow.
>
>
>
> This is honestly more a concern over future maintainability - a new
> engineer
> coming in will likely be unsure about what should be info vs. v_info and
> waste
> time working through it.  But almost any engineer who comes in and sees
> warning,
> debug will already have context around those distinctions.
>
>
>
> http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh
> File enter_chroot.sh (right):
>
> http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode74
> enter_chroot.sh:74: function v_info {
> Even if DEBUG doesn't exist I'd rather see you call your new function
> DEBUG than VINFO.  VINFO has no meaning to anyone but people all
> understand what DEBUG means.  Having the flag that enables it called
> verbose or similar is fine - a --verbose or -v flag is a common
> mechanism for shell scripts.
>
> Creating a new level just strikes me as a really bad idea.  Every other
> project I've been part of fights really hard to not add new log levels
> and there's a reason for it.  I'd like to see us do the same and use the
> standard levels that the industry is familiar with.  It should at least
> be done with some real thought - breaking convention shouldn't be done
> lightly.
>
>
> On 2011/02/04 23:57:43, dgarrett wrote:
>
>> I just checked, and DEBUG doesn't seem to exist for shell scripts.
>>
> There also
>
>> doesn't seem to be any way to adjust which levels are displayed. I'd
>>
> checked for
>
>> that before submitting this.
>>
>
>  I think created a new 'level' for this script is the only way to go.
>>
>
>  On 2011/02/04 22:46:50, kliegs wrote:
>> > Would it be better to just shift the output levels used? So things
>>
> that are
>
>> info
>> > but should stay get promoted to warning?
>> >
>> > And things that should be suppressed are either left at INFO or
>>
> moved to
>
>> > DEBUG/NOTICE?
>> >
>> > There are some (mostly) standard names for output messages (ERROR,
>>
> WARN, INFO,
>
>> > DEBUG, NOTICE).  Adding a new one (V_INFO) seems unnecessary.
>>
>
>
> http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode199
> enter_chroot.sh:199: v_info "Not mounting chrome source"
> We're not in the chromite world yet.  Our entire team mounts chrome
> outside the chroot and typos on the path are fairly common.  I know most
> teams ignore Chrome and only use whatever the ebuild gets them but
> losing this warning is bad.
>
> What I was suggesting wasn't to make it always say that.  But instead of
> removing it fully, only remove it when -z is true. So it'd become
>
> if [[ -z "$CHROME_ROOT" ]]; then
>
>  v_info "Not mounting chrome source"
>  sudo rm -rf "${FLAGS_chroot}${CHROME_ROOT_CONFIG}"
> elif  [[ ! -d "${CHROME_ROOT}/src" ]]; then
>  error "Not mounting chrome source"
> else
>
>  v_info "Mounting chrome source at: $INNER_CHROME_ROOT"
>   ...
>
>
> On 2011/02/04 23:57:43, dgarrett wrote:
>
>> Currently, this error is displayed every time you enter without valid
>>
> Chrome
>
>> source. That's just noise.
>>
>
>  On 2011/02/04 22:46:50, kliegs wrote:
>> > This can actually be an error if CHROME_ROOT is set but doesn't
>>
> exist. If
>
>> you're
>> > going to suppress it in the case where CHROME_ROOT is not set you
>>
> should add
>
>> it
>> > as an error for when it is
>>
>
>
> http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode285
> enter_chroot.sh:285: v_info "tearing down env."
> Its a very rare case for me.  I almost never see this unless something
> is wrong.  Its also been instrumental in finding some problems with the
> build bots (will they be running with verbose?)  How sure are you this
> is a common case?
>
> On 2011/02/04 23:57:43, dgarrett wrote:
>
>> It's an awfully normal case though.
>>
>
>  On 2011/02/04 22:46:50, kliegs wrote:
>> > This feels like a warning to me, not an error.  Depending on the
>>
> situation it
>
>> > can often mean something is wrong and not just be an advisory info.
>>
>
>
> http://codereview.chromium.org/6250170/
>

dgarrett

9 years, 10 months ago (2011-02-07 22:04:50 UTC) #11

Heh, I was just doing it, but seem to have done something nasty to my git
repo. I'll copy you on the review as soon as that's fixed.

On Mon, Feb 7, 2011 at 1:36 PM, Doug Anderson <dianders@chromium.org> wrote:

> FWIW, I agree w/ klieg's comments.  I'd be happy to implement his
> suggestions if nobody else is planning on doing it.
>
> Thanks!
>
> -Doug
>
> ---
>
> On Fri, Feb 4, 2011 at 11:55 PM, <kliegs@chromium.org> wrote:
>
>> I do commend you for trying to do an output cleanup.  Cleaning up trivial
>> messages makes the real ones stick out much cleaner.
>>
>> It feels to me like this could be better served by moving the messages
>> that
>> matter to warning and leaving the suppressed ones at info (using a wrapper
>> to
>> disable without the verbose flag) or at debug if you implement that (debug
>> would
>> just be your v_info function renamed).
>>
>> Its a bit late since you've already submitted this and it sounds like busy
>> work
>> now, but I'd rather see the changes done right and in a way that if
>> someone were
>> to use the code in enter_chroot as a template for how to write a new shell
>> script elsewhere that they'd have a solid, cruft-free example to follow.
>>
>>
>>
>> This is honestly more a concern over future maintainability - a new
>> engineer
>> coming in will likely be unsure about what should be info vs. v_info and
>> waste
>> time working through it.  But almost any engineer who comes in and sees
>> warning,
>> debug will already have context around those distinctions.
>>
>>
>>
>> http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh
>> File enter_chroot.sh (right):
>>
>> http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode74
>> enter_chroot.sh:74: function v_info {
>> Even if DEBUG doesn't exist I'd rather see you call your new function
>> DEBUG than VINFO.  VINFO has no meaning to anyone but people all
>> understand what DEBUG means.  Having the flag that enables it called
>> verbose or similar is fine - a --verbose or -v flag is a common
>> mechanism for shell scripts.
>>
>> Creating a new level just strikes me as a really bad idea.  Every other
>> project I've been part of fights really hard to not add new log levels
>> and there's a reason for it.  I'd like to see us do the same and use the
>> standard levels that the industry is familiar with.  It should at least
>> be done with some real thought - breaking convention shouldn't be done
>> lightly.
>>
>>
>> On 2011/02/04 23:57:43, dgarrett wrote:
>>
>>> I just checked, and DEBUG doesn't seem to exist for shell scripts.
>>>
>> There also
>>
>>> doesn't seem to be any way to adjust which levels are displayed. I'd
>>>
>> checked for
>>
>>> that before submitting this.
>>>
>>
>>  I think created a new 'level' for this script is the only way to go.
>>>
>>
>>  On 2011/02/04 22:46:50, kliegs wrote:
>>> > Would it be better to just shift the output levels used? So things
>>>
>> that are
>>
>>> info
>>> > but should stay get promoted to warning?
>>> >
>>> > And things that should be suppressed are either left at INFO or
>>>
>> moved to
>>
>>> > DEBUG/NOTICE?
>>> >
>>> > There are some (mostly) standard names for output messages (ERROR,
>>>
>> WARN, INFO,
>>
>>> > DEBUG, NOTICE).  Adding a new one (V_INFO) seems unnecessary.
>>>
>>
>>
>>
>> http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode199
>> enter_chroot.sh:199: v_info "Not mounting chrome source"
>> We're not in the chromite world yet.  Our entire team mounts chrome
>> outside the chroot and typos on the path are fairly common.  I know most
>> teams ignore Chrome and only use whatever the ebuild gets them but
>> losing this warning is bad.
>>
>> What I was suggesting wasn't to make it always say that.  But instead of
>> removing it fully, only remove it when -z is true. So it'd become
>>
>> if [[ -z "$CHROME_ROOT" ]]; then
>>
>>  v_info "Not mounting chrome source"
>>  sudo rm -rf "${FLAGS_chroot}${CHROME_ROOT_CONFIG}"
>> elif  [[ ! -d "${CHROME_ROOT}/src" ]]; then
>>  error "Not mounting chrome source"
>> else
>>
>>  v_info "Mounting chrome source at: $INNER_CHROME_ROOT"
>>   ...
>>
>>
>> On 2011/02/04 23:57:43, dgarrett wrote:
>>
>>> Currently, this error is displayed every time you enter without valid
>>>
>> Chrome
>>
>>> source. That's just noise.
>>>
>>
>>  On 2011/02/04 22:46:50, kliegs wrote:
>>> > This can actually be an error if CHROME_ROOT is set but doesn't
>>>
>> exist. If
>>
>>> you're
>>> > going to suppress it in the case where CHROME_ROOT is not set you
>>>
>> should add
>>
>>> it
>>> > as an error for when it is
>>>
>>
>>
>>
>> http://codereview.chromium.org/6250170/diff/1002/enter_chroot.sh#newcode285
>> enter_chroot.sh:285: v_info "tearing down env."
>> Its a very rare case for me.  I almost never see this unless something
>> is wrong.  Its also been instrumental in finding some problems with the
>> build bots (will they be running with verbose?)  How sure are you this
>> is a common case?
>>
>> On 2011/02/04 23:57:43, dgarrett wrote:
>>
>>> It's an awfully normal case though.
>>>
>>
>>  On 2011/02/04 22:46:50, kliegs wrote:
>>> > This feels like a warning to me, not an error.  Depending on the
>>>
>> situation it
>>
>>> > can often mean something is wrong and not just be an advisory info.
>>>
>>
>>
>> http://codereview.chromium.org/6250170/
>>
>
>


-- 
Don

Issue 6250170: Make enter_chroot not be chatty unless you use --verbose (Closed)

Description

Patch Set 1 #

Patch Set 2 : Add comment explaining v_info. #

Messages