Re: [FYI] {master} refactor: use modern semantics of 'open'
Eric Blake wrote: ... If anything, this issue shows that the Automake::XFile module (and other similar modules as well) should live in their own git repository, which both Autoconf and Automake should use as a submodule -- and which (for $DEITY's sake!), should be unit-tested. Any volunteer? Alas, my perl-fu is weak enough that I'm not sure how good my unit-testing attempts would be. But moving the files to a common repository seems doable (how about gnulib?). Fine with me.
Re: [FYI] {master} refactor: use modern semantics of 'open'
Hi Eric, sorry for the delay.
On 04/25/2012 01:25 AM, Eric Blake wrote:
Actually, this appears to make _all_ the XFile uses work; all that
remains broken is places such as bin/autoupdate.in calling raw open
instead of using XFile.
diff --git i/lib/Autom4te/XFile.pm w/lib/Autom4te/XFile.pm
index 19b73aa..95a452b 100644
--- i/lib/Autom4te/XFile.pm
+++ w/lib/Autom4te/XFile.pm
@@ -138,7 +138,14 @@ sub open
# comment in IO::Handle.
${*$fh}{'autom4te_xfile_file'} = $file;
- if (!$fh-SUPER::open (@_))
+ if (defined $mode $file eq '-')
+{
+ if (!$fh-SUPER::open ($mode$file))
What if mode is something like w or r+?
+ {
+ fatal cannot open $file: $!;
+ }
+}
+ elsif (!$fh-SUPER::open (@_))
In other words, since the point of XFile is to ensure binmode, and to
make open nicer to use, making XFile the owner of '-' magic seems like
the right thing to do, and the real remaining autoconf bug is that we
aren't using XFile everywhere.
Makes sense.
If anything, this issue shows that the Automake::XFile module (and
other similar modules as well) should live in their own git
repository, which both Autoconf and Automake should use as a
submodule -- and which (for $DEITY's sake!), should be unit-tested.
Any volunteer?
Alas, my perl-fu is weak enough that I'm not sure how good my
unit-testing attempts would be. But moving the files to a common
repository seems doable (how about gnulib?).
This could be a good interim solution, as I the paperwork in place for
Gnulib, as well as pushing rights for its repository. Unit tests can be
added later, as the need arise, and/or slowly step-by-step when we feel
like doing so.
Thanks,
Stefano
Re: [FYI] {master} refactor: use modern semantics of 'open'
On 04/26/2012 05:12 AM, Stefano Lattarini wrote:
Hi Eric, sorry for the delay.
On 04/25/2012 01:25 AM, Eric Blake wrote:
Actually, this appears to make _all_ the XFile uses work; all that
remains broken is places such as bin/autoupdate.in calling raw open
instead of using XFile.
diff --git i/lib/Autom4te/XFile.pm w/lib/Autom4te/XFile.pm
index 19b73aa..95a452b 100644
--- i/lib/Autom4te/XFile.pm
+++ w/lib/Autom4te/XFile.pm
@@ -138,7 +138,14 @@ sub open
# comment in IO::Handle.
${*$fh}{'autom4te_xfile_file'} = $file;
- if (!$fh-SUPER::open (@_))
+ if (defined $mode $file eq '-')
+{
+ if (!$fh-SUPER::open ($mode$file))
What if mode is something like w or r+?
I hadn't thought about that - but we would indeed need to add some
smarts to XFile to handle it properly.
In other words, since the point of XFile is to ensure binmode, and to
make open nicer to use, making XFile the owner of '-' magic seems like
the right thing to do, and the real remaining autoconf bug is that we
aren't using XFile everywhere.
Makes sense.
If anything, this issue shows that the Automake::XFile module (and
other similar modules as well) should live in their own git
repository, which both Autoconf and Automake should use as a
submodule -- and which (for $DEITY's sake!), should be unit-tested.
Any volunteer?
Alas, my perl-fu is weak enough that I'm not sure how good my
unit-testing attempts would be. But moving the files to a common
repository seems doable (how about gnulib?).
This could be a good interim solution, as I the paperwork in place for
Gnulib, as well as pushing rights for its repository. Unit tests can be
added later, as the need arise, and/or slowly step-by-step when we feel
like doing so.
Would we move all .pm files, or just the .pm files that both automake
and autoconf are sharing? (The list of shared files is:
Configure_ac.pm, Channels.pm, FileUtils.pm, Getopt.pm, XFile.pm)
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [FYI] {master} refactor: use modern semantics of 'open'
[SNIP] Eric Blake wrote: Alas, my perl-fu is weak enough that I'm not sure how good my unit-testing attempts would be. But moving the files to a common repository seems doable (how about gnulib?). I replied: This could be a good interim solution, as I the paperwork in place for Gnulib, as well as pushing rights for its repository. Unit tests can be added later, as the need arise, and/or slowly step-by-step when we feel like doing so. Eric replied: Would we move all .pm files, or just the .pm files that both automake and autoconf are sharing? (The list of shared files is: Configure_ac.pm, Channels.pm, FileUtils.pm, Getopt.pm, XFile.pm) I'd move only the shared files as a first step, for sake of simplicity. Once more syncing and testing infrastructure is in place, we might decide to move other files as well (if that will be deemed to offer some advantage). No need to hurry or overdo, right? Thanks, Stefano
[FYI] {master} refactor: use modern semantics of 'open'
* lib/Automake/XFile.pm: Update comments and POD documentation to
suggest a more idiomatic/modern usage.
(open): Be more robust in detecting whether the created file handle
is being opened for writing.
* lib/Automake/FileUtils.pm (update_file, contents): Call the
'Automake::XFile' and 'File::IO' constructors with two arguments
rather than one; this change obsoletes ...
(open_quote): ... this subroutine, which has thus been removed.
(@EXPORT): Drop 'open_quote'.
Signed-off-by: Stefano Lattarini address@hidden
---
lib/Automake/FileUtils.pm | 33 +++--
lib/Automake/XFile.pm | 13 +
2 files changed, 12 insertions(+), 34 deletions(-)
Change tested with perl 5.10.1 and perl 5.6.0. No regression in the
testsuite. Patch already pushed to master.
Help! I can't release autoconf 2.69 until I figure out how to work
around this patch. After updating to the latest shared files, as well
as applying this patch, I'm now stuck with output going to a literal
file named '-' instead of going to stdout. I suspect that the
conversion to the 2-arg form is mishandling our idiom of '-' as standard
in/out.
From 6ee479858ab4a4c4054d6ee334adae4e38295e52 Mon Sep 17 00:00:00 2001
From: Eric Blake ebl...@redhat.com
Date: Tue, 24 Apr 2012 16:06:03 -0600
Subject: [PATCH] bin: replace older perl open_quote workaround with modern
idiom
Now that we require more modern perl, we might as well use it. See
https://lists.gnu.org/archive/html/automake-patches/2012-03/msg00111.html
for an example of how using 2-argument open makes life nicer.
* bin/autoheader.in: Use two-arg open, not open_quote.
* bin/autom4te.in: Likewise.
* bin/autoscan.in: Likewise.
* bin/autoupdate.in: Likewise.
* bin/ifnames.in: Likewise.
Signed-off-by: Eric Blake ebl...@redhat.com
---
bin/autoheader.in | 12 ++--
bin/autom4te.in | 17 +
bin/autoscan.in | 12 ++--
bin/autoupdate.in | 16
bin/ifnames.in|2 +-
5 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/bin/autoheader.in b/bin/autoheader.in
index d81a313..f51382f 100644
--- a/bin/autoheader.in
+++ b/bin/autoheader.in
@@ -201,7 +201,7 @@ $config_h_in ||= $config_h.in;
# only the name of the macro.
%symbol = map { s/\(.*//; $_ = 1 } keys %symbol;
-my $out = new Autom4te::XFile ( . open_quote ($tmp/config.hin));
+my $out = new Autom4te::XFile $tmp/config.hin, ;
# Don't write do not edit -- it will get copied into the
# config.h, which it's ok to edit.
@@ -210,7 +210,7 @@ print $out /* $config_h_in. Generated from
$ARGV[0] by autoheader. */\n;
# Dump the top.
if ($config_h_top)
{
-my $in = new Autom4te::XFile ( . open_quote ($config_h_top));
+my $in = new Autom4te::XFile $config_h_top, ;
while ($_ = $in-getline)
{
print $out $_;
@@ -220,7 +220,7 @@ if ($config_h_top)
# Dump `acconfig.h', except for its bottom portion.
if ($acconfig_h)
{
-my $in = new Autom4te::XFile ( . open_quote ($acconfig_h));
+my $in = new Autom4te::XFile $acconfig_h, ;
while ($_ = $in-getline)
{
last if /\@BOTTOM\@/;
@@ -238,7 +238,7 @@ foreach (sort keys %verbatim)
# Dump bottom portion of `acconfig.h'.
if ($acconfig_h)
{
-my $in = new Autom4te::XFile ( . open_quote ($acconfig_h));
+my $in = new Autom4te::XFile $acconfig_h, ;
my $dump = 0;
while ($_ = $in-getline)
{
@@ -250,7 +250,7 @@ if ($acconfig_h)
# Dump the bottom.
if ($config_h_bot)
{
-my $in = new Autom4te::XFile ( . open_quote ($config_h_bot));
+my $in = new Autom4te::XFile $config_h_bot, ;
while ($_ = $in-getline)
{
print $out $_;
@@ -261,7 +261,7 @@ $out-close;
# Check that all the symbols have a template.
{
- my $in = new Autom4te::XFile ( . open_quote ($tmp/config.hin));
+ my $in = new Autom4te::XFile $tmp/config.hin, ;
my $suggest_ac_define = 1;
while ($_ = $in-getline)
{
diff --git a/bin/autom4te.in b/bin/autom4te.in
index 11773c9..c2424c9 100644
--- a/bin/autom4te.in
+++ b/bin/autom4te.in
@@ -258,7 +258,7 @@ sub load_configuration ($)
my ($file) = @_;
use Text::ParseWords;
- my $cfg = new Autom4te::XFile ( . open_quote ($file));
+ my $cfg = new Autom4te::XFile $file, ;
my $lang;
while ($_ = $cfg-getline)
{
@@ -527,7 +527,8 @@ sub handle_output ($$)
handle_traces ($req, $tmp/patterns,
('m4_pattern_forbid' = 'forbid:$1:$2',
'm4_pattern_allow' = 'allow:$1'));
- my @patterns = new Autom4te::XFile ( . open_quote
($tmp/patterns))-getlines;
+ my $pattern_file = new Autom4te::XFile $tmp/patterns, ;
+ my @patterns = $pattern_file-getlines;
chomp @patterns;
my %forbidden =
map { /^forbid:([^:]+):.+$/ = /^forbid:[^:]+:(.+)$/ } @patterns;
@@ -554,7 +555,7 @@ sub handle_output ($$)
}
fatal cannot create $output: $!
unless $out;
- my $in = new Autom4te::XFile ( . open_quote ($ocache .
Re: [FYI] {master} refactor: use modern semantics of 'open'
On 04/24/2012 04:50 PM, Russ Allbery wrote:
Eric Blake ebl...@redhat.com writes:
Help! I can't release autoconf 2.69 until I figure out how to work
around this patch. After updating to the latest shared files, as well
as applying this patch, I'm now stuck with output going to a literal
file named '-' instead of going to stdout. I suspect that the
conversion to the 2-arg form is mishandling our idiom of '-' as standard
in/out.
If you call open with three arguments, - has no special meaning and
refers to a file named - (since the whole point of three-argument open
is to remove all magic interpretations of the filename string). The
easiest way to work around this is probably to change the Automake helper
functions that sit between the code and the Perl open command and have
them switch to calling open with two arguments if the file name is -.
Indeed, this hack gets me further, but still not complete success:
diff --git i/lib/Autom4te/XFile.pm w/lib/Autom4te/XFile.pm
index 19b73aa..95a452b 100644
--- i/lib/Autom4te/XFile.pm
+++ w/lib/Autom4te/XFile.pm
@@ -138,7 +138,14 @@ sub open
# comment in IO::Handle.
${*$fh}{'autom4te_xfile_file'} = $file;
- if (!$fh-SUPER::open (@_))
+ if (defined $mode $file eq '-')
+{
+ if (!$fh-SUPER::open ($mode$file))
+ {
+ fatal cannot open $file: $!;
+ }
+}
+ elsif (!$fh-SUPER::open (@_))
{
fatal cannot open $file: $!;
}
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [FYI] {master} refactor: use modern semantics of 'open'
Hi Eric, Russ.
On 04/25/2012 01:08 AM, Eric Blake wrote:
On 04/24/2012 04:50 PM, Russ Allbery wrote:
Eric Blake ebl...@redhat.com writes:
Help! I can't release autoconf 2.69 until I figure out how to work
around this patch. After updating to the latest shared files, as well
as applying this patch, I'm now stuck with output going to a literal
file named '-' instead of going to stdout. I suspect that the
conversion to the 2-arg form is mishandling our idiom of '-' as standard
in/out.
If you call open with three arguments, - has no special meaning and
refers to a file named - (since the whole point of three-argument open
is to remove all magic interpretations of the filename string). The
easiest way to work around this is probably to change the Automake helper
functions that sit between the code and the Perl open command and have
them switch to calling open with two arguments if the file name is -.
Indeed, this hack gets me further, but still not complete success:
diff --git i/lib/Autom4te/XFile.pm w/lib/Autom4te/XFile.pm
index 19b73aa..95a452b 100644
--- i/lib/Autom4te/XFile.pm
+++ w/lib/Autom4te/XFile.pm
@@ -138,7 +138,14 @@ sub open
# comment in IO::Handle.
${*$fh}{'autom4te_xfile_file'} = $file;
- if (!$fh-SUPER::open (@_))
+ if (defined $mode $file eq '-')
+{
+ if (!$fh-SUPER::open ($mode$file))
+ {
+ fatal cannot open $file: $!;
+ }
+}
+ elsif (!$fh-SUPER::open (@_))
{
fatal cannot open $file: $!;
}
My hope is that you'll manage to quickly find a patch (preferably to
be applied to the Autoconf repository only) that can work around the
issue effectively. If you don't, I will (after Automake 1.12 is out)
accept a commit reverting my patch, since keeping Autoconf sane and
working is definitely, absolutely more worth than a having a ten-line
simplification in Automake.
If anything, this issue shows that the Automake::XFile module (and
other similar modules as well) should live in their own git
repository, which both Autoconf and Automake should use as a
submodule -- and which (for $DEITY's sake!), should be unit-tested.
Any volunteer?
Thanks, and sorry for the confusion,
Stefano
Re: [FYI] {master} refactor: use modern semantics of 'open'
On 04/24/2012 05:17 PM, Stefano Lattarini wrote:
Hi Eric, Russ.
On 04/25/2012 01:08 AM, Eric Blake wrote:
On 04/24/2012 04:50 PM, Russ Allbery wrote:
Eric Blake ebl...@redhat.com writes:
Help! I can't release autoconf 2.69 until I figure out how to work
around this patch. After updating to the latest shared files, as well
as applying this patch, I'm now stuck with output going to a literal
file named '-' instead of going to stdout. I suspect that the
conversion to the 2-arg form is mishandling our idiom of '-' as standard
in/out.
If you call open with three arguments, - has no special meaning and
refers to a file named - (since the whole point of three-argument open
is to remove all magic interpretations of the filename string). The
easiest way to work around this is probably to change the Automake helper
functions that sit between the code and the Perl open command and have
them switch to calling open with two arguments if the file name is -.
Indeed, this hack gets me further, but still not complete success:
Actually, this appears to make _all_ the XFile uses work; all that
remains broken is places such as bin/autoupdate.in calling raw open
instead of using XFile.
diff --git i/lib/Autom4te/XFile.pm w/lib/Autom4te/XFile.pm
index 19b73aa..95a452b 100644
--- i/lib/Autom4te/XFile.pm
+++ w/lib/Autom4te/XFile.pm
@@ -138,7 +138,14 @@ sub open
# comment in IO::Handle.
${*$fh}{'autom4te_xfile_file'} = $file;
- if (!$fh-SUPER::open (@_))
+ if (defined $mode $file eq '-')
+{
+ if (!$fh-SUPER::open ($mode$file))
+{
+ fatal cannot open $file: $!;
+}
+}
+ elsif (!$fh-SUPER::open (@_))
In other words, since the point of XFile is to ensure binmode, and to
make open nicer to use, making XFile the owner of '-' magic seems like
the right thing to do, and the real remaining autoconf bug is that we
aren't using XFile everywhere.
My hope is that you'll manage to quickly find a patch (preferably to
be applied to the Autoconf repository only) that can work around the
issue effectively. If you don't, I will (after Automake 1.12 is out)
accept a commit reverting my patch, since keeping Autoconf sane and
working is definitely, absolutely more worth than a having a ten-line
simplification in Automake.
I don't think we need to revert your patch, so much as add the '-'
handling enhancement. But I figured out a solution: release autoconf
2.69 _without_ fetching the latest from automake, which in turn will let
you release automake 1.12 depending on autoconf 2.69, and _then_ we fix
the fallout.
If anything, this issue shows that the Automake::XFile module (and
other similar modules as well) should live in their own git
repository, which both Autoconf and Automake should use as a
submodule -- and which (for $DEITY's sake!), should be unit-tested.
Any volunteer?
Alas, my perl-fu is weak enough that I'm not sure how good my
unit-testing attempts would be. But moving the files to a common
repository seems doable (how about gnulib?).
Thanks, and sorry for the confusion,
And sorry I didn't review your automake patch queue sooner, since I
wasn't thinking about the impact to autoconf.
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [FYI] {master} refactor: use modern semantics of 'open'
Eric Blake ebl...@redhat.com writes: Help! I can't release autoconf 2.69 until I figure out how to work around this patch. After updating to the latest shared files, as well as applying this patch, I'm now stuck with output going to a literal file named '-' instead of going to stdout. I suspect that the conversion to the 2-arg form is mishandling our idiom of '-' as standard in/out. If you call open with three arguments, - has no special meaning and refers to a file named - (since the whole point of three-argument open is to remove all magic interpretations of the filename string). The easiest way to work around this is probably to change the Automake helper functions that sit between the code and the Perl open command and have them switch to calling open with two arguments if the file name is -. -- Russ Allbery (r...@stanford.edu) http://www.eyrie.org/~eagle/
