Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] On eliminating external tools from the release process

9 views
Skip to first unread message

Max Maischein

unread,
Apr 3, 2013, 4:12:54 PM4/3/13
to perl5-...@perl.org
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.

During my release of 5.17.10, I patched away some of the external tools
of Porting/sync-with-cpan, so it mostly works under Windows, and without
reliance on the external programs. The patches are attached below, and
separated thematically but please do not immediately apply them.

I found two general steps for eliminating external tools:

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
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.

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.

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.

Happy Easter,
-max

0001-Eliminate-find-type-f.patch
0002-Eliminate-reliance-on-grep-ls-and-tar.patch
0003-Replace-hardcoded-tmp-by-ENV-TEMP.patch
0004-Use-Config-make-as-make-tool.-Run-make-from-the-appr.patch
0005-First-try-with-HTTP-Tiny-fallback-on-wget.patch
0006-Replace-touch-rm-rf-and-chmod-by-ExtUtils-Command.patch
0007-Allow-for-directories-other-than-.-cpan.patch

bulk 88

unread,
Apr 4, 2013, 4:09:47 AM4/4/13
to Max Maischein, perl5-...@perl.org


> Date: Wed, 3 Apr 2013 22:12:54 +0200
> From: cor...@cpan.org
> To: perl5-...@perl.org
> Subject: [PATCH] On eliminating external tools from the release process

>
> 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.
>
> During my release of 5.17.10, I patched away some of the external tools
> of Porting/sync-with-cpan, so it mostly works under Windows, and without
> reliance on the external programs. The patches are attached below, and
> separated thematically but please do not immediately apply them.
>
> I found two general steps for eliminating external tools:
>
> 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
> 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.
>


Is ExtUtils::Command a suitable replacement for natively calling the shell,
performance wise?  I looked at ExtUtils::Command's cp/copy, and from a
superficial glance, it never calls the shell copy even if the shell has a
usuable copy command. On Win32 it tries dynaload Win32::, if it
 can't it reverts to a for loop to do the copying in pure perl.
VMS also has a C implementation, on  POSIX
it seems to do a for loop with buffers. On some OSes a "native
copy" can be transactional or async, and it returns almost immediately.
On other OSes, prefetching or hinting to the IO scheduler that we are
doing sequential reading will serialize and buffer instead of interleave the
reads. On others OSes, zero copy DMA to DMA copying is done with
 kernel help. A for loop through stdio does none of that.

Nicholas Clark

unread,
Apr 4, 2013, 4:46:35 AM4/4/13
to bulk 88, Max Maischein, perl5-...@perl.org
On Thu, Apr 04, 2013 at 04:09:47AM -0400, bulk 88 wrote:

> Is ExtUtils::Command a suitable replacement for natively calling the shell,
> performance wise? I looked at ExtUtils::Command's cp/copy, and from a

In the general case, no. Definitely not.

But this case is more concerned with portability, maintainability and
reliability than speed. There are plenty of other steps in the release
process which are slower than this.

> On other OSes, prefetching or hinting to the IO scheduler that we are
> doing sequential reading will serialize and buffer instead of interleave the
> reads. On others OSes, zero copy DMA to DMA copying is done with
> kernel help. A for loop through stdio does none of that.

Sure, but in this case, the scripts will immediately block awaiting the
output, on a local machine which is only being used interactively for this
task, so disk IO speed is what's going to matter, not how to reduce CPU
use.

Ultimately, for the release process, we care about elapsed operator time,
reliability, and portability. The resources used to get a release don't
are secondary, as long as they aren't so excessive as to impact on the
primary concerns.

Nicholas Clark

Nicholas Clark

unread,
Apr 4, 2013, 5:11:41 AM4/4/13
to Max Maischein, perl5-...@perl.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 <cor...@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

Konovalov, Vadim (Vadim)** CTR **

unread,
Apr 4, 2013, 5:25:37 AM4/4/13
to Max Maischein, perl5-...@perl.org
> From: Max Maischein
> 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 );

the simpler the better

HTTP::Tiny->new()->mirror( $url => $new_file );

Nicholas Clark

unread,
Apr 4, 2013, 6:44:44 AM4/4/13
to bulk 88, Max Maischein, perl5-...@perl.org
On Thu, Apr 04, 2013 at 06:37:32AM -0400, bulk 88 wrote:

Quoting in full intentionally:
> > Date: Thu, 4 Apr 2013 09:46:35 +0100
> > From: ni...@ccl4.org
> > To: bul...@hotmail.com
> > CC: cor...@cpan.org; perl5-...@perl.org
> > Subject: Re: [PATCH] On eliminating external tools from the release process
> >
>
> >
> > > On other OSes, prefetching or hinting to the IO scheduler that we are
> > > doing sequential reading will serialize and buffer instead of interleave the
> > > reads. On others OSes, zero copy DMA to DMA copying is done with
> > > kernel help. A for loop through stdio does none of that.
> >
> > Sure, but in this case, the scripts will immediately block awaiting the
> > output, on a local machine which is only being used interactively for this
> > task, so disk IO speed is what's going to matter, not how to reduce CPU
> > use.
> >
> Disk IO doesn't matter if it was copied to ram disk cache in 1 ms. Then the
> new file is available for reading and writing,, and control is returned back
> to me and I can go on, but in the background the disk is trashing for next 3 minutes,
> but it doesn't affect anything visually.
>
> > Ultimately, for the release process, we care about elapsed operator time,
> So performance does matter during the build process?

You've snipped the part where I said that other parts of the process will
consume more time. That is a material part of my point.

Optimise the slow parts. This won't be one of them.

Nicholas Clark

David Golden

unread,
Apr 4, 2013, 2:33:45 PM4/4/13
to Nicholas Clark, bulk 88, Max Maischein, perl5-...@perl.org
On Thu, Apr 4, 2013 at 6:44 AM, Nicholas Clark <ni...@ccl4.org> wrote:

>> > Ultimately, for the release process, we care about elapsed operator time,
>> So performance does matter during the build process?
>
> You've snipped the part where I said that other parts of the process will
> consume more time. That is a material part of my point.
>
> Optimise the slow parts. This won't be one of them.

It depends. Don't we copy to a temp directory for making the archive
file? If every file is copied with EU::Command as a separate command,
that means invoking Perl once per copy. That is going to be very,
very slow on Windows.

This is a known issue with EU::MM on windows even just for ordinary
CPAN modules -- all the copies to blib and thence to site_lib need an
invocation of Perl per copy operation. There's been talk of replacing
it with native OS commands, but no one has done the work.

EU::Command needs to be used *carefully* if the number of operations is large.

David

--
David Golden <x...@xdg.me>
Take back your inbox! → http://www.bunchmail.com/
Twitter/IRC: @xdg

Nicholas Clark

unread,
Apr 5, 2013, 4:32:03 AM4/5/13
to Max Maischein, David Golden, bulk 88, perl5-...@perl.org
On Thu, Apr 04, 2013 at 08:51:17PM +0200, Max Maischein wrote:
> Am 04.04.2013 20:33, schrieb David Golden:
> >> [...]
> >>
> >> Optimise the slow parts. This won't be one of them.
> >
> > It depends. Don't we copy to a temp directory for making the archive
> > file? If every file is copied with EU::Command as a separate command,
> > that means invoking Perl once per copy. That is going to be very,
> > very slow on Windows.
>
> While this is a good point, the patch(es) so far only address changes to
> a program that is run once or twice when cutting a release, and ideally
> not run at all while cutting a release. In that sense, spending time on
> making that program perform faster is wasted in my opinion.

And I was suggesting that using ExtUtils::Command by shelling out to *it*
wasn't the best way (either). Call the code directly.

(The code that ExtUtils::Command uses, as its subroutines are just command
line parsing wrappers for other Perl code.)

Nicholas Clark

Max Maischein

unread,
Apr 4, 2013, 2:51:17 PM4/4/13
to David Golden, Nicholas Clark, bulk 88, perl5-...@perl.org
Am 04.04.2013 20:33, schrieb David Golden:
>> [...]
>>
>> Optimise the slow parts. This won't be one of them.
>
> It depends. Don't we copy to a temp directory for making the archive
> file? If every file is copied with EU::Command as a separate command,
> that means invoking Perl once per copy. That is going to be very,
> very slow on Windows.

While this is a good point, the patch(es) so far only address changes to
a program that is run once or twice when cutting a release, and ideally
not run at all while cutting a release. In that sense, spending time on
making that program perform faster is wasted in my opinion.

-max


Ricardo Signes

unread,
Oct 2, 2013, 10:06:19 PM10/2/13
to Max Maischein, perl5-...@perl.org

http://www.nntp.perl.org/group/perl.perl5.porters/2013/04/msg200825.html
and
http://www.nntp.perl.org/group/perl.perl5.porters/2013/04/msg200835.html

It sounded like these improvements were generally fine, and that maybe you
could go ahead and use File::Temp, etc. Are you interested in tweaking the
patches and applying them?

--
rjbs
signature.asc

Max Maischein

unread,
Oct 6, 2013, 3:10:23 PM10/6/13
to Ricardo Signes, perl5-...@perl.org
Hi all,

I've redone my changes, and even tested them. The process is not yet
completely smooth on Windows, and I haven't found out exactly why. But
testing the difference between the original sync-with-cpan and my
changes on Linux uncovered some bad assumptions on my part about what
the program was doing, so it was not all in vain.

> It sounded like these improvements were generally fine, and that
maybe you
> could go ahead and use File::Temp, etc. Are you interested in tweaking the
> patches and applying them?

In fact, using File::Temp runs counter to the idea of locally caching
the module index, so I backed out that change after realizing it. The
program now prefers $ENV{ TEMP } if set over the hardcoded '/tmp'.

The series of patches has entered blead as a non-fast-forward merge
below 4387bc3c6899993ec4504edc3be8b5440ea61521 .

-max

Ricardo Signes

unread,
Oct 7, 2013, 11:29:13 AM10/7/13
to Max Maischein, perl5-...@perl.org
* Max Maischein <cor...@cpan.org> [2013-10-06T15:10:23]
> The series of patches has entered blead as a non-fast-forward merge
> below 4387bc3c6899993ec4504edc3be8b5440ea61521 .

Thanks! Maybe I'll try doing a release from Win32 sometime soon...

--
rjbs
signature.asc
0 new messages