[perl #56948] [BUG] .parrot_current_rev broken
Patch applied in r29660 tonight.
[perl #56948] [BUG] .parrot_current_rev broken
On Fri Jul 18 09:17:19 2008, [EMAIL PROTECTED] wrote:
>
> I would find this clearer if you reversed the top level conditions. In
> English, this reads:
>
> if the revision isn't defined, then do this
> otherwise do that
>
> Flipping the order changes it to:
>
> if the revision is defined, do this
> otherwise, do that
>
> There's no double-negative.
No problem. I had to tease apart the program flow and knew that the
result would be less than PBP. The inversion is attached.
Apart from that, is there any problem? In particular, have I preserved
all the functionality and use cases that were there before?
If I hear no objection, I'll apply this patch sometime over the weekend
and close the ticket.
Thank you very much.
kid51
Index: lib/Parrot/Revision.pm
===
--- lib/Parrot/Revision.pm (.../trunk) (revision 29567)
+++ lib/Parrot/Revision.pm (.../branches/revisionpm) (revision 29601)
@@ -30,36 +30,50 @@
sub update {
my $prev = _get_revision();
my $revision = _analyze_sandbox();
-if (defined ($prev) && ($revision ne $prev)) {
-$revision = 'unknown' unless defined $revision;
-eval {
-open my $FH, ">", $cache;
-print $FH "$revision\n";
-close $FH;
-$current = $revision;
-};
+$current = _handle_update( {
+prev=> $prev,
+revision=> $revision,
+cache => $cache,
+current => $current,
+} );
+}
+
+sub _handle_update {
+my $args = shift;
+if (! defined $args->{revision}) {
+$args->{revision} = 'unknown';
+_print_to_cache($args->{cache}, $args->{revision});
+return $args->{revision};
+} else {
+if (defined ($args->{prev}) && ($args->{revision} ne $args->{prev})) {
+_print_to_cache($args->{cache}, $args->{revision});
+return $args->{revision};
+}
+else {
+return $args->{current};
+}
}
}
+sub _print_to_cache {
+my ($cache, $revision) = @_;
+open my $FH, ">", $cache
+or die "Unable to open handle to $cache for writing: $!";
+print $FH "$revision\n";
+close $FH or die "Unable to close handle to $cache after writing: $!";
+}
+
sub _get_revision {
my $revision;
if (-f $cache) {
-eval {
-open my $FH, "<", $cache;
-chomp($revision = <$FH>);
-close $FH;
-};
-return $revision unless $@;
+open my $FH, "<", $cache
+or die "Unable to open $cache for reading: $!";
+chomp($revision = <$FH>);
+close $FH or die "Unable to close $cache after reading: $!";
}
-
-$revision = _analyze_sandbox();
-
-if (! -f $cache) {
-eval {
-open my $FH, ">", $cache;
-print $FH "$revision\n";
-close $FH;
-};
+else {
+$revision = _analyze_sandbox();
+_print_to_cache($cache, $revision);
}
return $revision;
}
Index: MANIFEST
===
--- MANIFEST(.../trunk) (revision 29567)
+++ MANIFEST(.../branches/revisionpm) (revision 29601)
@@ -1,7 +1,7 @@
# ex: set ro:
# $Id$
#
-# generated by tools/dev/mk_manifest_and_skip.pl Tue Jul 15 23:45:17 2008 UT
+# generated by tools/dev/mk_manifest_and_skip.pl Thu Jul 17 22:40:30 2008 UT
#
# See tools/dev/install_files.pl for documentation on the
# format of this file.
@@ -3338,6 +3338,7 @@
t/configure/058-fatal_step.t[]
t/configure/059-silent.t[]
t/configure/060-silent.t[]
+t/configure/061-revision_from_cache.t []
t/configure/testlib/Make_VERSION_File.pm[]
t/configure/testlib/Tie/Filehandle/Preempt/Stdin.pm []
t/configure/testlib/init/alpha.pm []
Index: t/configure/061-revision_from_cache.t
===
--- t/configure/061-revision_from_cache.t (.../trunk) (revision 0)
+++ t/configure/061-revision_from_cache.t (.../branches/revisionpm)
(revision 29601)
@@ -0,0 +1,148 @@
+#! perl
+# Copyright (C) 2007, The Perl Foundation.
+# $Id$
+# 061-revision_from_cache.t
+
+use strict;
+use warnings;
+
+use Test::More;
+plan( skip_all => "\nRelevant only when working in checkout from repository
and during configuration" )
+unless (-e 'DEVELOPING' and ! -e 'Makefile');
+plan( tests => 25 );
+use Carp;
+use Cwd;
+use File::Copy;
+use File::Path ();
+use File::Temp qw| tempdir |;
+use lib qw( lib );
+use Parrot::Revision ();
+
+my $cwd = cwd();
+{ # revision undef
+my $rev = 16000;
+my ($cache, $libdir) = setup_cache($rev, $cwd);
+my $prev = 34567;
+my $revision = undef;
+my $curre
Re: [perl #56948] [BUG] .parrot_current_rev broken
On Thursday 17 July 2008 19:15:52 James Keenan via RT wrote:
> + if (! defined $args->{revision}) {
> + $args->{revision} = 'unknown';
> + _print_to_cache($args->{cache}, $args->{revision});
> + return $args->{revision};
> + } else {
> + if (defined ($args->{prev}) && ($args->{revision} ne
> $args->{prev})) { + _print_to_cache($args->{cache},
> $args->{revision});
> + return $args->{revision};
> + }
> + else {
> + return $args->{current};
> + }
> }
> }
I would find this clearer if you reversed the top level conditions. In
English, this reads:
if the revision isn't defined, then do this
otherwise do that
Flipping the order changes it to:
if the revision is defined, do this
otherwise, do that
There's no double-negative.
-- c
[perl #56948] [BUG] .parrot_current_rev broken
Here's the attachment.
Index: lib/Parrot/Revision.pm
===
--- lib/Parrot/Revision.pm (.../trunk) (revision 29567)
+++ lib/Parrot/Revision.pm (.../branches/revisionpm) (revision 29574)
@@ -30,36 +30,50 @@
sub update {
my $prev = _get_revision();
my $revision = _analyze_sandbox();
-if (defined ($prev) && ($revision ne $prev)) {
-$revision = 'unknown' unless defined $revision;
-eval {
-open my $FH, ">", $cache;
-print $FH "$revision\n";
-close $FH;
-$current = $revision;
-};
+$current = _handle_update( {
+prev=> $prev,
+revision=> $revision,
+cache => $cache,
+current => $current,
+} );
+}
+
+sub _handle_update {
+my $args = shift;
+if (! defined $args->{revision}) {
+$args->{revision} = 'unknown';
+_print_to_cache($args->{cache}, $args->{revision});
+return $args->{revision};
+} else {
+if (defined ($args->{prev}) && ($args->{revision} ne $args->{prev})) {
+_print_to_cache($args->{cache}, $args->{revision});
+return $args->{revision};
+}
+else {
+return $args->{current};
+}
}
}
+sub _print_to_cache {
+my ($cache, $revision) = @_;
+open my $FH, ">", $cache
+or die "Unable to open handle to $cache for writing: $!";
+print $FH "$revision\n";
+close $FH or die "Unable to close handle to $cache after writing: $!";
+}
+
sub _get_revision {
my $revision;
if (-f $cache) {
-eval {
-open my $FH, "<", $cache;
-chomp($revision = <$FH>);
-close $FH;
-};
-return $revision unless $@;
+open my $FH, "<", $cache
+or die "Unable to open $cache for reading: $!";
+chomp($revision = <$FH>);
+close $FH or die "Unable to close $cache after reading: $!";
}
-
-$revision = _analyze_sandbox();
-
-if (! -f $cache) {
-eval {
-open my $FH, ">", $cache;
-print $FH "$revision\n";
-close $FH;
-};
+else {
+$revision = _analyze_sandbox();
+_print_to_cache($cache, $revision);
}
return $revision;
}
Index: MANIFEST
===
--- MANIFEST(.../trunk) (revision 29567)
+++ MANIFEST(.../branches/revisionpm) (revision 29574)
@@ -1,7 +1,7 @@
# ex: set ro:
# $Id$
#
-# generated by tools/dev/mk_manifest_and_skip.pl Tue Jul 15 23:45:17 2008 UT
+# generated by tools/dev/mk_manifest_and_skip.pl Thu Jul 17 22:40:30 2008 UT
#
# See tools/dev/install_files.pl for documentation on the
# format of this file.
@@ -3338,6 +3338,7 @@
t/configure/058-fatal_step.t[]
t/configure/059-silent.t[]
t/configure/060-silent.t[]
+t/configure/061-revision_from_cache.t []
t/configure/testlib/Make_VERSION_File.pm[]
t/configure/testlib/Tie/Filehandle/Preempt/Stdin.pm []
t/configure/testlib/init/alpha.pm []
Index: t/configure/061-revision_from_cache.t
===
--- t/configure/061-revision_from_cache.t (.../trunk) (revision 0)
+++ t/configure/061-revision_from_cache.t (.../branches/revisionpm)
(revision 29574)
@@ -0,0 +1,148 @@
+#! perl
+# Copyright (C) 2007, The Perl Foundation.
+# $Id$
+# 061-revision_from_cache.t
+
+use strict;
+use warnings;
+
+use Test::More;
+plan( skip_all => "\nRelevant only when working in checkout from repository
and during configuration" )
+unless (-e 'DEVELOPING' and ! -e 'Makefile');
+plan( tests => 25 );
+use Carp;
+use Cwd;
+use File::Copy;
+use File::Path ();
+use File::Temp qw| tempdir |;
+use lib qw( lib );
+use Parrot::Revision ();
+
+my $cwd = cwd();
+{ # revision undef
+my $rev = 16000;
+my ($cache, $libdir) = setup_cache($rev, $cwd);
+my $prev = 34567;
+my $revision = undef;
+my $current = 12345;
+my $ret = Parrot::Revision::_handle_update( {
+prev=> $prev,
+revision=> $revision,
+cache => $cache,
+current => $current,
+} );
+is($ret, q{unknown}, "Got expected return value from _handle_update");
+unlink qq{$libdir/Parrot/Revision.pm}
+or croak "Unable to delete file after testing";
+ok( chdir $cwd, "Able to change back to starting directory");
+}
+
+{ # prev undef
+my $rev = 16000;
+my ($cache, $libdir) = setup_cache($rev, $cwd);
+my $revision = 67890;
+my $current = 12345;
+my $ret = Parrot::Revision::_handle_update( {
+prev=> undef,
+revision=> $revision,
+cache => $cache
[perl #56948] [BUG] .parrot_current_rev broken
On Wed Jul 16 06:27:16 2008, julianalbo wrote:
> On Wed, Jul 16, 2008 at 1:12 PM, James Keenan via RT
[snip]
>
> > If no one gets to this today I will try to work on this this evening.
>
> Go for it.
>
On Wed Jul 16 04:12:35 2008, [EMAIL PROTECTED] wrote:
> [snip]
> The control flow here is somewhat awkward and creates additional
> branches which will need coverage in testing (cf t/configure/017 and 018):
>
> 33 if (defined ($prev) && ($revision ne $prev)) {
> 34 $revision = 'unknown' unless defined $revision;
>
This is a case where things can be improved by (a) making the control
flow a bit more explicit and (b) using Devel::Cover for coverage
analysis during testing.
Suppose we rewrite line 34 as:
> if (! defined $revision) {
>$revision = 'unknown';
> } else {
># do something else
> }
If we were to try to write tests for both the defined and undefined
cases for $revision, we'd soon see that in the undefined case we would
get an uninitialized value warning on line 33 in the second part of the
outer 'if' branch:
> ($revision ne $prev)
Line 33 strongly assumes that $revision is defined, so to question its
definedness on Line 34 is dubious.
So I re-ordered this part of the module, then wrote tests which cover
each branch and placed them at the end of
t/configure/017-revision_from_cache.t and in new test file
t/configure/061-revision_from_cache.t. The code is more verbose, less
idiomatic -- those things we can fix later. But in the meantime we can
see the flow better and write tests which exercise more of its nooks and
crannies.
Reini et al: Please evaluate the attached patch. Alternatively, you
can do: svn co https://svn.perl.org/parrot/branches/revisionpm.
As usual, I'll apply in a couple of days if there is no objection.
Thank you very much.
kid51
Re: [perl #56948] [BUG] .parrot_current_rev broken
> > The desired behavior is creating the file if not present or his number > is outdated, not touching it if the number is already correct. > At 29516, that seems to be what it's doing -- Email and shopping with the feelgood factor! 55% of income to good causes. http://www.ippimail.com
Re: [perl #56948] [BUG] .parrot_current_rev broken
On Wed, Jul 16, 2008 at 1:12 PM, James Keenan via RT <[EMAIL PROTECTED]> wrote: > I think this new subroutine could use some refactoring: Yes, my goal was to fix the problem as fast as possible, and I'm not very fluent with perl. > If no one gets to this today I will try to work on this this evening. Go for it. -- Salu2
Re: [perl #56948] [BUG] .parrot_current_rev broken
On Wed, Jul 16, 2008 at 12:45 AM, <[EMAIL PROTECTED]> wrote: > I just ran a test on XP-Home (using Strawberry Perl) after updating to > r29495. Configure.pl creates the file (whether or not it was present), but > the value appears to be a constant 0. (Make test and perl6 have no > effect.) Please retest with r29509 The desired behavior is creating the file if not present or his number is outdated, not touching it if the number is already correct. -- Salu2
Re: [perl #56948] [BUG] .parrot_current_rev broken
> I fixed the problem in r29488, but I don't have any windows > environment available to test. > I just ran a test on XP-Home (using Strawberry Perl) after updating to r29495. Configure.pl creates the file (whether or not it was present), but the value appears to be a constant 0. (Make test and perl6 have no effect.) Is that the desired behavior? -- Email and shopping with the feelgood factor! 55% of income to good causes. http://www.ippimail.com
Re: [perl #56948] [BUG] .parrot_current_rev broken
Confirmed fixed on windows in r29509. -- tjh On Jul 16, 2008, at 5:02 AM, NotFound wrote: On Wed, Jul 16, 2008 at 6:08 AM, Tim Heckman <[EMAIL PROTECTED]> wrote: Starting in r29489, .parrot_current_rev contains "0" instead of the revision number. I was using a simplified way to call svn info avoiding locale dependencies, and forgot to replace with a more generic way. Fixed in r29509 Don't know if this will work on windows in case of locale dependant svn output, though. -- Salu2
[perl #56948] [BUG] .parrot_current_rev broken
I think this new subroutine could use some refactoring:
30 sub update {
31 my $prev = _get_revision();
32 my $revision = _analyze_sandbox();
33 if (defined ($prev) && ($revision ne $prev)) {
34 $revision = 'unknown' unless defined $revision;
35 eval {
36 open my $FH, ">", $cache;
37 print $FH "$revision\n";
38 close $FH;
39 $current = $revision;
40 };
41 }
42 }
Line 39 does not need to be inside the eval{}. If we pull it out, then
we can, and should refactor the eval{} into an internal subroutine, as
it would then exactly match this code farther down:
58 eval {
59 open my $FH, ">", $cache;
60 print $FH "$revision\n";
61 close $FH;
62 };
The control flow here is somewhat awkward and creates additional
branches which will need coverage in testing (cf t/configure/017 and 018):
33 if (defined ($prev) && ($revision ne $prev)) {
34 $revision = 'unknown' unless defined $revision;
If no one gets to this today I will try to work on this this evening.
kid51
Re: [perl #56948] [BUG] .parrot_current_rev broken
On Wed, Jul 16, 2008 at 6:08 AM, Tim Heckman <[EMAIL PROTECTED]> wrote: > Starting in r29489, .parrot_current_rev contains "0" instead of the revision > number. I was using a simplified way to call svn info avoiding locale dependencies, and forgot to replace with a more generic way. Fixed in r29509 Don't know if this will work on windows in case of locale dependant svn output, though. -- Salu2
Re: [perl #56948] [BUG] .parrot_current_rev broken
On Wed, Jul 16, 2008 at 1:32 AM, James Keenan via RT <[EMAIL PROTECTED]> wrote: >> Worse, the logic the set the current revision for svn updates is >> wrong. > No, the logic was correct. As particle said on #parrot yesterday, > ".parrot_current_rev should always reflect the last time you ran 'perl > Configure.pl' successfully." But it not doed that. It only updated the file if it does not existed, and it were only deleted on make realclean. > Since there has been a lot of confusion about this, we/I could probably > have done a better job of documenting all this. But I do want to stress > that the design of lib/Parrot/Revision.pm was the result of considerable > thought, discussion and negotiation on this list. I haven't had a > chance to fully assess the changes that have been made in the last two > days, but I would be reluctant to see us return to the maintenance > difficulties we formerly had. The only changes I do are ensuring that the value is updated at the start of Configure.pl and a change in the way svn info is parsed. -- Salu2
Re: [perl #56948] [BUG] .parrot_current_rev broken
Starting in r29489, .parrot_current_rev contains "0" instead of the revision number. -- tjh On Jul 15, 2008, at 11:23 PM, Tim Heckman wrote: Seems to be broken as of r29503. nmake realclean (.parrot_current_rev is deleted) svn update perl Configure.pl .parrot_current_rev contains "0" Also, if I run Configure.pl again, the timestamp on the file does not change. -- tjh On Jul 15, 2008, at 1:26 PM, NotFound wrote: I fixed the problem in r29488, but I don't have any windows environment available to test. -- Salu2
Re: [perl #56948] [BUG] .parrot_current_rev broken
For r29488, what I observe (on Windows) is that * Configure.pl generates .parrot_current_rev with the correct revision number (but this is broken in r29503 -- see my earlier message) * Configure.pl does update .parrot_current_rev when the file already exists, provided the revision number in the file differs from the current actual revision number. -- tjh On Jul 15, 2008, at 1:26 PM, NotFound wrote: I fixed the problem in r29488, but I don't have any windows environment available to test. -- Salu2
Re: [perl #56948] [BUG] .parrot_current_rev broken
Seems to be broken as of r29503. nmake realclean (.parrot_current_rev is deleted) svn update perl Configure.pl .parrot_current_rev contains "0" Also, if I run Configure.pl again, the timestamp on the file does not change. -- tjh On Jul 15, 2008, at 1:26 PM, NotFound wrote: I fixed the problem in r29488, but I don't have any windows environment available to test. -- Salu2
[perl #56948] [BUG] .parrot_current_rev broken
> > -- Forwarded message -- > From: Reini Urban <[EMAIL PROTECTED]> > Date: Tue, Jul 15, 2008 at 9:52 AM > Subject: .parrot_current_rev > To: [EMAIL PROTECTED] > > > The file .parrot_current_rev is missing in the Release, and also the > revision is nowhere > mentioned in any Release Note, not the ChangeLog and not in news. > This is annoying, because you don't know if a particular bugfix is > included or not. > > Worse, the logic the set the current revision for svn updates is > wrong. No, the logic was correct. As particle said on #parrot yesterday, ".parrot_current_rev should always reflect the last time you ran 'perl Configure.pl' successfully." > .parrot_current_rev is not updated on a svn up, Correct: feature, not bug. If you want the revision after you've run 'svn up', call 'svn info' or some other subversion command. > it is just the cache > for $Parrot::Revision::current, Correct. > but the cache is not ensured to be cleared when doing a Configure.pl. > Only a make realclean will > get you a correct revision in bugreports. > Some historical perspective is warranted here. Up until April of this year Parrot::Revision attempted to be all things to all people. It attempted to provide both the SVN revision at which you last configured. It also attempted to the SVN revision resulting from any 'svn up' run after you last ran Configure.pl. And it attempted to provide all this functionality not just for Subversion clients, but for SVK and git as well. The code was convoluted and a maintenance nightmare. The only way it could have been reasonably maintained would have been to recruit someone who was expert in all three of those VCSes. (And we weren't even aware of Win32-related problems such as those described by Tim Heckman elsewhere in this thread.) After considerable discussion, we simplified the code in Parrot::Revision: r26888 | jkeenan | 2008-04-09 21:21:42 -0400 (Wed, 09 Apr 2008) | 5 lines "Eliminate svk- and git-related code from Parrot::Revision. Repository revision number will no longer be available in 'parrot --version'. Adjustments in other files as needed; eliminate two t/postconfigure/ and two t/tools/revision tests." As part of this revision, we created .parrot_current_rev to serve, as described above, for the SVN revision number at your last run of Configure.pl. It is generated by Configure.pl and, like all files generated by Configure.pl, it was intended to be cleaned via 'make realclean' rather than 'make clean'. Since there has been a lot of confusion about this, we/I could probably have done a better job of documenting all this. But I do want to stress that the design of lib/Parrot/Revision.pm was the result of considerable thought, discussion and negotiation on this list. I haven't had a chance to fully assess the changes that have been made in the last two days, but I would be reluctant to see us return to the maintenance difficulties we formerly had. Thank you very much. kid51
Re: [perl #56948] [BUG] .parrot_current_rev broken
I fixed the problem in r29488, but I don't have any windows environment available to test. -- Salu2
Re: [perl #56948] [BUG] .parrot_current_rev broken
It may also be relevant that on the Windows box where I run the smoke tests, my ".svn" directories are actually "_svn". http://svn.collab.net/repos/svn/trunk/notes/asp-dot-net-hack.txt --- tjh > On Tue, Jul 15, 2008 at 3:58 PM, via RT Will Coleda > <[EMAIL PROTECTED]> wrote: >> -- Forwarded message -- >> From: Reini Urban <[EMAIL PROTECTED]> >> Date: Tue, Jul 15, 2008 at 9:52 AM >> Subject: .parrot_current_rev >> To: [EMAIL PROTECTED] > >> Worse, the logic the set the current revision for svn updates is wrong. (NotMuch brought this up yesterday on #parrot) >> .parrot_current_rev is not updated on a svn up, it is just the cache > > The problem was first reported on #parrot by tjh on sunday. We tell it to just do a make realclean in his automated smoke report system, but this solves only automated builds case. > > And other point: 'parrot_config revision' gives the value obtained from .parrot_current_rev > > -- > Salu2 >
Re: [perl #56948] [BUG] .parrot_current_rev broken
On Tue, Jul 15, 2008 at 3:58 PM, via RT Will Coleda <[EMAIL PROTECTED]> wrote: > -- Forwarded message -- > From: Reini Urban <[EMAIL PROTECTED]> > Date: Tue, Jul 15, 2008 at 9:52 AM > Subject: .parrot_current_rev > To: [EMAIL PROTECTED] > Worse, the logic the set the current revision for svn updates is > wrong. (NotMuch brought this up yesterday on #parrot) > .parrot_current_rev is not updated on a svn up, it is just the cache The problem was first reported on #parrot by tjh on sunday. We tell it to just do a make realclean in his automated smoke report system, but this solves only automated builds case. And other point: 'parrot_config revision' gives the value obtained from .parrot_current_rev -- Salu2
[perl #56948] [BUG] .parrot_current_rev broken
# New Ticket Created by Will Coleda # Please include the string: [perl #56948] # in the subject line of all future correspondence about this issue. # http://rt.perl.org/rt3/Ticket/Display.html?id=56948 > Forward to RT so a ticket is opened. -- Forwarded message -- From: Reini Urban <[EMAIL PROTECTED]> Date: Tue, Jul 15, 2008 at 9:52 AM Subject: .parrot_current_rev To: [EMAIL PROTECTED] The file .parrot_current_rev is missing in the Release, and also the revision is nowhere mentioned in any Release Note, not the ChangeLog and not in news. This is annoying, because you don't know if a particular bugfix is included or not. Worse, the logic the set the current revision for svn updates is wrong. (NotMuch brought this up yesterday on #parrot) .parrot_current_rev is not updated on a svn up, it is just the cache for $Parrot::Revision::current, but the cache is not ensured to be cleared when doing a Configure.pl. Only a make realclean will get you a correct revision in bugreports. Configure should enforce a fresh cache update if there's a .svn subdir available, otherwise you'll get false bugreports. -- Reini Urban -- Will "Coke" Coleda
