Re: [Bug-wget] broken progressbar in 1.16
On Tue, Oct 28, 2014 at 04:54:06PM +0800, Darshit Shah wrote: I'll probably look into this tomorrow, but this information should help save a few minutes for anyone who tries looking into this issue before I can. Just to make it clear instead of simply disappearing: 1.16.2 is fine for me regarding that (former) regression, thanks a lot! -- WBR, Michael Shigorin / http://altlinux.org -- http://opennet.ru / http://anna-news.info
Re: [Bug-wget] broken progressbar in 1.16
On 11/15, Darshit Shah wrote: On 11/07, Darshit Shah wrote: The last patch breaks the download but is only available on the git repository and will not be included in any release. I will be pushing a patch soon which will remove all assert statements from production releases so that the downloads continue with a minor visual annoyance. Because real life is catching up, I've very little time till the end of this year. But I'm going to try and debug this issue and fix it asap. And eventually I found time to look into this with a fresh mind. It turns out that I have been looking at the code in a wrong fashion, and hence the assumption I wished to validate with the assert was wrong too. A simple off-by-one error that made my life living hell. I've fixed the issue with the following patch. Please take a look at it. I really hope there's no more issues remaining with the progress bar. Since no one objected, pushed! On 06-Nov-2014 7:02 pm, Noël Köthe n...@debian.org wrote: Hello, Am Montag, den 03.11.2014, 10:12 +0530 schrieb Darshit Shah: e-flightgear-201411 0%[ ] 1,05M 286KB/s eta 1h 41m wget: progress.c:1161: create_image: Проверочное утверждение «count_cols (bp-buffer) = bp-width» не выполнено. zsh: abort wget -c (it's [Assertion count_cols (bp-buffer) = bp-width failed] I think) This bug keeps getting more interesting and surprising. While the progress bar draws correctly for the original file you mentioned, the assertion fails with the new ISO (live-flightgear). The funny thing is, this bug is now firing based on the filename of the file being downloaded, which is not something I expect since the filename section of the progress bar is isolated from the rest and should not create such an effect. I'm going to spend some more time debugging this issue. I applied the progress bar patch on 1.16 and run into the same problem. Connecting to fly.osdn.org.ua (fly.osdn.org.ua)|212.40.45.6|:80... connected. HTTP request sent, awaiting response... 206 Partial Content Length: 1599602688 (1.5G), 1590685248 (1.5G) remaining [application/octet-stream] Saving to: 'live-flightgear-20141101-x86_64.iso' ar-20141101-x86_64. 0%[ ] 12.47M 1.41MB/s wget: progress.c:1161: create_image: Assertion `count_cols (bp-buffer) = bp-width' failed. Looks like shipping wget with the broken progress bar because the patch could stop downloading. Maybe the default could be the old but stable progress bar and when the new feature works it could be changed to the new default? Regards Noël Addendum == I added the assertion there to ensure that the basic assumption about the progress bar is always held. When I added the assert statement, the only time it was expected to fail was when wget is executed using a non English locale. However, through my own experience and via other reports on this mailing list, I have to realize that the assert fails even on English locale and when the progress bar displays correctly. The fact that the progress bar manages to print on a single line even when the assertion that `no_of_cols(progress_bar) = columns_in_line` fails means that there is a subtle bug hidden deep in the implementation which is beyond the changes I made. It also implies that we do not currently know how this code works since it performs contrary to our expectation. This is definitely a high priority issue that needs more investigation and a fix as soon as possible. --- end quoted text --- -- Thanking You, Darshit Shah From 94805ad55ae53039717a7f705fe41f0cbe0d8ebc Mon Sep 17 00:00:00 2001 From: Darshit Shah dar...@gmail.com Date: Sat, 15 Nov 2014 00:13:13 +0530 Subject: [PATCH 1/2] Fix progress bar assertion --- src/ChangeLog | 7 ++- src/progress.c | 10 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 32eebda..d085953 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,8 @@ +2014-11-15 Darshit Shah dar...@gmail.com + + * progress.c (create_image): Fix assertion that checks progress bar length + Should fix bug #43593 + 2014-11-12 Tim Ruehsen tim.rueh...@gmx.de * openssl.c (ssl_init): Fix error handling for CRL loading @@ -7,7 +12,7 @@ * html-parse.c (map_html_tags): Fix range check 2014-11-11 Tim Ruehsen tim.rueh...@gmx.de - + * openssl.c (ssl_init): Load CRL file given by --crl-file 2014-11-11 Tim Ruehsen tim.rueh...@gmx.de diff --git a/src/progress.c b/src/progress.c index a0b48e4..d23654d 100644 --- a/src/progress.c +++ b/src/progress.c @@ -1158,7 +1158,15 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done) while (p - bp-buffer - bytes_cols_diff bp-width) *p++ = ' '; *p = '\0'; - assert (count_cols (bp-buffer) = bp-width); + + /* 2014-11-14 Darshit Shah dar...@gmail.com + * Assert that the length of the progress bar is lesser than the size of the
Re: [Bug-wget] broken progressbar in 1.16
On 11/07, Darshit Shah wrote: The last patch breaks the download but is only available on the git repository and will not be included in any release. I will be pushing a patch soon which will remove all assert statements from production releases so that the downloads continue with a minor visual annoyance. Because real life is catching up, I've very little time till the end of this year. But I'm going to try and debug this issue and fix it asap. And eventually I found time to look into this with a fresh mind. It turns out that I have been looking at the code in a wrong fashion, and hence the assumption I wished to validate with the assert was wrong too. A simple off-by-one error that made my life living hell. I've fixed the issue with the following patch. Please take a look at it. I really hope there's no more issues remaining with the progress bar. On 06-Nov-2014 7:02 pm, Noël Köthe n...@debian.org wrote: Hello, Am Montag, den 03.11.2014, 10:12 +0530 schrieb Darshit Shah: e-flightgear-201411 0%[ ] 1,05M 286KB/s eta 1h 41m wget: progress.c:1161: create_image: Проверочное утверждение «count_cols (bp-buffer) = bp-width» не выполнено. zsh: abort wget -c (it's [Assertion count_cols (bp-buffer) = bp-width failed] I think) This bug keeps getting more interesting and surprising. While the progress bar draws correctly for the original file you mentioned, the assertion fails with the new ISO (live-flightgear). The funny thing is, this bug is now firing based on the filename of the file being downloaded, which is not something I expect since the filename section of the progress bar is isolated from the rest and should not create such an effect. I'm going to spend some more time debugging this issue. I applied the progress bar patch on 1.16 and run into the same problem. Connecting to fly.osdn.org.ua (fly.osdn.org.ua)|212.40.45.6|:80... connected. HTTP request sent, awaiting response... 206 Partial Content Length: 1599602688 (1.5G), 1590685248 (1.5G) remaining [application/octet-stream] Saving to: 'live-flightgear-20141101-x86_64.iso' ar-20141101-x86_64. 0%[ ] 12.47M 1.41MB/s wget: progress.c:1161: create_image: Assertion `count_cols (bp-buffer) = bp-width' failed. Looks like shipping wget with the broken progress bar because the patch could stop downloading. Maybe the default could be the old but stable progress bar and when the new feature works it could be changed to the new default? Regards Noël Addendum == I added the assertion there to ensure that the basic assumption about the progress bar is always held. When I added the assert statement, the only time it was expected to fail was when wget is executed using a non English locale. However, through my own experience and via other reports on this mailing list, I have to realize that the assert fails even on English locale and when the progress bar displays correctly. The fact that the progress bar manages to print on a single line even when the assertion that `no_of_cols(progress_bar) = columns_in_line` fails means that there is a subtle bug hidden deep in the implementation which is beyond the changes I made. It also implies that we do not currently know how this code works since it performs contrary to our expectation. This is definitely a high priority issue that needs more investigation and a fix as soon as possible. --- end quoted text --- -- Thanking You, Darshit Shah From 94805ad55ae53039717a7f705fe41f0cbe0d8ebc Mon Sep 17 00:00:00 2001 From: Darshit Shah dar...@gmail.com Date: Sat, 15 Nov 2014 00:13:13 +0530 Subject: [PATCH 1/2] Fix progress bar assertion --- src/ChangeLog | 7 ++- src/progress.c | 10 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 32eebda..d085953 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,8 @@ +2014-11-15 Darshit Shah dar...@gmail.com + + * progress.c (create_image): Fix assertion that checks progress bar length + Should fix bug #43593 + 2014-11-12 Tim Ruehsen tim.rueh...@gmx.de * openssl.c (ssl_init): Fix error handling for CRL loading @@ -7,7 +12,7 @@ * html-parse.c (map_html_tags): Fix range check 2014-11-11 Tim Ruehsen tim.rueh...@gmx.de - + * openssl.c (ssl_init): Load CRL file given by --crl-file 2014-11-11 Tim Ruehsen tim.rueh...@gmx.de diff --git a/src/progress.c b/src/progress.c index a0b48e4..d23654d 100644 --- a/src/progress.c +++ b/src/progress.c @@ -1158,7 +1158,15 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done) while (p - bp-buffer - bytes_cols_diff bp-width) *p++ = ' '; *p = '\0'; - assert (count_cols (bp-buffer) = bp-width); + + /* 2014-11-14 Darshit Shah dar...@gmail.com + * Assert that the length of the progress bar is lesser than the size of the + * screen with which we are dealing. This assertion *MUST* always be
Re: [Bug-wget] broken progressbar in 1.16
On 11/09, Ángel González wrote: On 07/11/14 06:12, Darshit Shah wrote: Because real life is catching up, I've very little time till the end of this year. But I'm going to try and debug this issue and fix it asap. A small suggestion before you stop touching this part of the codebase: do not include the directory-prefix (when set) in the progressbar. Its addition may cause the filename piece to marquee (it's not uncommon that they get long), and it's unnecessary, as the goal of adding --directory-prefix was precisely to prefix every filename with that. I'll look into this. I would like to remove the directory-prefix as well. -- Thanking You, Darshit Shah pgpKRoFEREm8m.pgp Description: PGP signature
Re: [Bug-wget] broken progressbar in 1.16
On 07/11/14 06:12, Darshit Shah wrote: Because real life is catching up, I've very little time till the end of this year. But I'm going to try and debug this issue and fix it asap. A small suggestion before you stop touching this part of the codebase: do not include the directory-prefix (when set) in the progressbar. Its addition may cause the filename piece to marquee (it's not uncommon that they get long), and it's unnecessary, as the goal of adding --directory-prefix was precisely to prefix every filename with that.
Re: [Bug-wget] broken progressbar in 1.16
Hello, Am Montag, den 03.11.2014, 10:12 +0530 schrieb Darshit Shah: e-flightgear-201411 0%[ ] 1,05M 286KB/s eta 1h 41m wget: progress.c:1161: create_image: Проверочное утверждение «count_cols (bp-buffer) = bp-width» не выполнено. zsh: abort wget -c (it's [Assertion count_cols (bp-buffer) = bp-width failed] I think) This bug keeps getting more interesting and surprising. While the progress bar draws correctly for the original file you mentioned, the assertion fails with the new ISO (live-flightgear). The funny thing is, this bug is now firing based on the filename of the file being downloaded, which is not something I expect since the filename section of the progress bar is isolated from the rest and should not create such an effect. I'm going to spend some more time debugging this issue. I applied the progress bar patch on 1.16 and run into the same problem. Connecting to fly.osdn.org.ua (fly.osdn.org.ua)|212.40.45.6|:80... connected. HTTP request sent, awaiting response... 206 Partial Content Length: 1599602688 (1.5G), 1590685248 (1.5G) remaining [application/octet-stream] Saving to: 'live-flightgear-20141101-x86_64.iso' ar-20141101-x86_64. 0%[ ] 12.47M 1.41MB/s wget: progress.c:1161: create_image: Assertion `count_cols (bp-buffer) = bp-width' failed. Looks like shipping wget with the broken progress bar because the patch could stop downloading. Maybe the default could be the old but stable progress bar and when the new feature works it could be changed to the new default? Regards Noël signature.asc Description: This is a digitally signed message part
Re: [Bug-wget] broken progressbar in 1.16
On Mon, Nov 03, 2014 at 10:12:55AM +0530, Darshit Shah wrote: This is a new assertion I just added to ensure that the logic of the progress bar is correct. I know :) Just tried to both keep the output authentic and make it legible to you. http://fly.osdn.org.ua/~mike/iso/tmp/live-flightgear-20141101-x86_64.iso This bug keeps getting more interesting and surprising. While the progress bar draws correctly for the original file you mentioned, the assertion fails with the new ISO (live-flightgear). The funny thing is, this bug is now firing based on the filename of the file being downloaded, which is not something I expect since the filename section of the progress bar is isolated from the rest and should not create such an effect. I'm going to spend some more time debugging this issue. Thank you; in addition to RC proposal (and volunteering to spend some time testing before release): might be worth it to ask the community to pound more heavily on features with defaults to be changed (like this time), seems like this might have been caught in already released versions given that the new progress bar is enabled by hand. -- WBR, Michael Shigorin / http://altlinux.org -- http://opennet.ru / http://anna-news.info
Re: [Bug-wget] broken progressbar in 1.16
On 11/01, Giuseppe Scrivano wrote: Darshit Shah dar...@gmail.com writes: From 1a29f21bb24c531f450dd59446a0c5a0f7e81110 Mon Sep 17 00:00:00 2001 From: Darshit Shah dar...@gmail.com Date: Sat, 1 Nov 2014 05:34:04 +0530 Subject: [PATCH] Remove extra padding from the progress bar --- src/ChangeLog | 7 +++ src/progress.c | 7 ++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 58bf905..214441f 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,10 @@ +2014-11-01 Darshit Shah dar...@gmail.com + + * progress.c (create_image): Extra padding for size_grouped_diff has already + been added. Do not add that again. + (create_image): Assert that the progress bar being drawn is lesser than the + size of the screen. + 2014-10-29 Peter Meiser mei...@gmx-topmail.de (tiny change) * openssl.c (ssl_init) [! OPENSSL_NO_SSL3]: Add guard for OpenSSL diff --git a/src/progress.c b/src/progress.c index 5ba542d..f211998 100644 --- a/src/progress.c +++ b/src/progress.c @@ -907,10 +907,6 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done) char *p = bp-buffer; wgint size = bp-initial_length + bp-count; - const char *size_grouped = with_thousand_seps (size); - int size_grouped_len = count_cols (size_grouped); - /* Difference between num cols and num bytes: */ - int size_grouped_diff = strlen (size_grouped) - size_grouped_len; int size_grouped_pad; /* Used to pad the field width for size_grouped. */ struct bar_progress_hist *hist = bp-hist; @@ -1159,9 +1155,10 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done) move_to_end (p); } - while (p - bp-buffer - bytes_cols_diff - size_grouped_diff bp-width) + while (p - bp-buffer - bytes_cols_diff bp-width) *p++ = ' '; *p = '\0'; + assert (count_cols(bp-buffer) = bp-width); } /* Print the contents of the buffer as a one-line ASCII image so ACK with a whitespace after count_cols. Pushed! -- Thanking You, Darshit Shah pgpBdjbBrqELf.pgp Description: PGP signature
Re: [Bug-wget] broken progressbar in 1.16
On Sat, Nov 01, 2014 at 11:39:55PM +0530, Darshit Shah wrote: Pushed! Pulled; not there yet: e-flightgear-201411 0%[ ] 1,05M 286KB/s eta 1h 41m wget: progress.c:1161: create_image: Проверочное утверждение «count_cols (bp-buffer) = bp-width» не выполнено. zsh: abort wget -c (it's [Assertion count_cols (bp-buffer) = bp-width failed] I think) I've filtered the patch to exclude changelog hunk and applied it on top of 1.16 release; the downloaded size is 1168780 of 1599602688 bytes, the image is (my homegrown testing one) http://fly.osdn.org.ua/~mike/iso/tmp/live-flightgear-20141101-x86_64.iso Second run resulted in 1002020 bytes downloaded with the same failure. -- WBR, Michael Shigorin / http://altlinux.org -- http://opennet.ru / http://anna-news.info
Re: [Bug-wget] broken progressbar in 1.16
On 10/28, Darshit Shah wrote: Hi Michael, Thanks a lot for your email. While we try our best to regression test every new feature, some issues often seep through the cracks. I'm traveling so testing this is a little hard for me. But I managed to do some preliminary testing and identified that the given issue is being triggered when run in a different locale and the size being displayed in the progress bar is greater than 1M. As soon as the size being displayed there exceeds the 1M mark, the issue gets triggered and I see a new line being created for each refresh of the progress bar. I'll probably look into this tomorrow, but this information should help save a few minutes for anyone who tries looking into this issue before I can. On 28-Oct-2014 2:13 pm, Michael Shigorin m...@altlinux.org wrote: Hello, please do pay attention to regression testing for innovative bells and especially whistles as the new and improved progress bar quickly becomes annoying if the filename is longer than the part of the line before percent meter: the new code tries to scroll it (this alone might be irritating to some including myself) and for some cases fails to do that properly falling into spam out a new line every cycle in xterm with an UTF-8 locale. Adding the filename was important for some of the other features that we are currently planning on for future releases of Wget. Some people want the filename to scroll, others find it irritating. We set scrolling as the default, but a new option, --no-scroll has also been added for people who would prefer the progress bar not have a scrolling filename. You could set it in your wgetrc file to add the option globally. e.g., $ LANG=ru_RU.UTF-8 wget -c http://fly.osdn.org.ua/~mike/iso/rescue/live-rescue-20141027-x86_64.iso --2014-10-28 00:28:44-- http://fly.osdn.org.ua/~mike/iso/rescue/live-rescue-20141027-x86_64.iso Распознаётся fly.osdn.org.ua (fly.osdn.org.ua)... 212.40.45.6 Подключение к fly.osdn.org.ua (fly.osdn.org.ua)|212.40.45.6|:80... соединение установлено. HTTP-запрос отправлен. Ожидание ответа... 200 OK Длина: 620756992 (592M) [application/octet-stream] Сохранение в: live-rescue-20141027-x86_64.iso -rescue-20141027-x8 0%[ ] 1,25M 1,57MB/s rescue-20141027-x86 0%[ ] 1,65M 1,65MB/s escue-20141027-x86_ 0%[ ] 2,06M 1,71MB/s scue-20141027-x86_6 0%[ ] 2,44M 1,74MB/s cue-20141027-x86_64 0%[ ] 2,84M 1,78MB/s ue-20141027-x86_64. 0%[ ] 3,26M 1,81MB/s e-20141027-x86_64.i 0%[ ] 3,68M 1,84MB/s ^C I'm currently traveling, but will take a look into what causes this issue as soon as I can. Having a stable release of Wget is our main priority. while disabling l18n would result in most of the spam missing: $ LANG=C wget -c http://fly.osdn.org.ua/~mike/iso/rescue/live-rescue-20141027-x86_64.iso converted ' http://fly.osdn.org.ua/~mike/iso/rescue/live-rescue-20141027-x86_64.iso' (ANSI_X3.4-1968) - ' http://fly.osdn.org.ua/~mike/iso/rescue/live-rescue-20141027-x86_64.iso' (UTF-8) --2014-10-28 00:30:09-- http://fly.osdn.org.ua/~mike/iso/rescue/live-rescue-20141027-x86_64.iso Resolving fly.osdn.org.ua (fly.osdn.org.ua)... 212.40.45.6 Connecting to fly.osdn.org.ua (fly.osdn.org.ua)|212.40.45.6|:80... connected. HTTP request sent, awaiting response... 200 OK Length: 620756992 (592M) [application/octet-stream] Saving to: 'live-rescue-20141027-x86_64.iso' live-rescue-2014102 0%[ ] 4.55M 1.89MB/s ^C ...but adding the conversion spam. This is being caused due to some other idn fixes. I'll see if it can be prevented. Guys, please do RCs or whatever to avoid brown paper bag effects in products that were mature years ago. I love you and thank for your work but please please don't feel cool, people depend on wget. We would generally release a beta version before making a new release. However, we were forced to make an urgent release this time around due to a security issue coming up that had to be fixed immediately. -- WBR, Michael Shigorin / http://altlinux.org -- http://opennet.ru / http://anna-news.info --- end quoted text --- -- Thanking You, Darshit Shah pgpFQo3XwJg3a.pgp Description: PGP signature
Re: [Bug-wget] broken progressbar in 1.16
Michael Shigorin m...@altlinux.org writes: Hello, please do pay attention to regression testing for innovative bells and especially whistles as the new and improved progress bar quickly becomes annoying if the filename is longer than the part of the line before percent meter: the new code tries to scroll it (this alone might be irritating to some including myself) and for some cases fails to do that properly falling into spam out a new line every cycle in xterm with an UTF-8 locale. Guys, please do RCs or whatever to avoid brown paper bag effects in products that were mature years ago. I love you and thank for your work but please please don't feel cool, people depend on wget. don't you think you are a bit exaggerating for the minor issue you have reported? We put a lot of effort to not break backward compatibility, except when are forced to do it (like for CVE-2014-4877 that we had to change a default setting), but we cannot promise that we have no regressions between releases. Thanks, Giuseppe
Re: [Bug-wget] broken progressbar in 1.16
On Tue, Oct 28, 2014 at 09:22:14PM +0100, Giuseppe Scrivano wrote: don't you think you are a bit exaggerating for the minor issue you have reported? I see a pattern unfortunately being ALT Linux wget maintainer since 2005 (and using it since last century). GNU projects like this used to do no-glitch releases for me, so it's just natural to try and continue with that -- hope you do understand that a minor glitch in a release is a major offense to a good release manager irrespective of who caught it. Experience comes with errors and it's no surprise, so take no offense from me but rather from the bug to learn from it. but we cannot promise that we have no regressions between releases. Not a Psaki to expect promises, it's better to actually do things :-) On Tue, Oct 28, 2014 at 09:10:07PM +0800, Yousong Zhou wrote: I guess it's `--progress=bar:noscroll' of commit 4eeabffee6e5b348d36c4f3ba0579ed086226603 Thank you. On Tue, Oct 28, 2014 at 04:54:06PM +0800, Darshit Shah wrote: As soon as the size being displayed there exceeds the 1M mark, the issue gets triggered and I see a new line being created for each refresh of the progress bar. Ah, didn't spot this exact margin although the postponed start of that behaviour was clear enough. Travel well, don't hurry! -- WBR, Michael Shigorin / http://altlinux.org -- http://opennet.ru / http://anna-news.info
