236664 - on Win32 checksetup.pl prints wrong install instructions for Perl modules

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Andrei Benea]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1) Opera 7.50  [en]
Build Identifier:  

On Windows, checksetup.pl prints commands like
perl -MCPAN -e'install "Date::Format"'
instead of
ppm install TimeDate
 

Reproducible: Always
Steps to Reproduce:

Comment 1

[Vlad Dascalu]

Andrei: bugzilla.org is used for bugs related to the bugzilla.org website. :)

Comment 2

[Andrei Benea]

Attachment: v1

Comment 3

[Vlad Dascalu]

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

+my %ppm_modules = (

Well I guess that works too, although we could have added them in the $modules
variable.

+	 return "ppm install " . $module;
+    } else {

Do we need the "else" here? The "if" branch already exited until now. 

+    if ($^O !~ /MSWin32/i) {
...
+    } else {
+	 print "If you want to see pretty HTML views of patches, you should ";
+	 print "install the \nPatchReader module:\n";
+	 print "PatchReader: " . install_command("PatchReader") . "\n";
+    }

Why do we use the install_command in a Win32-only block? Shouldn't
install_command be platform-independent by design? Couldn't we use PatchReader
to display the install instructions for the Linux branch as well?

r=vlad with those questions answered or addressed in version 2.

Comment 4

[Vlad Dascalu]

s/PatchReader/install_command/ (in the previous comment)

Comment 5

[Andrei Benea]

(In reply to comment #3)
> (From update of attachment 143177 [details] [diff] [review])
> +my %ppm_modules = (
> 
> Well I guess that works too, although we could have added them in the $modules
> variable.

The $modules variable is for required packages, but we need to translate the 
optional packages also.

> +	 return "ppm install " . $module;
> +    } else {
> 
> Do we need the "else" here? The "if" branch already exited until now. 

Well, I thought it is more clear this way.

> +    if ($^O !~ /MSWin32/i) {
> ...
> +    } else {
> +	 print "If you want to see pretty HTML views of patches, you should ";
> +	 print "install the \nPatchReader module:\n";
> +	 print "PatchReader: " . install_command("PatchReader") . "\n";
> +    }
> 
> Why do we use the install_command in a Win32-only block? Shouldn't
> install_command be platform-independent by design? Couldn't we use PatchReader
> to display the install instructions for the Linux branch as well?

I'll switch to linux to see if perl -MCPAN works for PatchReader, and if it does 
I'll make another patch.
 
> r=vlad with those questions answered or addressed in version 2.
> 

Comment 6

[Andrei Benea]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 143177 [details] [diff] [review])
> I'll switch to linux to see if perl -MCPAN works for PatchReader, and if it 
does 
> I'll make another patch.

I tried installing like this, but it doesn't work. I think I have answered all 
the questions.

Comment 7

[Dave Miller [:justdave]]

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

>+    print "Most ActivePerl modules are available at Apache ppm repository.\n";

This should be "at the Apache ppm repository" or "at Apache's ppm repository"

>+    print "located at http://www.apache.org/dyn/closer.cgi/perl/win32-bin/ppms/\n";
>+    print "You can add the repository with the following command:\n";
>+    print "    ppm rep add apache http://apache.roweboat.net/perl/win32-bin/ppms/\n\n";
>+}

I hadn't seen this one mentioned anywhere before...  Can we post this to
developers and get comments on whether this should be an officially recommended
repository to use?

>+    if ($^O !~ /MSWin32/i) {
>+        print "If you want to see pretty HTML views of patches, you should ";
>+        print "install the \nPatchReader module, which can be downloaded at:\n";
>+        print "http://search.cpan.org/CPAN/authors/id/J/JK/JKEISER/PatchReader-0.9.2.tar.gz\n";
>+        print "When you get it, do the following to install:\n";
>+        print "tar xzvf PatchReader-0.9.2.tar.gz\n";
>+        print "cd PatchReader-0.9.2\n";
>+        print "perl Makefile.PL\n";
>+        print "make install\n\n";

PatchReader is available via CPAN now.	We should fix this to use
install_command like everything else.

Comment 8

[Andrei Benea]

(In reply to comment #7)
> (From update of attachment 143177 [details] [diff] [review])
> >+    print "Most ActivePerl modules are available at Apache ppm repository.
\n";
> 
> This should be "at the Apache ppm repository" or "at Apache's ppm repository"
> 
> >+    print "located at http://www.apache.org/dyn/closer.
cgi/perl/win32-bin/ppms/\n";
> >+    print "You can add the repository with the following command:\n";
> >+    print "    ppm rep add apache http://apache.roweboat.
net/perl/win32-bin/ppms/\n\n";
> >+}
> 
> I hadn't seen this one mentioned anywhere before...  Can we post this to
> developers and get comments on whether this should be an officially 
recommended
> repository to use?

I had a hard time finding the maintainer these modules. (I found the repository 
on google). It seems to be related to their Perl subproject.

> 
> >+    if ($^O !~ /MSWin32/i) {
> >+        print "If you want to see pretty HTML views of patches, you should 
";
> >+        print "install the \nPatchReader module, which can be downloaded at:
\n";
> >+        print "http://search.cpan.
org/CPAN/authors/id/J/JK/JKEISER/PatchReader-0.9.2.tar.gz\n";
> >+        print "When you get it, do the following to install:\n";
> >+        print "tar xzvf PatchReader-0.9.2.tar.gz\n";
> >+        print "cd PatchReader-0.9.2\n";
> >+        print "perl Makefile.PL\n";
> >+        print "make install\n\n";
> 
> PatchReader is available via CPAN now.	We should fix this to use
> install_command like everything else.
> 

I tried 
perl -MCPAN -e'install "PatchReader"'
but on my Mandrake 10 it says
Going to read /root/.cpan/Metadata
  Database was generated on Thu, 02 Jan 2003 10:16:18 GMT
Warning: Cannot install PatchReader, don't know what it is.
Try the command

    i /PatchReader/

to find objects with matching identifiers.

Comment 9

[Dave Miller [:justdave]]

(In reply to comment #8)
> > I hadn't seen this one mentioned anywhere before...  Can we post this to
> > developers and get comments on whether this should be an officially 
> > recommended repository to use?
> 
> I had a hard time finding the maintainer these modules. (I found the
> repository on google). It seems to be related to their Perl subproject.

Sorry, I wasn't clear enough.  I meant the [e-mail] mailing list.
 This PPM repository isn't officially supported by ActiveState, thus we need
some discussion on it before just blinding starting to recommend that people use it.

> > PatchReader is available via CPAN now.	We should fix this to use
> > install_command like everything else.
> 
> I tried 
> perl -MCPAN -e'install "PatchReader"'
> but on my Mandrake 10 it says
> Going to read /root/.cpan/Metadata
>   Database was generated on Thu, 02 Jan 2003 10:16:18 GMT
                              ^^^^^^^^^^^^^^^^
Your CPAN database is WAY out of date.  It's supposed to download a new one
automatically if the current one is more than 24 hours old.  I think your CPAN
is broken.

Comment 10

[Andrei Benea]

Attachment: v2

Unix: PatchReader is handled through CPAN now.
Windows: Changed the message explaining how to add a ppm repository.

Comment 11

[Vlad Dascalu]

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

r=vlad assuming you got the green-light from justdave regarding the repository.

Comment 12

[Andrei Benea]

(In reply to comment #7)
> I hadn't seen this one mentioned anywhere before...  Can we post this to
> developers and get comments on whether this should be an officially 
recommended
> repository to use?

The suggested repository requires ActivePerl 5.8. So until someone finds a 
better repository or Bugzilla requires 5.8, the patch will sit here waiting to 
bitrot...

Comment 13

[Andrei Benea]

Attachment: v3

The recommended repository now dependends on the ActivePerl version.

Comment 14

[Vlad Dascalu]

Comment on attachment 143813 [details] [diff] [review]
v3

+	 print "The required ActivePerl modules are available OpenInteract's
ppm repository.\n";

That doesn't make much sense in English. You could make that "are available at
OpenInteract's".

Probably you can make "The required" --> "Most" for consistency's sake as well,
since you're at it.

+	 print "    ppm rep add oi
http://openinteract.sourceforge.net/ppmpackages\n\n";

That URL returns a HTTP redirect code. Probably adding a slash at the end, like
in Apache's case, will solve the issue.

The patch looks good (except the English text mentioned at the beginning of
this comment). You'll need justdave to approve the repository URLs.

Comment 15

[Andrei Benea]

Attachment: v4

Comment 16

[Dave Miller [:justdave]]

Looks good to me. :)  People can feel free to complain later if the PPM repo is
bad ;)

Comment 17

[Vlad Dascalu]

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