| Message ID | 49498ed0-cfd5-2305-cee7-5c5939a19bcf@campoint.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | progress test failure on fedora34 | expand |
On Wed, Jul 14 2021, Fabian Stelzer wrote: > Hi, > The test t0500-progress-display.sh in current master fails on latest > fedora34. > The break was introduced with: > > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit > > Kind regards, > Fabian I have not been able to reproduce this, it seems the below E-Mail was word-wrapped by your mailer, which is especially bad here since getting to the bottom of this requires looking at the whitespace. Is there a way you could tar that up and send it (to me personally is fine, or some pastebin or whatever). I am able to reproduce something that looks like this if I s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to 80, and that the progress.c code just ends up with an atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy logic) in progress.c, I'm not seeing right now how this could happen...
On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Wed, Jul 14 2021, Fabian Stelzer wrote: > > > Hi, > > The test t0500-progress-display.sh in current master fails on latest > > fedora34. > > The break was introduced with: > > > > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 > > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit > > > > Kind regards, > > Fabian > > I have not been able to reproduce this, it seems the below E-Mail was > word-wrapped by your mailer, which is especially bad here since getting > to the bottom of this requires looking at the whitespace. > > Is there a way you could tar that up and send it (to me personally is > fine, or some pastebin or whatever). > > I am able to reproduce something that looks like this if I > s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to > 80, and that the progress.c code just ends up with an > atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy > logic) in progress.c, I'm not seeing right now how this could happen... This test also fails for me when using QTerminal or Konsole, but it passes on XTerm and LXTerminal. -Alex
On Wed, Jul 14 2021, Alex Henrie wrote: > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> >> >> On Wed, Jul 14 2021, Fabian Stelzer wrote: >> >> > Hi, >> > The test t0500-progress-display.sh in current master fails on latest >> > fedora34. >> > The break was introduced with: >> > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit >> > >> > Kind regards, >> > Fabian >> >> I have not been able to reproduce this, it seems the below E-Mail was >> word-wrapped by your mailer, which is especially bad here since getting >> to the bottom of this requires looking at the whitespace. >> >> Is there a way you could tar that up and send it (to me personally is >> fine, or some pastebin or whatever). >> >> I am able to reproduce something that looks like this if I >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to >> 80, and that the progress.c code just ends up with an >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy >> logic) in progress.c, I'm not seeing right now how this could happen... > > This test also fails for me when using QTerminal or Konsole, but it > passes on XTerm and LXTerminal. I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it, resized the window etc., always get COLUMNS=80 if I add some printf debugging. Do you mind testing with an ad-hoc patch like this on top? It will fail right away, but should say COLUMNS = 80 in the output. The only thing I can think of right now is that some terminals are doing some evil trickery to LD_PRELOAD or whatever and intercept getenv() for COLUMNS and the like, but that's an entirely unfounded hunch. diff --git a/progress.c b/progress.c index 680c6a8bf9..dca254b515 100644 --- a/progress.c +++ b/progress.c @@ -144,6 +144,7 @@ static void display(struct progress *progress, uint64_t n, const char *done) size_t progress_line_len = progress->title_len + counters_sb->len + 2; int cols = term_columns(); + fprintf(stderr, "cols = %d\n", cols); if (progress->split) { fprintf(stderr, " %s%*s", counters_sb->buf,
On Sun, Jul 18, 2021 at 10:05:44AM +0200, Ævar Arnfjörð Bjarmason wrote: > > This test also fails for me when using QTerminal or Konsole, but it > > passes on XTerm and LXTerminal. > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it, > resized the window etc., always get COLUMNS=80 if I add some printf > debugging. > > Do you mind testing with an ad-hoc patch like this on top? It will fail > right away, but should say COLUMNS = 80 in the output. > > The only thing I can think of right now is that some terminals are doing > some evil trickery to LD_PRELOAD or whatever and intercept getenv() for > COLUMNS and the like, but that's an entirely unfounded hunch. That would be truly evil. :) Another possible source of weirdness: some shells are picky about assigning to COLUMNS, and are eager to set it themselves. E.g.: $ echo $COLUMNS 119 $ COLUMNS=80 bash -c 'echo $COLUMNS' 80 $ COLUMNS=80 zsh -c 'echo $COLUMNS' 119 So zsh, even in a non-interactive shell, will set it. It does at least accept setting it, and will preserve it in sub-shells and forks. But it will silently ignore invalid values, instead going back to the ioctl: $ zsh -c 'COLUMNS=80; echo $COLUMNS; COLUMNS=foo; echo $COLUMNS' 80 119 mksh behaves the same way; I was tipped off to this by b082687cba (test-lib: skip test with COLUMNS=1 under mksh, 2012-04-27). I have trouble seeing how this could cause a problem since "80" seems like a perfectly sensible value. And one would imagine that the same shell is being used in all cases above (but maybe not, depending on the configuration of the terminal programs?). But it's another possible avenue of investigation. > diff --git a/progress.c b/progress.c > index 680c6a8bf9..dca254b515 100644 > --- a/progress.c > +++ b/progress.c > @@ -144,6 +144,7 @@ static void display(struct progress *progress, uint64_t n, const char *done) > size_t progress_line_len = progress->title_len + > counters_sb->len + 2; > int cols = term_columns(); > + fprintf(stderr, "cols = %d\n", cols); > > if (progress->split) { > fprintf(stderr, " %s%*s", counters_sb->buf, Yeah, this seems like a good start to see if the value we're getting is bogus. Likewise, it might be interesting for term_columns() to tell us if it's getting the value from $COLUMNS or from the ioctl (but it's hard to believe it wouldn't be from $COLUMNS, given the code). -Peff
On Sun, Jul 18, 2021 at 2:08 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Wed, Jul 14 2021, Alex Henrie wrote: > > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason > > <avarab@gmail.com> wrote: > >> > >> On Wed, Jul 14 2021, Fabian Stelzer wrote: > >> > >> > Hi, > >> > The test t0500-progress-display.sh in current master fails on latest > >> > fedora34. > >> > The break was introduced with: > >> > > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit > >> > > >> > Kind regards, > >> > Fabian > >> > >> I have not been able to reproduce this, it seems the below E-Mail was > >> word-wrapped by your mailer, which is especially bad here since getting > >> to the bottom of this requires looking at the whitespace. > >> > >> Is there a way you could tar that up and send it (to me personally is > >> fine, or some pastebin or whatever). > >> > >> I am able to reproduce something that looks like this if I > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to > >> 80, and that the progress.c code just ends up with an > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy > >> logic) in progress.c, I'm not seeing right now how this could happen... > > > > This test also fails for me when using QTerminal or Konsole, but it > > passes on XTerm and LXTerminal. > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it, > resized the window etc., always get COLUMNS=80 if I add some printf > debugging. Actually, it looks like the difference was that I didn't resize the XTerm or LXTerminal windows. The tests pass on all four if the terminal emulator window is exactly 80 columns wide, and they fail on all four if the window is any wider or narrower. -Alex
On Mon, Jul 19, 2021 at 11:18 AM Alex Henrie <alexhenrie24@gmail.com> wrote: > > On Sun, Jul 18, 2021 at 2:08 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > > > > On Wed, Jul 14 2021, Alex Henrie wrote: > > > > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason > > > <avarab@gmail.com> wrote: > > >> > > >> On Wed, Jul 14 2021, Fabian Stelzer wrote: > > >> > > >> > Hi, > > >> > The test t0500-progress-display.sh in current master fails on latest > > >> > fedora34. > > >> > The break was introduced with: > > >> > > > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 > > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit > > >> > > > >> > Kind regards, > > >> > Fabian > > >> > > >> I have not been able to reproduce this, it seems the below E-Mail was > > >> word-wrapped by your mailer, which is especially bad here since getting > > >> to the bottom of this requires looking at the whitespace. > > >> > > >> Is there a way you could tar that up and send it (to me personally is > > >> fine, or some pastebin or whatever). > > >> > > >> I am able to reproduce something that looks like this if I > > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to > > >> 80, and that the progress.c code just ends up with an > > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy > > >> logic) in progress.c, I'm not seeing right now how this could happen... > > > > > > This test also fails for me when using QTerminal or Konsole, but it > > > passes on XTerm and LXTerminal. > > > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it, > > resized the window etc., always get COLUMNS=80 if I add some printf > > debugging. > > Actually, it looks like the difference was that I didn't resize the > XTerm or LXTerminal windows. The tests pass on all four if the > terminal emulator window is exactly 80 columns wide, and they fail on > all four if the window is any wider or narrower. I have narrowed the problem down to the `tput` command in test-lib.sh: When `tput` runs, $COLUMNS is reset to the width of the terminal emulator window. -Alex
Ævar Arnfjörð Bjarmason wrote: > > On Wed, Jul 14 2021, Alex Henrie wrote: > > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason > > <avarab@gmail.com> wrote: > >> > >> > >> On Wed, Jul 14 2021, Fabian Stelzer wrote: > >> > >> > Hi, > >> > The test t0500-progress-display.sh in current master fails on latest > >> > fedora34. > >> > The break was introduced with: > >> > > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit > >> > > >> > Kind regards, > >> > Fabian > >> > >> I have not been able to reproduce this, it seems the below E-Mail was > >> word-wrapped by your mailer, which is especially bad here since getting > >> to the bottom of this requires looking at the whitespace. > >> > >> Is there a way you could tar that up and send it (to me personally is > >> fine, or some pastebin or whatever). > >> > >> I am able to reproduce something that looks like this if I > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to > >> 80, and that the progress.c code just ends up with an > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy > >> logic) in progress.c, I'm not seeing right now how this could happen... > > > > This test also fails for me when using QTerminal or Konsole, but it > > passes on XTerm and LXTerminal. > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it, > resized the window etc., always get COLUMNS=80 if I add some printf > debugging. > > Do you mind testing with an ad-hoc patch like this on top? It will fail > right away, but should say COLUMNS = 80 in the output. > > The only thing I can think of right now is that some terminals are doing > some evil trickery to LD_PRELOAD or whatever and intercept getenv() for > COLUMNS and the like, but that's an entirely unfounded hunch. I'm able to reproduce this. The test fails when running directly with bash, but not with prove. And it seems to be a bug in bash: export COLUMNS=80 echo "COLUMNS: $COLUMNS" cat > /tmp/expect <<EOF foobar EOF echo "COLUMNS: $COLUMNS" I get: COLUMNS: 80 COLUMNS: 115 Even directly in the console.
Felipe Contreras wrote: > Ævar Arnfjörð Bjarmason wrote: > > > > On Wed, Jul 14 2021, Alex Henrie wrote: > > > > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason > > > <avarab@gmail.com> wrote: > > >> > > >> > > >> On Wed, Jul 14 2021, Fabian Stelzer wrote: > > >> > > >> > Hi, > > >> > The test t0500-progress-display.sh in current master fails on latest > > >> > fedora34. > > >> > The break was introduced with: > > >> > > > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 > > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit > > >> > > > >> > Kind regards, > > >> > Fabian > > >> > > >> I have not been able to reproduce this, it seems the below E-Mail was > > >> word-wrapped by your mailer, which is especially bad here since getting > > >> to the bottom of this requires looking at the whitespace. > > >> > > >> Is there a way you could tar that up and send it (to me personally is > > >> fine, or some pastebin or whatever). > > >> > > >> I am able to reproduce something that looks like this if I > > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to > > >> 80, and that the progress.c code just ends up with an > > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy > > >> logic) in progress.c, I'm not seeing right now how this could happen... > > > > > > This test also fails for me when using QTerminal or Konsole, but it > > > passes on XTerm and LXTerminal. > > > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it, > > resized the window etc., always get COLUMNS=80 if I add some printf > > debugging. > > > > Do you mind testing with an ad-hoc patch like this on top? It will fail > > right away, but should say COLUMNS = 80 in the output. > > > > The only thing I can think of right now is that some terminals are doing > > some evil trickery to LD_PRELOAD or whatever and intercept getenv() for > > COLUMNS and the like, but that's an entirely unfounded hunch. > > I'm able to reproduce this. The test fails when running directly with > bash, but not with prove. > > And it seems to be a bug in bash: > > export COLUMNS=80 > > echo "COLUMNS: $COLUMNS" > cat > /tmp/expect <<EOF > foobar > EOF > echo "COLUMNS: $COLUMNS" > > I get: > > COLUMNS: 80 > COLUMNS: 115 > > Even directly in the console. Hmm, from man bash: checkwinsize If set, bash checks the window size after each external (non‐builtin) com‐ mand and, if necessary, updates the values of LINES and COLUMNS. This op‐ tion is enabled by default. Seems like since bash 5.0 this is on by default.
On Mon, Jul 19, 2021 at 1:34 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Felipe Contreras wrote: > > Ævar Arnfjörð Bjarmason wrote: > > > > > > On Wed, Jul 14 2021, Alex Henrie wrote: > > > > > > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason > > > > <avarab@gmail.com> wrote: > > > >> > > > >> > > > >> On Wed, Jul 14 2021, Fabian Stelzer wrote: > > > >> > > > >> > Hi, > > > >> > The test t0500-progress-display.sh in current master fails on latest > > > >> > fedora34. > > > >> > The break was introduced with: > > > >> > > > > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 > > > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit > > > >> > > > > >> > Kind regards, > > > >> > Fabian > > > >> > > > >> I have not been able to reproduce this, it seems the below E-Mail was > > > >> word-wrapped by your mailer, which is especially bad here since getting > > > >> to the bottom of this requires looking at the whitespace. > > > >> > > > >> Is there a way you could tar that up and send it (to me personally is > > > >> fine, or some pastebin or whatever). > > > >> > > > >> I am able to reproduce something that looks like this if I > > > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to > > > >> 80, and that the progress.c code just ends up with an > > > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy > > > >> logic) in progress.c, I'm not seeing right now how this could happen... > > > > > > > > This test also fails for me when using QTerminal or Konsole, but it > > > > passes on XTerm and LXTerminal. > > > > > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it, > > > resized the window etc., always get COLUMNS=80 if I add some printf > > > debugging. > > > > > > Do you mind testing with an ad-hoc patch like this on top? It will fail > > > right away, but should say COLUMNS = 80 in the output. > > > > > > The only thing I can think of right now is that some terminals are doing > > > some evil trickery to LD_PRELOAD or whatever and intercept getenv() for > > > COLUMNS and the like, but that's an entirely unfounded hunch. > > > > I'm able to reproduce this. The test fails when running directly with > > bash, but not with prove. > > > > And it seems to be a bug in bash: > > > > export COLUMNS=80 > > > > echo "COLUMNS: $COLUMNS" > > cat > /tmp/expect <<EOF > > foobar > > EOF > > echo "COLUMNS: $COLUMNS" > > > > I get: > > > > COLUMNS: 80 > > COLUMNS: 115 > > > > Even directly in the console. > > Hmm, from man bash: > > checkwinsize > If set, bash checks the window size after each external (non‐builtin) com‐ > mand and, if necessary, updates the values of LINES and COLUMNS. This op‐ > tion is enabled by default. > > Seems like since bash 5.0 this is on by default. Indeed, running `shopt -u checkwinsize` right after exporting COLUMNS in test-lib.sh fixes the tests. Great work! -Alex
Alex Henrie wrote: > On Mon, Jul 19, 2021 at 1:34 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Felipe Contreras wrote: > > > Ævar Arnfjörð Bjarmason wrote: > > > > > > > > On Wed, Jul 14 2021, Alex Henrie wrote: > > > > > > > > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason > > > > > <avarab@gmail.com> wrote: > > > > >> > > > > >> > > > > >> On Wed, Jul 14 2021, Fabian Stelzer wrote: > > > > >> > > > > >> > Hi, > > > > >> > The test t0500-progress-display.sh in current master fails on latest > > > > >> > fedora34. > > > > >> > The break was introduced with: > > > > >> > > > > > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21 > > > > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit > > > > >> > > > > > >> > Kind regards, > > > > >> > Fabian > > > > >> > > > > >> I have not been able to reproduce this, it seems the below E-Mail was > > > > >> word-wrapped by your mailer, which is especially bad here since getting > > > > >> to the bottom of this requires looking at the whitespace. > > > > >> > > > > >> Is there a way you could tar that up and send it (to me personally is > > > > >> fine, or some pastebin or whatever). > > > > >> > > > > >> I am able to reproduce something that looks like this if I > > > > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to > > > > >> 80, and that the progress.c code just ends up with an > > > > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy > > > > >> logic) in progress.c, I'm not seeing right now how this could happen... > > > > > > > > > > This test also fails for me when using QTerminal or Konsole, but it > > > > > passes on XTerm and LXTerminal. > > > > > > > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it, > > > > resized the window etc., always get COLUMNS=80 if I add some printf > > > > debugging. > > > > > > > > Do you mind testing with an ad-hoc patch like this on top? It will fail > > > > right away, but should say COLUMNS = 80 in the output. > > > > > > > > The only thing I can think of right now is that some terminals are doing > > > > some evil trickery to LD_PRELOAD or whatever and intercept getenv() for > > > > COLUMNS and the like, but that's an entirely unfounded hunch. > > > > > > I'm able to reproduce this. The test fails when running directly with > > > bash, but not with prove. > > > > > > And it seems to be a bug in bash: > > > > > > export COLUMNS=80 > > > > > > echo "COLUMNS: $COLUMNS" > > > cat > /tmp/expect <<EOF > > > foobar > > > EOF > > > echo "COLUMNS: $COLUMNS" > > > > > > I get: > > > > > > COLUMNS: 80 > > > COLUMNS: 115 > > > > > > Even directly in the console. > > > > Hmm, from man bash: > > > > checkwinsize > > If set, bash checks the window size after each external (non‐builtin) com‐ > > mand and, if necessary, updates the values of LINES and COLUMNS. This op‐ > > tion is enabled by default. > > > > Seems like since bash 5.0 this is on by default. > > Indeed, running `shopt -u checkwinsize` right after exporting COLUMNS > in test-lib.sh fixes the tests. Great work! Yeah, this fixes it, but it doesn't seem we are setting any bash-specific options right now, and additionally I don't think bash should be doing that in the first place. If the shell is non-interactive, why is checkwinsize being honored? Moreover, why does it work with prove? I'm investigating that right now, but so far I haven't found any reason.
Hi, I met this bug too on my ArchLinux. Felipe Contreras <felipe.contreras@gmail.com> 于2021年7月20日周二 上午9:04写道： > > > Yeah, this fixes it, but it doesn't seem we are setting any > bash-specific options right now, and additionally I don't think bash > should be doing that in the first place. If the shell is > non-interactive, why is checkwinsize being honored? > > Moreover, why does it work with prove? I'm investigating that right now, > but so far I haven't found any reason. > I ask this question on IRC #git, and ikke said that after bisecting, he thought that this bug was introduced in c49a177be. > -- > Felipe Contreras -- ZheNing Hu
ZheNing Hu wrote: > Hi, I met this bug too on my ArchLinux. > > Felipe Contreras <felipe.contreras@gmail.com> 于2021年7月20日周二 上午9:04写道： > > Yeah, this fixes it, but it doesn't seem we are setting any > > bash-specific options right now, and additionally I don't think bash > > should be doing that in the first place. If the shell is > > non-interactive, why is checkwinsize being honored? > > > > Moreover, why does it work with prove? I'm investigating that right now, > > but so far I haven't found any reason. > > I ask this question on IRC #git, and ikke said that after bisecting, > he thought that > this bug was introduced in c49a177be. Yes, this was already known. The root of this thread [1] mentions 83ae1edff7, c49a177bec is the second parent of that comit. But I don't think the bug is in git, it's in bash. [1] https://lore.kernel.org/git/49498ed0-cfd5-2305-cee7-5c5939a19bcf@campoint.net/
--- expect 2021-07-14 12:29:49.576970165 +0000 +++ out 2021-07-14 12:29:49.580970145 +0000 @@ -1,6 +1,5 @@ Working hard.......2.........3.........4.........5.........6: 0% (100/100000)<CR> Working hard.......2.........3.........4.........5.........6: 1% (1000/100000)<CR> -Working hard.......2.........3.........4.........5.........6: - 10% (10000/100000)<CR> - 100% (100000/100000)<CR> - 100% (100000/100000), done. +Working hard.......2.........3.........4.........5.........6: 10%