[perl #41020] [PATCH] pmc2c.pl functionality extracted into separate package
James Keenan via RT
>
> Problem confirmed. I am working on it. I think I have misunderstood
> how to use
> Getopt::Long::GetOptions().
>
I fixed the problem with GetOptions() -- but there are additional problems that will take time to
diagnose. So my patches are not yet ready for prime time.
kid51
James Keenan via RT
patch submission for tools/build/pmc2c.pl.
BUT ... today it began to dawn on me that I have seriously misunderstood some of the
problems we face in testing Parrot's build tools and other parts of the distribution.
In retrospect, I'm not surprised that problems with GetOptions() would cause my version of
pmc2c.pl to fail. As I explained in an earlier post, my objective was to take as much of the
functionality in the current pmc2c.pl out and place it in a module, Parrot::Pmc2c::Utils,
thereby making that functionality more amenable to testing and coverage analysis. And my
test suite reflected that. But the one bit of functionality that I could *not* extract from
pmc2c.pl was the options processing. And that's where the problem lay.
But not the *only* problem. After I fixed the options processing, I was still having 'make'
failures with my version of pmc2c.pl. Worse, most of the seven files in my test suite were
starting to fail as well -- when previously they had been passing with flying colors!
That was Thursday. Then last night my tests started to pass again -- even though I had
made no further changes to pmc2c.pl and no significant changes to the tests and none at all
to Parrot::Pmc2c::Utils. Ever more surprising was that at least once, when I edited Makefile to
substitute 'mypmc2c.pl' for 'pmc2c.pl', 'make' started to succeed. I excitedly asked Coke,
with whom I was talking on #parrot, to double-check. I did a 'make clean' to start fresh and
reconfirm the good news -- only to have 'make' once again no longer succeed and, worse
yet, *all* my test files start to fail.
It was at that point that I began to realize that by going through the cycle of 'make clean',
'Configure.pl', 'make' and 'make test' interspersed with runs of my test suite, I was *changing
the environment in which my tests were running*. And the major determinant of whether my
tests and pmc2c.pl would pass or not was the stage in the cycle at which I was running the
tests.
From Nov 11 until last night, I had not rebuilt Parrot on my machine (though I had done
frequent 'svn update's). So all during that time my tests for functions such as find_file() and
read_dump() were passing because they were finding files which had been created by calling
'make' on Nov 11. But once I called 'make clean', many of those files were no longer there,
so the tests of the Utils.pm functions which looked for them necessarily failed. When I called
Configure.pl and make once again, those files were regenerated and the tests of the Utils.pm
functions which look for them began to pass anew.
This was most startlingly evident at a point where I called 'make clean', then called 'prove t/
tools/pmc2cutils/*.t' and *all* my tests began to fail with a message indicating that package
Parrot::PMC could not be located. It turned out that this was a correct failure, because
Parrot::PMC is *not* included in the svn distribution; it's automatically generated by the
operation of Configure.pl -- which I had not yet called! Once I called Configure.PL, that
package got created, which meant that Parrot::Pmc2c could 'use' it successfully, which in turn
meant that Parrot::Pmc2c::Utils could load as well. But many of my tests continued to fail,
and now I understood why: because I had only called Configure.PL but had not yet called
make, many of the files which my tests were searching for did not yet exist!
In most of the code I've written, either for CPAN or $job(s), I've taken pains to make sure that
any files created during, and solely for, the testing process are not left on the user's machine
after testing and installation. And most of the test suites I've written make no assumptions
about the presence or absence of files on the user's machine at the onset of testing. A well
written test suite should give idempotent results.
This approach does not suffice in this case. The functionality in pmc2c.pl (whether in the
script itself or extracted to a separate package) is strictly dependent on the point in the
'make' process where it is called. Call the subroutines embodying that functionality too early
(i.e., before calling Configure.pl or before calling make) and you will necessarily get failures.
But call them, in a sense, too late (once you have already successfully called make) and you
get spurious successes, because then you're just finding or re-creating files which already
exist. The conditions prevailing at different points militate against idempotent results.
Tests of Parrot's build tools are not tests of Parrot itself. Once Parrot's make call has
succeeded, testing the components of the build tools is irrelevant. Hence, including tests of
the build tools in the suite of tests called by 'make test' is also irrelevant. The build tools can
only be validly tested in a 'pre-make' environment. But that raises the question: How far
'pre-make': a pre-Configure.pl environment, or a post-Configure.pl, pre-make environment
(or some combination of the two)?
Which boils down to a more fundamental question: What exactly are we trying to accomplish
via testing of Parrot's build tools?
Ladies and gentlemen, the floor is open for discussion. Thank you for reading this far.
kid51
James Keenan via RT
>
> Tests of Parrot's build tools are not tests of Parrot itself. Once
> Parrot's make call has
> succeeded, testing the components of the build tools is irrelevant.
> Hence, including tests of
> the build tools in the suite of tests called by 'make test' is also
> irrelevant. The build tools can
> only be validly tested in a 'pre-make' environment. But that raises
> the question: How far
> 'pre-make': a pre-Configure.pl environment, or a post-Configure.pl,
> pre-make environment
> (or some combination of the two)?
>
> Which boils down to a more fundamental question: What exactly are we
> trying to accomplish
> via testing of Parrot's build tools?
>
Continuing my thinking-out-loud on this subject: Perhaps what we need is a 'pre-make' test
suite whose tests a developer (but not a regular user) would run before calling 'make'.
The purpose of these tests would be to answer the question, "Are the tools we are about to
use to build the Parrot installation capable of doing so?" The tests would test the most
important features of the various Perl scripts invoked during 'make' without doing a real
build. Any files created during these tests would be created in self-eliminating temporary
directories.
The test suite would begin by doing a spot check of directories and files to see that files that
ought to be present after running Configure.pl (e.g., Makefile) *are* present but that files
that are only created during 'make' (e.g. src/pmc/*.c) *are not* present at that time.
Comments? Thanks.
kid51
Chromatic
A few style comments here.
> Utils.pm
> unshift @{$allargsref->{include}}, (
> ".",
> "$FindBin::Bin/../..",
> "$FindBin::Bin/../../src/pmc/"
> );
Why no File::Spec here?
> if (File::Spec->file_name_is_absolute($file) && -e $file) {
> return $file;
> }
There's File::Spec here.
> =head3 C<dump_vtable()>
>
> $self->dump_vtable("$FindBin::Bin/../../vtable.tbl");
... but not here (this is documentation).
> sub dump_vtable {
> my $self = shift;
> my $file = shift;
Why two shifts here, when @_ goes unused through the rest of the method?
> sub print_tree {
> my $self = shift;
> my $argsref = shift;
Ditto here.
> sub find_and_parse_pmc {
> my ($self, $file) = @_;
... but not here.
> sub gen_parent_list {
> my $self = shift;
> my ($name, $all) = @_;
This one just confuses me.
-- c
James E Keenan
> On Monday 25 December 2006 21:44, James Keenan via RT wrote:
>
> A few style comments here.
>
>
>>Utils.pm
>> unshift @{$allargsref->{include}}, (
>> ".",
>> "$FindBin::Bin/../..",
>> "$FindBin::Bin/../../src/pmc/"
>> );
>
>
> Why no File::Spec here?
There are certain ways in which maintenance programming is like open
heart surgery. "First, do no harm" applies with a vengeance. You have
to make sure you don't kill the patient (translation: cause 'make' to
fail), then accomplish your stated objective -- and only then can you
consider fixing what's not fatally broken.
I agree that all that $FindBin::Bin stuff is ugly, but it appears to
have been needed because 'make' changes its working directory a couple
of times and calls pmc2c.pl from at least two different directories. It
was pure hell trying to use it when constructing tests, since my tests
live at a level (t/tools/pmc2cutils) one level deeper than pmc2c.pl.
This meant that I would be forced to decipher error messages with
*three* sets of '../' in them.
In the test suite I deal with this in two ways. In 00-qualify_.t (a
file which is intended to ensure that all the other files can plausibly
work), I have this code:
use Test::More tests => 8;
use FindBin;
use lib (
"$FindBin::Bin/../..",
"$FindBin::Bin/../../lib",
"$FindBin::Bin/../../../lib",
);
use_ok( 'Parrot::Pmc2c::Utils' );
ok(-f "$FindBin::Bin/../../../Makefile", "Makefile located");
ok(-f "$FindBin::Bin/../../../myconfig", "myconfig located");
ok(-f "$FindBin::Bin/../../../lib/Parrot/PMC.pm", "lib/Parrot/PMC.pm
located");
And in all the other test files I have this BEGIN block:
BEGIN {
use FindBin qw($Bin);
use Cwd qw(cwd realpath);
realpath($Bin) =~ m{^(.*\/parrot)\/[^/]*\/[^/]*\/[^/]*$};
$topdir = $1;
if (defined $topdir) {
print "\nOK: Parrot top directory located\n";
} else {
die "Unable to locate top-level Parrot directory";
}
unshift @INC, qq{$topdir/lib};
}
Once I've applied Cwd::realpath() to $Bin, I get a value for $topdir in
which all those ugly '../' segments are cleaned up, which meant my error
messages began to approach intelligibility. The price was that in the
balance of the test files I had to call $main::topdir.
>
> [snip]
>
>> if (File::Spec->file_name_is_absolute($file) && -e $file) {
>> return $file;
>> }
>
>
> There's File::Spec here.
>
>
>>=head3 C<dump_vtable()>
>>
>> $self->dump_vtable("$FindBin::Bin/../../vtable.tbl");
>
>
> ... but not here (this is documentation).
>
>
>>sub dump_vtable {
>> my $self = shift;
>> my $file = shift;
>
>
> Why two shifts here, when @_ goes unused through the rest of the method?
>
>
>>sub print_tree {
>> my $self = shift;
>> my $argsref = shift;
>
>
> Ditto here.
>
>
>>sub find_and_parse_pmc {
>> my ($self, $file) = @_;
>
>
> ... but not here.
>
>
I'll look into the above.
>>sub gen_parent_list {
>> my $self = shift;
>> my ($name, $all) = @_;
>
>
> This one just confuses me.
>
Confuses me too. Most of the real action in pmc2c.pl takes place in
subroutines called several levels down inside dump_pmc(). The farther
down you go (a) the less I understand what's going on; (b) the more
difficult is it for me to write tests for what's going on (especially
tests that *only* call an internal subroutine rather than implicitly
testing it via testing its caller); and hence (c) the greater the
likelihood that Devel::Cover is going to report uncovered code. (You
can view coverage reports at:
http://thenceforward.net/parrot/cover_db/coverage.html).
In the case of gen_parent_list(), I never encountered a problem until
9:45 PM last night when -- with all my tests passing -- I went to 'make'
(which had succeeded several times with my new code) and it failed when
it tried to build 'null.dump'. Took me three hours to debug.
Some of the confusion you see arises from the fact that in the current
pmc2c.pl, all of the above are subroutines inside a script. In my
version, pmc2c.pl builds a Parrot::Pmc2c::Utils object and then calls
methods on the object as directed by processing of command-line options.
So much of my early refactoring consisted of eliminating variables
passed to subroutines in favor of extracting that information from the
data structure inside the object. Some methods rely solely on data from
the object itself, while others (including a post-make diagnostic method
like print_tree()) require additional arguments. Other subroutines are
still pure subroutines.
Fertile ground for future refactoring, eh?
kid51
James E Keenan
> On Sun Dec 03 14:08:31 2006, jk...@verizon.net wrote:
>
>>
>
> The files attach refactor tools/build/pmc2c.pl, extracting most of its functionality into lib/
> Parrot/Pmc2c/Utils.pm and provide 8 test files to be stored in t/tools/pmc2cutils/. These
> substitute for my files originally submitted on (I think) Nov 30. That version of pmc2c.pl and
> Utils.pm failed to 'make'; these succeed (at least for me).
>
Since these tests are meant to be run on the assumption that 'make' has
not yet run, I have found that a good way to run them is this:
$ make realclean
$ svn update
$ perl Configure.pl (# or a shell script that calls Configure.pl)
$ mkdir t/tools/pmc2cutils
$ mv *.t t/tools/pmc2cutils/
$ mv Utils.pm lib/Parrot/Pmc2c/
$ mv tools/build/pmc2c.pl tools/build/oldpmc2c.pl
$ mv pmc2c.pl tools/build/mypmc2c.pl
$ ln -s tools/build/mypmc2c.pl pmc2c.pl
(I submitted a patch on pmc2c.pl last night, but you can pick up my new
version at http://thenceforward.net/parrot/tools/build/pmc2c.pl.)
You have now stashed the current version of pmc2c.pl as oldpmc2c.pl.
pmc2c.pl now links to mypmc2c.pl -- which means that you don't have to
futz with the Makefile.
At this point, call:
$ prove -v t/tools/pmc2cutils/*.t
Alternatively, you could edit the Makefile to include this:
PROVE = /usr/local/bin/prove -v
PMC2CUTILS_DIR = t/tools/pmc2cutils
testbuildtools :
$(PROVE) \
$(PMC2CUTILS_DIR)/00-qualify.t \
$(PMC2CUTILS_DIR)/01-pmc2cutils.t \
$(PMC2CUTILS_DIR)/02-find_file.t \
$(PMC2CUTILS_DIR)/03-dump_vtable.t \
$(PMC2CUTILS_DIR)/04-dump_pmc.t \
$(PMC2CUTILS_DIR)/05-gen_c.t \
$(PMC2CUTILS_DIR)/06-print_tree.t \
$(PMC2CUTILS_DIR)/07-open_file.t
... anywhere after the declaration of the 'all' target. (This is
available at http://thenceforward.net/parrot/my_makefile_additions).
Then call:
$ make testbuildtools
I'd eventually like to have Configure.pl build this into the Makefile,
but I don't know how to do that myself.
Feedback encouraged.
kid51
James E Keenan
James Keenan
On Dec 30, 2006, at 1:06 AM, Kevin Tew via RT wrote:
> I modified the root.in changes to follow the conventions already
> present
> in the file.
>
> The following composite patch builds and passes parrot tests.
> However the pmc2cutil tests are not passing. Could you post a new
> diff
> that includes passing pmc2cutil tests?
>
Can you send me some output with test failures? They've passed
repeatedly on my machine.
Please note that the pmc2cutil tests should only be run at a point
*after* you've called perl Configure.pl but *before* you've called
'make'. That's because if you've already run 'make', you've created
some of the files whose creatability is the thing to be tested by the
pmc2cutil/*.t tests. If those files already exist on your disk,
rather than give you false positives, some of the tests are
deliberately designed to fail.
I was getting burned by false positives for three weeks before I
realized this. But before I conclude that this is your problem, I'll
need to see some output. Thank you very much.
kid51
Will Coleda
--
Will "Coke" Coleda
wi...@coleda.com
chromatic
> Can you send me some output with test failures? They've passed
> repeatedly on my machine.
>
> Please note that the pmc2cutil tests should only be run at a point
> *after* you've called perl Configure.pl but *before* you've called
> 'make'. That's because if you've already run 'make', you've created
> some of the files whose creatability is the thing to be tested by the
> pmc2cutil/*.t tests. If those files already exist on your disk,
> rather than give you false positives, some of the tests are
> deliberately designed to fail.
>
> I was getting burned by false positives for three weeks before I
> realized this. But before I conclude that this is your problem, I'll
> need to see some output. Thank you very much.
Here's what I get on an x86 Linux machine:
/usr/bin/perl t/harness t/tools/pmc2cutils/00-qualify.t
t/tools/pmc2cutils/01-pmc2cutils.t t/tools/pmc2cutils/02-find_file.t
t/tools/pmc2cutils/03-dump_vtable.t t/tools/pmc2cutils/04-dump_pmc.t
t/tools/pmc2cutils/05-gen_c.t t/tools/pmc2cutils/06-print_tree.t
t/tools/pmc2cutils/07-open_file.t
t/tools/pmc2cutils/00-qualify........ok
t/tools/pmc2cutils/01-pmc2cutils.....
# Failed test (t/tools/pmc2cutils/01-pmc2cutils.t at line 48)
# Parrot::Pmc2c::Utils->can('add_defaulted') failed
# Looks like you failed 1 test of 28.
dubious
Test returned status 1 (wstat 256, 0x100)
DIED. FAILED test 18
Failed 1/28 tests, 96.43% okay
t/tools/pmc2cutils/02-find_file......ok
t/tools/pmc2cutils/03-dump_vtable....ok
t/tools/pmc2cutils/04-dump_pmc.......
# Failed test (t/tools/pmc2cutils/04-dump_pmc.t at line 478)
# got: '1167508484'
# expected: '1167508487'
# Looks like you failed 1 test of 107.
dubious
Test returned status 1 (wstat 256, 0x100)
DIED. FAILED test 94
Failed 1/107 tests, 99.07% okay
t/tools/pmc2cutils/05-gen_c..........ok
t/tools/pmc2cutils/06-print_tree.....ok
t/tools/pmc2cutils/07-open_file......ok
Failed Test Stat Wstat Total Fail List of Failed
-------------------------------------------------------------------------------
t/tools/pmc2cutils/01-pmc2cutils.t 1 256 28 1 18
t/tools/pmc2cutils/04-dump_pmc.t 1 256 107 1 94
Failed 2/8 test scripts. 2/297 subtests failed.
Files=8, Tests=297, 6 wallclock secs ( 3.80 cusr + 0.15 csys = 3.95 CPU)
Failed 2/8 test programs. 2/297 subtests failed.
make: *** [buildtools_tests] Error 255
-- c
Chromatic
> You didn't get the most recent version of the tests.
>
> I think part of the problem is that, in the case of the 8 test files
> and lib/Parrot/Pmc2c/Utils.pm, I was submitting completely new files
> (hence, svn adds) rather than patching existing files. Since I don't
> have commit privileges, I was dependent on others to commit them for
> me. tewk generously offered to do that.
>
> But from the time I began to submit what I think are correct files, I
> kept making revisions and submitting patches to RT. So different
> versions of as-yet-never-committed files were available on RT. What
> ended up getting an svn commit was not the latest version of certain
> files. In particular, I had deleted the 'add_defaulted' subroutine
> from lib/Parrot/Pmc2c/Utils.pm and the corresponding test in 01-
> pmc2cutils.t, but the version of 01-pmc2cutils.t that got committed
> still had a test for the deleted subroutine. Hence, your failure above.
Okay; with these patches and a realclean, all of the tests pass now for me.
If there aren't any more changes by tonight, I'll apply the patches.
-- c
chromatic
> I think part of the problem is that, in the case of the 8 test files
> and lib/Parrot/Pmc2c/Utils.pm, I was submitting completely new files
> (hence, svn adds) rather than patching existing files. Since I don't
> have commit privileges, I was dependent on others to commit them for
> me. tewk generously offered to do that.
I just committed these patches as r16345, thanks!
-- c
James Keenan
On Dec 30, 2006, at 12:31 PM, Will Coleda via RT wrote:
> Perhaps a helpful failure message when run at the wrong time would
> help.
>
In a patch to t/tools/pmc2cutils/00-qualify.t which I just submitted,
I have included such an explanatory message which will print out if
you run it with 'prove -v'.
Thanks for the suggestion!
kid51
James Keenan
On Dec 30, 2006, at 2:56 PM, chromatic via RT wrote:
>
>
> Here's what I get on an x86 Linux machine:
>
> /usr/bin/perl t/harness t/tools/pmc2cutils/00-qualify.t
> [snip]
You didn't get the most recent version of the tests.
I think part of the problem is that, in the case of the 8 test files
and lib/Parrot/Pmc2c/Utils.pm, I was submitting completely new files
(hence, svn adds) rather than patching existing files. Since I don't
have commit privileges, I was dependent on others to commit them for
me. tewk generously offered to do that.
But from the time I began to submit what I think are correct files, I
James Keenan
James Keenan
On Dec 30, 2006, at 12:31 PM, Will Coleda via RT wrote:
> Perhaps a helpful failure message when run at the wrong time would
> help.
>
And, inspired by your t/perl/README, I have submitted one for t/tools/
pmc2cutils/.
kid51