303413 - Bugzilla.pm causes a circular dependency somewhere which throws a warning about SHUTDOWNHTML_EXEMPT

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Tiago Mello [:timello]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+

When using 'use Bugzilla;' in some lib like Product.pm & cia, some circular
dependecy occurs and generates the follow error when running test 001:

#     Failed test (t/001compile.t at line 89)
[Thu Aug  4 12:11:04 2005] Bugzilla.pm: Constant subroutine
Bugzilla::SHUTDOWNHTML_EXEMPT redefined at /usr/share/perl/5.8/constant.pm line 108.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Tiago Mello [:timello]]

Sorry, I forgot to mention that the problem appers when I put:

use Bugzilla::Product; in User.pm.

Comment 2

[Tiago Mello [:timello]]

Attachment: v1: change constant to variable

Comment 3

[Max Kanat-Alexander]

Comment on attachment 191596 [details] [diff] [review]
v1: change constant to variable

Um, the actual solution is to avoid circular dependencies, which justdave tells
me won't work under mod_perl anyway.

The fix here is just a hack...

Something else is going wrong during compilation if you're seeing the
"subroutine redefined" message.

Comment 4

[Max Kanat-Alexander]

Attachment: Remove "use Bugzilla" from libraries

OK, I did some testing, and we don't have to actually "use Bugzilla" in any
library.

Either this is because of something in perl that I don't understand, or because
our callers have always already said "use Bugzilla."

For some reason, this also caused a "used only once" warning for $::userid in
editcomponents.cgi, so I changed that to Bugzilla->user->id.

This patch does fix the SHUTDOWNHTML_EXEMPT warning, I tested it with a patch
that used to trigger it.

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 192131 [details] [diff] [review]
Remove "use Bugzilla" from libraries

I'm not sure we're on the right track here. I wonder whether we're about to
start doing something like global variables on a perl module level.

I think every module should be self-contained in the sense that it should make
sure itself that all requirements are met. It should not rely (or have to rely)
on pre-imports.

Consider this script:
--------------
#!/usr/bin/perl -wT

use strict;
use lib '.';
use Bugzilla::Product;

print new Bugzilla::Product(1)->name();
--------------

This script runs on an unpatched Bugzilla, but it does not run any more after
the patch. It runs again if we |use Bugzilla;|. From the script itself, it is
not obvious why this should be done.

Comment 6

[Max Kanat-Alexander]

The problem we're running into seems to be a perl bug, as far as I can tell.

"Constant subroutine redefined" warnings are thrown if perl actually loads and
compiles a module more than once. Ideally, it should never have to compile
Bugzilla.pm more than once, but something about the nature of the circular
dependency causes it.

One way or another, if we want to eliminate circular dependencies, we're going
to have to at some point rely on pre-imports like this one, as far as I know,
particularly for Bugzilla.pm.

Comment 7

[Frédéric Buclin]

Looks like we should get rid of Bugzilla.pm completely.

Comment 8

[Frédéric Buclin]

Increasing the severity to blocker. Looks like any new 'use Bugzilla::xxx' makes
runtests.pl to fail, or even worse, return a software error.

Note that some strange errors like
"deprecated" is not defined in %Bugzilla::DB::EXPORT_TAGS at Bugzilla/Token.pm
line 34
are appearing now too (testcase: add 'use Bugzilla::Token in User.pm). Maybe is
it due to the redefinition of import() in Bugzilla::DB::import()? If yes, then
that's another bug, but note that the order of the 'use' statements now becomes
important.

Comment 9

[Dave Miller [:justdave]]

Attachment: use2dot.pl - generate dot file from "use Bugzilla::foo" lines

The attached script will generate a dot file of the dependencies (it's hacked
to only count Bugzilla::* modules) when pointed at a directory.

This is loosely based on the script found at
http://lists.gnu.org/archive/html/guile-user/2001-03/msg00055.html and modified
to work on Perl.

Usage:

cd /path/to/bugzilla
perl use2dot.pl > ~/BugzillaDeps.dot
dot -Tpng ~/BugzillaDeps.dot -o ~/BugzillaDeps.png

Comment 10

[Dave Miller [:justdave]]

Attachment: Graph of current deps

Comment 11

[Dave Miller [:justdave]]

This graph doesn't catch things done with "require", but there are some things
that stand out at me on it:

Bugzilla::Flag and Bugzilla::Attachment cross-depend on each other
Bugzilla::Util and Bugzilla::Error cross-depend on each other
Bugzilla::User::Setting depends on itself???

Comment 12

[Dave Miller [:justdave]]

(In reply to comment #11)
> Bugzilla::User::Setting depends on itself???

OK, this was bogus. :)  It was an example in the pod docs. :)  A few tricks to
get the script to ignore the pod docs and that self-reference is gone.

The cross-depends on the other two are still there though.

Comment 13

[Dave Miller [:justdave]]

See http://www.justdave.net/BugzillaDeps.png for the current rev of the graph.

Comment 14

[Max Kanat-Alexander]

> Bugzilla::Util and Bugzilla::Error cross-depend on each other
> Bugzilla::User::Setting depends on itself???

   It seems like Bugzilla::Error and Bugzilla::Util do actually need each
other... perhaps one of them could "require" the other.

  I really don't understand how perl could be the one modern language where
mutual dependency is a problem. :-( And why hasn't this ever caused us a problem
before? There must be something we're missing...

Comment 15

[Dave Miller [:justdave]]

Modules cross-depending on each other is a recipe for disaster. :) (In most
languages)

The only thing Bugzilla::Error needs Bugzilla::Util for is html_quote.

the only other places in the entire code that use html_quote are:
a) places that haven't been templatized yet
b) the quoteUrls sub in globals.pl.
c) one of the upggrade routines in checksetup.pl?

There's three possible solutions for this:
1) move html_quote into Bugzilla::Template
2) define it separately in each file that uses it (hey, it is only 3 or 4 lines
of code)
3) Fix Bugzilla::Util to return an error condition to the caller, and make the
caller responsible for Throw*Error on bad results.

#3 is probably the best long-term solution as Bugzilla::Util should not need to
know whether it's in a CGI environment or not.
#1 or #2 is probably the easiest.

Comment 16

[Max Kanat-Alexander]

Comment on attachment 192131 [details] [diff] [review]
Remove "use Bugzilla" from libraries

<justdave> also, if we can guarantee that a module is already in the global
namespace, there's no reason to invoke it via "use" from one of the other
modules unless we're importing subs from it
<justdave> we don't export any subs from Bugzilla.  We only use abstract class
methods
<justdave> and as long as all the CGIs use it, it's safe to assume it's present
from all of the other modules.

Comment 17

[Frédéric Buclin]

Comment on attachment 192131 [details] [diff] [review]
Remove "use Bugzilla" from libraries

>Index: editcomponents.cgi

>         my $series = new Bugzilla::Series(undef, $product, $component,
>-                                          $sdata->[0], $::userid, 1,
>+                                          $sdata->[0], Bugzilla->user->id, 1,
>                                           $sdata->[1], 1);


This change should not be part of this patch. I denied review about this change
already in another patch. Please update this patch with this change removed or
make sure it will be removed on checkin!

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 192131 [details] [diff] [review]
Remove "use Bugzilla" from libraries

ok, this doesn't seem to break anything, and it makes the dependency graph look
a LOT cleaner.

Comment 19

[Dave Miller [:justdave]]

(In reply to comment #5)
> Consider this script:
> --------------
> #!/usr/bin/perl -wT
> 
> use strict;
> use lib '.';
> use Bugzilla::Product;
> 
> print new Bugzilla::Product(1)->name();
> --------------
> 
> This script runs on an unpatched Bugzilla, but it does not run any more after
> the patch. It runs again if we |use Bugzilla;|. From the script itself, it is
> not obvious why this should be done.

Another option here is to make the Bugzilla module able to create and return
various of the other objects, so that "use Bugzilla" is all you ever need.

use Bugzilla;

my $product = Bugzilla->product(1)->name();

Comment 20

[Dave Miller [:justdave]]

Comment on attachment 192131 [details] [diff] [review]
Remove "use Bugzilla" from libraries

>Index: editcomponents.cgi
>-                                          $sdata->[0], $::userid, 1,
>+                                          $sdata->[0], Bugzilla->user->id, 1,

Please leave this chunk out when you check it in, it'll conflict with bug
304075, and it'll make more sense to do it there anyway since that's the point
of that bug.

Comment 21

[Dave Miller [:justdave]]

We're moving on other stuff that depends on this on IRC so I'm going to go ahead
and check it in.

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.89; previous revision: 1.88
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.48; previous revision: 1.47
done
Checking in Bugzilla/Classification.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v  <-- 
Classification.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Group.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Group.pm,v  <--  Group.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Milestone.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Milestone.pm,v  <--  Milestone.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/Version.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Version.pm,v  <--  Version.pm
new revision: 1.6; previous revision: 1.5
done