Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

threads-2.07 t/test.pl uses re::is_regexp() which requires 5.10+ #15302

Closed
p5pRT opened this issue May 2, 2016 · 8 comments
Closed

threads-2.07 t/test.pl uses re::is_regexp() which requires 5.10+ #15302

p5pRT opened this issue May 2, 2016 · 8 comments

Comments

@p5pRT
Copy link

p5pRT commented May 2, 2016

Migrated from rt.perl.org#128052 (status was 'resolved')

Searchable as RT128052$

@p5pRT
Copy link
Author

p5pRT commented May 2, 2016

From andy@hybridized.org

threads-2.07 on CPAN has this change to t/test.pl line 448​:

- unless (ref($expected) && ref($expected) =~ /Regexp/) {
+ unless (re​::is_regexp($expected)) {

This breaks the test on anything older than 5.10. The fix is just to revert this line.

@p5pRT
Copy link
Author

p5pRT commented May 2, 2016

From @jkeenan

On Mon May 02 10​:38​:44 2016, andy@​hybridized.org wrote​:

threads-2.07 on CPAN has this change to t/test.pl line 448​:

- unless (ref($expected) && ref($expected) =~ /Regexp/) {
+ unless (re​::is_regexp($expected)) {

This breaks the test on anything older than 5.10. The fix is just to
revert this line.

I don't see any 'dist/threads/t/test.pl' in the Perl 5 core distribution.

I do, however, see it in the CPAN distribution​:
http​://cpansearch.perl.org/src/JDHEDDEN/threads-2.07/t/test.pl

Jerry, can you look into this?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented May 2, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented May 3, 2016

From andy@hybridized.org

When I sent this, I didn't fully understand how this dist was related to core, but in fact the file in question is t/test.pl in blead. In addition to the re​::is_regexp issue, there is another issue in the same file where unpack 'W*' is used (line 299). bulk88 sent me this patch for it on IRC earlier to use U* instead of W* on Perls < 5.10​:

perl11/p5-Config@d4fd527#diff-9608c9f2a031ad0f9b427c09e2c65bd5

@p5pRT
Copy link
Author

p5pRT commented May 3, 2016

From @cpansprout

On Mon May 02 17​:08​:43 2016, andy@​hybridized.org wrote​:

When I sent this, I didn't fully understand how this dist was related
to core, but in fact the file in question is t/test.pl in blead. In
addition to the re​::is_regexp issue, there is another issue in the
same file where unpack 'W*' is used (line 299). bulk88 sent me this
patch for it on IRC earlier to use U* instead of W* on Perls < 5.10​:

https://github.com/perl11/p5-
Config/commit/d4fd527ff8393fe5c747a9330304e39e989b9885#diff-
9608c9f2a031ad0f9b427c09e2c65bd5

Since threads.pm, a dual-life module, depends on test.pl, which exists for perl’s own testing purposes, it has to be included in the distribution for the tests to work.

Hence, I think it would be good to keep perl’s test.pl 5.8-compatible as long as this arrangement exists. But patching test.pl can wait until after the release of 5.24.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented May 3, 2016

From @jdhedden

On Mon May 02 18​:00​:07 2016, sprout wrote​:

On Mon May 02 17​:08​:43 2016, andy@​hybridized.org wrote​:

When I sent this, I didn't fully understand how this dist was related
to core, but in fact the file in question is t/test.pl in blead. In
addition to the re​::is_regexp issue, there is another issue in the
same file where unpack 'W*' is used (line 299). bulk88 sent me this
patch for it on IRC earlier to use U* instead of W* on Perls < 5.10​:

https://github.com/perl11/p5-
Config/commit/d4fd527ff8393fe5c747a9330304e39e989b9885#diff-
9608c9f2a031ad0f9b427c09e2c65bd5

Since threads.pm, a dual-life module, depends on test.pl, which exists
for perl’s own testing purposes, it has to be included in the
distribution for the tests to work.

Hence, I think it would be good to keep perl’s test.pl 5.8-compatible
as long as this arrangement exists. But patching test.pl can wait
until after the release of 5.24.

This affects both 'threads' and 'threads​::shared' and means the CPAN versions for both of these are 'broken'. My inclination is to release updated versions of these modules to CPAN to accommodate the fixes for both the regexp and pack problems mentioned. This would entail version bumps, of course. This would mean that the CPAN versions would be ahead of the 5.24 versions. Is this a problem?

Additionally, for whoever 'fixes' t/test.pl, documentation should be added mentioning this matter and the need to keep t/test.pl compatible with Perl 5.8.

@p5pRT
Copy link
Author

p5pRT commented May 16, 2016

From @arc

Jerry D. Hedden via RT <perlbug-followup@​perl.org> wrote​:

On Mon May 02 18​:00​:07 2016, sprout wrote​:

Since threads.pm, a dual-life module, depends on test.pl, which exists
for perl’s own testing purposes, it has to be included in the
distribution for the tests to work.

Hence, I think it would be good to keep perl’s test.pl 5.8-compatible
as long as this arrangement exists. But patching test.pl can wait
until after the release of 5.24.

I've patched t/test.pl as of blead commit
94b9cb5, and verified that it can run
a simple test suite tickling both of the affected parts of test.pl
under Perl 5.8; and I'm therefore closing this ticket.

This affects both 'threads' and 'threads​::shared' and means the CPAN versions for both of these are 'broken'. My inclination is to release updated versions of these modules to CPAN to accommodate the fixes for both the regexp and pack problems mentioned. This would entail version bumps, of course. This would mean that the CPAN versions would be ahead of the 5.24 versions. Is this a problem?

I don't think anyone's answered this question; I'm sorry about that.
At this point, I believe t/test.pl can be imported into the CPAN
versions of threads and threads​::shared, to make their test suites
work under 5.8. And AFAICT, it won't cause any problem for blead if
you issue CPAN releases with the new version of test.pl (or with any
other version, for that matter, because that file is explicitly
excluded when reimporting the modules into blead).

Thanks.

--
Aaron Crane ** http​://aaroncrane.co.uk/

@p5pRT p5pRT closed this as completed May 16, 2016
@p5pRT
Copy link
Author

p5pRT commented May 16, 2016

@arc - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant