Skip to main content

[pkg-discuss] Re: Fwd: Re: Review request -> 17734557 Bad formatting of error text from pkg update command

  • From: Erik Trauschke < >
  • To: Xiaobo Shen < >
  • Cc:
  • Subject: [pkg-discuss] Re: Fwd: Re: Review request -> 17734557 Bad formatting of error text from pkg update command
  • Date: Thu, 19 Dec 2013 18:05:31 -0800



On 12/19/13 02:49 PM, Xiaobo Shen wrote:
On 12/18/13 11:53 AM, Erik Trauschke wrote:


On 12/18/13 11:43 AM, Xiaobo Shen wrote:
This one is almost on its way to put back. Could somebody send more
suggestions? Thanks.

xiaobo


-------- Original Message --------
Subject:     [pkg-discuss] Re: Review request -> 17734557 Bad
formatting of
error text from pkg update command
Date:     Fri, 15 Nov 2013 11:20:23 -0800
From:     Xiaobo Shen 
< >
Reply-To:     

To:     




On 11/13/13 07:10 AM, Erik Trauschke wrote:


On 11/12/13 04:31 PM, Xiaobo Shen wrote:
On 11/12/13 01:22 PM, Erik Trauschke wrote:


On 11/12/13 12:48 PM, Tim Foster wrote:
On 11/13/13 09:10 AM, Xiaobo Shen wrote:
On 11/11/13 01:40 PM, Tim Foster wrote:
That's the easiest way to fix the problem, but it's not the
best way
to do it.

I found we might call flush() in exception handling. It works
and the
output is the same as above.
Please see:
https://ips.java.net/webrev/xiaoshen/pkgbadformat_17734557_2/

That's much nicer - thanks. I'm happy with the changes as you have
them.

As Bart is fond of saying, "all bugs have friends" - if you have
time,
it could be interesting to have a quick scan through the rest of the
code in the gate to see if there's other places where we might run
into
the same issue with clients using the ProgressTracker. You don't
need
to fix them in this wad, but filing bugs against them would be
helpful.

- if you don't have time, that's ok.

I'm wondering if we should put the flush into src/client.py:error().
The imageplan shouldn't be bothered with correct print-outs of error
messages, this should be the job of the CLI code.
I found that the msg "The running system has not been modified...." is
from restore_image call in client/bootenv.py. This msg will be printed
before handling exception msgs in client.py. For instance,
If we press ctr_c, the latest place we need a "\n" is before
be.restore_image() at line 2682 in api.py.
I can add the flush() call at the beginning of this exception handling
and a couple of other ones. Let me
know if it is applicable. Otherwise we need to figure out some other
ways.

Ah, yes. This happens because it uses logger directly instead of
ProgressTracker. This looks like a good follow-on bug to fix.
However, seeing that all of bootenv.py uses logging instead of
exceptions and doesn't use ProgressTracker at all this could be a bit
more involved.

I think in this case we'd have to print a "\n" before the message
since we have no access to the ProgressTracker. I'm also wondering if
we would run into the same issue with all the other logger.error()
calls in this class.

I'm not too familiar with the ProgressTracker but would it be an
issue
to run a flush every time we print messages with error()?

After flush(), The ProgressTracker will set the __need_nl in
printengine
into false. So I did not see a problem, but I can test it for sure.

Cool, thanks

Erik
I did some change about the bootenv.py to allow it using
progress_tracker flush() before
the log msgs are printed out. Let me know what you guys think. Thanks.

new link:
https://ips.java.net/webrev/xiaoshen/pkgbadformat_17734557_2/

src/modules/client/bootenv.py:
general
    I'd think you only want to flush the progress tracker if you are
printing a message. But I'm not sure if that actually matters. And we
only call it if we have a failed install attempt. I just want to make
sure you don't get newlines where they are not intended.

Updated:
https://ips.java.net/webrev/xiaoshen/pkgbadformat_17734557_3/

Tested with multiple flush() calles at once. Only one newline is
produced. I think the printengine maintain
a status with a flag "__need_nl" which tells whether we need a new line.
After flush(), the flag is set to false.
So no matter how many flush() calles afterwards, only one newline is
produced.
It is set to true only if we use the printengine again. So we do not
need to worry about it in this case.

My concern was more about printing a new line while we are still updating a certain line so that you end up with something like this:

   Doing stuff 1234/5000
   Doing stuff 1235/5000

But I don't think that's a problem here since all these calls only happen if something got screwed anyway.
Maybe add a comment for the flush saying it's necessary so that warnings get printed on a new line.

Otherwise +1

Erik



76++
    The default for progress_tracker is None, so just doing
self.progress_tracker = progress_tracker achieves exactly the same thing.


fixed.

Otherwise I'd be ok with this.

Erik

xiaobo


[pkg-discuss] Fwd: Re: Review request -> 17734557 Bad formatting of error text from pkg update command

Xiaobo Shen 12/18/2013

[pkg-discuss] Re: Fwd: Re: Review request -> 17734557 Bad formatting of error text from pkg update command

Erik Trauschke 12/18/2013

[pkg-discuss] Re: Fwd: Re: Review request -> 17734557 Bad formatting of error text from pkg update command

Xiaobo Shen 12/19/2013

[pkg-discuss] Re: Fwd: Re: Review request -> 17734557 Bad formatting of error text from pkg update command

Erik Trauschke 12/20/2013

[pkg-discuss] Re: Fwd: Re: Review request -> 17734557 Bad formatting of error text from pkg update command

Xiaobo Shen 12/20/2013

[pkg-discuss] Re: Fwd: Re: Review request -> 17734557 Bad formatting of error text from pkg update command

Erik Trauschke 12/20/2013
 
 
Close
loading
Please Confirm
Close