say optimization breaks Test::Output

[AlexDaniel]

PERL6LIB=lib bin/blin.p6 --old=2018.12 --new=HEAD Test::Output

Test::Output – Fail, Bisected: 9770c1a

Originally reported on IRC and on raku-community-modules/Test-Output#3.

[AlexDaniel]

Obligatory pings: @scovit, @lizmat

[scovit]

The module declare an IO::Handle class and do not define the method say

[scovit]

Can I allow myself a comment on the style? I find the two tags with urgency banners as aggressive, and the term obligatory as extremely rude.
That said, to me the bug seems to be that it is possible to create wrong code that still works but then breaks, I don't know if this bug can be fixed..

[JJ]

OK, let's see if we can fix that "wrong code" on the other side.

[scovit]

Thank you JJ, I'm sorry but I am in a personal condition right now, and even this message is typed on a cellphone, otherwise I would have been glad to help

[JJ]

@scovit no problem. Working on it right now. Best of luck.

[JJ]

The exception is in this line:

    $PROCESS::ERR = $err;

Really have no idea why.

[JJ]

Looks like I managed to fix it. A new release has been made. It does not seem to be a Rakudo error, unless you consider as such a LTA error message . Main problem was that it was not using WRITE for the IO::Handle that was defined, and then that said Handle didn't use UTF8 encoding.

[AlexDaniel]

Can I allow myself a comment on the style? I find the two tags with urgency banners as aggressive, and the term obligatory as extremely rude.

That's not the intent, sorry. When I was the release manager, my understanding was that we shouldn't be breaking modules on a whim. When I see a commit that makes things faster but doesn't have a written justification for breaking things, I file a bug report. blocker label was added because this is something we definitely want to figure out (one way or another) before the release. regression label in this case is perhaps not necessary, but the issue didn't exist before (and was bisected), so technically it's right. Authors are pinged because IMO they need to know that there is an issue. Obligatory in a sense that I'm pinging you because that's the process, not because you have to drop all things and fix that issue asap.

Does that make sense? Maybe we should have a written document about it somewhere. Maybe on https://github.com/perl6/problem-solving.

@kawaii

[AlexDaniel]

I don't remember the status of this why it still has a label.

[kawaii]

@AlexDaniel it doesn't seem like too much of a priority right now regardless of the status, so I'll remove the blocker label from it.

[JJ]

Is this still blocking anything?

[niner]

Reading up on this issue and the linked one in Test::Output I'm stumped about why we've not yet reverted the broken change. Breaking modules for some optimizations in Rakudo is not OK unless those modules did something that was clearly wrong. The latter has not been discussed at all, neither here, nor in the other issue.

[lizmat]

sub say has evolved since then. It is now basically a modern version of the original:

    $_ := $*OUT;
    .print: nqp::concat(x.gist,.nl-out)

So I'm unsure how 9770c1a can still be the issue here?

[niner]

Is the bug actually still present? Unfortunately neither bug report actually mentions what was wrong. Many of the links don't work anymore either. That's why it's so important to always include all the relevant information and e.g. commit messages like "Fix GH issue 1234" are no use at all.

[jonathanstowe]

If there was a test failure, there isn't now:

===> Testing: Test::Output:ver<1.001002>
[Test::Output] t/00-use.t ...... ok
[Test::Output] t/01-capture.t .. ok
[Test::Output] t/02-Test.pm.t .. ok
[Test::Output] All tests successful.
[Test::Output] Files=3, Tests=11,  1 wallclock secs
[Test::Output] Result: PASS
===> Testing [OK] for Test::Output:ver<1.001002>

With:

Welcome to Rakudo™ v2021.10-150-gd301860e5.
Implementing the Raku® Programming Language v6.d.
Built on MoarVM version 2021.10-124-g6fd623291.

[JJ]

Actually, that was due to a workaround, mainly upgrading to 6.d. It's probably still present in 6.c

[niner]

It's probably still present in 6.c

So what is "it"?

[JJ]

Sorry, the bug. Answering to @jonathanstowe in that comment.

[jonathanstowe]

Right, but I can't find anywhere that says explicitly what that was just that there was a test failure. Maybe I'm not looking hard enough, but a reproduction with the actual error might be useful to save us going round in circles.

[JJ]

Trying to reproduce it here, lowering the version to 6.c, but no go. I would say it's fixed. Closing.