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

enhance HARNESS_TIMER functionality #11636

Closed
p5pRT opened this issue Sep 7, 2011 · 32 comments
Closed

enhance HARNESS_TIMER functionality #11636

p5pRT opened this issue Sep 7, 2011 · 32 comments

Comments

@p5pRT
Copy link

p5pRT commented Sep 7, 2011

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

Searchable as RT98662$

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2011

From @jimc

Created by @jimc

Ive got patches against blead to collect more timing data using TEST_HARNESS;

measures and prints elapsed time for each test (current functionality)
  HARNESS_TIMER=1 make test;
  t/io/nargv ................ ok 18 ms

calls times() before and after each test, prints child user, system times
  HARNESS_TIMER=2 make test;
  t/comp/colon .............. ok 17 ms 10 ms 0 ms
  t/comp/decl ............... ok 4 ms 0 ms 0 ms
  t/comp/final_line_num ..... ok 5 ms 0 ms 10 ms
  t/comp/fold ............... ok 10 ms 10 ms 0 ms
  t/comp/form_scope ......... ok 8 ms 0 ms 10 ms

calls times() at END of each test, passes 4 measures back to t/TEST as
diag-output
t/TEST prints them at end of line for each tested file.
  HARNESS_TIMER=3 make test;
also writes all collected timing data to Storable file in the directory.
  HARNESS_TIMER=../../perf make test; # (existing directory)
  t/cmd/elsif ............... ok 9 ms 0 ms 0 ms
# ET​: (ms) u​:0 s​:0 cu​:0 cs​:0
  t/cmd/for ................. ok 19 ms 20 ms 0 ms
# ET​: (ms) u​:10 s​:0 cu​:0 cs​:0
  t/cmd/mod ................. ok 5 ms 0 ms 10 ms
# ET​: (ms) u​:0 s​:0 cu​:0 cs​:0
  t/cmd/subval .............. ok 14 ms 0 ms 0 ms
# ET​: (ms) u​:0 s​:0 cu​:0 cs​:0

Perl Info

Flags:
    category=core
    severity=low

This perlbug was built using Perl 5.12.3 in the Fedora build system.
It is being executed now by Perl 5.15.2 - Wed Sep  7 12:17:39 MDT 2011.

Site configuration information for perl 5.15.2:

Configured by jimc at Wed Sep  7 12:17:39 MDT 2011.

Summary of my perl5 (revision 5 version 15 subversion 2) configuration:
  Commit id: d39de89300b9384bad8b2cf88917ce9f104ae8b2
  Platform:
    osname=linux, osvers=2.6.35.14-95.fc14.x86_64, archname=x86_64-linux
    uname='linux groucho.jimc.earth 2.6.35.14-95.fc14.x86_64 #1 smp
tue aug 16 21:01:58 utc 2011 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Dusedevel -DDEBUGGING=both'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-DDEBUGGING -fno-strict-aliasing -pipe
-fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g',
    cppflags='-DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include'
    ccversion='', gccversion='4.5.1 20100924 (Red Hat 4.5.1-4)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib/../lib64 /usr/lib/../lib64 /lib
/usr/lib /lib64 /usr/lib64 /usr/local/lib64
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.13.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.13'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -L/usr/local/lib
-fstack-protector'

Locally applied patches:



@INC for perl 5.15.2:
    lib
    /usr/local/lib/perl5/site_perl/5.15.2/x86_64-linux
    /usr/local/lib/perl5/site_perl/5.15.2
    /usr/local/lib/perl5/5.15.2/x86_64-linux
    /usr/local/lib/perl5/5.15.2
    /usr/local/lib/perl5/site_perl
    .


Environment for perl 5.15.2:
    HOME=/home/jimc
    LANG=en_US.utf8
    LANGUAGE (unset)
    LD_LIBRARY_PATH=/usr/lib64/mpich2/lib:/usr/lib64/alliance/lib:/usr/lib64/alliance/lib
    LOGDIR (unset)
    PATH=/usr/lib64/qt-3.3/bin:/usr/lib64/mpich2/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/lib64/alliance/bin:/usr/libexec/sdcc:/home/jimc/bin:./bin:.:/usr/lib64/alliance/bin:/usr/libexec/sdcc
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2011

From @jimc

patches attached.

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2011

From @jimc

0001-add-space-after-testfile-name-and-before-ok-not-ok.patch
From 71772f46a18db1397ee9ac22531ec97b2de10537 Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 13 Mar 2011 12:44:34 -0600
Subject: [PATCH 1/5] add space after testfile name, and before ok/not ok

Adding space between testfile name and ...... lets user double-click
on just the name, instead of getting all the dots too, reducing the
cmdline editing to resubmit the test manually.  Space before ok/not-ok
allows easier parsing with split /\s/, $line.  Both make output agree
more closely with that from Test::*
---
 t/TEST |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/TEST b/t/TEST
index 0c24c17..219a16f 100755
--- a/t/TEST
+++ b/t/TEST
@@ -509,7 +509,7 @@ EOT
     }
     my $maxlen = 0;
     foreach (@::path_to_name{@tests}) {
-	s/\.\w+\z/./;
+	s/\.\w+\z/ /; # space gives easy doubleclick to select fname
 	my $len = length ;
 	$maxlen = $len if $len > $maxlen;
     }
@@ -539,7 +539,7 @@ EOT
 	    }
 	}
 	my $te = $::path_to_name{$test} . '.'
-		    x ($dotdotdot - length($::path_to_name{$test}));
+		    x ($dotdotdot - length($::path_to_name{$test})) .' ';
 
 	if ($^O ne 'VMS') {  # defer printing on VMS due to piping bug
 	    print $te;
-- 
1.7.4.4

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2011

From @jimc

0002-save-elapsed-time-ms-in-global-hash-for-later-report.patch
From a0b6db53a57f34bcf6b71b2d44690ec020647bcb Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 13 Mar 2011 13:07:42 -0600
Subject: [PATCH 2/5] save elapsed-time-ms in global hash for later reporting

---
 t/TEST |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/TEST b/t/TEST
index 219a16f..30abcf8 100755
--- a/t/TEST
+++ b/t/TEST
@@ -159,6 +159,7 @@ $ENV{PERL_DESTRUCT_LEVEL} = 2 unless exists $ENV{PERL_DESTRUCT_LEVEL};
 $ENV{EMXSHELL} = 'sh';        # For OS/2
 
 if ($show_elapsed_time) { require Time::HiRes }
+my %timings = (); # testname => [@et] pairs if $show_elapsed_time.
 
 my %skip = (
 	    '.' => 1,
@@ -725,12 +726,11 @@ EOT
 	}
 	else {
 	    if ($max) {
-		my $elapsed;
+		my ($elapsed, $etms) = ("", 0);
 		if ( $show_elapsed_time ) {
-		    $elapsed = sprintf( " %8.0f ms", (Time::HiRes::time() - $test_start_time) * 1000 );
-		}
-		else {
-		    $elapsed = "";
+		    $etms = (Time::HiRes::time() - $test_start_time) * 1000;
+		    $elapsed = sprintf( " %8.0f ms", $etms);
+		    $timings{$test} = $etms;
 		}
 		print "${te}ok$elapsed\n";
 		$good_files = $good_files + 1;
-- 
1.7.4.4

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2011

From @jimc

0003-if-d-HARNESS_TIMER-t-TEST-saves-timings-in-Storable-.patch
From 859d879897e65860dc420509c2ad2bc15f388d8f Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 13 Mar 2011 13:13:27 -0600
Subject: [PATCH 3/5] if -d HARNESS_TIMER, t/TEST saves timings in Storable
 file

if HARNESS_TIMER envar is an existing directory, write timings data
and various platform and configuration data to a Storable file.
Given a large collection of files, the variance of each test can be
determined.

The configuration data should be sufficient to compare different
builds done on the same box.  The platform data will hopefully allow
meaningful comparison of tests done on similar boxes, with same or
other OS, compiler, memory, etc.  Both are subject to change, for both
content and format, latter being less important because of the
normalization possible during analysis, if the data is there.

Harness output still looks the same:

t/porting/cmp_version ......................................... ok      757 ms
t/porting/diag ................................................ ok     1172 ms
t/porting/dual-life ........................................... ok       88 ms
t/porting/exec-bit ............................................ ok       86 ms
t/porting/filenames ........................................... ok      176 ms
t/porting/globvar ............................................. ok       99 ms
t/porting/maintainers ......................................... ok      501 ms
t/porting/manifest ............................................ ok      251 ms
t/porting/podcheck ............................................ ok    15013 ms
t/porting/regen ............................................... ok     1033 ms
t/porting/test_bootstrap ...................................... ok       36 ms
All tests successful.
u=11.67  s=5.07  cu=375.07  cs=84.26  scripts=2045  tests=471995
wrote storable file: ../../perf/2011-9-7-2-45.ttimes

The Storable file data looks like:

$VAR1 = {
  'conf' => {
    'byacc' => 'byacc',
    'cc' => 'cc',
    'cccdlflags' => '-fPIC',
    'ccdlflags' => '-Wl,-E',
    ...
  },
  'host' => 'groucho.jimc.earth',
  'perf' => {
    '../cpan/Archive-Extract/t/01_Archive-Extract.t' => '3960.50214767456',
    '../cpan/Archive-Tar/t/01_use.t' => '94.3360328674316',
    '../cpan/Archive-Tar/t/02_methods.t' => '737.880945205688',
    '../cpan/Archive-Tar/t/03_file.t' => '118.676900863647',
    '../cpan/Archive-Tar/t/04_resolved_issues.t' => '130.842924118042',
    ...
---
 t/TEST |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/t/TEST b/t/TEST
index 30abcf8..f8726d9 100755
--- a/t/TEST
+++ b/t/TEST
@@ -795,8 +795,26 @@ SHRDLU_5
 	}
     }
     my ($user,$sys,$cuser,$csys) = times;
-    print sprintf("u=%.2f  s=%.2f  cu=%.2f  cs=%.2f  scripts=%d  tests=%d\n",
-	$user,$sys,$cuser,$csys,$tested_files,$totmax);
+    my $tot = sprintf("u=%.2f  s=%.2f  cu=%.2f  cs=%.2f  scripts=%d  tests=%d",
+		      $user,$sys,$cuser,$csys,$tested_files,$totmax);
+    print "$tot\n";
+    if ($good_files) {
+	if (-d $show_elapsed_time) {
+	    # HARNESS_TIMER = <a-directory>.  Save timings etc to
+	    # storable file there.  NB: the test cds to ./t/, so
+	    # relative path must account for that, ie ../../perf
+	    # points to dir next to source tree.
+	    require Storable;
+	    my @dt = localtime;
+	    $dt[5] += 1900; $dt[4] += 1; # fix year, month
+	    my $fn = "$show_elapsed_time/".join('-', @dt[5,4,3,2,1]).".ttimes";
+	    Storable::store({ perf => \%timings,
+			      gather_conf_platform_info(),
+			      total => $tot,
+			    }, $fn);
+	    print "wrote storable file: $fn\n";
+	}
+    }
     if ($ENV{PERL_VALGRIND}) {
 	my $s = $valgrind == 1 ? '' : 's';
 	print "$valgrind valgrind report$s created.\n", ;
@@ -804,4 +822,35 @@ SHRDLU_5
 }
 exit ($::bad_files != 0);
 
+# Collect platform, config data that should allow comparing
+# performance data between different machines.  With enough data,
+# and/or clever statistical analysis, it should be possible to
+# determine the effect of config choices, more memory, etc
+
+sub gather_conf_platform_info {
+    # currently rather quick & dirty, and subject to change
+    # for both content and format.
+    require Config;
+    my (%conf, @platform) = ();
+    $conf{$_} = $Config::Config{$_} for
+	grep /cc|git|config_arg\d+/, keys %Config::Config;
+    if (-f '/proc/cpuinfo') {
+	open my $fh, '/proc/cpuinfo' or warn "$!: /proc/cpuinfo\n";
+	@platform = grep /name|cpu/, <$fh>;
+	chomp $_ for @platform;
+    }
+    unshift @platform, $^O;
+
+    return (
+	conf => \%conf,
+	platform => {cpu => \@platform,
+		     mem => [ grep s/\s+/ /,
+			      grep chomp, `free` ],
+		     load => [ grep chomp, `uptime` ],
+	},
+	host => (grep chomp, `hostname -f`),
+	version => '0.03', # bump for conf, platform, or data collection changes
+	);
+}
+
 # ex: set ts=8 sts=4 sw=4 noet:
-- 
1.7.4.4

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2011

From @jimc

0004-t-TEST-collect-user-sys-cpu-times-for-each-testfile.patch
From a85228a97ee9226ea436a69b217e43a67ff20415 Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 13 Mar 2011 14:13:15 -0600
Subject: [PATCH 4/5] t/TEST: collect user, sys cpu times for each testfile

In t/TEST, run times() before and after each testfile, and save diffs
into $timings{$testname}, currently containing $etms only.

When run as HARNESS_TIMER=../../perl make test, (also when HARNESS_TIMER=2 or more)
harness output now looks like this:

t/base/cond ................................................... ok        7 ms     0 ms     0 ms
t/base/if ..................................................... ok        4 ms     0 ms     0 ms
t/base/lex .................................................... ok       13 ms     0 ms     0 ms
t/base/num .................................................... ok        9 ms    10 ms     0 ms
t/base/pat .................................................... ok        4 ms     0 ms    10 ms
t/base/rs ..................................................... ok       14 ms    10 ms     0 ms
t/base/term ................................................... ok       20 ms     0 ms    10 ms
t/base/while .................................................. ok        8 ms     0 ms    10 ms
t/comp/bproto ................................................. ok        9 ms    10 ms     0 ms

The additional timing data is also written into the Storable file:

  'perf' => {
    '../cpan/Archive-Extract/t/01_Archive-Extract.t' => [
      '3916.87417030334',
      '1700',
      '2380'
    ],
    '../cpan/Archive-Tar/t/01_use.t' => [
      '92.1041965484619',
      '70.0000000000003',
      '19.9999999999996'
    ],
    ...

The numbers are: elapsed time, user-time, system-time.  The latter 2
are children-times from times(); self-times are those of the harness,
which are uninteresting.

They often dont add up (in naive sense); ET can be greater than sum of
others, especially if the process blocks on IO, or can be less than
others, if the process forks and both children are busy.  Also, child
times have 10 ms resolution on Linux, other OS or kernel build options
may vary.

Calling times() in harness will likely also collect bogus child data
if 2 testfiles are run in parallel.
---
 t/TEST |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/t/TEST b/t/TEST
index f8726d9..a691a37 100755
--- a/t/TEST
+++ b/t/TEST
@@ -524,8 +524,18 @@ EOT
     my %failed_tests;
 
     while (my $test = shift @tests) {
-        my $test_start_time = $show_elapsed_time ? Time::HiRes::time() : 0;
-
+        my ($test_start_time, @starttimes) = 0;
+	if ($show_elapsed_time) {
+	    $test_start_time = Time::HiRes::time();
+	    # times() reports usage by TEST, but we want usage of each
+	    # testprog it calls, so record accumulated times now,
+	    # subtract them out afterwards.  Ideally, we'd take times
+	    # in BEGIN/END blocks (giving better visibility of self vs
+	    # children of each testprog), but that would require some
+	    # IPC to send results back here, or a completely different
+	    # collection scheme (Storable isnt tuned for incremental use)
+	    @starttimes = times;
+	}
 	if ($test =~ /^$/) {
 	    next;
 	}
@@ -729,8 +739,14 @@ EOT
 		my ($elapsed, $etms) = ("", 0);
 		if ( $show_elapsed_time ) {
 		    $etms = (Time::HiRes::time() - $test_start_time) * 1000;
-		    $elapsed = sprintf( " %8.0f ms", $etms);
-		    $timings{$test} = $etms;
+		    $elapsed = sprintf(" %8.0f ms", $etms);
+
+		    my (@endtimes) = times;
+		    $endtimes[$_] -= $starttimes[$_] for 0..$#endtimes;
+		    splice @endtimes, 0, 2;    # drop self/harness times
+		    $_ *= 1000 for @endtimes;  # and scale to ms
+		    $timings{$test} = [$etms,@endtimes];
+		    $elapsed .= sprintf(" %5.0f ms", $_) for @endtimes;
 		}
 		print "${te}ok$elapsed\n";
 		$good_files = $good_files + 1;
-- 
1.7.4.4

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2011

From @jimc

0005-print-times-in-TestInit-END-read-them-in-t-TEST.patch
From c7e42f69bf37561d4d7f23fb2b2d37eccc772ea7 Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Thu, 4 Nov 2010 16:38:05 -0600
Subject: [PATCH 5/5] print times() in TestInit END, read them in t/TEST

TestInit.pm now calls times() from END block, and prints the values as
a diagnostic output line (# ET: ...).  t/TEST collects and saves these
lines, and may print them, depending upon $ENV{HARNESS_TIMER}.

This addresses the shortcomings of the previous approach (calling
times() from t/TEST before and after each testfile), but has its own
limitations:

  Tests in t/{base,comp,run} dont use TestInit, and thus
  dont get the per-process times().

  TestInit: END{} block must untie a tied STDOUT to avoid failures,
  which foregos this output from a small number of tests.  The END{}
  will execute in all perl children (see below) which provides more
  data, but can be confusing, and the order which they happen depends
  upon the test specifics.

To minimize disruption of existing harnesses/parsing, printing of
additional timing data depends on HARNESS_TIMER value.

HARNESS_TIMER=1 make test;
t/io/nargv ................ ok       18 ms

HARNESS_TIMER=2 make test;
t/comp/colon .............. ok       17 ms    10 ms     0 ms
t/comp/decl ............... ok        4 ms     0 ms     0 ms
t/comp/final_line_num ..... ok        5 ms     0 ms    10 ms
t/comp/fold ............... ok       10 ms    10 ms     0 ms
t/comp/form_scope ......... ok        8 ms     0 ms    10 ms

HARNESS_TIMER=3 make test;
HARNESS_TIMER=../../perf make test; # (existing directory)
t/cmd/elsif ............... ok        9 ms     0 ms     0 ms	# ET: (ms) u:0 s:0 cu:0 cs:0
t/cmd/for ................. ok       19 ms    20 ms     0 ms	# ET: (ms) u:10 s:0 cu:0 cs:0
t/cmd/mod ................. ok        5 ms     0 ms    10 ms	# ET: (ms) u:0 s:0 cu:0 cs:0
t/cmd/subval .............. ok       14 ms     0 ms     0 ms	# ET: (ms) u:0 s:0 cu:0 cs:0

As in previous patch, if HARNESS_TIMER names an existing directory,
all timing data is written to a Storable file in that directory,
allowing them to be compared against each other later.

    '../cpan/Archive-Tar/t/02_methods.t' => [
      '766.416788101196',
      '460.000000000001',
      '290.000000000001',
      '# ET: (ms) u:440 s:130 cu:10 cs:150 '
    ],
    ...

Again, note the sometimes large discrepancies in the times collected.
Its typical that the t/TEST and TestInit numbers will differ by at
least 10 ms (the timing quantization), and this applies to each of the
6 numbers collected (2, 4) from respective times() callsites.

Note also the  many reporting children in the getppid test.

t/op/glob ........... ok       68 ms    40 ms    20 ms	# ET: (ms) u:30 s:0 cu:0 cs:0
t/op/gmagic ......... ok       16 ms    10 ms     0 ms	# ET: (ms) u:0 s:0 cu:0 cs:0
t/op/goto ........... ok      115 ms    60 ms    60 ms	# ET: (ms) u:20 s:10 cu:20 cs:30

t/op/getppid ........ ok     4022 ms    20 ms     0 ms	# ET: (ms) u:0 s:0 cu:0 cs:0
  # ET: (ms) u:0 s:0 cu:0 cs:0 # ET: (ms) u:0 s:0 cu:0 cs:0 # ET: (ms) u:0 s:0 cu:0 cs:0
  # ET: (ms) u:10 s:0 cu:0 cs:0

t/porting/podcheck .. ok    14983 ms 14560 ms   200 ms	# ET: (ms) u:14510 s:190 cu:0 cs:0
t/porting/regen ..... ok     1034 ms   890 ms   120 ms	# ET: (ms) u:20 s:10 cu:860 cs:100

Lastly, t/op/magic.t includes a hack in parent process to strip
trailing # ET: ...  added to child's output, so that test passes.
---
 TestInit.pm  |   19 ++++++++++++++++++-
 t/TEST       |   23 +++++++++++++++++++----
 t/op/magic.t |    1 +
 3 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/TestInit.pm b/TestInit.pm
index 16eb318..75867c8 100644
--- a/TestInit.pm
+++ b/TestInit.pm
@@ -18,7 +18,24 @@
 
 package TestInit;
 
-$VERSION = 1.04;
+$VERSION = 1.05;
+END {
+    if ($ENV{HARNESS_TIMER}) {
+	# emit execution times so that harness can read it.
+	# Testing for -d $ENV{HARNESS_TIMER} or $ENV{HARNESS_TIMER} > 1
+	# is narrower, but this form is executed more often, exposing 
+	# any probs sooner.  Given the primary purpose of this package,
+	# narrower test may be better.
+	eval { untie *STDOUT };
+	warn "untieing STDOUT: $@" if $@;
+	# tied STDOUT caused 3~6 failures, inner references, missing
+	# methods, etc..  but we dont *need* timings for everything
+	my @t = times;
+	$_ *= 1000 for @t;
+	eval { printf "# ET: (ms) u:%d s:%d cu:%d cs:%d\n", @t;	};
+	warn "printf STDOUT: $@" if $@ and $@ !~ /object method "PRINTF"/;
+    }
+}
 
 # Let tests know they're running in the perl core.  Useful for modules
 # which live dual lives on CPAN.
diff --git a/t/TEST b/t/TEST
index a691a37..99fe209 100755
--- a/t/TEST
+++ b/t/TEST
@@ -160,6 +160,7 @@ $ENV{EMXSHELL} = 'sh';        # For OS/2
 
 if ($show_elapsed_time) { require Time::HiRes }
 my %timings = (); # testname => [@et] pairs if $show_elapsed_time.
+my %test_ET = (); # filled from "ET:..." issued by TestInit
 
 my %skip = (
 	    '.' => 1,
@@ -583,7 +584,7 @@ EOT
 	    unless (/^\#/) {
 		if ($trailing_leader) {
 		    # shouldn't be anything following a postfix 1..n
-		    $failure = 'FAILED--extra output after trailing 1..n';
+		    $failure = "FAILED--extra output ($_) after trailing 1..n";
 		    last;
 		}
 		if (/^1\.\.([0-9]+)( todo ([\d ]+))?/) {
@@ -650,6 +651,11 @@ EOT
 		    }
 		}
 	    }
+	    elsif (/# ET:/) {
+		# save ET issued by TestInit.pm, with trailing space in case there are >1
+		chomp;
+		$test_ET{$test} .= "$_ ";
+	    }
 	}
 	close $results;
 
@@ -745,8 +751,17 @@ EOT
 		    $endtimes[$_] -= $starttimes[$_] for 0..$#endtimes;
 		    splice @endtimes, 0, 2;    # drop self/harness times
 		    $_ *= 1000 for @endtimes;  # and scale to ms
-		    $timings{$test} = [$etms,@endtimes];
-		    $elapsed .= sprintf(" %5.0f ms", $_) for @endtimes;
+
+		    # append timings from TestInit.pm
+		    $timings{$test} = [$etms, @endtimes, $test_ET{$test}];
+
+		    if ($show_elapsed_time > 1 or -d $show_elapsed_time) {
+			# print TESTs times also
+			$elapsed .= sprintf(" %5.0f ms", $_) for @endtimes;
+		    }
+		    # print TestInit.pm times too, if more verbose
+		    $elapsed .= "\t$test_ET{$test}"
+			if ($show_elapsed_time > 2 or -d $show_elapsed_time);
 		}
 		print "${te}ok$elapsed\n";
 		$good_files = $good_files + 1;
@@ -823,7 +838,7 @@ SHRDLU_5
 	    require Storable;
 	    my @dt = localtime;
 	    $dt[5] += 1900; $dt[4] += 1; # fix year, month
-	    my $fn = "$show_elapsed_time/".join('-', @dt[5,4,3,2,1]).".ttimes";
+	    my $fn = "$show_elapsed_time/".join('-', @dt[5,4,3,2,1], $$).".ttimes";
 	    Storable::store({ perf => \%timings,
 			      gather_conf_platform_info(),
 			      total => $tot,
diff --git a/t/op/magic.t b/t/op/magic.t
index dd6d28e..3e14c12 100644
--- a/t/op/magic.t
+++ b/t/op/magic.t
@@ -217,6 +217,7 @@ SKIP: {
     (my $kidpid = open my $fh, "-|") // skip "cannot fork: $!", 1;
     if($kidpid) { # parent
 	my $kiddollars = <$fh>;
+	$kiddollars =~ s/\#.*//s if $ENV{HARNESS_TIMER}; # strip trailing "# ET: ..\n"
 	close $fh or die "cannot close pipe from kid proc: $!";
 	is $kiddollars, $kidpid, '$$ is reset on fork';
     }
-- 
1.7.4.4

@p5pRT
Copy link
Author

p5pRT commented Sep 8, 2011

From @cpansprout

On Wed Sep 07 16​:54​:48 2011, yoduh wrote​:

patches attached.

I see nothing wrong with the first four patches. Does anyone else have
an opinion?

I have reservations about the fifth patch. I thought that TestInit.pm
was supposed to be written in such a way that most test scripts don’t
need to take it into account. But if it has an END block, things start
to get complicated. Test authors will now have to worry about whether
they need POSIX​::_exit, whether that’s portable, etc.

@p5pRT
Copy link
Author

p5pRT commented Sep 8, 2011

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

@p5pRT
Copy link
Author

p5pRT commented Sep 9, 2011

From @jimc

On Thu, Sep 8, 2011 at 3​:40 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Wed Sep 07 16​:54​:48 2011, yoduh wrote​:

patches attached.

I see nothing wrong with the first four patches.  Does anyone else have
an opinion?

I have reservations about the fifth patch.  I thought that TestInit.pm
was supposed to be written in such a way that most test scripts don’t
need to take it into account.  But if it has an END block, things start
to get complicated.  Test authors will now have to worry about whether
they need POSIX​::_exit, whether that’s portable, etc.

it gave me a little heartburn too,
esp when I had to add untie STDOUT to get a few tests to pass.
But the END block is a noop unless HARNESS_TIMER is set,
and as comment notes, the condition could be tighter still.

Ive no objection to it sitting in a smoke-me branch for a while,
but in that case, HARNESS_TIMER should be forced to a value
that activates the END block.

The big knock against perlbench is the variance of the results,
systematic performance differences are lost in the noise.
Having lots of detailed data per run, esp if collected by default,
could provide the raw data for statistical methods to reduce the
uncertainty inherent in the noise. This doesnt defend patch 5
per se (Storable file is in patch 4), but the additional data does
get into multi-process tests, and should also work better in parallel tests.

FWIW, Ive been running this for a while with TEST_HARNESS=../../perf,
though I dont always look at the results (goal was to pool lots of timing data).

[jimc@​groucho perl]$ ls ../perf |wc
  569 569 15643

If the patchset gets traction,
I'll look to add a bit more data to it​:
- rc & fails to each test-file record
- test-command;
  make test vs make test.valgrind produce dramatically different numbers
  and theres nothing in the Storable file to distinguish them.
- track subprocesses more closely (this requires patch 5)
  add ($$, $0, @​ARGV) to '# ET (ms) ..." message
  use that to parse and store more data
  timingdetail => {
  $$ => { cmd => [$0, @​ARGV],
  u => $usr, s => $sys, cu=> $childusr, cs => $childsys }}

thanks
~jimc

@p5pRT
Copy link
Author

p5pRT commented Sep 10, 2011

From @cpansprout

On Fri Sep 09 10​:41​:35 2011, yoduh wrote​:

On Thu, Sep 8, 2011 at 3​:40 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Wed Sep 07 16​:54​:48 2011, yoduh wrote​:

patches attached.

I see nothing wrong with the first four patches.  Does anyone else
have
an opinion?

I have reservations about the fifth patch.  I thought that
TestInit.pm
was supposed to be written in such a way that most test scripts
don’t
need to take it into account.  But if it has an END block, things
start
to get complicated.  Test authors will now have to worry about
whether
they need POSIX​::_exit, whether that’s portable, etc.

it gave me a little heartburn too,
esp when I had to add untie STDOUT to get a few tests to pass.
But the END block is a noop unless HARNESS_TIMER is set,
and as comment notes, the condition could be tighter still.

Ive no objection to it sitting in a smoke-me branch for a while,
but in that case, HARNESS_TIMER should be forced to a value
that activates the END block.

I’ve applied the first four patches. Thank you. Their commit IDs are​:

f7b9b04
b49055e
8e03ad8
25a2b27

The fifth patch I’m going to leave for someone who understands the test
infrastructure a lot better. (Nick, if you are reading this, could you
please have a look? :-)

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2011

From @nwc10

On Thu, Sep 08, 2011 at 02​:40​:12PM -0700, Father Chrysostomos via RT wrote​:

On Wed Sep 07 16​:54​:48 2011, yoduh wrote​:

patches attached.

Gah. I don't like the interaction of Jesse's desire for patches to be sent
to RT, with the usual solution of attaching patches *to* RT. It makes them
a pain to review because

a​: they are now (at least) a click away. (or more, if my web browser insists
  that I should download them and fire up a text editor to look at them)
  (Or infinitely far away if I have an offline mailer, and I'm on a $DHH
  plane)
b​: it's not trivial to comment on them in an e-mail message, which is the
  workflow round here. Or at least, has been

I see nothing wrong with the first four patches. Does anyone else have
an opinion?

I'm troubled by the unconditional use of

VALGRIND ?= valgrind
VG_TEST ?= ./perl -e 1 2>/dev/null

in Makefile, as it is (I think) a GNU make-ism. But nothing seems to have
choked on it so far.

[I think valgrind only works well on platforms that have gmake as standard
(OS X and Linux), so possibly Makefile.SH should write this out
conditionally based on which make we have.]

I have reservations about the fifth patch. I thought that TestInit.pm
was supposed to be written in such a way that most test scripts don't
need to take it into account. But if it has an END block, things start
to get complicated. Test authors will now have to worry about whether
they need POSIX​::_exit, whether that's portable, etc.

That's a very good point. The way it's written, it looks to be sane, safe
and clean, but if we keep adding functionality to a module that's supposed
to "not be there", we keep increasing the risk of some side effect somehow
getting something "wrong", tearing the invisibility cloak.

Given that all this code is controlled by t/TEST, which is also controlling
the command line passed to the test, I think it would equally possible to
implement the END injection in its own module. That way it would only added
to the test's command line with -M by t/TEST, conclusively isolating it as
a cause of any problems in the general case. That also means that TestInit
continues only to be about "Init". Which I think I find pleasing.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2011

From @nwc10

On Fri, Sep 09, 2011 at 11​:40​:30AM -0600, Jim Cromie wrote​:

The big knock against perlbench is the variance of the results,
systematic performance differences are lost in the noise.
Having lots of detailed data per run, esp if collected by default,
could provide the raw data for statistical methods to reduce the
uncertainty inherent in the noise. This doesnt defend patch 5

Doesn't that reasoning for "lots of data and then stats" also apply to
perlbench - run it many times and then use statistical methods?

Whilst it's potentially useful to know if the regression tests start
taking significantly different time, I'm still not convinced that they
make a good benchmark suite. They serve different purposes​:

regression tests

* try to test obscure corner cases
* focus on one thing in isolation
* should run as quickly as possible, to avoid programmers getting bored or
  complacent
* often end up being dominated by startup time

benchmarks should

* focus on common code
* perform complex behaviour using multiple features
* should stress things with the scale of data needed to detect real problems
  [such as a change to O(bad) behaviour where previously it was O(acceptable)]
* unless benchmarking startup time, strive to avoid it influencing the result

and I don't think it's useful to try to make the *regression* tests pretend
to be a benchmark.

I've not looked at it yet, but I'm hoping that Steffen Schwingon's work
on Benchmark​::Perl​::Formance is going to produce a more comprehensive
benchmark than perlbench​:

https://metacpan.org/module/Benchmark::Perl::Formance

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2011

From @nwc10

On Fri, Sep 09, 2011 at 09​:42​:08PM -0700, Father Chrysostomos via RT wrote​:

infrastructure a lot better. (Nick, if you are reading this, could you
please have a look? :-)

I am now. I had a backlog. I feel troubled that the backlog grows faster
than I can deal with it, unless I stop writing code and just deal with
e-mail full time, answering questions.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2011

From @nwc10

On Wed, Sep 14, 2011 at 04​:56​:27PM +0100, Nicholas Clark wrote​:

On Thu, Sep 08, 2011 at 02​:40​:12PM -0700, Father Chrysostomos via RT wrote​:

I see nothing wrong with the first four patches. Does anyone else have
an opinion?

I'm troubled by the unconditional use of

Actually, I do have an opinion on the fourth.

Why are we adding this to t/TEST, which is *supposed* to be the fail-safe
way to run the test suite, and keep working even when sophisticated stuff
is broken.

Surely it should be in t/harness? And doesn't some of it start to duplicate
what TAP​::Harness already does, such as recording test timings in
t/test_state

I don't like the idea of adding functionality to something core only,
when it feels like such functionality could be in a CPAN module, and
hence available to all.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2011

From @ap

* Nicholas Clark <nick@​ccl4.org> [2011-09-14 18​:00]​:

a​: they are now (at least) a click away. (or more, if my web
browser insists that I should download them and fire up
a text editor to look at them)

You can reduce it to just a second click, at least​:
https://addons.mozilla.org/en-US/firefox/addon/open-in-browser/
http​://spasche.net/openinbrowser/

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2011

From @nwc10

On Wed, Sep 14, 2011 at 06​:39​:51PM +0200, Aristotle Pagaltzis wrote​:

* Nicholas Clark <nick@​ccl4.org> [2011-09-14 18​:00]​:

a​: they are now (at least) a click away. (or more, if my web
browser insists that I should download them and fire up
a text editor to look at them)

You can reduce it to just a second click, at least​:
https://addons.mozilla.org/en-US/firefox/addon/open-in-browser/
http​://spasche.net/openinbrowser/

I have some addon in Firefox to do this already. I don't know how to force
it in Chrome, which was what I initially used to look at RT.

[And in the general case this would be more useful, as Chrome has a built-in
PDF viewer. Firefox only does text, HTML and images]

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2011

From @cpansprout

On Wed Sep 14 09​:27​:09 2011, nicholas wrote​:

On Wed, Sep 14, 2011 at 04​:56​:27PM +0100, Nicholas Clark wrote​:

On Thu, Sep 08, 2011 at 02​:40​:12PM -0700, Father Chrysostomos via RT
wrote​:

I see nothing wrong with the first four patches. Does anyone else
have
an opinion?

I'm troubled by the unconditional use of

Actually, I do have an opinion on the fourth.

Why are we adding this to t/TEST, which is *supposed* to be the fail-
safe
way to run the test suite, and keep working even when sophisticated
stuff
is broken.

Surely it should be in t/harness? And doesn't some of it start to
duplicate
what TAP​::Harness already does, such as recording test timings in
t/test_state

I don't like the idea of adding functionality to something core only,
when it feels like such functionality could be in a CPAN module, and
hence available to all.

Nicholas Clark

Since this is not really an area of perl I feel comfortable about making
decisions on, have I leave the rest of this ticket to you? (There are
other outstanding patches.)

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2011

From @cpansprout

On Wed Sep 14 13​:32​:55 2011, sprout wrote​:

On Wed Sep 14 09​:27​:09 2011, nicholas wrote​:

Nicholas Clark

Since this is not really an area of perl I feel comfortable about making
decisions on, have I leave the rest of this ticket to you? (There are
other outstanding patches.)

s/have I leave/may I leave/

@p5pRT
Copy link
Author

p5pRT commented Sep 16, 2011

From @jimc

I have reservations about the fifth patch.  I thought that TestInit.pm
was supposed to be written in such a way that most test scripts don't
need to take it into account.  But if it has an END block, things start
to get complicated.  Test authors will now have to worry about whether
they need POSIX​::_exit, whether that's portable, etc.

That's a very good point. The way it's written, it looks to be sane, safe
and clean, but if we keep adding functionality to a module that's supposed
to "not be there", we keep increasing the risk of some side effect somehow
getting something "wrong", tearing the invisibility cloak.

Given that all this code is controlled by t/TEST, which is also controlling
the command line passed to the test, I think it would equally possible to
implement the END injection in its own module. That way it would only added
to the test's command line with -M by t/TEST, conclusively isolating it as
a cause of any problems in the general case. That also means that TestInit
continues only to be about "Init". Which I think I find pleasing.

Nicholas Clark

OK. on a 2nd reading, I think the light went on.

under some modes t/TEST will issue each subtest with
$tperl -MTimingHarness​:@​args $file_to_test

possibly triggered by
HARNESS_TIMER="-MTimingHarness​:@​args"

t/TEST will still have to do some work itself,
like gathering config,platform,etc, but that can also be
defined in the TimingHarness.pm, and called
from t/TEST with a single statement.

pre_DUT​: start unit timing,
post_DUT​: collect timing, etc, write to Sqllite db-file

test_run_start​: starts overall elapsed time, setup DB
test_run_end​: gathers config, platform info and test-state

This looks to be easier to add to t/harness too.

I'll try this out..
thanks.

@p5pRT
Copy link
Author

p5pRT commented Sep 16, 2011

From @jimc

On Wed, Sep 14, 2011 at 9​:56 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

On Thu, Sep 08, 2011 at 02​:40​:12PM -0700, Father Chrysostomos via RT wrote​:

I see nothing wrong with the first four patches.  Does anyone else have
an opinion?

I'm troubled by the unconditional use of

VALGRIND ?= valgrind
VG_TEST  ?= ./perl -e 1 2>/dev/null

in Makefile, as it is (I think) a GNU make-ism. But nothing seems to have
choked on it so far.

So. VMS chokes on '?=' (sorry Craig)
Anyone know a portable form ?

[I think valgrind only works well on platforms that have gmake as standard
(OS X and Linux), so possibly Makefile.SH should write this out
conditionally based on which make we have.]

I'll do this, if nobody beats me to it.

Rather than checking platform, is it enough to check for
the valgrind executable ? Its a pretty good proxy for a devbox. Its
not perfect for a test.perf target (OS X wont have it), but works on
linux - and if perf isnt installed, make test.perf will clearly say
so.

@p5pRT
Copy link
Author

p5pRT commented Sep 16, 2011

From @craigberry

On Fri, Sep 16, 2011 at 3​:01 PM, Jim Cromie <jim.cromie@​gmail.com> wrote​:

On Wed, Sep 14, 2011 at 9​:56 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

On Thu, Sep 08, 2011 at 02​:40​:12PM -0700, Father Chrysostomos via RT wrote​:

I see nothing wrong with the first four patches.  Does anyone else have
an opinion?

I'm troubled by the unconditional use of

VALGRIND ?= valgrind
VG_TEST  ?= ./perl -e 1 2>/dev/null

in Makefile, as it is (I think) a GNU make-ism. But nothing seems to have
choked on it so far.

So. VMS chokes on  '?='  (sorry Craig)
Anyone know a portable form ?

I think he meant make that is not GNU make. VMS does not use
Makefile.SH nor is it currently viable to build with any flavor of
make. Nor does valgrind exist that I am aware of, though there is a
native heap analyzer.

@p5pRT
Copy link
Author

p5pRT commented Sep 16, 2011

From @jimc

On Fri, Sep 16, 2011 at 2​:25 PM, Craig A. Berry <craig.a.berry@​gmail.com> wrote​:

On Fri, Sep 16, 2011 at 3​:01 PM, Jim Cromie <jim.cromie@​gmail.com> wrote​:

On Wed, Sep 14, 2011 at 9​:56 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

On Thu, Sep 08, 2011 at 02​:40​:12PM -0700, Father Chrysostomos via RT wrote​:

I see nothing wrong with the first four patches.  Does anyone else have
an opinion?

I'm troubled by the unconditional use of

VALGRIND ?= valgrind
VG_TEST  ?= ./perl -e 1 2>/dev/null

in Makefile, as it is (I think) a GNU make-ism. But nothing seems to have
choked on it so far.

So. VMS chokes on  '?='  (sorry Craig)
Anyone know a portable form ?

I think he meant make that is not GNU make.

agreed. What form of if-then works in your make ?
Maybe its a portable form.

VMS does not use
Makefile.SH nor is it currently viable to build with any flavor of
make.

So VMS uses pre-packaged Makefile,
and you hand edited out the "?=" ?

 Nor does valgrind exist that I am aware of, though there is a
native heap analyzer.

Hmm. Since Makefile.SH is unused on VMS,
it cant skip writing the linux related targets.
maybe tailoring Makefile.SH output per platform isnt worth it.

@p5pRT
Copy link
Author

p5pRT commented Sep 16, 2011

From @craigberry

On Fri, Sep 16, 2011 at 3​:44 PM, Jim Cromie <jim.cromie@​gmail.com> wrote​:

On Fri, Sep 16, 2011 at 2​:25 PM, Craig A. Berry <craig.a.berry@​gmail.com> wrote​:

On Fri, Sep 16, 2011 at 3​:01 PM, Jim Cromie <jim.cromie@​gmail.com> wrote​:

On Wed, Sep 14, 2011 at 9​:56 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

On Thu, Sep 08, 2011 at 02​:40​:12PM -0700, Father Chrysostomos via RT wrote​:

I see nothing wrong with the first four patches.  Does anyone else have
an opinion?

I'm troubled by the unconditional use of

VALGRIND ?= valgrind
VG_TEST  ?= ./perl -e 1 2>/dev/null

in Makefile, as it is (I think) a GNU make-ism. But nothing seems to have
choked on it so far.

So. VMS chokes on  '?='  (sorry Craig)
Anyone know a portable form ?

I think he meant make that is not GNU make.

agreed.  What form of if-then works in your make ?
Maybe its a portable form.

.IF "$(FOO)" .EQ "BAR"
  BAZ = $(FOO)
.ELSE
  BAZ =
.ENDIF

but that's unlikely to be relevant to what you're interested in doing.

VMS does not use
Makefile.SH nor is it currently viable to build with any flavor of
make.

So VMS uses pre-packaged Makefile,
and you hand edited out the "?=" ?

The VMS port uses an independent configuration and semi-independent
build system. The key files are configure.com and
vms/descrip_mms.template, the latter of which gets processed by the
former into a description file called descrip.mms, which is very
similar to a Makefile but is not exactly the same thing. It is then
processed by HP's MMS (Module Management System) or the freeware
equivalent MMK, which are make-like tools but are not make.

That's more than you wanted to know, but the point is you can't hurt
the VMS build by mangling Makefile.SH. I think the portability
concern Nicholas raised has entirely to do with various unixen that
have their own, non-GNU make.

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2011

From @nwc10

On Wed, Sep 14, 2011 at 01​:32​:56PM -0700, Father Chrysostomos via RT wrote​:

Since this is not really an area of perl I feel comfortable about making
decisions on, have I leave the rest of this ticket to you? (There are
other outstanding patches.)

I thought that only the fifth patch was outstanding, and I'd commented on
it, that there was probably a better approach.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2011

From @nwc10

On Fri, Sep 16, 2011 at 04​:27​:03PM -0500, Craig A. Berry wrote​:

On Fri, Sep 16, 2011 at 3​:44 PM, Jim Cromie <jim.cromie@​gmail.com> wrote​:

On Fri, Sep 16, 2011 at 2​:25 PM, Craig A. Berry <craig.a.berry@​gmail.com> wrote​:

On Fri, Sep 16, 2011 at 3​:01 PM, Jim Cromie <jim.cromie@​gmail.com> wrote​:

On Wed, Sep 14, 2011 at 9​:56 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

On Thu, Sep 08, 2011 at 02​:40​:12PM -0700, Father Chrysostomos via RT wrote​:

I see nothing wrong with the first four patches.  Does anyone else have
an opinion?

I'm troubled by the unconditional use of

VALGRIND ?= valgrind
VG_TEST  ?= ./perl -e 1 2>/dev/null

in Makefile, as it is (I think) a GNU make-ism. But nothing seems to have
choked on it so far.

So. VMS chokes on  '?='  (sorry Craig)
Anyone know a portable form ?

I think he meant make that is not GNU make.

agreed.  What form of if-then works in your make ?
Maybe its a portable form.

.IF "$(FOO)" .EQ "BAR"
BAZ = $(FOO)
.ELSE
BAZ =
.ENDIF

but that's unlikely to be relevant to what you're interested in doing.

I don't think that there *is* a portable form.

That's more than you wanted to know, but the point is you can't hurt
the VMS build by mangling Makefile.SH. I think the portability
concern Nicholas raised has entirely to do with various unixen that
have their own, non-GNU make.

ie most of them. :-)

I think that only Linux, OS X (and I guess Hurd, if anyone uses that))
have a "make" which is actually GNU make. It seems that the *BSD
makes (at least current-ish FreeBSD, NetBSD and OpenBSD) have ?=

Not sure how many OSes have it as an option in some way, but we can't rely
on it being there.

Which seems to boil down to 3 solutions​:

1​: Change Makefile.SH Write a Makefile with ?= on Linux and OS X, and with
  = everywhere else. (Which really doesn't make any difference, because
  valgrind only really works on Linux and OS X
2​: Just write = everywhere, and document that this form of override works​:

  make -j20 test.valgrind VALGRIND=/bin/false

  but using environment variables doesn't​:

  VALGRIND=/bin/false make test.valgrind

3​: Redo the duplicates for $(VALGRIND) and $(VG_TEST) everywhere they are used.
  ie expand an empty string using shell conditionals in the relevant
  Makefile rules, and in t/TEST.
  [actually, that one isn't *evil* as we already have this in t/TEST​:
  my $valgrind_exe = $ENV{VALGRIND} // 'valgrind';

  and it's simpler than I first thought if we rule that one must override
  VALGRIND if one overrides VG_TEST]

Nicholas Clark
 

@p5pRT
Copy link
Author

p5pRT commented Nov 27, 2011

From @cpansprout

On Tue Sep 20 07​:17​:08 2011, nicholas wrote​:

On Wed, Sep 14, 2011 at 01​:32​:56PM -0700, Father Chrysostomos via RT
wrote​:

Since this is not really an area of perl I feel comfortable about making
decisions on, have I leave the rest of this ticket to you? (There are
other outstanding patches.)

I thought that only the fifth patch was outstanding, and I'd commented on
it, that there was probably a better approach.

But you had some qualms about the fourth. I thought you might want to
modify or revert it.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2014

From @jkeenan

On Sat Nov 26 19​:42​:52 2011, sprout wrote​:

On Tue Sep 20 07​:17​:08 2011, nicholas wrote​:

On Wed, Sep 14, 2011 at 01​:32​:56PM -0700, Father Chrysostomos via RT
wrote​:

Since this is not really an area of perl I feel comfortable about making
decisions on, have I leave the rest of this ticket to you? (There are
other outstanding patches.)

I thought that only the fifth patch was outstanding, and I'd commented on
it, that there was probably a better approach.

But you had some qualms about the fourth. I thought you might want to
modify or revert it.

There has been no discussion in this ticket in nearly three years.

If I read the ticket correctly, 4 of the OP's original patches were applied; the fifth was more controversial and was never resolved.

I recommend that we close this ticket and ask anyone who wants to continue the discussion to submit a new RT.

Thank you very much.

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

@p5pRT
Copy link
Author

p5pRT commented Nov 9, 2014

From @Leont

On Sun, Oct 19, 2014 at 2​:48 AM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

There has been no discussion in this ticket in nearly three years.

If I read the ticket correctly, 4 of the OP's original patches were
applied; the fifth was more controversial and was never resolved.

I recommend that we close this ticket and ask anyone who wants to continue
the discussion to submit a new RT.

Thank you very much.

Actually, Jarkko recently supplied a patch to achieve just this, and very
recently released it as Test​::Harness 3.34 :-)

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2016

From @mauke

On Sun Nov 09 12​:55​:00 2014, LeonT wrote​:

On Sun, Oct 19, 2014 at 2​:48 AM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

There has been no discussion in this ticket in nearly three years.

If I read the ticket correctly, 4 of the OP's original patches were
applied; the fifth was more controversial and was never resolved.

I recommend that we close this ticket and ask anyone who wants to continue
the discussion to submit a new RT.

Thank you very much.

Actually, Jarkko recently supplied a patch to achieve just this, and very
recently released it as Test​::Harness 3.34 :-)

Leon

Does this mean we can close this ticket now?

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2016

From @Leont

On Mon, Feb 22, 2016 at 10​:46 PM, l.mai@​web.de via RT <
perlbug-followup@​perl.org> wrote​:

On Sun Nov 09 12​:55​:00 2014, LeonT wrote​:

On Sun, Oct 19, 2014 at 2​:48 AM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

There has been no discussion in this ticket in nearly three years.

If I read the ticket correctly, 4 of the OP's original patches were
applied; the fifth was more controversial and was never resolved.

I recommend that we close this ticket and ask anyone who wants to
continue
the discussion to submit a new RT.

Thank you very much.

Actually, Jarkko recently supplied a patch to achieve just this, and very
recently released it as Test​::Harness 3.34 :-)

Leon

Does this mean we can close this ticket now?

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=98662

Yes

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2016

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

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

No branches or pull requests

1 participant