158033 - misc commandline args for unix install wizard (help and ignore-run-apps)

Status: VERIFIED FIXED

(SeaMonkey :: Installer, defect)

Description

[Andrew Schultz]

The unix installer does not handle most of the commandline arguments that the
windows installer does.  This bug is for -h (help) and -ira (Ignore Run Apps). 
Both should be simple to implement and (if all goes well), I will attach a patch
here soon.

see also bug 157672 -- handle -ms (silent install)

Comment 1

[Andrew Schultz]

Attachment: patch

handle -h (help) and -ira (ignore run app sections)

the patch bails if there is no help message in config.ini, which doesn't seem
like a real good idea, but I couldn't find any fallback in the windows
implementation either.	the help message should be in the same format as the
windows one... something like:

UsageMsg Usage=Usage: %s [options]%s	[options] can be any of the following
combination:%s		  -h:		  This help.%s
		-ma:		Run setup in Auto mode.%s		-ms:   
	Run setup in Silent mode.%s		-ira
:		Ignore the [RunAppX] sections%s

Comment 2

[Andrew Schultz]

Attachment: patch v1.1

windows doesn't have a fallback, but doesn't bail.  
now, if usage is missing from config.ini, the installer still installs ok.   if
"-h" is specified, it fails silently.

Comment 3

[Andrew Schultz]

dveditz, can you review the patch here?

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 91905 [details] [diff] [review]
patch v1.1

>+    int     mIgnoreRunAppX;

Later on you have essentially "Don't ignore RunApps". Could we
reverse the sense and default true to avoid the double-negative?
(The command-line -ira is OK for consistency with windows, just
nitpicking variable names)

>+#define USAGE               "UsageMsg Usage"

Just "UsageMsg" would work, right? Don't understand what the second
instance of Usage gets you.

By the way, where are the config.it changes with this message
in it?

>     for (int argNum = 1; argNum < aArgc; ++argNum)
>     {
>+        /* Print usage
>+         */
>+        if (strcmp(aArgv[argNum], "-h") == 0)

Let's add  "|| strcmp(aArgv[argNum], "--help") == 0)" just to
fit in with GNU standard practice.

>+	    if (gCtx->opt->mUsageMsg)
>+                fprintf (stderr, gCtx->opt->mUsageMsg, aArgv[0], "\n", 
>+		      "\n", "\n", "\n", "\n", "\n", "\n", "\n", "\n");

Why all the separate newline args? I guess I'd need to see
the actual message used here, but it's hard to imagine your purpose.

>+    err = aParser->GetStringAlloc(GENERAL, USAGE, &usage, &size);
>+    if (err == OK && size > 0)
>+    {
>+        gCtx->opt->mUsageMsg = usage;

Where does this get freed?

Comment 5

[Andrew Schultz]

> Later on you have essentially "Don't ignore RunApps". Could we reverse the 
> sense and default true to avoid the double-negative?

> Just "UsageMsg" would work, right? Don't understand what the second instance 
> of Usage gets you.

consistency with Windows on both counts (although I agree 100%).  Windows has
gbIgnoreRunAppX and "UsageMsg Usage"

> Why all the separate newline args? I guess I'd need to see the actual message 
> used here, but it's hard to imagine your purpose.

Ya, I left out the config.ini changes.  The message has "%s" where a newline is
desired.  The Windows wizard does the same.

>>+        gCtx->opt->mUsageMsg = usage;
>
> Where does this get freed?

the same place as the other gCtx->opt strings (mTitle, mDestination and
mProxy*).  Never-never land.

Comment 6

[Andrew Schultz]

Attachment: patch v1.2

patch with dveditz's suggestions

Comment 7

[Jeremy M. Dolan]

Adding myself to cc, my autoinstall script needs this. dveditz, have time for
another review?

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 100688 [details] [diff] [review]
patch v1.2

I'm sorry I didn't notice the new patch. Feel free to mail me reminders, I
quite often miss things in teh deluge of bugzilla mail.
r=dveditz 

If you get a review from Samir you can convert my r= to sr=

Comment 9

[Andrew Schultz]

Attachment: patch v1.2a

oops.  diff against packager's config.it instead of wizard's sample config.ini

Comment 10

[Andrew Schultz]

Comment on attachment 103021 [details] [diff] [review]
patch v1.2a

carrying forward r=dveditz

Comment 11

[Samir Gehani]

Comment on attachment 103021 [details] [diff] [review]
patch v1.2a

I don't think we need to expose the usage message in the config.ini.  The
command line argument interpretation can only change if you recompile so usage
should just be hardcoded.  For localization purposes the strings (usage
message) should come out of installer.ini.

One other nit: use the ``TRUE'' and ``FALSE'' macro definitions instead of 0
and 1 for boolean values of mDoRunAppX.  And while we are here maybe an apt
name would be ``mShouldRunApps'' rather than the double-verbed ``mDoRunAppX.''

>+    mDoRunAppX(1),

Also, I'm not sure I understand why this error is ``unknown'': we could just
define a new error E_USAGE_DISPLAYED (or something more brief) which would be
more descriptive.

Comment 12

[Andrew Schultz]

Attachment: patch v1.3

usage message string ==> installer.ini
uses mShouldRunApps and TRUE/FALSE
E_UNKOWN ==> E_USAGE_SHOWN (?)

Comment 13

[Samir Gehani]

Comment on attachment 106063 [details] [diff] [review]
patch v1.3

Please explain what auto mode and silent mode mean: be explicit about the level
of UI being shown.  With that change, 
r=sgehani

Comment 14

[Andrew Schultz]

Attachment: patch 1.3a

use more explicit descriptions for different UI levels

Comment 15

[Andrew Schultz]

Comment on attachment 108574 [details] [diff] [review]
patch 1.3a

carrying forward r=sgehani

Comment 16

[Matthias Versen [:Matti]]

[e-mail]: you got a SR from dveditz with comment#8 .
Do you need soneone for the checking ?
 

Comment 17

[Andrew Schultz]

> Do you need soneone for the checking ?

yes.

Comment 18

[Matthias Versen [:Matti]]

Comment on attachment 108574 [details] [diff] [review]
patch 1.3a

marking sr+ (see comment #8)

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 108574 [details] [diff] [review]
patch 1.3a

a=asa (on behalf of drivers) for checkin to 1.4b.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 21

[Paul Wyskoczka]

updated qa contact to [e-mail]

Comment 22

[Grace Bush]

verified - mozilla build 2003042505

with one nit- running commercial installer with -h (for help) displays the help
options twice (does not do that with mozilla  installer)
-ira option works 

Comment 23

[Grace Bush]

verified, opened bug 23772 for problem with commercial installer