256151 - checksetup.pl should provide --help output

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, enhancement)

Description

[Tobias Sager]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040707 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040707 Firefox/0.8

For testing reasons during upgrade checksetup.pl should have a --dry-run or
--test option.
This would allow a first glance at the steps that are considered and possible
missing modules.

Besides, it would be very userfriendly if checksetup.pl had a --help option
(even if there would be not much to tell the user!) as simply trying
`./checksetup.pl --help` runs the update on DB and templates...

Reproducible: Always
Steps to Reproduce:

Comment 1

[GavinS]

Note that there is already a --check-modules option, which I think will just do
the module checking

Comment 2

[Tobias Sager]

Didn't know about that. It's not documented anywhere, is it?

Comment 3

[Tobias Sager]

Attachment: checksetup.pl help page

This patch adds a short help page to checksetup.pl. What do you think about it?

I put in all the options I found while browsing the code. Did I miss any?

Comment 4

[Christian Reis]

Morphing.

Comment 5

[Christian Reis]

Comment on attachment 156574 [details] [diff] [review]
checksetup.pl help page


Nice idea!

>Index: checksetup.pl
>+    print "$programname - checks your setup and updates Bugzillas environment\n";
>+    print "\nUsage: $programname [SCRIPT [--no-silent]] [--check-modules¦--help]\n";
>+    print "\n";
>+    print "    SCRIPT          Name of non-interactive perl script. (See comments in $programname.)\n";

How about placing this one at the end of the list and actually including the
comments (perhaps in a synthesized form)? Maybe:

  Name of script to drive non-interactive mode. 

  This script should define an %answer hash whose 
  names are tags and whose values are answers to 
  all the questions checksetup.pl asks.

Any idea what tags are? Maybe that should be clarified there as well.

>+    print "   --no-silent      Silent mode. Only with non-interactive mode.\n";

Weird. --no-silent indicates.. Silent mode?!

Maybe 

  Only applies to non-interactive mode?

if that is indeed correct.

Comment 6

[Gervase Markham [:gerv]]

--check-modules was designed for the first step in the install process, but the
docs haven't been updated to use it yet...

Gerv

Comment 7

[Tobias Sager]

Attachment: checksetup.pl.patch (the second)

Implemented ideas from comment 5.

Are the params clearer now?

Comment 8

[Christian Reis]

Comment on attachment 156723 [details] [diff] [review]
checksetup.pl.patch (the second)

>Index: checksetup.pl
>+sub help_page {
>+    my $programname = $0; $programname =~ s#^\./##;

No need to use the same line here, is there? :)

>+    print "$programname - checks your setup and updates Bugzilla environment\n";

and "updates your Bugzilla installation"?

>+    print "   --help           Display this help.\n";

"this help text"

>+    print "   --check-modules  Only check for correct module dependencies and quit thereafter.\n";
>+    print "                    Do not perform any changes.\n";

You can use a ";" to join these phrases 

   Only check for correct module dependencies and quit thereafter; 
   does not perform any changes

>+    print "    SCRIPT          Name of script to drive non-interactive mode.\n";
>+    print "                    This script should define an \%answer hash whose\n"; 
>+    print "                    keys are variable names and the values answers to\n";
>+    print "                    all the questions checksetup.pl asks.\n";
>+    print "                    (See comments at top of $programname for more info.)\n";
>+    print "   --no-silent      Disable default silent mode of non-interactive processing.\n";

I hate this flag. It should be --verbose, of course -- a double negative is
just insane. If you don't want to fix that, just use

  --no-silent		  Output results of SCRIPT being processed

r=kiko with those changes, just attach and add my r+ if you agree with all
comments; otherwise, wait for comments :)

Comment 9

[Tobias Sager]

(In reply to comment #8)
> I hate this flag. It should be --verbose, of course -- a double negative is
> just insane. If you don't want to fix that, just use
> 
>   --no-silent		  Output results of SCRIPT being processed

Can change it to verbose, no problem.
Should I keep parsing for no-silent for backwards compatibility or simply drop it?


> r=kiko with those changes, just attach and add my r+ if you agree with all
> comments; otherwise, wait for comments :)

I'm fine with these. Will attach new patch soon.

Comment 10

[Tobias Sager]

Attachment: checksetup.help.patch

Implementing comment 8 as detailed in comment 9.

Comment 11

[Marc Schumann [:Wurblzap]]

Comment on attachment 156723 [details] [diff] [review]
checksetup.pl.patch (the second)

Removing r? from obsolete attachment.

Comment 12

[GavinS]

Attachment: checksetup.help.patch v2 (fixing dodgy char)

Unless something funny went on with my patch downloading, or patching, there
was a funny pipe character (i.e. looked like a pipe, but wasn't) in the
previous patch.

This simple fixes that one character.

Comment 13

[GavinS]

Comment on attachment 158175 [details] [diff] [review]
checksetup.help.patch

Removing review request on obsolete patch

Comment 14

[GavinS]

Kiko said to set his r+ on the fixed patch, so asking for approval on the back
of that.

It looks OK to me as well, and I would r+ it, but seeing as I just attached the
typo-fixed patch, I'd better not. If a 3rd party wants to r+ as well, then
please do...

Comment 15

[Tobias Sager]

(In reply to comment #14)
> It looks OK to me as well, and I would r+ it, but seeing as I just attached the
> typo-fixed patch, I'd better not. If a 3rd party wants to r+ as well, then
> please do...

What are your doubts?

Comment 16

[GavinS]

(In reply to comment #15)
> (In reply to comment #14)
> > It looks OK to me as well, and I would r+ it, but seeing as I just attached the
> > typo-fixed patch, I'd better not. If a 3rd party wants to r+ as well, then
> > please do...
> 
> What are your doubts?

Nothing at all related to the contents of the patch, just a procedural issue in
case someone saw that I attached the patch and also set r+, which is frowned upon :)

Comment 17

[Vlad Dascalu]

Comment on attachment 168899 [details] [diff] [review]
checksetup.help.patch v2 (fixing dodgy char)

+    $silent = !grep(/^--no-silent$/, @ARGV) || !grep(/^--verbose$/, @ARGV);

This is incorrect. If I specify --no-silent but I don't specify --verbose, then
!grep(/^--no-silent$/, ...) will be false and !grep(/^--verbose$/, ...) will be
true, and $silent will therefore be true despite my --no-silent flag.

Comment 18

[Vlad Dascalu]

One solution to that might be to replace || with &&.

Comment 19

[GavinS]

Attachment: checksetup.pl help v3

Changes as per Vlad's suggestion

Comment 20

[Vlad Dascalu]

Comment on attachment 170010 [details] [diff] [review]
checksetup.pl help v3

+    print "   --verbose	Output results of SCRIPT being processed.\n";
+
+	 
+print "\n";

This one is way off regarding identation.

Looks good to me otherwise.

Comment 21

[Vlad Dascalu]

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

Comment 22

[Dave Miller [:justdave]]

--no-silent is stupid for people to get used to.  I like --verbose.

checked in on the 2.18 branch:

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