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
Term::ReadLine generates empty &STDERR files #16134
Comments
From @toddrCreated by @toddrWe just upgraded to 5.26.0 and noticed that an empty &STDERR file was perl526 -MPOSIX -MTerm::ReadLine -E'exit if(fork); POSIX::setsid(); then ls -ld \&STDERR This problem bisects back to a conversion of a 2 arg open to a 3 arg The problem is that redirecting file handles to STDERR (open (my $fh, I suggest reverting the fix (Only for the one file Todd Perl Info
|
From @jkeenanOn Thu, 31 Aug 2017 21:55:28 GMT, TODDR wrote:
Patch attached; also available in the smoke-me/jkeenan/132008-term-readline branch. But I don't consider it ready to apply to blead as it lacks a test. Todd: Can you add a test for the non-creation of '&STDERR' that fails in blead (or 5.26) but passes once patch is applied? Thank you very much. |
From @jkeenan132008-0001-Revert-to-2-arg-open-in-one-case.patchFrom e4dc68d725b19f46c6fca9423e6e7a0eaeff47f4 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Thu, 31 Aug 2017 22:57:06 -0400
Subject: [PATCH] Revert to 2-arg open in one case.
If /dev/tty is inaccessible, redirecting file handles to STDERR:
open (my $fh, ">&STDERR))
... cannot be done as a 3 arg open or it'll actually try to write to that
file.
Bump $Term::ReadLine::VERSION.
For: RT #132008
---
dist/Term-ReadLine/lib/Term/ReadLine.pm | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/dist/Term-ReadLine/lib/Term/ReadLine.pm b/dist/Term-ReadLine/lib/Term/ReadLine.pm
index 88d5a75..e0f4e1b 100644
--- a/dist/Term-ReadLine/lib/Term/ReadLine.pm
+++ b/dist/Term-ReadLine/lib/Term/ReadLine.pm
@@ -267,10 +267,13 @@ sub new {
my($console, $consoleOUT) = $_[0]->findConsole;
+
+
# the Windows CONIN$ needs GENERIC_WRITE mode to allow
# a SetConsoleMode() if we end up using Term::ReadKey
open FIN, (( $^O eq 'MSWin32' && $console eq 'CONIN$' ) ? '+<' : '<' ), $console;
- open FOUT,'>', $consoleOUT;
+ # RT #132008: Still need 2-arg open here
+ open FOUT,">$consoleOUT";
#OUT->autoflush(1); # Conflicts with debugger?
my $sel = select(FOUT);
@@ -319,7 +322,7 @@ sub Features { \%features }
package Term::ReadLine; # So late to allow the above code be defined?
-our $VERSION = '1.16';
+our $VERSION = '1.17';
my ($which) = exists $ENV{PERL_RL} ? split /\s+/, $ENV{PERL_RL} : undef;
if ($which) {
--
2.7.4
|
The RT System itself - Status changed from 'new' to 'open' |
From blgl@stacken.kth.seQuoth Todd Rinaldo:
Yes, it can! open(my $fh, ">&", \*STDERR) Getting Term::ReadLine to use this may involve breaking compatibility, /Bo Lindbergh |
From @jkeenanOn Fri, 01 Sep 2017 03:13:59 GMT, blgl@stacken.kth.se wrote:
findConsole is clearly documented, so we should assume that it's callable from outside that space. #####
-- |
From @atoomicYou would find attached to this message the extra unit test (as patch #1) to show the failure before/after the fix from James. I've pushed to my github branch the test + James commit on top of it to fix the issue, 99333cd...atoomic:devel/rt-132008 Note: looks like the upstream git repo from the cpan distribution points to https://github.com/rafl/term-readline which is outdated, not sure if there is a more recent git repo for upstream where we can submit these changes, and update the META informations On Thu, 31 Aug 2017 20:24:58 -0700, jkeenan wrote:
|
From @atoomic0001-Add-unit-test-for-RT-132008.patchFrom 05a3ecccf5b448be98bfd7befe474011db50db77 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Thu, 7 Sep 2017 11:12:26 -0600
Subject: [PATCH 1/2] Add unit test for RT #132008
Term::ReadLine generates empty &STDERR files
---
dist/Term-ReadLine/lib/Term/ReadLine.pm | 12 ++++++----
dist/Term-ReadLine/t/ReadLine-STDERR.t | 41 +++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+), 4 deletions(-)
create mode 100644 dist/Term-ReadLine/t/ReadLine-STDERR.t
diff --git a/dist/Term-ReadLine/lib/Term/ReadLine.pm b/dist/Term-ReadLine/lib/Term/ReadLine.pm
index 88d5a75877..87fdfb86c4 100644
--- a/dist/Term-ReadLine/lib/Term/ReadLine.pm
+++ b/dist/Term-ReadLine/lib/Term/ReadLine.pm
@@ -229,12 +229,17 @@ sub readline {
}
sub addhistory {}
+# used for testing purpose
+sub devtty { return '/dev/tty' }
+
sub findConsole {
my $console;
my $consoleOUT;
- if ($^O ne 'MSWin32' and -e "/dev/tty") {
- $console = "/dev/tty";
+ my $devtty = devtty();
+
+ if ($^O ne 'MSWin32' and -e $devtty) {
+ $console = $devtty;
} elsif ($^O eq 'MSWin32' or $^O eq 'msys' or -e "con") {
$console = 'CONIN$';
$consoleOUT = 'CONOUT$';
@@ -248,7 +253,7 @@ sub findConsole {
$consoleOUT = $console unless defined $consoleOUT;
$console = "&STDIN" unless defined $console;
- if ($console eq "/dev/tty" && !open(my $fh, "<", $console)) {
+ if ($console eq $devtty && !open(my $fh, "<", $console)) {
$console = "&STDIN";
undef($consoleOUT);
}
@@ -266,7 +271,6 @@ sub new {
if (@_==2) {
my($console, $consoleOUT) = $_[0]->findConsole;
-
# the Windows CONIN$ needs GENERIC_WRITE mode to allow
# a SetConsoleMode() if we end up using Term::ReadKey
open FIN, (( $^O eq 'MSWin32' && $console eq 'CONIN$' ) ? '+<' : '<' ), $console;
diff --git a/dist/Term-ReadLine/t/ReadLine-STDERR.t b/dist/Term-ReadLine/t/ReadLine-STDERR.t
new file mode 100644
index 0000000000..621530226e
--- /dev/null
+++ b/dist/Term-ReadLine/t/ReadLine-STDERR.t
@@ -0,0 +1,41 @@
+#!./perl -w
+use strict;
+
+use Test::More;
+
+## unit test for RT 132008 - https://rt.perl.org/Ticket/Display.html?id=132008
+
+if ( $^O eq 'MSWin32' || !-e q{/dev/tty} ) {
+ plan skip_all => "Test not tested on windows or when /dev/tty do not exists";
+}
+else {
+ plan tests => 9;
+}
+
+if ( -e q[&STDERR] ) {
+ note q[Removing existing file &STDERR];
+ unlink q[&STDERR] or die q{Cannot remove existign file &STDERR [probably created from a previous run]};
+}
+
+use_ok('Term::ReadLine');
+can_ok( 'Term::ReadLine::Stub', qw{new devtty findConsole} );
+
+is( Term::ReadLine->devtty(), q{/dev/tty} );
+my @out = Term::ReadLine::Stub::findConsole();
+is_deeply \@out, [ q{/dev/tty}, q{/dev/tty} ], "findConsole is using /dev/tty";
+
+{
+ no warnings 'redefine';
+ my $donotexist = q[/this/should/not/exist/hopefully];
+
+ ok !-e $donotexist, "File $donotexist does not exist";
+ local *Term::ReadLine::Stub::devtty = sub { $donotexist };
+ is( Term::ReadLine->devtty(), $donotexist, "devtty mocked" );
+
+ my @out = Term::ReadLine::Stub::findConsole();
+ is_deeply \@out, [ q{&STDIN}, q{&STDERR} ], "findConsole is using /dev/tty" or diag explain \@out;
+
+ ok !-e q[&STDERR], 'file &STDERR do not exist before Term::ReadLine call';
+ my $tr = Term::ReadLine->new('whatever');
+ ok !-e q[&STDERR], 'file &STDERR was not created by mistake';
+}
--
2.14.1
|
From @atoomic0002-Revert-to-2-arg-open-in-one-case.patchFrom 0ce978ecf961173f55b95ab5e78d06b5e8a3e1f6 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Thu, 31 Aug 2017 22:57:06 -0400
Subject: [PATCH 2/2] Revert to 2-arg open in one case.
If /dev/tty is inaccessible, redirecting file handles to STDERR:
open (my $fh, ">&STDERR))
... cannot be done as a 3 arg open or it'll actually try to write to that
file.
Bump $Term::ReadLine::VERSION.
For: RT #132008
(cherry picked from commit e4dc68d725b19f46c6fca9423e6e7a0eaeff47f4)
Signed-off-by: Nicolas R <atoomic@cpan.org>
---
dist/Term-ReadLine/lib/Term/ReadLine.pm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/dist/Term-ReadLine/lib/Term/ReadLine.pm b/dist/Term-ReadLine/lib/Term/ReadLine.pm
index 87fdfb86c4..e00fb376cd 100644
--- a/dist/Term-ReadLine/lib/Term/ReadLine.pm
+++ b/dist/Term-ReadLine/lib/Term/ReadLine.pm
@@ -274,7 +274,8 @@ sub new {
# the Windows CONIN$ needs GENERIC_WRITE mode to allow
# a SetConsoleMode() if we end up using Term::ReadKey
open FIN, (( $^O eq 'MSWin32' && $console eq 'CONIN$' ) ? '+<' : '<' ), $console;
- open FOUT,'>', $consoleOUT;
+ # RT #132008: Still need 2-arg open here
+ open FOUT,">$consoleOUT";
#OUT->autoflush(1); # Conflicts with debugger?
my $sel = select(FOUT);
@@ -323,7 +324,7 @@ sub Features { \%features }
package Term::ReadLine; # So late to allow the above code be defined?
-our $VERSION = '1.16';
+our $VERSION = '1.17';
my ($which) = exists $ENV{PERL_RL} ? split /\s+/, $ENV{PERL_RL} : undef;
if ($which) {
--
2.14.1
|
From @atoomicJust pinging the case, as everything seem ready to be reviewed/merged, thanks On Thu, 31 Aug 2017 20:24:58 -0700, jkeenan wrote:
|
From @jkeenanOn Wed, 13 Sep 2017 15:35:39 GMT, atoomic wrote:
atoomic, to be honest I'm not sure that this ticket and the patches so far proposed have been thoroughly reviewed yet. In particular: (a) I'm not sure that Bo Lindbergh's comment of Sep 01 has been addressed. (b) I wonder if there's any way of dealing with the original problem other than the special-case reversion implemented in my Aug 31 patch. Can we get additional eyeballs on this? Thank you very much. -- |
From @atoomicAfter our recent discussion, it seems all the issues have been addressed, Can someone else have a look at it & merge/close at this case. thanks On Wed, 13 Sep 2017 09:40:35 -0700, jkeenan wrote:
|
From @atoomic0001-Term-ReadLine-generates-empty-STDERR-files.patchFrom d105368121006ef87ec74235dd033b79d0dbd519 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Thu, 31 Aug 2017 22:57:06 -0400
Subject: [PATCH] Term::ReadLine generates empty &STDERR files
Revert to 2-arg open in one case.
If /dev/tty is inaccessible, redirecting file handles to STDERR:
open (my $fh, ">&STDERR))
... cannot be done as a 3 arg open or it'll actually try to write to that
file.
Bump $Term::ReadLine::VERSION.
Add unit test for RT #132008
For: RT #132008
(cherry picked from commit e4dc68d725b19f46c6fca9423e6e7a0eaeff47f4)
Signed-off-by: Nicolas R <atoomic@cpan.org>
---
dist/Term-ReadLine/lib/Term/ReadLine.pm | 17 +++++++++-----
dist/Term-ReadLine/t/ReadLine-STDERR.t | 41 +++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 6 deletions(-)
create mode 100644 dist/Term-ReadLine/t/ReadLine-STDERR.t
diff --git a/dist/Term-ReadLine/lib/Term/ReadLine.pm b/dist/Term-ReadLine/lib/Term/ReadLine.pm
index 88d5a75877..e00fb376cd 100644
--- a/dist/Term-ReadLine/lib/Term/ReadLine.pm
+++ b/dist/Term-ReadLine/lib/Term/ReadLine.pm
@@ -229,12 +229,17 @@ sub readline {
}
sub addhistory {}
+# used for testing purpose
+sub devtty { return '/dev/tty' }
+
sub findConsole {
my $console;
my $consoleOUT;
- if ($^O ne 'MSWin32' and -e "/dev/tty") {
- $console = "/dev/tty";
+ my $devtty = devtty();
+
+ if ($^O ne 'MSWin32' and -e $devtty) {
+ $console = $devtty;
} elsif ($^O eq 'MSWin32' or $^O eq 'msys' or -e "con") {
$console = 'CONIN$';
$consoleOUT = 'CONOUT$';
@@ -248,7 +253,7 @@ sub findConsole {
$consoleOUT = $console unless defined $consoleOUT;
$console = "&STDIN" unless defined $console;
- if ($console eq "/dev/tty" && !open(my $fh, "<", $console)) {
+ if ($console eq $devtty && !open(my $fh, "<", $console)) {
$console = "&STDIN";
undef($consoleOUT);
}
@@ -266,11 +271,11 @@ sub new {
if (@_==2) {
my($console, $consoleOUT) = $_[0]->findConsole;
-
# the Windows CONIN$ needs GENERIC_WRITE mode to allow
# a SetConsoleMode() if we end up using Term::ReadKey
open FIN, (( $^O eq 'MSWin32' && $console eq 'CONIN$' ) ? '+<' : '<' ), $console;
- open FOUT,'>', $consoleOUT;
+ # RT #132008: Still need 2-arg open here
+ open FOUT,">$consoleOUT";
#OUT->autoflush(1); # Conflicts with debugger?
my $sel = select(FOUT);
@@ -319,7 +324,7 @@ sub Features { \%features }
package Term::ReadLine; # So late to allow the above code be defined?
-our $VERSION = '1.16';
+our $VERSION = '1.17';
my ($which) = exists $ENV{PERL_RL} ? split /\s+/, $ENV{PERL_RL} : undef;
if ($which) {
diff --git a/dist/Term-ReadLine/t/ReadLine-STDERR.t b/dist/Term-ReadLine/t/ReadLine-STDERR.t
new file mode 100644
index 0000000000..621530226e
--- /dev/null
+++ b/dist/Term-ReadLine/t/ReadLine-STDERR.t
@@ -0,0 +1,41 @@
+#!./perl -w
+use strict;
+
+use Test::More;
+
+## unit test for RT 132008 - https://rt.perl.org/Ticket/Display.html?id=132008
+
+if ( $^O eq 'MSWin32' || !-e q{/dev/tty} ) {
+ plan skip_all => "Test not tested on windows or when /dev/tty do not exists";
+}
+else {
+ plan tests => 9;
+}
+
+if ( -e q[&STDERR] ) {
+ note q[Removing existing file &STDERR];
+ unlink q[&STDERR] or die q{Cannot remove existign file &STDERR [probably created from a previous run]};
+}
+
+use_ok('Term::ReadLine');
+can_ok( 'Term::ReadLine::Stub', qw{new devtty findConsole} );
+
+is( Term::ReadLine->devtty(), q{/dev/tty} );
+my @out = Term::ReadLine::Stub::findConsole();
+is_deeply \@out, [ q{/dev/tty}, q{/dev/tty} ], "findConsole is using /dev/tty";
+
+{
+ no warnings 'redefine';
+ my $donotexist = q[/this/should/not/exist/hopefully];
+
+ ok !-e $donotexist, "File $donotexist does not exist";
+ local *Term::ReadLine::Stub::devtty = sub { $donotexist };
+ is( Term::ReadLine->devtty(), $donotexist, "devtty mocked" );
+
+ my @out = Term::ReadLine::Stub::findConsole();
+ is_deeply \@out, [ q{&STDIN}, q{&STDERR} ], "findConsole is using /dev/tty" or diag explain \@out;
+
+ ok !-e q[&STDERR], 'file &STDERR do not exist before Term::ReadLine call';
+ my $tr = Term::ReadLine->new('whatever');
+ ok !-e q[&STDERR], 'file &STDERR was not created by mistake';
+}
--
2.14.1
|
From @tonycozOn Thu, 14 Sep 2017 12:45:54 -0700, atoomic wrote:
Thanks, applied as d8b6190, which a spelling error fix and adding the new test to MANIFEST. Tony |
@tonycoz - Status changed from 'open' to 'pending release' |
From @toddr
Thanks for the review Tony. What do we need to do to get it on the maintenance backport list? Todd |
From @tonycozOn Mon, Sep 18, 2017 at 07:56:49AM -0500, Todd E Rinaldo wrote:
https://perl5.git.perl.org/perl.git/commit/f734222aca96cf491ce1a3fab8d76650a9275cc9 Tony |
From @jkeenanOn Mon, 18 Sep 2017 05:16:07 GMT, tonyc wrote:
This ticket needs to be re-opened because one of the newly added tests is failing on all of tux's Linux smokers. Example: http://perl5.test-smoke.org/report/57994 The failing test is test 4 in newly added file dist/Term-ReadLine/t/ReadLine-STDERR.t. (Note that though 'git log' and 'git blame' attribute this file to me, it was actually contributed by someone else. I didn't write any tests for this ticket and only two lines survive out of what I originally contributed (which was just a reversion). I have been unable to reproduce this test failure locally in either Linux or FreeBSD. There must be some reason why the following test (#4 in the file) fails in tux's smokers' environments but passes elsewhere. ##### Test (4) is repeated farther down (7) in a mocking context, but there appears to pass on tux's smokers (as it does for me on both OSes mentioned above). Thank you very much. |
@jkeenan - Status changed from 'pending release' to 'open' |
From @tonycozOn Mon, 18 Sep 2017 19:15:13 -0700, jkeenan wrote:
Fixed in 1d217c6 The problem was the smokers run the tests without a tty, and findConsole() doesn't return "/dev/tty" in that case. Tony |
@tonycoz - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#132008 (status was 'resolved')
Searchable as RT132008$
The text was updated successfully, but these errors were encountered: