280573 - checksetup fails in Bugzilla::Auth BEGIN block

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Colin Ogilvie [:cso]]

After the checkin of bug 278792 I can not run checksetup.pl on Windows (can't
verify on Linux).

Prior to this checkin, it works as expected.

No value for param user_verify_class (try running checksetup.pl again) at
Bugzilla/Config.pm line 162.
BEGIN failed--compilation aborted at Bugzilla/Auth.pm line 45.
Compilation failed in require at checksetup.pl line 1473.
BEGIN failed--compilation aborted at checksetup.pl line 1473.

Comment 1

[Max Kanat-Alexander]

That's weird. All parameters are supposed to be defined by the time I inserted
that module in checksetup. user_verify_class should already exist...

But apparently, if I delete the data/ directory and run checksetup, it doesn't work.

We can't re-open bug 278792 and back it out, because other code that depends on
it has already been checked-in.

So I'll have to look into this ASAP.

Comment 2

[Max Kanat-Alexander]

OK, so I just need to move bz_crypt to Bugzilla::Util instead of Bugzilla::Auth. :-(

Comment 3

[Max Kanat-Alexander]

Attachment: "require" instead of "use" Bugzilla::Auth

Actually, as justdave pointed out, we should only "require" in checksetup,
never "use."

Comment 4

[Dave Miller [:justdave]]

Comment on attachment 173026 [details] [diff] [review]
"require" instead of "use" Bugzilla::Auth

how are you getting bz_crypt into checksetup.pl's namespace?  I think you might
need an import here.  (require doesn't automatically import exported symbols
like use does)

Comment 5

[:glob ✱]

Comment on attachment 173026 [details] [diff] [review]
"require" instead of "use" Bugzilla::Auth

> # It's safe to use Bugzilla::Auth here because parameters have now been
> # defined.
>-use Bugzilla::Auth;
>+require Bugzilla::Auth;

nit:  i don't think the comment is valid anymore.

r=glob with the comment removed.

Comment 6

[Dave Miller [:justdave]]

(In reply to comment #5)
> (From update of attachment 173026 [details] [diff] [review] [edit])
> r=glob with the comment removed.

Does it actually work?  (See comment 4)

Comment 7

[Dave Miller [:justdave]]

ok, as discussed on IRC, we should just remove that section altogether for now.
 It's already getting pulled in via globals.pl, which is why it actually works
without the import.  It either needs to have the import added so as not to
confuse people later, or just remove it altogether.  I'd vote for removing it at
this point.  We can put it back when globals.pl actually goes away.

Comment 8

[Dave Miller [:justdave]]

make sure you do the tweak per comment 7 before checking in.  You can post a
revision here if you want, preapproved pending review as long as that's all you
do to it.

Comment 9

[Max Kanat-Alexander]

Attachment: Do "require" and "import"

OK, here I do require *and* import.

Comment 10

[Max Kanat-Alexander]

  Oh, also:

(In reply to comment #5)
> nit:  i don't think the comment is valid anymore.

  The comment is still very valid -- it's not safe to "require" Bugzilla::Auth
before parameters are defined. Perhaps the comment should say "require" instead
of "use", but I don't think that's too important.

Comment 11

[Max Kanat-Alexander]

Also, for whoever does checkin:

<mkanat> justdave: By the way, I submitted a new patch that does both "require"
and "import."
<mkanat> justdave: I'd rather leave it in there, so it's clearer what's going on
for the person (maybe not me, who knows) who removes globals.pl.
<justdave> that was on the list of acceptible things to do with the patch :)

Comment 12

[Shane H. W. Travis]

Given all the comments on this patch and how this has to be changed or left or 
modified on checkin... I'd rather just have a new patch please.

Comment 13

[Shane H. W. Travis]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.334; previous revision: 1.333
done