Front page | perl.perl5.porters |
Postings from April 2013
Re: [PATCH] On eliminating external tools from the release process
Thread Previous
|
Thread Next
From:
Nicholas Clark
Date:
April 4, 2013 09:11
Subject:
Re: [PATCH] On eliminating external tools from the release process
Message ID:
20130404091141.GD3729@plum.flirble.org
Whatever your mailer does with attachments, I like it, because my mailer
will inline them in this reply...
On Wed, Apr 03, 2013 at 10:12:54PM +0200, Max Maischein wrote:
> Hi p5p,
>
> several of our tools for the release process rely on Unix shell tools
> being available. This somewhat hampers the ability to do a release using
> only Windows or other OSes where the "unix centric" toolset is
> unavailable, but keeps the scripts themselves fairly simple.
Yes, to my mind it seems ugly that we have programs that shell out to
tools which can all be done natively. There's nothing as gratuitous as
`cat foo`, but it's getting close.
Whilst I don't think that anything of that the external tools are doing
*here* is non-portable, it can be a risk. But avoiding shelling out should
make error handling easier.
(There is the counter-risk, as Abigail correctly notes, that in some places
the system's specific tool has features that the Perl module can't do,
or can't do well, so not shelling out is bad. Particularly if you don't need
portability. But here we do.)
> 1) Perl already comes with ExtUtils::Command, which is a good
> replacement for many common shell commands. I assume that most of
> ExtUtils::Command was even written to replace common shell commands. The
Yes, for Makefiles generated by ExtUtils::MakeMaker
And from looking at the implementation of them (see below) it looks like
they are only flexible enough to do the things that ExtUtils::MakeMaker
needs, and nothing more.
> first patch changes many calls to these external tools to external calls
> to ExtUtils::Command. Supposedly, I could instead appropriate (code
> from) Shell::Command to remove the reliance on an external (Perl)
> process, but in my limited testing, this didn't always work.
If the "tool" is written in Perl, and this is Perl, why is there even need
to shell out?
> 2) As second example, Perl (now) has HTTP::Tiny in the core. We /could/
> thus replace the reliance on `wget` in the script with use of
> HTTP::Tiny, which provides a fair bit of HTTP capability, even some proxies.
>
> The immediate downside of this is that we lose all additional features
> that `wget` may bring, like SOCKS proxies, HTTPS etc.
I doubt that we're using any of those additional features.
> The attached version of my changes adds five lines for every simple call
> to `wget`, trying with HTTP::Tiny first and falling back to the original
> `wget` second:
>
> + eval {
> + require HTTP::Tiny;
> + my $http= HTTP::Tiny->new();
> + $http->mirror( $url => $new_file );
> + 1
> + } or system wget => $url, '-qO', $new_file;
>
> I'm not sure if keeping both code paths is "worth it". Also, I'm not
> sure whether eliminating the use of external tools is a goal of any merit.
I think that it is, providing it isn't massive code churn, and it doesn't
introduce annoying bugs.
> --- a/Porting/sync-with-cpan
> +++ b/Porting/sync-with-cpan
> -system tar => 'xfz', $new_file;
> +system "../perl" => qw(../utils/ptar xzf ), $new_file;
ptar itself is 142 lines total wrapping Archive::Tar and File::Find.
Wouldn't it be better still to use them directly, and avoid the shell out?
> >From eceb490c86afa96afa30be0099470a0e1d7f136c Mon Sep 17 00:00:00 2001
> From: Max Maischein <corion@corion.net>
> Date: Sun, 31 Mar 2013 11:53:03 +0200
> Subject: [PATCH 3/7] Replace hardcoded /tmp by $ENV{TEMP}
>
> Ideally, we would use File::Temp, but that is not in the core.
Eh? :-)
$ grep File-Temp MANIFEST
cpan/File-Temp/lib/File/Temp.pm create safe temporary files and file handles
cpan/File-Temp/t/cmp.t See if File::Temp works
cpan/File-Temp/t/fork.t See if File::Temp works
cpan/File-Temp/t/lock.t See if File::Temp works
cpan/File-Temp/t/mktemp.t See if File::Temp works
cpan/File-Temp/t/object.t See if File::Temp works
cpan/File-Temp/t/posix.t See if File::Temp works
cpan/File-Temp/t/rmtree.t See if File::Temp works
cpan/File-Temp/t/security.t See if File::Temp works
cpan/File-Temp/t/seekable.t See if File::Temp works
cpan/File-Temp/t/tempfile.t See if File::Temp works
I think that we can use File::Temp.
> These subroutines were written as replacement for shell tools,
> so we should be able to use them here as well. Ideally,
> we wouldn't shell out but crib the relevant code from Shell::Command.
> In my tests, cribbing the code from Shell::Command did not immediately
> work out, which is why I keep calling another process.
> +for my $cmd (qw(touch rm_rf chmod)) {
> + no strict 'refs';
> + *{ $cmd } = sub {
> + warn getcwd . "> perl -MExtUtils::Command -e $cmd @_";
> + system "perl -MExtUtils::Command -e $cmd @_";
> + };
> +};
I think it would be even better to call or inline the gubbins of the code from
ExtUtils::Command. For example, touch is this, and I think can be called
directly, if one localises @ARGV:
sub touch {
my $t = time;
expand_wildcards();
foreach my $file (@ARGV) {
open(FILE,">>$file") || die "Cannot write $file:$!";
close(FILE);
utime($t,$t,$file);
}
}
It seems that ExtUtils::Command::chmod *doesn't* do what is needed here,
as it only takes octal arguments:
> -system chmod => 'a-x', "$pkg_dir/$_" for @de_exec;
> +chmod( 'a-x', "$pkg_dir/$_" ) for @de_exec;
$ ls -l pie
-rw------- 1 nick admin 0 Apr 3 12:46 pie
$ perl -MExtUtils::Command -we chmod a+x pie
$ ls -l pie
---------- 1 nick admin 0 Apr 3 12:46 pie
:-(
[And where did my warning go, dammit?]
Looks like rm_rf could be replaced by direct use of File::Path:
sub rm_rf
{
expand_wildcards();
rmtree([grep -e $_,@ARGV],0,0);
}
So seems that actually for those three, two should be "inlined", and for
chmod the 3 or so lines of CORE::stat, munge file permissions, CORE::chmod
should be used instead.
But yes, I think that it's worth doing.
Nicholas Clark
Thread Previous
|
Thread Next