67663 - globals.pl and CGI.pl emit "subroutine redefined" messages

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P2)

Description

[Alan Shields]

globals.pl and bug_form.pl emit "subroutine redefined" messages, and a huge
amount of other stuff when run.

Comment 1

[Alan Shields]

Attachment: proposed patch

Comment 2

[Alan Shields]

This is caused by RelationSet.pm being used instead of required, as
RelationSet.pm uses these files, causing them to be run twice. This was
cluttering up the logs, and was not "perfect code". joy.

Comment 3

[Jacob Steenhagen]

-> New Bugzilla Product

Comment 4

[Christian Reis]

If you can tell me how to reproduce this I will gladly r= this harmless change.

Comment 5

[Alan Shields]

"perl -Wall -c bug_form.pl" will complain about confess croak and carp being
redefined. That's a symptom of this.

Also, when bug_form.pl is requested, the "subroutine redefined" error message
should show up in Apache's error log.

Comment 6

[Christian Reis]

This patch is stale. Could you update it to the latest CVS bugzilla and repatch?
I'm getting:

bash-2.03# perl5.6.1 -Wall bug_form.pl
[Sun Oct  7 19:04:03 2001] bug_form.pl: Subroutine confess redefined at (eval 3)
line 1.
[Sun Oct  7 19:04:03 2001] bug_form.pl: Subroutine croak redefined at (eval 3)
line 2.
[Sun Oct  7 19:04:03 2001] bug_form.pl: Subroutine carp redefined at (eval 3)
line 3.
Content-type: text/html


<H1>Software error:</H1>
<PRE>Undefined subroutine &amp;main::quietly_check_login called at bug_form.pl
line 48.
</PRE>
<P>
For help, please send mail to this site's webmaster, giving this error message 
and the time and date of the error.


[Sun Oct  7 19:04:03 2001] bug_form.pl: Undefined subroutine
&main::quietly_check_login called at bug_form.pl line 48.

If you don't someday I might fix this :-)

Comment 7

[Adam Kennedy]

While it is possible to fix the "huge amount of other stuff", which HAS been 
fixed, you can't fix those subroutine redefined warnings short of something 
spectacular.

A bit of debugger work shows that the warning are being thrown by CGI::Carp

CGI::Carp specifically uses Carp to initially create confess croak and carp, 
and then overwrites it shortly afterwards. The code that is causing the 
warnings is listed here for posterity...

# Avoid generating "subroutine redefined" warnings with the following
# hack:
{
    local $^W=0;
    eval <<EOF;
sub confess { CGI::Carp::die Carp::longmess \@_; }
sub croak   { CGI::Carp::die Carp::shortmess \@_; }
sub carp    { CGI::Carp::warn Carp::shortmess \@_; }
sub cluck   { CGI::Carp::warn Carp::longmess \@_; }
EOF
    ;
}

So for some reason the hack isn't working...
I'd suggest that the only way that you can fix this is to not use CGI::Carp...

CGI::Carp is mentioned on the following lines

bugzilla\Bug.pm(34): use CGI::Carp qw(fatalsToBrowser);
bugzilla\CGI.pl(49): use CGI::Carp qw(fatalsToBrowser);
bugzilla\RelationSet.pm(42): use CGI::Carp qw(fatalsToBrowser);
bugzilla\checksetup.pl(196): # If CGI::Carp was loaded successfully for version 
checking, it changes the
bugzilla\checksetup.pl(201): unless (have_vers("CGI::Carp",0))    { push 
@missing,"CGI::Carp" }
bugzilla\temp.pl(3): use CGI::Carp qw{fatalsToBrowser};


Now, given it's called in modules, which it really shouldn't... you can't do 
something like

BEGIN {
    if ( $developermode ) {
        eval { use CGI::Carp qw{fatalsToBrowser} };
    }
}

Which would only use it if you had some developer flag set...

Just to enforce that it's CGI::Carp doing it, try this

legba:/mnt/CVS/bugzilla# perl -Wall -e 'use CGI::Carp qw{fatalsToBrowser}'
[Sat Nov  3 01:03:36 2001] (eval 1): Subroutine confess redefined at (eval 1) 
line 1.
[Sat Nov  3 01:03:36 2001] (eval 1): Subroutine croak redefined at (eval 1) 
line 2.
[Sat Nov  3 01:03:36 2001] (eval 1): Subroutine carp redefined at (eval 1) line 
3.
legba:/mnt/CVS/bugzilla#

SO, in conclusion, your three options are

1) RESOLVED WONTFIX - Live with it
2) Stop using CGI::Carp qw{fatalsToBrowser};
3) Make your own version of CGI::Carp that doesn't throw that error

But if the CGI::Carp author can't stop the warnings, I don't think I want to go 
there....

Comment 8

[Dave Miller [:justdave]]

All we really want from CGI::Carp here is the FatalsToBrowser override...  and
that's a trivial thing to create on our own just by hooking into $::SIG{__DIE__}
and $::SIG{__WARN__}.  My vote is to remove our dependency on CGI::Carp and do
our own routines to output the errors to the browser (and/or format them for web
logs).

Comment 9

[Adam Kennedy]

Attachment: CGI::FatalsToBrowser

Comment 10

[Adam Kennedy]

Added CGI::FatalsToBrowser

This needs to be added as bugzilla/CGI/FatalsToBrowser.pm

It's basically just CGI::Carp with the Following changes

1) NameSpace CGI::Carp -> CGI::FatalsToBrowser
2) use Carp  ->  BEGIN { require Carp; }
3) Removed the hack in my comment above
4) Move fatalsToBrowser from @EXPORT_OK to @EXPORT
5) Removed all that bloaty documentation, and replaced with a reference to 
CGI::Carp

I'll a patch to use the module instead of CGI::Carp shortly

Comment 11

[Adam Kennedy]

Attachment: Proposed patch

Comment 12

[Adam Kennedy]

Added patch to change to the new module

We only need to use the module in CGI.pl, since anything that is CGI will be 
using CGI.pl... right?

You should probably test this properly but it should be ok

perl -Wall -e 'use CGI::FatalsToBrowser; die "Blah";'

Comment 13

[Adam Kennedy]

OK

I've submitted a patch to Lincoln Stein to fix the subroutine redefined problem 
in the new version of CGI::Carp itself on CPAN, so in time this bug will go 
away. It should come in with version 2.79 of the CGI.pm package.

So... the patch is somewhat un-nescesary, although we might want to use it to 
put some other stuff to happen when something dies...

Comment 14

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 15

[Adam Kennedy]

Okie dokie.

Fix for this has been applied to the CPAN version of CGI::Carp.

This bug can be fixed by changing the required version of the CGI distribution 
to 2.79.

Comment 16

[Adam Kennedy]

Attachment: Patch for checksetup.pl instead of CGIs

Adding patch to checksetup.pl to make sure the right version of CGI::Carp is on
the system.

Comment 17

[Adam Kennedy]

This is right to go.

I'm guessing we will need to add the version requirement to Bundle::Bugzilla 
though right?

Since it's more likely someone will have an out of date and not just missing 
module now, I've added Bug 118953 to fix the message only mentioning missing 
modules, and not old.

Comment 18

[Christian Reis]

Is the version really 1.22?

Comment 19

[Bradley Baetz (:bbaetz)]

So is the only fix here for the later CGI version? We don't need the other patches?

Bundle::Bugzilla can be ignored; zach updates it just before a release.

Comment 20

[chaduv]

Has this been fixed through other bugs? The fix for bug 147833 changed 
checksetup from checking CGI:Carp to checking for CGI v2.88, and the fix for 
bug 201816 pushed the check to v2.93.

Comment 21

[chaduv]

Comment on attachment 64129 [details] [diff] [review]
Patch for checksetup.pl instead of CGIs

Patch has rotted, cancelling review request.

Comment 22

[Christian Reis]

On CVS HEAD:

kiko@anthem:~/devel/bugzilla$ perl -Wall -c globals.pl 
[Wed Oct 29 14:23:28 2003] (eval 3): Subroutine confess redefined at (eval 3)
line 1.
[Wed Oct 29 14:23:28 2003] (eval 3): Subroutine croak redefined at (eval 3) line 2.
[Wed Oct 29 14:23:28 2003] (eval 3): Subroutine carp redefined at (eval 3) line 3.
[Wed Oct 29 14:23:28 2003] globals.pl: Name "main::SqlStateStack" used only
once: possible typo at globals.pl line 42.
globals.pl syntax OK

kiko@anthem:~/devel/bugzilla$ perl -Wall -c CGI.pl       
[Wed Oct 29 14:25:56 2003] (eval 2): Subroutine confess redefined at (eval 2)
line 1.
[Wed Oct 29 14:25:56 2003] (eval 2): Subroutine croak redefined at (eval 2) line 2.
[Wed Oct 29 14:25:56 2003] (eval 2): Subroutine carp redefined at (eval 2) line 3.
[Wed Oct 29 14:25:56 2003] CGI.pl: Name "main::buffer" used only once: possible
typo at CGI.pl line 422.
[Wed Oct 29 14:25:56 2003] CGI.pl: Name "main::dontchange" used only once:
possible typo at CGI.pl line 45.
CGI.pl syntax OK

The first 3 warnings are probably caused by the Carp problem (how do I check my
version?). The latter problem seems to derive from leftovers. I'll attach the
patch, and someone tell me if it's the wrong approach.

We don't have a bug_form.pl anymore, btw.

Comment 23

[Christian Reis]

Attachment: Fix remaining warnings

I'm not sure if the previous patch should be applied -- how widespread is that
version of CGI::Carp?

Comment 24

[Christian Reis]

Comment on attachment 134394 [details] [diff] [review]
Fix remaining warnings

In case you want to help me out, since you recently touched this one. ;)

Comment 25

[chaduv]

A suggestion on perlmonks to check a module version
     perl -MSomeModule -e'print $SomeModule::VERSION'

Replacing SomeModule with CGI and/or CGI::Carp

The carp problems with redefining subroutines should've been fixed (looking at 
changelogs) in CGI::Carp version 1.22, and CGI bundle 2.79, as Adam said. We 
require more recent than that (2.93) in checksetup.pl.

I'll take a closer look at this when I get home.

Am I actually allowed to review? :)

Comment 26

[chaduv]

Comment on attachment 64129 [details] [diff] [review]
Patch for checksetup.pl instead of CGIs

Per comments, obsoleted.

Comment 27

[Christian Reis]

As a secondary reviewer, yes, I'd like your opinion.

I may have an old version of CGI::Carp on Woody, but checksetup runs without
complaining, so I'm not sure it's fixed for sure. Checking...

kiko@anthem:~$ perl -MCGI -e'print $CGI::VERSION'
2.752

kiko@anthem:~$ perl -MCGI::Carp -e'print $CGI::Carp::VERSION'
1.20kiko

and yes, checksetup requires a newer version :-) So we need no additional patch
if a newer version of CGI::Carp fixes it.

Comment 28

[chaduv]

Comment on attachment 134394 [details] [diff] [review]
Fix remaining warnings

Patch clears up errors, doesn't seem to cause any ill effects. The two lines of
silliness being removed are trivially fine - they were missed when other
cleanups were done (SqlStateStack, for example, stopped being used when
Bugzilla::DB was created).

r=chaduv

Comment 29

[chaduv]

Newer version of CGI/CGI::Carp does fix it.

[mozdev@hermit bugzilla]$ perl -MCGI -e'print $CGI::VERSION'
3.00
[mozdev@hermit bugzilla]$ perl -MCGI::Carp -e'print $CGI::Carp::VERSION'
1.26

[mozdev@hermit bugzilla]$ perl -Wall -c globals.pl
globals.pl syntax OK
[mozdev@hermit bugzilla]$ perl -Wall -c CGI.pl
CGI.pl syntax OK

That's with patch. Doesn't checksetup force you to up your CGI package? There
was some weirdness with the 2.7x version numbers, but around line 190 is some
code that should work around it.

Comment 30

[Christian Reis]

See comment 27 -- it does complain when I run checksetup.pl.
I have no idea how this installation was performed!

Comment 31

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 134394 [details] [diff] [review]
Fix remaining warnings

Looks good, works.

Comment 32

[Christian Reis]

/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.209; previous revision: 1.208
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.248; previous revision: 1.247

Thanks!