bug#46925: Ripgrep tests failures due to bstr update

bug#46925: Ripgrep tests failures due to bstr update

[John Soo]

Hi Guix,

I just talked to burntsushi on github about the failing ripgrep tests.
It looks like the version of bstr we are now using changed the
representation of bstrs and caused us test failures.  Changing the
version of bstr used in ripgrep@12.1.1 to 0.2.12 should fix the problem.

See https://github.com/BurntSushi/ripgrep/discussions/1813

Thanks!

John

bug#46925: Ripgrep tests failures due to bstr update

[Nicolas Goaziou]

Hello,

John Soo <jsoo1@asu.edu> writes:

> I just talked to burntsushi on github about the failing ripgrep tests.
> It looks like the version of bstr we are now using changed the
> representation of bstrs and caused us test failures.  Changing the
> version of bstr used in ripgrep@12.1.1 to 0.2.12 should fix the
> problem.

I disabled tests in ripgrep a few hours ago for the same reason. This is
not great, but I couldn't find another solution. In particular,
replacing rust-bstr-0.2 with a special rust-bstr-0.2.12 did not work
because regular rust-bstr-0.2 is still used as a transitive dependency.

Do you think of a better way to solve this?

Regards,
-- 
Nicolas Goaziou

bug#46925: Ripgrep tests failures due to bstr update

[JOULAUD François via Bug reports for GNU Guix]

Hello,

On Thu, Mar 04, 2021 at 09:38:39PM +0100, Nicolas Goaziou wrote:
> John Soo <jsoo1@asu.edu> writes:
> 
> > I just talked to burntsushi on github about the failing ripgrep tests.
> > It looks like the version of bstr we are now using changed the
> > representation of bstrs and caused us test failures.  Changing the
> > version of bstr used in ripgrep@12.1.1 to 0.2.12 should fix the
> > problem.
> 
> I disabled tests in ripgrep a few hours ago for the same reason. This is
> not great, but I couldn't find another solution. In particular,
> replacing rust-bstr-0.2 with a special rust-bstr-0.2.12 did not work
> because regular rust-bstr-0.2 is still used as a transitive dependency.
> 
> Do you think of a better way to solve this?

Two possibilities seems to be possible here.

Upgrade rust-bstr-0.2 to be 0.2.12, possibly only upgrading needed
dependent packages.

Patch new ripgrep to understand the old bstr debug format in its tests.
This patch must be transitionnal while waiting for a compatible rust-bstr
package in Guix.

Disabling tests is bad. Disabling tests when we diverge from supported
upstream configuration (in this cas as we don't use recommended version
of deps) is in my opinion even worst.

François

bug#46925: Ripgrep tests failures due to bstr update

[Nicolas Goaziou]

Hello,

JOULAUD François via Bug reports for GNU Guix <bug-guix@gnu.org> writes:

> Two possibilities seems to be possible here.
>
> Upgrade rust-bstr-0.2 to be 0.2.12, possibly only upgrading needed
> dependent packages.

Just to be clear, your are suggesting a downgrade, because currently,
Guix packages bstr 0.2.15. Downgrading is not trivial because bstr
0.2.15 is mandatory for bat 0.18.

We might provide both bstr 0.2.12 and 0.2.15. Note that 11 packages are
requiring bstr. Providing two bstr is only possible if none of those
packages are both among bat and ripgrep dependencies. I didn't check but
I'm not very optimistic.

> Patch new ripgrep to understand the old bstr debug format in its
> tests.

I'll pass, sorry.

> Disabling tests is bad. Disabling tests when we diverge from supported
> upstream configuration (in this cas as we don't use recommended version
> of deps) is in my opinion even worst.

I'm not sure this kind of generalization helps here. Even if you may be
right in theory, looking at the number of "#:tests #f" among packages,
disabling tests is probably a necessary evil in practice.

Moreover, AFAICT, this incompatibility does not break functionality in
ripgrep. It just breaks its tests. Disabling them for a while does not
sound so bad, after all. Is it?

Regards,
-- 
Nicolas Goaziou

bug#46925: Ripgrep tests failures due to bstr update

[JOULAUD François via Bug reports for GNU Guix]

Hello,

On Mon, Mar 08, 2021 at 10:40:27PM +0100, Nicolas Goaziou wrote:
> JOULAUD François via Bug reports for GNU Guix <bug-guix@gnu.org> writes:
> > Upgrade rust-bstr-0.2 to be 0.2.12, possibly only upgrading needed
> > dependent packages.
> 
> Just to be clear, your are suggesting a downgrade, because currently,
> Guix packages bstr 0.2.15. Downgrading is not trivial because bstr
> 0.2.15 is mandatory for bat 0.18.

My mistake here. I thought upstream ripgrep was using a more recent
version than the packaged one. Downgrade could indeed be more work.

> We might provide both bstr 0.2.12 and 0.2.15. Note that 11 packages are
> requiring bstr. Providing two bstr is only possible if none of those
> packages are both among bat and ripgrep dependencies. I didn't check but
> I'm not very optimistic.

I'm not either. ;-)

> > Patch new ripgrep to understand the old bstr debug format in its
> > tests.
> 
> I'll pass, sorry.

I understand that. TBH I don't volunteer for the work either even if in
this specific case it seems to be a pretty basic substitution and one
that upstream will need to do anyway when it will upgrade to new bstr.

> > Disabling tests is bad. Disabling tests when we diverge from supported
> > upstream configuration (in this cas as we don't use recommended version
> > of deps) is in my opinion even worst.
> 
> I'm not sure this kind of generalization helps here. Even if you may be
> right in theory, looking at the number of "#:tests #f" among packages,
> disabling tests is probably a necessary evil in practice.

I think it is a necessary evil. I just fear that it could become an
initial reflex when dealing with library incompatibility.

But you're probably right that this is a discussion for an other venue.

> Moreover, AFAICT, this incompatibility does not break functionality in
> ripgrep. It just breaks its tests. Disabling them for a while does not
> sound so bad, after all. Is it?

Yes, in this specific case it seems to only break tests without breaking
anything else (and I trust upstream explanation on this one). But at the
same time, if it ought to be breaking something else you know don't have
the tests to try to catch it.

Regards,
François

bug#46925: Ripgrep tests failures due to bstr update

[John Soo]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

     Hey Nicolas, 

 
I think the breaking tests do actually indicate breaking functionality in this case since there is a whole test suite dedicated to the bytestring representation. Maybe the fix could be using a later commit in which the later bstr is used. There are definitely unreleased commits in ripgrep.
 

 
Thanks!
 

 
John
     

[-- Attachment #2: Type: text/html, Size: 417 bytes --]

bug#46925: Ripgrep tests failures due to bstr update

[Nicolas Goaziou]

Hello,

John Soo <jsoo1@asu.edu> writes:

> I think the breaking tests do actually indicate breaking functionality
> in this case since there is a whole test suite dedicated to the
> bytestring representation.

I use ripgrep daily, and I haven't encountered a regression so far. Of
course, this proves nothing.

> Maybe the fix could be using a later commit
> in which the later bstr is used. There are definitely unreleased
> commits in ripgrep.

Latest ripgrep commit still requires bstr 0.2.12, so this is not an
option either.

Regards,
-- 
Nicolas Goaziou

bug#46925: Ripgrep tests failures due to bstr update

[John Soo]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]

     Hey Nicolas, 

 
I think the problem is that the version bound is too loose in cargo.toml upstream. What does the cargo.lock say on the current master?    I am beginning to think we need to follow the cargo.lock to resolve dependencies first if it exists.
 

 
Thanks,
 

 
John
     

[-- Attachment #2: Type: text/html, Size: 368 bytes --]

bug#46925: Ripgrep tests failures due to bstr update

[Nicolas Goaziou]

John Soo <jsoo1@asu.edu> writes:

> I think the problem is that the version bound is too loose in
> cargo.toml upstream. What does the cargo.lock say on the current
> master? I am beginning to think we need to follow the cargo.lock to
> resolve dependencies first if it exists.

It's 0.2.12 in Cargo.toml and 0.2.14 in Cargo.lock.

See <https://github.com/BurntSushi/ripgrep/blob/master/Cargo.lock>

Regards,