Skip to contentSkip to conversation

Test t9500 fails if Time::HiRes is missing

PATCH In review Hallvard Breien Furuseth 4 participants 13 messages Jun 15, 2016

lore
Share & tools
lore

Hallvard Breien Furuseth

author
#0 lore raw

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:

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +5 -0
@@ -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
Signature · 1 line
Hallvard
#1 lore raw
Quoted 19 lines · Hallvard Breien Furuseth

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:

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +0 -0
@@ -113,4 +113,9 @@

       test_done
 }

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +5 -0
+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {
+       skip_all='skipping gitweb tests, Time::HiRes module not available'
+       test_done
+}
+
Quoted 1 line

 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.

#2 lore raw

PATCH 1/1 Revert "gitweb: Time::HiRes is in core for Perl 5.8"

Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Hallvard Breien Furuseth

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 3 lines

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.

Quoted 3 lines

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 3 lines

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 <[email protected]>
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 <[email protected]>
Suggested-by Ævar Arnfjörð Bjarmason <[email protected]>
Signed-off-by Jakub Narębski <[email protected]>
---
 gitweb/gitweb.perl |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
gitweb/gitweb.perl +7 -5
index abb5a79..c86224a 100755
@@ -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";
Signature · 1 line
1.7.6
#3 lore raw

Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"

Quoted 3 lines · Jakub Narebski

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.

I do not think we have seen the end of Redhat/Fedora Perl saga. I am hoping that either one of the two things to happen:

(1) Redhat/Fedora distrubution reconsiders the situation and fix their packages so that by default when its users ask for "Perl" they get what the upstream distributes as "Perl" in full, while still allowing people who know what they are doing to install a minimum subset "perl-base"; or

(2) Many applications that use and rely on Perl like we do are hit by this issue, and Redhat/Fedora users are trained to install the perl-full (or whatever it is called) package when applications want "Perl".

In other words, I am hoping that "it doesn't necessarily mean" will not stay true for a long time. So please hold onto this patch until the dust settles, and resend it if (1) does not look to be happening in say 3 months.

Quoted 16 lines

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

Noticed-by Hallvard Breien Furuseth <[email protected]>
Suggested-by Ævar Arnfjörð Bjarmason <[email protected]>
Signed-off-by Jakub Narębski <[email protected]>
---
 gitweb/gitweb.perl |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
gitweb/gitweb.perl +0 -0
index abb5a79..c86224a 100755
@@ -17,10 +17,12 @@ use Encode;
 use Fcntl ':mode';
 use File::Find qw();

gitweb/gitweb.perl use Encode; · line 26

Quoted 10 lines
gitweb/gitweb.perl +4 -2
 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;

Why should these even be initialized here? Doesn't reset_timer gets called at the beginning of run_request()?

gitweb/gitweb.perl sub dispatch { · line 1149

Quoted 10 lines
gitweb/gitweb.perl +1 -1
 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;

The statement modifier look ugly.

More importantly, if you are not profiling, i.e. if we didn't initialize $t0 at the beginning, do you need to reset $number_of_git_cmds at all?

I also think this should take gitweb_check_feature('timed') into account, perhaps like this:

	sub reset_timer {
        	return unless gitweb_check_feature('timed');
                our $t0 = ...
                our $number_of_git_cmds = 0;
	}

Then all the other

	if (defined $t0 && gitweb_check_feature('timed'))

can become

if (defined $t0)

If you go this route, even though tee-zero, the beginning of the time, is a good name for the variable, you may want to rename it to avoid confusing readers who might take it as a temporary variable #0.

#4 lore raw

Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"

Quoted 20 lines · Junio C Hamano
Quoted 3 lines · Jakub Narebski

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.

I do not think we have seen the end of Redhat/Fedora Perl saga. I am hoping that either one of the two things to happen:

(1) Redhat/Fedora distrubution reconsiders the situation and fix their packages so that by default when its users ask for "Perl" they get what the upstream distributes as "Perl" in full, while still allowing people who know what they are doing to install a minimum subset "perl-base"; or

(2) Many applications that use and rely on Perl like we do are hit by this issue, and Redhat/Fedora users are trained to install the perl-full (or whatever it is called) package when applications want "Perl".

Quoted 4 lines

In other words, I am hoping that "it doesn't necessarily mean" will not stay true for a long time. So please hold onto this patch until the dust settles, and resend it if (1) does not look to be happening in say 3 months.

So for the time being (those "3 months") you would apply instead your change to INSTALL (or equivalent to gitweb/INSTALL) mentioning Time::HiRes issue, and perhaps also original patch by Hallvard skipping gitweb tests if Time::HiRes is not available, isn't it?

Quoted 1 line
Quoted 1 line

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

[...]

Quoted 11 lines
Quoted 11 lines
gitweb/gitweb.perl +0 -1
index abb5a79..c86224a 100755
@@ -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';
Quoted 9 lines

gitweb/gitweb.perl use Encode; · line 26

Quoted 6 lines
gitweb/gitweb.perl +4 -1
-our $t0 = [ gettimeofday() ];
+our $t0;
+if (eval { require Time::HiRes; 1; }) {
+	$t0 = [Time::HiRes::gettimeofday()];
+}
 our $number_of_git_cmds = 0;

Why should these even be initialized here? Doesn't reset_timer gets called at the beginning of run_request()?

I think it predates adding reset_timer() to gitweb. Anyway $t0 has to be set to something defined anyway to denote that Time::HiRes is available... though if Time::HiRes is required unconditionally it would not be really needed, and we can always check $INC{'Time/HiRes.pm'} if it was loaded or not.

Quoted 24 lines

gitweb/gitweb.perl sub dispatch { · line 1149

Quoted 9 lines
gitweb/gitweb.perl +1 -1
 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;

The statement modifier look ugly.

More importantly, if you are not profiling, i.e. if we didn't initialize $t0 at the beginning, do you need to reset $number_of_git_cmds at all?

I also think this should take gitweb_check_feature('timed') into account, perhaps like this:

	sub reset_timer {
        	return unless gitweb_check_feature('timed');
                our $t0 = ...
                our $number_of_git_cmds = 0;
	}
Quoted 7 lines

Then all the other

	if (defined $t0 && gitweb_check_feature('timed'))

can become

if (defined $t0)

I think this is a good idea... though it would complicate applying revert a bit ;-(

Quoted 3 lines

If you go this route, even though tee-zero, the beginning of the time, is a good name for the variable, you may want to rename it to avoid confusing readers who might take it as a temporary variable #0.

Good idea. $request_start_time perhaps? Or $time_start?

Signature · 2 lines
Jakub Narebski
Poland
#5 lore raw

Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"

In reply to Junio C Hamano
Quoted 7 lines · Junio C Hamano

       if (defined $t0)

If you go this route, even though tee-zero, the beginning of the time, is a good name for the variable, you may want to rename it to avoid confusing readers who might take it as a temporary variable #0.

<trivia>

Personally I'd have written it as $START_TIME, but as a bit of Perl trivia you might not realize $t0 is a commonly used and undestood variable for dealing with a start time in Perl in the same way that i is common for dealing with array indexes in C.

I.e. someone used to Perl will immediately think "oh that's the start time" having seen it hundreds of times before, but someone not used to Perl will go "what's this t-zero thing?".

Meanwhile some Lisp programmer is wondering what the hell "i" means in your C for-loops, iterator? :)

#6 lore raw

Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"

In reply to Jakub Narebski
Quoted 9 lines · Jakub Narebski
Quoted 8 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Hallvard Breien Furuseth

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.

Quoted 8 lines
Quoted 1 line
Quoted 1 line

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

[...]

Quoted 3 lines

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 7 lines
Quoted 3 lines

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 6 lines
Quoted 3 lines

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.

author
#7 lore raw
In reply to Ævar Arnfjörð Bjarmason
Quoted 20 lines · Ævar Arnfjörð Bjarmason
Quoted 14 lines · Hallvard Breien Furuseth

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

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +0 -0
@@ -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

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +2 -0
+}
+
Quoted 1 line

 gitweb_init

[Adding Jakub to CC]

Quoted 3 lines

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.

Signature · 1 line
Hallvard
#8 lore raw
Quoted 24 lines · Hallvard B Furuseth
Quoted 22 lines · Ævar Arnfjörð Bjarmason
Quoted 14 lines · Hallvard Breien Furuseth

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

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +0 -0
@@ -113,4 +113,9 @@

       test_done
 }

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +5 -0
+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {
+       skip_all='skipping gitweb tests, Time::HiRes module not available'
+       test_done
+}
+
Quoted 1 line

 gitweb_init

[Adding Jakub to CC]

Quoted 7 lines
Quoted 3 lines

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

Signature · 1 line
Jakub Narebski
#9 lore raw
In reply to Ævar Arnfjörð Bjarmason
Quoted 3 lines · Ævar Arnfjörð Bjarmason

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.

In the short term for 1.7.9, let's at least warn users about this issue.

-- >8 --
Subject: INSTALL: warn about recent Fedora breakage

Recent releases of Redhat/Fedora are reported to ship Perl binary package with some core modules stripped away (see http://lwn.net/Articles/477234/)
against the upstream Perl5 people's wishes. The Time::HiRes module used by gitweb one of them.

Signed-off-by Junio C Hamano <[email protected]>

---

* Hopefully, this may resolve itself over time.

 INSTALL |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
INSTALL +5 -1
index 8120641..6fa83fe 100644
@@ -83,7 +83,11 @@ Issues of note:
 	- "Perl" version 5.8 or later is needed to use some of the
 	  features (e.g. preparing a partial commit using "git add -i/-p",
 	  interacting with svn repositories with "git svn").  If you can
-	  live without these, use NO_PERL.
+	  live without these, use NO_PERL.  Note that recent releases of
+	  Redhat/Fedora are reported to ship Perl binary package with some
+	  core modules stripped away (see http://lwn.net/Articles/477234/),
+	  so you might need to install additional packages other than Perl
+	  itself, e.g. Time::HiRes.
 
 	- "openssl" library is used by git-imap-send to use IMAP over SSL.
 	  If you don't need it, use NO_OPENSSL.
#10 lore raw
Quoted 8 lines · Junio C Hamano
Quoted 3 lines · Ævar Arnfjörð Bjarmason

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.

In the short term for 1.7.9, let's at least warn users about this issue.

Quoted 7 lines

-- >8 --
Subject: INSTALL: warn about recent Fedora breakage

Recent releases of Redhat/Fedora are reported to ship Perl binary package with some core modules stripped away (see http://lwn.net/Articles/477234/)
against the upstream Perl5 people's wishes. The Time::HiRes module used by gitweb one of them.

Since I wrote that E-Mail I learned what RedHat was doing, I think that's a far better option. They're splitting up the perl core into multiple packages, and anyone who has issues with this on RedHat can trivially just install those packages. So we should just note it in the INSTALL file as a platform-specific issue and leave it at that.

We *could* deal with this in our code, but I don't think dealing with every vendor's slightly different perl version is a viable strategy in the long run.

#11 lore raw
In reply to Ævar Arnfjörð Bjarmason
Quoted 29 lines · Ævar Arnfjörð Bjarmason
Quoted 20 lines · Hallvard Breien Furuseth

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:

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +0 -0
@@ -113,4 +113,9 @@

       test_done
 }

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +5 -0
+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {
+       skip_all='skipping gitweb tests, Time::HiRes module not available'
+       test_done
+}
+
Quoted 1 line

 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.

Quoted 7 lines

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.

Signature · 1 line
Jakub Narebski
author
#12 lore raw
In reply to Hallvard Breien Furuseth
Quoted 1 line · I wrote:

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

Perhaps I should follow my own advise...

Quoted 1 line

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

t/gitweb-lib.sh renamed from t/gitweb-lib.sh~ +6 -0
@@ -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
Signature · 1 line
Hallvard