Test t9500 fails if Time::HiRes is missing

t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL
6.2, Perl 5.10.1.  Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1
fixes it.  The commit expects Time::HiRes to be present, but RedHat
has split it out to a separate RPM perl-Time-HiRes.  Better add a
comment about that, so it doesn't get re-reverted.

Or pacify the test and expect gitweb@RHEL-users to install the RPM:

--- git-1.7.9.rc2/t/gitweb-lib.sh~
+++ git-1.7.9.rc2/t/gitweb-lib.sh

@@ -113,4 +113,9 @@
 	test_done
 }
 
+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {
+	skip_all='skipping gitweb tests, Time::HiRes module not available'
+	test_done
+}
+
 gitweb_init

-- 
Hallvard

I wrote:

Better add a comment about that, so it doesn't get re-reverted.

Perhaps I should follow my own advise...

Or pacify the test and expect gitweb@RHEL-users to install the RPM:

--- git-1.7.9.rc2/t/gitweb-lib.sh~
+++ git-1.7.9.rc2/t/gitweb-lib.sh

@@ -113,4 +113,10 @@
 	test_done
 }
 
+# RedHat has moved Time::HiRes out from core Perl to a separate package.
+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {
+	skip_all='skipping gitweb tests, Time::HiRes module not available'
+	test_done
+}
+
 gitweb_init

-- 
Hallvard

On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth
[off-list ref] wrote:

[Adding Jakub to CC]

This doesn't actually fix the issue, it only sweeps it under the rug
by making the tests pass, gitweb will still fail to compile on Red
Hat once installed.

I think the right solution is to partially revert
3962f1d756ab41c1d180e35483d1c8dffe51e0d1, but add a comment in the
code indicating that it's to deal with RedHat's broken fork of Perl.

However even if it's required in an eval it might still fail at
runtime in the reset_timer() function, which'll need to deal with it
too.

On Mon, Jan 23, 2012 at 10:42 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth [off-list ref] wrote:

quoted

t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL
6.2, Perl 5.10.1.  Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1
fixes it.  The commit expects Time::HiRes to be present, but RedHat
has split it out to a separate RPM perl-Time-HiRes.  Better add a
comment about that, so it doesn't get re-reverted.

Or pacify the test and expect gitweb@RHEL-users to install the RPM:

--- git-1.7.9.rc2/t/gitweb-lib.sh~
+++ git-1.7.9.rc2/t/gitweb-lib.sh

@@ -113,4 +113,9 @@

       test_done
 }

+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {
+       skip_all='skipping gitweb tests, Time::HiRes module not available'
+       test_done
+}
+
 gitweb_init

[Adding Jakub to CC]

This doesn't actually fix the issue, it only sweeps it under the rug
by making the tests pass, gitweb will still fail to compile on Red
Hat once installed.

I think the right solution is to partially revert
3962f1d756ab41c1d180e35483d1c8dffe51e0d1, but add a comment in the
code indicating that it's to deal with RedHat's broken fork of Perl.

However even if it's required in an eval it might still fail at
runtime in the reset_timer() function, which'll need to deal with it
too.

I'll try to send a fix today.  Time::HiRes is needed only for optional timing
info.

-- 
Jakub Narebski

 On Mon, 23 Jan 2012 10:42:02 +0100, Ævar Arnfjörð Bjarmason 
 [off-list ref] wrote:

On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth
[off-list ref] wrote:

quoted

Or pacify the test and expect gitweb@RHEL-users to install the RPM:

--- git-1.7.9.rc2/t/gitweb-lib.sh~
+++ git-1.7.9.rc2/t/gitweb-lib.sh

@@ -113,4 +113,9 @@

       test_done
 }

+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {
+       skip_all='skipping gitweb tests, Time::HiRes module not 
available'
+       test_done
+}
+
 gitweb_init

[Adding Jakub to CC]

This doesn't actually fix the issue, it only sweeps it under the rug
by making the tests pass, gitweb will still fail to compile on Red
Hat once installed.

 Is that relevant?  gitweb-lib.sh already has code to pass the tests if
 Encode, CGI, CGI::Util or CGI::Carp are missing.  I just added another.

-- 
 Hallvard

On Fri, Jan 27, 2012 at 11:15 AM, Hallvard B Furuseth
[off-list ref] wrote:

On Mon, 23 Jan 2012 10:42:02 +0100, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth
[off-list ref] wrote:

quoted

Or pacify the test and expect gitweb@RHEL-users to install the RPM:

--- git-1.7.9.rc2/t/gitweb-lib.sh~
+++ git-1.7.9.rc2/t/gitweb-lib.sh

@@ -113,4 +113,9 @@

       test_done
 }

+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {
+       skip_all='skipping gitweb tests, Time::HiRes module not available'
+       test_done
+}
+
 gitweb_init

[Adding Jakub to CC]

This doesn't actually fix the issue, it only sweeps it under the rug
by making the tests pass, gitweb will still fail to compile on Red
Hat once installed.

Is that relevant?  gitweb-lib.sh already has code to pass the tests if
Encode, CGI, CGI::Util or CGI::Carp are missing.  I just added another.

The difference is that:
1.) Time::HiRes is a core Perl module, so theoretically it should be always
    installed.
2.) Time::HiRes is not really required for gitweb to work, only for optional
    feature (timing information).

-- 
Jakub Narebski

On Mon, 23 Jan 2012, Ævar Arnfjörð Bjarmason wrote:

On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth [off-list ref] wrote:

quoted

t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL
6.2, Perl 5.10.1.  Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1
fixes it.  The commit expects Time::HiRes to be present, but RedHat
has split it out to a separate RPM perl-Time-HiRes.  Better add a
comment about that, so it doesn't get re-reverted.

Or pacify the test and expect gitweb@RHEL-users to install the RPM:

[...]
 

This doesn't actually fix the issue, it only sweeps it under the rug
by making the tests pass, gitweb will still fail to compile on Red
Hat once installed.

I think you meant "fail to run" here.

I think the right solution is to partially revert
3962f1d756ab41c1d180e35483d1c8dffe51e0d1, but add a comment in the
code indicating that it's to deal with RedHat's broken fork of Perl.

I have added comment in commit message, but not in code.  I wonder if
it would be enough.

However even if it's required in an eval it might still fail at
runtime in the reset_timer() function, which'll need to deal with it
too.

It shouldn't; everything else related to timer is protected with
'if defined $t0', which is false if Time::HiRes module is not available.

Here is the patch
-- >8 --
From: Jakub Narebski <redacted>
Subject: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"

This reverts commit 3962f1d756ab41c1d180e35483d1c8dffe51e0d1.

Though Time::HiRes is a core Perl module, it doesn't necessarily mean
that it is included in 'perl' package, and that it is installed if
Perl is installed.

For example RedHat has split it out to a separate RPM perl-Time-HiRes.

Noticed-by: Hallvard Breien Furuseth [off-list ref]
Suggested-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Jakub Narębski <redacted>
---
 gitweb/gitweb.perl |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abb5a79..c86224a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -17,10 +17,12 @@ use Encode;
 use Fcntl ':mode';
 use File::Find qw();
 use File::Basename qw(basename);
-use Time::HiRes qw(gettimeofday tv_interval);
 binmode STDOUT, ':utf8';
 
-our $t0 = [ gettimeofday() ];
+our $t0;
+if (eval { require Time::HiRes; 1; }) {
+	$t0 = [Time::HiRes::gettimeofday()];
+}
 our $number_of_git_cmds = 0;
 
 BEGIN {

@@ -1142,7 +1144,7 @@ sub dispatch {
 }
 
 sub reset_timer {
-	our $t0 = [ gettimeofday() ]
+	our $t0 = [Time::HiRes::gettimeofday()]
 		if defined $t0;
 	our $number_of_git_cmds = 0;
 }

@@ -3974,7 +3976,7 @@ sub git_footer_html {
 		print "<div id=\"generating_info\">\n";
 		print 'This page took '.
 		      '<span id="generating_time" class="time_span">'.
-		      tv_interval($t0, [ gettimeofday() ]).
+		      Time::HiRes::tv_interval($t0, [Time::HiRes::gettimeofday()]).
 		      ' seconds </span>'.
 		      ' and '.
 		      '<span id="generating_cmd">'.

@@ -6253,7 +6255,7 @@ sub git_blame_common {
 		print 'END';
 		if (defined $t0 && gitweb_check_feature('timed')) {
 			print ' '.
-			      tv_interval($t0, [ gettimeofday() ]).
+			      Time::HiRes::tv_interval($t0, [Time::HiRes::gettimeofday()]).
 			      ' '.$number_of_git_cmds;
 		}
 		print "\n";

-- 
1.7.6

On Fri, Jan 27, 2012 at 18:45, Jakub Narebski [off-list ref] wrote:

On Mon, 23 Jan 2012, Ævar Arnfjörð Bjarmason wrote:

quoted

On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth [off-list ref] wrote:

quoted

t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL
6.2, Perl 5.10.1.  Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1
fixes it.  The commit expects Time::HiRes to be present, but RedHat
has split it out to a separate RPM perl-Time-HiRes.  Better add a
comment about that, so it doesn't get re-reverted.

Or pacify the test and expect gitweb@RHEL-users to install the RPM:

[...]

quoted

This doesn't actually fix the issue, it only sweeps it under the rug
by making the tests pass, gitweb will still fail to compile on Red
Hat once installed.

I think you meant "fail to run" here.

I mean fail to compile, "use" statements are executed at compile time,
if it was a "require" outside of BEGIN-time it would fail at runtime.

I realize though that you probably thought I meant fail in Git's
Makefile-driven compilation phase, but no, it'll install just fine,
however the perl interpreter won't compile it.

quoted

I think the right solution is to partially revert
3962f1d756ab41c1d180e35483d1c8dffe51e0d1, but add a comment in the
code indicating that it's to deal with RedHat's broken fork of Perl.

I have added comment in commit message, but not in code.  I wonder if
it would be enough.

quoted

However even if it's required in an eval it might still fail at
runtime in the reset_timer() function, which'll need to deal with it
too.

It shouldn't; everything else related to timer is protected with
'if defined $t0', which is false if Time::HiRes module is not available.

Correct, I didn't look carefully enough.