342736 - checksetup should show mod_perl requirements

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, enhancement)

Description

[Max Kanat-Alexander]

Right now we require mod_perl 1.999022 and CGI 3.11 for mod_perl.

checksetup should say so.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. Pretty simple.

Comment 2

[Vlad Dascalu]

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

There's no point in adding those, until we actually have experimental mod_perl support in trunk.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> (From update of attachment 227064 [details] [diff] [review] [edit])
> There's no point in adding those, until we actually have experimental mod_perl
> support in trunk.

  We basically do now. We will have it in testable condition once all the pending patches are reviewed, I'm pretty sure.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

We also require Apache::DBI, now.

Comment 5

[Frédéric Buclin]

Comment on attachment 228888 [details] [diff] [review]
v2

>Index: checksetup.pl

>+if (!$cgi_mod_perl) {
>+    print "\nmod_perl support requires a newer version of CGI than you have.\n";

This message makes sense to me only if I have the mod_perl module installed. If mod_perl is not installed, I really don't care about this message about CGI, as it's talking about something I don't have. IMO, you should write:

if ($mod_perl && !$cgi_mod_perl)

Moreover, can you explain to me why we require CGI 3.11? Give me a URL to the changelog or something which explains why. I have CGI 3.08 installed and I first want to understand why I should upgrade.


>+    print "You should install the newest version of CGI if you want to\n";
>+    print "use mod_perl:\n";
>+    print "    CGI: " . install_command('CGI') . "\n";

You need to write "\n\n" after install_command(). That's what the other places in checksetup.pl do and this makes the output nicer.


>+if (!$apache_dbi) {

Same comment as above. I don't care about this message if I don't have mod_perl installed.


>+  print "mod_perl support requires a modern version of Apache::DBI be",
>+        "installed:\n\n",
>+        "Apache::DBI: " . install_command('Apache::DBI') . "\n";

Same comment as above: "\n\n".

Comment 6

[Frédéric Buclin]

(In reply to comment #4)
> We also require Apache::DBI, now.

I also don't see where Apache::DBI is used.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #5)
> This message makes sense to me only if I have the mod_perl module installed. If
> mod_perl is not installed, I really don't care about this message about CGI, as
> it's talking about something I don't have. IMO, you should write:
> 
> if ($mod_perl && !$cgi_mod_perl)

  Users complain when we do that. They want to know all the requirements, up front.

> Moreover, can you explain to me why we require CGI 3.11?

  Our version of mod_perl explicitly requires CGI 3.11. Versions of CGI before that do not work with mod_perl 1.999022 and later.

> You need to write "\n\n" after install_command(). That's what the other places
> in checksetup.pl do and this makes the output nicer.

  Okay.

> >+if (!$apache_dbi) {
> 
> Same comment as above. I don't care about this message if I don't have mod_perl
> installed.

  Same response as above.

(In reply to comment #6)
> I also don't see where Apache::DBI is used.

  It's used in the startup script, which I haven't checked-in yet, because I'm still fine-tuning it. But we will require Apache::DBI, trust me.

Comment 8

[Frédéric Buclin]

(In reply to comment #7)

>   Our version of mod_perl explicitly requires CGI 3.11. Versions of CGI before
> that do not work with mod_perl 1.999022 and later.

Weird. I have mod_perl 2.0.1 installed and CGI 3.08. I suppose Mandriva are releasing RPMs which can work together.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Weird. I have mod_perl 2.0.1 installed and CGI 3.08. I suppose Mandriva are
> releasing RPMs which can work together.

  Maybe. CGI is usually packaged with perl, so distros probably don't upgrade the version beyond the perl version. CGI works fine by itself, at 3.08, and mod_perl works fine by itself, but if you want to use them *together*, you have to have CGI 3.11. It's clearly specified in the mod_perl docs, somewhere.

Comment 10

[Frédéric Buclin]

The end of the section in the URL field says:

"If using CGI.pm you will need to upgrade to version 3.11 - versions 3.10 and older contain bugs and versions 3.07 and older do not support support the new mod_perl API."

Comment 11

[Frédéric Buclin]

Comment on attachment 228888 [details] [diff] [review]
v2

>+if (!$mod_perl) {

>+if (!$cgi_mod_perl) {

>+if (!$apache_dbi) {

--silent is ignored. You should have && !$silent.


>+  print "mod_perl support requires a modern version of Apache::DBI be",
>+        "installed:\n\n",

Missing \n after "be" on the first line.


>+        "Apache::DBI: " . install_command('Apache::DBI') . "\n";

As said already, should be "\n\n" after install_command().

Else this looks good, especially now that I know why we want CGI 3.11.

Comment 12

[Max Kanat-Alexander]

Attachment: v3

Okay, I modified it to use Bugzilla::Install::Requirements, and I also made the message more compact, so it's one message for all of mod_perl.

Comment 13

[Max Kanat-Alexander]

Attachment: v3.1

There was a bug with detecting CGI and printing the message, in the previous patch, fixed in this patch.

Comment 14

[Frédéric Buclin]

Comment on attachment 230031 [details] [diff] [review]
v3.1

>Index: checksetup.pl

>+    print "\nIf you would like mod_perl support, you must install at\n",
>+          "least the minimum required version of mod_perl, Apache::DBI,",
>+          " and CGI.\n";
>+
>+          if (!$have_mod{'Apache::DBI'} || !$have_mod{'CGI'}) {
>+              print "You must also install the following module(s) for",
>+                    " mod_perl support:\n";
>+              print "    Apache::DBI: " . install_command('Apache::DBI')
>+                    . "\n" if !$have_mod{'Apache::DBI'};
>+              print "    CGI:         " . install_command('CGI') . "\n"
>+                    if !$have_mod{'CGI'};
>+          }

Nit: my config is:

The following modules are required for mod_perl support:
Checking for       mod_perl2 (v1.999022) ok: found v2.000001
Checking for             CGI (v3.11)    found v3.08
Checking for     Apache::DBI (v0.96)    not found

And so the output of checksetup.pl is:

If you would like mod_perl support, you must install at
least the minimum required version of mod_perl, Apache::DBI, and CGI.
You must also install the following module(s) for mod_perl support:
    Apache::DBI: /usr/bin/perl5.8.7 -MCPAN -e 'install "Apache::DBI"'
    CGI:         /usr/bin/perl5.8.7 -MCPAN -e 'install "CGI"'

In the last sentence, the "also" is awkward IMO, or even the whole sentence is useless, but that's definitely a nit (and english is not my mother language anyway). r=LpSolit

Comment 15

[Max Kanat-Alexander]

Attachment: v4

I agree that that wording was no good. In order to fix the wording I had to fix some logic in checksetup.pl, so I wanted to have you review it again just because I changed around some actual code.

Comment 16

[Max Kanat-Alexander]

Attachment: Actual v4

Attached the wrong version by mistake. Here is the correct one.

Comment 17

[Frédéric Buclin]

Comment on attachment 230072 [details] [diff] [review]
Actual v4

Yeah, looks much better. r=LpSolit

Comment 18

[Max Kanat-Alexander]

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