* Re: Test t9500 fails if Time::HiRes is missing
2012-01-23 9:42 ` Ævar Arnfjörð Bjarmason
@ 2012-01-27 5:48 ` Junio C Hamano
2012-01-27 9:32 ` Ævar Arnfjörð Bjarmason
2012-01-27 9:18 ` Jakub Narębski
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2012-01-27 5:48 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Hallvard Breien Furuseth, git, Jakub Narebski
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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 <gitster@pobox.com>
---
* Hopefully, this may resolve itself over time.
INSTALL | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/INSTALL b/INSTALL
index 8120641..6fa83fe 100644
--- a/INSTALL
+++ b/INSTALL
@@ -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.
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: Test t9500 fails if Time::HiRes is missing
2012-01-27 5:48 ` Junio C Hamano
@ 2012-01-27 9:32 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 13+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2012-01-27 9:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Hallvard Breien Furuseth, git, Jakub Narebski
On Fri, Jan 27, 2012 at 06:48, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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.
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Test t9500 fails if Time::HiRes is missing
2012-01-23 9:42 ` Ævar Arnfjörð Bjarmason
2012-01-27 5:48 ` Junio C Hamano
@ 2012-01-27 9:18 ` Jakub Narębski
2012-01-27 10:15 ` Hallvard B Furuseth
2012-01-27 17:45 ` [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8" Jakub Narebski
3 siblings, 0 replies; 13+ messages in thread
From: Jakub Narębski @ 2012-01-27 9:18 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Hallvard Breien Furuseth, git
On Mon, Jan 23, 2012 at 10:42 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth <h.b.furuseth@usit.uio.no> wrote:
>>
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Test t9500 fails if Time::HiRes is missing
2012-01-23 9:42 ` Ævar Arnfjörð Bjarmason
2012-01-27 5:48 ` Junio C Hamano
2012-01-27 9:18 ` Jakub Narębski
@ 2012-01-27 10:15 ` Hallvard B Furuseth
2012-01-27 10:59 ` Jakub Narębski
2012-01-27 17:45 ` [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8" Jakub Narebski
3 siblings, 1 reply; 13+ messages in thread
From: Hallvard B Furuseth @ 2012-01-27 10:15 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jakub Narebski
On Mon, 23 Jan 2012 10:42:02 +0100, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth
> <h.b.furuseth@usit.uio.no> wrote:
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Test t9500 fails if Time::HiRes is missing
2012-01-27 10:15 ` Hallvard B Furuseth
@ 2012-01-27 10:59 ` Jakub Narębski
0 siblings, 0 replies; 13+ messages in thread
From: Jakub Narębski @ 2012-01-27 10:59 UTC (permalink / raw)
To: Hallvard B Furuseth; +Cc: Ævar Arnfjörð Bjarmason, git
On Fri, Jan 27, 2012 at 11:15 AM, Hallvard B Furuseth
<h.b.furuseth@usit.uio.no> wrote:
> On Mon, 23 Jan 2012 10:42:02 +0100, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth
>> <h.b.furuseth@usit.uio.no> wrote:
>>>
>>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"
2012-01-23 9:42 ` Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2012-01-27 10:15 ` Hallvard B Furuseth
@ 2012-01-27 17:45 ` Jakub Narebski
2012-01-27 20:44 ` Junio C Hamano
2012-01-29 2:21 ` Ævar Arnfjörð Bjarmason
3 siblings, 2 replies; 13+ messages in thread
From: Jakub Narebski @ 2012-01-27 17:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Hallvard Breien Furuseth, git
On Mon, 23 Jan 2012, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth <h.b.furuseth@usit.uio.no> wrote:
> >
> > 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 <jnareb@gmail.com>
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 <h.b.furuseth@usit.uio.no>
Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jakub Narębski <jnareb@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"
2012-01-27 17:45 ` [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8" Jakub Narebski
@ 2012-01-27 20:44 ` Junio C Hamano
2012-01-28 17:48 ` Jakub Narebski
2012-01-29 2:29 ` Ævar Arnfjörð Bjarmason
2012-01-29 2:21 ` Ævar Arnfjörð Bjarmason
1 sibling, 2 replies; 13+ messages in thread
From: Junio C Hamano @ 2012-01-27 20:44 UTC (permalink / raw)
To: Jakub Narebski
Cc: Ævar Arnfjörð Bjarmason, Hallvard Breien Furuseth,
git
Jakub Narebski <jnareb@gmail.com> writes:
> 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.
> For example RedHat has split it out to a separate RPM perl-Time-HiRes.
>
> Noticed-by: Hallvard Breien Furuseth <h.b.furuseth@usit.uio.no>
> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Jakub Narębski <jnareb@gmail.com>
> ---
> 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;
Why should these even be initialized here? Doesn't reset_timer gets
called at the beginning of run_request()?
>
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"
2012-01-27 20:44 ` Junio C Hamano
@ 2012-01-28 17:48 ` Jakub Narebski
2012-01-29 2:29 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 13+ messages in thread
From: Jakub Narebski @ 2012-01-28 17:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Hallvard Breien Furuseth,
git
On Fri, 27 Jan 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > 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.
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?
> > For example RedHat has split it out to a separate RPM perl-Time-HiRes.
[...]
> > 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;
>
> 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.
> > 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)
I think this is a good idea... though it would complicate applying revert
a bit ;-(
> 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?
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"
2012-01-27 20:44 ` Junio C Hamano
2012-01-28 17:48 ` Jakub Narebski
@ 2012-01-29 2:29 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 13+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2012-01-29 2:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, Hallvard Breien Furuseth, git
On Fri, Jan 27, 2012 at 21:44, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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? :)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"
2012-01-27 17:45 ` [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8" Jakub Narebski
2012-01-27 20:44 ` Junio C Hamano
@ 2012-01-29 2:21 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 13+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2012-01-29 2:21 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Hallvard Breien Furuseth, git
On Fri, Jan 27, 2012 at 18:45, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 23 Jan 2012, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth <h.b.furuseth@usit.uio.no> wrote:
>> >
>> > 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 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.
>> 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.
Correct, I didn't look carefully enough.
^ permalink raw reply [flat|nested] 13+ messages in thread