286701 - Tests fail compiling Bugzilla::DB::Pg.pm

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect, P2)

Description

[Frédéric Buclin]

(08:18:08) mkanat: LpSolit: The problem is that *your system* does not have the
necessary modules installed to run the testsuite, even though you have the
necessary modules installed to run Bugzilla.
(08:18:40) travis_home: mkanat: Does the test suite test for the correct version
of things that Checksetup labels as optional? (I honestly don't know... I'm
thinking of some of the graphing stuff.)
(08:19:00) LpSolit: I don't think Mandrake has rpm for Pg :(
(08:19:02) mkanat: travis_home: The test suite actually currently doesn't check
modules at all, but we have a bug filed for that.
(08:19:17) mkanat: LpSolit: Really? It's called perl-DBD-Pg in RH...
(08:20:03) travis_home: mkanat: okay, but the question remains as a future
issue, then: *will* the test suite be testing for current versions of optional
modules, and failing tests if people don't have them?
(08:20:20) travis_home: because if so, that seems to me to break the definition
of 'optional'
(08:20:39) LpSolit: mkanat: no such rpm :(
(08:20:51) mkanat: LpSolit: Hrm...
(08:21:05) mkanat: travis_home: I suppose that's an interesting point.
(08:21:14) mkanat: travis_home: So perhaps we should do a require instead of a
use, there.
(08:21:25) mkanat: travis_home: I'd agree to that; feel free to file a bug for it.
(08:23:05) mkanat: It's a one-line change.
(08:23:14) mkanat: Just change the "use DBD::Pg" to "require DBD::Pg".
(08:23:19) mkanat: You don't even need an import statement.
(08:23:28) mkanat: Just add a comment about why the require is happening instead
of a use.
(08:24:07) LpSolit: mkanat: in which file is this "use DBD::Pg"?
(08:24:37) LpSolit: Bugzilla/DB/Pg.pm ?
(08:24:40) mkanat: LpSolit: Bugzilla/DB/Pg.pm
(08:28:17) LpSolit: mkanat: does this change should make runtests.pl happy?
(08:28:25) mkanat: LpSolit: It should.
(08:28:35) mkanat: LpSolit: Since runtests.pl will then never see a need for
DBD::Pg.
(08:28:53) mkanat: LpSolit: require happens at runtime, while use happens at
compile-time.

Comment 1

[Frédéric Buclin]

FYI, changing "use DBD::Pg" to "require DBD::Pg" in Bugzilla/DB/Pg.pm, line 45,
reduces the error message when running runtests.pl to:

#     Failed test (t/001compile.t at line 81)
not ok 86 - Bugzilla/DB/Pg.pm --ERROR
Bareword "DBD::Pg::PG_BYTEA" not allowed while "strict subs" in use at
Bugzilla/DB/Pg.pm line 50.
BEGIN not safe after errors--compilation aborted at Bugzilla/DB/Pg.pm line 51.

:(

Comment 2

[Max Kanat-Alexander]

Then we have to keep the "use" in there, or wrap the use constant in an eval,
which I don't really want to do.

This makes me tempted to WONTFIX this bug, and have developers/testers be
required to install PostgreSQL.

However, I can't imagine that we'll require testers to have Oracle installed,
and this same situation might pop up.

So perhaps we should just wrap both the constant and the use in an eval{} block.

Comment 3

[Tomas Kopal]

(In reply to comment #2)
> Then we have to keep the "use" in there, or wrap the use constant in an eval,
> which I don't really want to do.
> 
> This makes me tempted to WONTFIX this bug, and have developers/testers be
> required to install PostgreSQL.
> 
> However, I can't imagine that we'll require testers to have Oracle installed,
> and this same situation might pop up.
> 
> So perhaps we should just wrap both the constant and the use in an eval{} block.


I am afraid even that will not help. The problem is that if the compiler does
not see the "use", it does not know anything about the Pg namespace and will
keep complaining about the PG_BYTEA being a bareword.
You can enclose the PG_BYTEA line in eval, and use 'require' to include the
driver. That should fix the error, but it's ugly.
Can't we somehow get rid of the PG_BYTEA constant and hence dependency on
Pg::DBD? Maybe re-defining it (which is prone to problems if the constant in Pg
ever changes)?

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> I am afraid even that will not help. The problem is that if the compiler does
> not see the "use", it does not know anything about the Pg namespace and will
> keep complaining about the PG_BYTEA being a bareword.

  No, it ought to be nice about that inside of an eval block. At the worst, we
can put it in an eval *string*. Or maybe just the PG_BYTEA part of the constant.

Comment 5

[Marc Schumann [:Wurblzap]]

We could check for all DBD::xyz (xyz being mysql, Pg, and maybe oracle and
others at some time), and depending on what we find, we'd decide which of the
Bugzilla::DB::*.pm to test.

Comment 6

[Frédéric Buclin]

*** Bug 288198 has been marked as a duplicate of this bug. ***

Comment 7

[Gervase Markham [:gerv]]

As Marc says - the test should not attempt to compile files which could not
possibly be used. 001compile.t needs to look to see what DBD drivers are
installed. That's the only scalable way. I certainly don't want to (and can't
afford to ;-) install Oracle.

Gerv

Comment 8

[GavinS]

Attachment: in-elegant incomplete start

This is what I started with. It now reports that it is doing one less test than
it expects.

Comment 9

[GavinS]

Attachment: ignore a BZ DB module if the corresponding DBD module is not installed v2

Comment 10

[Max Kanat-Alexander]

FWIW, I'm not convinced this is the right solution.

This will cause some Tinderboxen to say that the codes passes runtests when
Pg.pm is borked, and it will do it silently.

Comment 11

[Jacob Steenhagen]

(In reply to comment #10)
> This will cause some Tinderboxen to say that the codes passes runtests when
> Pg.pm is borked, and it will do it silently.

The obvious solution to that problem is that we have a Tinderbox client that
runs with Pg.pm instead of Mysql.pm. We should probably do that anyway once we
get to the point where the tests can do more than just simply make sure files
exist and compile.

Comment 12

[Marc Schumann [:Wurblzap]]

(In reply to comment #10)
> FWIW, I'm not convinced this is the right solution.
> 
> This will cause some Tinderboxen to say that the codes passes runtests when
> Pg.pm is borked, and it will do it silently.

The idea is to do tests for all DBD modules _installed_. Install all DBD
modules, and you get all Bugzilla::DB modules tested.

Comment 13

[Max Kanat-Alexander]

(In reply to comment #12)
> The idea is to do tests for all DBD modules _installed_. Install all DBD
> modules, and you get all Bugzilla::DB modules tested.

  OK, fair enough.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 179212 [details] [diff] [review]
ignore a BZ DB module if the corresponding DBD module is not installed v2

Jouni said that he'd like to do test suite reviews... :-)

Comment 15

[Jouni Heikniemi]

Comment on attachment 179212 [details] [diff] [review]
ignore a BZ DB module if the corresponding DBD module is not installed v2

>+foreach my $module ( grep( !/^Perl$/, $installed->modules() ) )
>+{
>+    if ($module =~ /DBD::(.*)$/) {
>+        $installed_DBD_modules{lc($1)} = 1;
>+    }
>+} 

Umm, no. For me,
perl -e 'use ExtUtils::Installed; foreach my $x
(ExtUtils::Installed->new()->modules()) { print "$x\n"; }
prints
--
Bundle::NetSNMP
Image::Magick
Perl
--

... and you can be sure that's not the whole story of installed modules. Now
even MySQL is getting skipped!

By the way, that grep statement in the foreach line is quite unnecessary... The
next regex will do quite nicely anyway.

>+    if ($file =~ /Bugzilla\/DB\/([^\/]*).pm$/ &&

Don't use / separators for path regexes, it's unreadable and thus error prone.
Find the mistake (actually two) above!

How about: #Bugzilla/DB/([^/]+)\.pm$# (fixed both the * and the unescaped dot).

Comment 16

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

"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave

Comment 17

[Marc Schumann [:Wurblzap]]

(In reply to comment #16)
> "If it's not a regression from 2.18 and it's not a critical problem with
> something that's already landed, let's push it off." - Dave

Myk, imho, this *is* a regression from 2.18 in the sense that at that time all
tests ran successfully and now they don't. Please reconsider making this a 2.20
blocker.

Comment 18

[Frédéric Buclin]

I agree with Marc, this a blocker IMO.

Comment 19

[Frédéric Buclin]

Gavin, ping? No new patch for a long time... :(

Comment 20

[GavinS]

(In reply to comment #19)
> Gavin, ping? No new patch for a long time... :(

Sorry! Been on holiday, and slow to get back into things...

I had a look at this last night. Since the last patch, I've upgraded to OSX
Tiger, which comes with a different version of Perl, which must have messed
around with my install and .packlist files, because ExtUtils no longer reports
all my modules (as Jouni said happened for him.)

Can any Perl experts explain to me either:

a) how to get extutils to correctly report the modules installed/in use, or

b) a better way of checking for installed modules

Comment 21

[Colin Ogilvie [:cso]]

(In reply to comment #20)
> b) a better way of checking for installed modules

Specificially for DBI you could try:

use DBI;
use Data::Dumper;
my @drivers = DBI->available_drivers;
print Dumper(@drivers);

Comment 22

[Colin Ogilvie [:cso]]

Attachment: Patch v3

Patch using my technique from the previous comment.

ok 85 - Bugzilla/DB/Mysql.pm
ok 86 - Bugzilla/DB/Pg.pm - Skipping, as the DBD module not installed

All tests successful.
Files=1, Tests=93, 79 wallclock secs ( 0.00 cusr +  0.00 csys =  0.00 CPU)

Comment 23

[Colin Ogilvie [:cso]]

Attachment: Patch v3 (Unix Line Endings)

LPSolit pointed out that the previous patch had windows line-endings, so heres
one without them.

Comment 24

[Max Kanat-Alexander]

Comment on attachment 184488 [details] [diff] [review]
Patch v3 (Unix Line Endings)

OK, but this solution is a little fragile, in the way that if we add any other
Bugzilla::DB files that aren't drivers, we'll have to modify this code. Also,
there's always a chance that we could have a driver that wasn't named after its
DBI equivalent (although this would only happen in some strange case), but
that's not something we have now, so I'm not too worried, yet.

Really, we should have had a Bugzilla::DB::Driver directory, probably.

Comment 25

[Frédéric Buclin]

reassigning to patch's author...

Comment 26

[Max Kanat-Alexander]

Checking in t/001compile.t;
/cvsroot/mozilla/webtools/bugzilla/t/001compile.t,v  <--  001compile.t
new revision: 1.13; previous revision: 1.12
done