FTBFS against glibc 2.34

[Dan Bungert]

The package grep failed to build in a recent archive rebuild, see
https://people.canonical.com/~doko/ftbfs-report/test-
rebuild-20210805-impish-impish.html#foundations-bugs .

The failure is in automated test, as follows:

stack-overflow: failed test: grep never printed "stack overflow"
FAIL: stack-overflow

[Dan Bungert (dbungert)]

To address this I propose picking up the new version of grep.

[Dan Bungert (dbungert)]

This summary file is the differences to the debian directories between 3.6-1 and 3.7

[Robie Basak (racb)]

Thank you for working on this!

> To address this I propose picking up the new version of grep.

But you're picking up the new version of grep and then also disabling the failing test in that update. So why is the new version of grep necessary at all? Can't you just disable the failing test in the current version of grep in Impish?

> + * New upstream version 3.7 (LP: #1940999)
> + * Add d/p/06-disable-test-regex.patch to disable failing test-regex

This bug is entitled "FTBFS against glibc 2.34" so the bug reference should go against the second item there, not the first. Placing the reference against the first entry makes it sound like this bug is tracking the update of grep to 3.7, but according to the bug title as it is at the moment, that isn't what this bug is tracking.

Please avoid refreshing quilt patches unnecessarily. Refreshing is only needed for conflicts and if fuzz appears. Otherwise, not refreshing them minimises diff noise and thus helps with review. Additionally, if forced to refresh I always use QUILT_REFRESH_ARGS="-p ab --no-timestamps --no-index" as documented at https://wiki.debian.org/UsingQuilt to reduce the noise for next time.

It looks like fundamentally you're resolving the FTBFS by disabling the failing test. But have you checked that the test is actually a false positive? What if the test is working and picking up something that is actually wrong with the new glibc? If you think it's a false positive then I'd expect to see some explanation that it's a false positive and why you think that, but I couldn't find anything. And if it is a false positive, then a bug report to the test's origin (presumably upstream) and a dep3 header linking to that would be appropriate.

> +Forwarded: no - Debian not yet on glibc 2.34

It's still useful to forward, as Debian is expected to need that anyway. I usually file such bugs in the Debian BTS explaining that it isn't necessary to apply right now but will be useful in the future. However, before forwarding, maybe we should get the fix right here first :)

[Robie Basak (racb)]

> +Last-Update: Aug-24-2021

This should use ISO format according to the spec please (https://dep.debian.net/deps/dep3/)

[Dan Bungert (dbungert)]

On Wed, Aug 25, 2021 at 12:00:44PM -0000, Robie Basak wrote:
> Thank you for working on this!

And thank you for the thoughtful review.

> > To address this I propose picking up the new version of grep.
> But you're picking up the new version of grep and then also disabling
> the failing test in that update. So why is the new version of grep
> necessary at all? Can't you just disable the failing test in the current
> version of grep in Impish?

I have been unclear. With grep 3.6 + glibc 2.34, the failing test is a
different test - the 'stack-overflow' test fails with the following log
output:

  stack-overflow: failed test: grep never printed "stack overflow"
  FAIL: stack-overflow

More verbose log attached.

So my general plan here is to add grep 3.7 to pick up the new version,
which addresses the 'stack-overflow' test, but does regress on
'test-regex', which I then disable. There's another option here, to use
the built-in regex for grep, but I presumed that would be generally
undesired.

> > + * New upstream version 3.7 (LP: #1940999)
> > + * Add d/p/06-disable-test-regex.patch to disable failing test-regex
> This bug is entitled "FTBFS against glibc 2.34" so the bug reference
> should go against the second item there, not the first. Placing the
> reference against the first entry makes it sound like this bug is
> tracking the update of grep to 3.7, but according to the bug title as it
> is at the moment, that isn't what this bug is tracking.

With the new information above, I think the way I have structured it
matches the problem I'm trying to address - what do you think?

> Please avoid refreshing quilt patches unnecessarily. Refreshing is only
> needed for conflicts and if fuzz appears. Otherwise, not refreshing them
> minimises diff noise and thus helps with review.

Sounds reasonable to me. I'll fix it.

> Additionally, if forced
> to refresh I always use QUILT_REFRESH_ARGS="-p ab --no-timestamps --no-
> index" as documented at https://wiki.debian.org/UsingQuilt to reduce the
> noise for next time.

Right, I used to hide the suggested quilt config behind a dquilt alias
but the use case for me doing that in the first place is no longer
relevant. An easy fix.

> It looks like fundamentally you're resolving the FTBFS by disabling the
> failing test. But have you checked that the test is actually a false
> positive? What if the test is working and picking up something that is
> actually wrong with the new glibc? If you think it's a false positive
> then I'd expect to see some explanation that it's a false positive and
> why you think that, but I couldn't find anything. And if it is a false
> positive, then a bug report to the test's origin (presumably upstream)
> and a dep3 header linking to that would be appropriate.

I believe the issue with the stack-overflow test is consistent with this
description from upstream:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46227
Admittedly I have not analyzed further other than comparing the verbose
log output I have to that and concluding that it was the same problem.
I will dig further if you believe it worthwhile.

> > +Forwarded: no - Debian not yet on glibc 2.34
> It's still useful to f...

[Dan Bungert (dbungert)]

One possibility that's starting to grow on me is adjusting grep to go back to using the built-in regex instead of getting it from glibc. This causes some size bloat (+80k on amd64) but that might be tolerable.

Proof of concept:
https://launchpad.net/~dbungert/+archive/ubuntu/grep-lp-1940999

[Robie Basak (racb)]

Ah, sorry. I saw "disable-regex" and thought that was a description of the resolution to the failing stack-overflow test!

If updating grep to 3.7 is the easiest way to fix the problem then we can consider that. But since we're in Feature Freeze for Impish, this needs an assessment for feature changes. I see you posted the changes from 3.6 to 3.7. Does this mean you already made the assessment and if so, what was your conclusion? If feature changes exist then we'll need a freeze exception from the release team to proceed down this path.

> With the new information above, I think the way I have structured it
> matches the problem I'm trying to address - what do you think?

It's certainly closer but I still think it's a little misleading. Maybe if we expanded the text it'd be closer. How about:

* New upstream version 3.7 to fix FTBFS (LP: #1940999)
* Add d/p/06-disable-test-regex.patch to disable failing test-regex

But in any case, we can leave it until we've concluded the question on the best approach to take.

[Dan Bungert (dbungert)]

Add upstream NEWS file diff.
One change that upstream felt relevant enough to point out, outside of bug fixes, was a change to a Windows only option.

[Dan Bungert (dbungert)]

> How about:
> * New upstream version 3.7 to fix FTBFS (LP: #1940999)

Yes, I like that better.

OK, I have updated my proposal. I recommend that we address this FTBFS by using grep 3.7 with built-in regex. This is a change from current and will increase binary size. (180k -> 260k when built locally) but passes all tests without adding a skip directive.

grep 3.7 looks reasonably benign in terms of a new release. One change is called out as neither a feature nor a bug fix, but upstream says it's only relevant to Windows. See the NEWS diff for the wording there. I'm happy to go the feature freeze exception route, but I think we are covered based on that ChangeLog/NEWS. That said, I will defer to suggestions here.

> If updating grep to 3.7 is the easiest way to fix the problem then we can consider that.

We could pull out the fix(es) as needed from the gnulib updates but that sounds like not a generally wise choice. I have not investigated this angle.

I believe all feedback has been addressed, except for the forward to Debian step which I'm intentionally deferring while we discuss solutions.

Please let me know what you think, Robie. Thanks!

[Launchpad Janitor (janitor)]

This bug was fixed in the package grep - 3.7-0ubuntu1

---------------
grep (3.7-0ubuntu1) impish; urgency=medium

  * New upstream version 3.7 to fix FTBFS (LP: #1940999)
  * Use built-in regex instead of the one from glibc, to address failure of
    test-regex with version 3.7.

 -- Dan Bungert <email address hidden> Mon, 30 Aug 2021 16:41:22 -0600