507493 - checksetup.pl's output should use colors for missing and too old Perl modules

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, enhancement, P1)

Description

[Frédéric Buclin]

Since we released Bugzilla 3.4 two days ago, a lot of people jump on IRC complaining that their upgrade doesn't work. When we ask them to paste the output of checksetup.pl on pastebin, we can see that the problem is most of the time because they have a missing or too old required Perl module. I think using colors to highlight missing or too old modules would help a lot. justdave suggested to use Term::Cap for this.

I'm suggesting to use red for missing or too old required modules, and orange for optional ones.

Comment 1

[Max Kanat-Alexander]

Probably best to use Term::ANSIColor and set $ENV{ANSI_COLOR_DISABLED} if the terminal doesn't support colors.

Comment 2

[Frédéric Buclin]

(In reply to comment #0)
> I'm suggesting to use red for missing or too old required modules, and orange
> for optional ones.

orange is actually not a legal color for Term::ANSIColor (which is indeed very easy to use, so let's use it!), but yellow is. In my shell, yellow appears orange, so that's fine. :-D

Comment 3

[Frédéric Buclin]

Term::ANSIColor doesn't work on Windows (I tested on Windows 2000 and Windows 7). But it works great if Win32::Console::ANSI is installed (including Windows 2000), which is unfortunately not installed by default (but is available via PPM). So we could use colors on Windows if eval { use Win32::Console::ANSI; } is successful, else set $ENV{ANSI_COLOR_DISABLED}.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

Tested successfully on Linux, Windows 2000 and Windows 7 (with and without Win32::Console::ANSI installed).

I also added two strings to make problems/completion more obvious.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 392977 [details] [diff] [review]
patch, v1

Okay, looks cool!

A few things:

When our STDOUT is not capable of ANSI Colors, we should not be outputting the codes for them (for example, if we're being redirected to a file). I suspect there is some way to determine this, and I suspect that the Internet knows. :-)

Let's not print the "Installation Complete" thing.

Let's use red for all important messages--yellow is too hard to read in some terminal color schemes.

On Windows, the instructions about adding the PPM repo should also be in red.

The Installation Aborted message should always be displayed--even if we are $silent. Also, it should be printed inside of Bugzilla::Install::Requirements, not inside of checksetup.pl itself.

It might be best, also, to set ANSI_COLORS_DISABLED inside of a subroutine with "local", in case this is loaded by mod_perl or otherwise loaded during runtime.

Comment 6

[:glob ✱]

> When our STDOUT is not capable of ANSI Colors, we should not be outputting the
> codes for them (for example, if we're being redirected to a file). I suspect
> there is some way to determine this, and I suspect that the Internet knows.

$outputIsTerminal = -t *STDOUT;

Comment 7

[Frédéric Buclin]

Attachment: patch, v2

I fixed everything you said, besides putting $ENV{...} in a subroutine and using local. I don't know where you want it to be called from, as it should be called only once. I tested redirecting the output of checksetup.pl to a file, and color characters are correctly skipped. I also tested on Windows.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 393512 [details] [diff] [review]
patch, v2

  Okay, looks good, but:

>+eval { ON_WINDOWS && require Win32::Console::ANSI; };
>+$ENV{'ANSI_COLORS_DISABLED'} = 1 if ($@ || !-t *STDOUT);

  Let's create a Bugzilla::Install::Util::init_console routine, and this plus the get_console_locale call in checksetup.pl can go there. We could even call it in Bugzilla.pm for USAGE_MODE_CMDLINE scripts.

Comment 9

[Frédéric Buclin]

Attachment: patch, v3

Implement Bugzilla::Install::Util::init_console().

Comment 10

[Max Kanat-Alexander]

Comment on attachment 393758 [details] [diff] [review]
patch, v3

Looks great!

Also, I was thinking about your "Installation completed" message, and actually, I think it's good to let people know that checksetup has finished, because sometimes they don't know! I think perhaps it should just say "checksetup.pl complete." And it should indeed be in green, and it should not show up when $silent.

Comment 11

[Max Kanat-Alexander]

Anyhow, you can add the "complete" message on checkin or I can add it afterward.

Thanks for this, by the way! I think this is going to help a lot! :-)

Comment 12

[Frédéric Buclin]

I will let you add the "complete" message, so that you can decide where to put it (either checksetup.pl or one of Bugzilla::Install::*).

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.564; previous revision: 1.563
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.66; previous revision: 1.65
done
Checking in Bugzilla/Install/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Util.pm,v  <--  Util.pm
new revision: 1.19; previous revision: 1.18
done
Checking in template/en/default/setup/strings.txt.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/setup/strings.txt.pl,v  <--  strings.txt.pl
new revision: 1.13; previous revision: 1.12
done

Comment 14

[Max Kanat-Alexander]

Added to the release notes in bug 547466.

Comment 15

[Frédéric Buclin]

I filed bug 559549 to add the "Installation complete" message.