344617 - Move data about optional requirements into Bugzilla::Install::Requirements

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, enhancement)

Description

[Max Kanat-Alexander]

We've moved the mandatory requirements stuff into Bugzilla::Install::Requirements, now we just have to get the optional stuff in there.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

This patch requires the patch from bug 344612.

We move all the optional requirement data into Requirements.pm, including Net::LDAP. This means that we check for Net::LDAP just like any other module, which I've always thought we should do anyway. It's a bit odd to check for it only *after* it would be needed.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 229196 [details] [diff] [review]
v1

Wicked, I just need you to 2xr this.

Comment 3

[Colin Ogilvie [:cso]]

Comment on attachment 229196 [details] [diff] [review]
v1

>diff -u checksetup.pl checksetup.pl
>--- checksetup.pl	14 Jul 2006 00:27:05 -0000
>+++ checksetup.pl	14 Jul 2006 01:00:54 -0000
>+if (!$have_mod{'Net::LDAP'} && !$silent) {
>+    print "If you wish to use LDAP authentication, then you must",
>+          " install Net::LDAP:\n",
>+          "Net::LDAP: " . install_command('Net::LDAP') . "\n";
>+}

Nit: Could this have two \n's at it as the output on my install has it directly above the "Checking user setup..." line.

   /usr/bin/perl -MCPAN -e 'install "Image::Magick"'

If you wish to use LDAP authentication, then you must install Net::LDAP:
Net::LDAP: /usr/bin/perl -MCPAN -e 'install "Net::LDAP"'
Checking user setup ...

Comment 4

[Teemu Mannermaa (:wicked)]

Attachment: v1.1

Original patch didn't apply cleanly (hunks 1 and 4 of checksetup.pl) because mkanat's original code used, for some reason, have_vers calls that had $silent as a parameter.

Here's a corrected patch against current CVS tip and also with fixed Colin's nit about output. As such, this can be committed as is.

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> mkanat's original code used, for some reason, have_vers calls that had $silent
> as a parameter.

Read comment 1. Adding 344612 as a blocker to make things clear.

Comment 6

[Teemu Mannermaa (:wicked)]

(In reply to comment #1)
> This patch requires the patch from bug 344612.

OK, this explains the problem I saw. I don't like these kinds of requirements between patches. This patch is now ready and that isn't so what we do? Wait until this is reviewed patch bitrotted or what?

Comment 7

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

Before this gets approval, can the assignee decide either to go with version 1 (and then fix bug 344612 first) or to go with version 1.1 (and then remove the dependency on bug 344612)?

Comment 8

[Max Kanat-Alexander]

(In reply to comment #6)
> OK, this explains the problem I saw. I don't like these kinds of requirements
> between patches. 

  I actually asked you to review the $silent patch more than a day before this one, I believe, but I didn't get any response so I switched it to bkor, who also hasn't reviewed it, even though it's a major bug.

> This patch is now ready and that isn't so what we do? Wait
> until this is reviewed patch bitrotted or what?

  Review the other one, which is extremely simple, and then review this one and check it in.

Comment 9

[Max Kanat-Alexander]

Okay, bug 344612 is now also pending approval, so we can check this one in--the first patch, not the second.

Comment 10

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.510; previous revision: 1.509
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.5; previous revision: 1.4
done

And then I also fixed Colin's nit, which I remembered after I did the checkin:

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