169580 - Selectively add to "Add/Remove Software" list

Status: VERIFIED FIXED

(Core Graveyard :: Installer: GRE, defect)

Description

[Curt Patrick (gone)]

Do NOT make an entry in the Windows "Add/Remove Software" listing for MRE when
MRE is installed by an app.  It is the app's responsiblity to uninstall mre. 
Only when MRE is installed by a user directly (i.e., without using the /app
command-line option) should the entry be made.

Comment 1

[Curt Patrick (gone)]

Attachment: Patch 1

Comment 2

[Curt Patrick (gone)]

Comments concerning Patch 1:

I added a new keyword called CONDITION.  The idea is that the CONDITION
represents some condition which must be true in order for the action to be
taken.  Currently the only CONDITION which I have defined is
CONDITION=DefaultApp, which means that the DefaultApp must be performing the
install.  But we can define other conditions as necassary and add the code to
evaluate those conditions to the MeetCondition() function.

I should mention that I thought long and hard about just moving the code that
registers the product to be added to the "Add/Remove Programs" into the engine
code instead of doing it in the config.ini.  It sort of makes sense to me that
the engine should provide such a basic functionality as this is.  But logic for
decryping [WINDIR] and logic for confirming Windows registry restraints and such
would have had to be duplicated or engine, too, and this is all working and
tested config.ini functionality so I decided to leave it where it is.  

Comment 3

[[e-mail] (away - not reading bugmail)]

Comment on attachment 100325 [details] [diff] [review]
Patch 1

r=jbetak

Curt, the only nit I could think of are some whitespace changes. Did you mean
to changes these, or was it an oversight?

-; Run Mode values:
+; Run Mode values:  

- GetPrivateProfileString(szSection, "Root Key", "", szBuf, sizeof(szBuf),
szFileIniConfig);
+GetPrivateProfileString(szSection, "Root Key",  "", szBuf,	   
sizeof(szBuf),	     szFileIniConfig);

-GetPrivateProfileString(szSection, "Root Key", "", szBuf, sizeof(szBuf),
szFileIniConfig);
+GetPrivateProfileString(szSection, "Root Key",  "", szBuf,	   
sizeof(szBuf), szFileIniConfig);

Comment 4

[Curt Patrick (gone)]

Well, 2 out of 3.  The latter two cases I intentionally added white space so the
parameter list would line up in columns with the new line I added following. 
The first one, though, has some unintentional white space added which I will
make go away.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 100325 [details] [diff] [review]
Patch 1

>+; Condition must be true, i.e., DefaultApp--in this case, the user instead of an app--
>+;   must be installing.
>+Condition=DefaultApp

Any chance of reusing the "Criterion ID" syntax from the
RunApp section? I hate the name, but this is essentially
the same thing. You can't re-use the code, unfortunately,
since that's buried in the RunApp function itself.

If you like "Condition" better could you change the RunApp section?

What sort of syntax are you expecting in the future? Here
we have two instances of the same type of thing, I'm sure that
means we'll need it more and more in the future. Instead of
making it up as we go along let's figure out something that
makes sense to grow with. Your function name seems to imply
you forsee such growth in use as well.

What if, like RunApp, you have two mutually exclusive cases,
depending on true and false values? The RunApp case makes up
a second key (ugly imho), perhaps you could use '!' to mean
not, or the explicit word "not DefaultApp" or "notDefaultApp".

What if you need something to depend on some boolean combination
of two keys? try to parse "DefaultApp && FooSetting" ? Set up
Condition0 and Condition1 and loop through (would imply 'and')?

I know you just want to get it working, but an inconsistent
syntax will make it harder and harder for each new person
that has to maintain these files.

>+HRESULT MeetCondition(LPSTR szCondition)

This function is pretty magical at first glance. Please
add comments explaining what this means. Like supported
values for szCondition, why comparing szBuf and sfProduct.szAppID
means anything, and why you return true when the arg
szCondition didn't appear to match anything.

You probably want an early bail-out for the special case
szCondition = "" since that'll be the most common one.

>-    if(TimingCheck(dwTiming, szSection, szFileIniConfig))
>+    if(TimingCheck(dwTiming, szSection, szFileIniConfig) && MeetCondition(szCondition))

Hm, it's probably more flexible if you give MeetCondition a syntax
similar to TimingCheck -- MeetCondition(szSection, szFileIniConfig).
Then we can beef up the condition checking syntax later (multiple
conditions, use in other sections, etc) without further uglifying
the current function with additional GetPrivateProfileStrings
that are irrelevant to the main task at hand.

Comment 6

[Curt Patrick (gone)]

You are correct.  This can be implemented with the same syntax as CriterionID. 
Of course, I think I'm the one who came up with that keyword so you've really
hurt my feelings by not liking it.  :-)  Actually, I just think of "Condition"
when we were doing RunApp or I might have gone with it.  As it is, I don't think
it is worth going through and changing it.  Maybe.

So my thinking with MeetCondition(), or MeetCritereon() as we may want to call
it now, was that we could give a name to any condition we care about which can
only be known at run time and define it in this function.  I was thinking that
conditions are independant of sections in the config.ini, but if we add the idea
that we can different sections may want operate depending upon whether the
condition is true or false, then it does become section specific.  So, I agree,
that it makes sense to pass in the szSection parameter.

As for complex conditions, I think we can just give them their own Criterion
ID's and define the logic complex logig in MeetCriterion().  This could get
messy if there end up being lots of permutations of AND's and OR's to give
Criterion ID's to, but I'm going to be really surprised if that happens.  I
think we will probably run into a new criteon that we need to define only
infrequently and it will probably apply only to a single section and in the same
way all the time.

So here is what I come up with:


CONFIG.INI
-----------
In the config.ini we support the following two keywords:

Criterion ID=  and  Perform If Criterion=  (We'll need to rename this from "Run
App If Criterion=" for the RunApp section.)


MEETCRITERION():
---------------
We'll write HRESULT MeetCriterion(LPSTR szSection, LPSTR szCriterion) to behave
as follows:

There will be a section (an if block) for each legal szCriterion, defining how
to determine whether the Criterion is met.  These sections will always result in
a TRUE or FALSE result.  TRUE if the criterion is met.  FALSE if the criterion
is not met.

For the time being we'll support two Criterion:
- "RecaptureHP", which is met if the user has checked the "Recapture Homepage"
(for RunApp sections)
- "IsDefaultApp", which is met if the app performing the installation is the app
defined as the DefaultApp in the config.ini.

Having determined whether the basic criteon has been met, MeetCriterion() will
look up the "Perform If Criterion=" setting from the relevant section of the
config.ini and will return the XOR of these two (i.e. result_of_basic_criterion
XOR perform_if_value).

In a nutshell, the function will return true or false depending upon whether the
section should be should or should not be performed.  That will determined based
upon whether the rules for the Criterion ID have been met, and which case
"Perform If Criterion=" says we should perform the section.

I'll make sure that each Criterion block has good comments describing the
Criterion ID in plain English.


OTHER CONSIDERATIONS:
--------------------
I'll rewrite the RunApp() function to use MeetCriterion().

Comment 7

[Curt Patrick (gone)]

All new mini-spec after further talks with Dan:

CONFIG.INI
-----------
In the config.ini we continue to support the keyword Criterion ID= but eliminate 
the confusing keyword Run App If Criterion=.  Instead we'll have a rule that
preceeding the criterion id with "not " (notice the space) will negate the
critereon.  So for the criterion "RecaptureHP", both of the following are
acceptable:

Criterion ID=IsDefaultApp
Criterion ID=not IsDefaultApp

MEETCRITERIA():
---------------
We'll write HRESULT MeetCriteria(LPSTR szSection) to behave
as follows:

There will be a section (an if block) for each criterion which is supported,
defining how to determine whether the criterion is met.  These sections will
always result in a TRUE or FALSE result.  TRUE if the criterion is met.  FALSE
if the criterion is not met.

For the time being we'll support two Criterion:
- "RecaptureHPChecked", which is met if the user has checked the "Recapture
Homepage" (for RunApp sections)
- "IsDefaultApp", which is met if the app performing the installation is the app
defined as the DefaultApp in the config.ini.

Having determined if the basic criterion has been met:
If the criterion has been prepended with "not " 
  return the the opposite of result
Else we return result

I'll make sure that each Criterion block has good comments describing the
Criterion ID in plain English.


OTHER CONSIDERATIONS:
--------------------
I'll rewrite the RunApp() function to use MeetCriteria().

Comment 8

[Curt Patrick (gone)]

Attachment: Patch2

This matches the last mini-spec except that it turned out to be no more work
really to use "Condition" instead of "Critereon" and it really does seem much
clearer.  So that is what I went with.

Comment 9

[Curt Patrick (gone)]

I just thought of something that maybe should be changed.  The patch works fine
as long as we allow only one condition.  I expect that at some time in the
future we may want to support stringing conditions together into something like
"condition1 and condition2", in which case pointing into szBuf isn't going to
work because szBuf gets reused inside the if blocks.  So if/when we support
compound conditions we'll need to introduce a more permanent string other than
szBuf, I think. Of course the whole subroutine will need rewritten then anyway,
but I wonder if I should introduce the new string now just so it doesn't trip
someone up when they go to expand the functionality?

Comment 10

[Curt Patrick (gone)]

Attachment: NS Patch2

I think this is the only product impacted the config.ini syntax change.

Comment 11

[Curt Patrick (gone)]

Attachment: NS Patch2

I think this is the only product impacted the config.ini syntax change.

Comment 12

[Curt Patrick (gone)]

Comment on attachment 101602 [details] [diff] [review]
NS Patch2

Oops.  Double click (about an hour apart).

Comment 13

[[e-mail] (away - not reading bugmail)]

Comment on attachment 101499 [details] [diff] [review]
Patch2

r=jbetak

Comment 14

[[e-mail] (away - not reading bugmail)]

Comment on attachment 101560 [details] [diff] [review]
NS Patch2

r=jbetak

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 101560 [details] [diff] [review]
NS Patch2

sr=dveditz

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 101499 [details] [diff] [review]
Patch2

>+  BOOL bResult = TRUE;

I think this should be FALSE. If someone puts in a condition we
don't know about then how could it be true?

>+  // See if "not " is prepended to the condition.
>+  if(pszCondition[3] == ' ')
>+  {
>+    pszCondition[3] = '\0';
>+    if(lstrcmpi(pszCondition, "not") == 0)
>+    {

This seems a bit clunky... please use _strnicmp() instead, or don't
be so flexible about case and do strncmp() (which is actually my
preference). e.g
   #define SIZEOF_NEGATION 4
   if (strncmp( pszCondition, "not ", SIZEOF_NEGATION) == 0)
      bNegateTheResult = TRUE;
      pszCondition = szBuf+SIZEOF_NEGATION;
   }

>+  if(strcmpi(pszCondition, "DefaultApp") == 0)

Does this need to be case insensitive? Ditto in the Recapture case.

>+  if(bNegateTheResult == TRUE)
>+    return !bResult;
>+  else
>+    return bResult;

Mozilla style would say to junk the else clause after an "if foo return" case.
Comparing a boolean variable to TRUE/FALSE, especially one using the Hungarian
'b' notation, is likewise discouraged.

Comment 17

[Curt Patrick (gone)]

Attachment: Patch3

Addresses Dan's points.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 101642 [details] [diff] [review]
Patch3

>+  if(strncmp(pszCondition, "not ", SIZEOFNOTSTRING) == 0)
>+  {
>+    bNegateTheResult = TRUE;
>+    pszCondition = pszCondition + 4;

The remaining magic '4' should be SIZEOFNOTSTRING too.

>+  else if(strcmpi(pszCondition, "RecaptureHPChecked") == 0)

This one should be strcmp() also. Whether the one inside the DefaultApp block
should be is up to you.

With those changes sr=dveditz

Comment 19

[Curt Patrick (gone)]

This was checked in on Friday, i.e. 10/4.

Comment 20

[Charles Rosendahl]

Using the 10082002 Build, this is the behavior that I see:

Using a clean system (no traces of prior installations), I install from the
command line:

gre-win32-installer /ms /app "foo" /app_path "C:\x.txt"

The installation succeeds, and only "foo" is in the applist within the registry.

However, the GRE entry is still added to the add/remove programs list.  Clicking
on the entry in the list results in no action.

Expected result:
The entry should not be present in the add/remove programs list.

Comment 21

[Curt Patrick (gone)]

Yup.  You are correct.  I noticed this myself with today's build and am looking
into it.  I sure *thought* I had it working.

Comment 22

[Curt Patrick (gone)]

Attachment: Additional Patch 2

The patch I checked in failed to take for two reasons:

1) I made a last minute change in MeetCondition() so that the default return
value would be FALSE if we receive a condition that is not defined. 
Unwittingly that flip-flopped the logic for the DefaultApp condition and (yes,
I got caught) I did not retest sufficiently.

2) On top of that, the patch was incomplete anyway.  It did not include the
patch for the GRE installer's config.it so which turned the DefaultApp
condition on.

This patch addresses both of this issues.

Comment 23

[Curt Patrick (gone)]

BTW, I created bug #173839 to track the uninstall equivalent of this bug.  Until
that bug is addressed uninstall will not behave consistent with the installer's
new behavior.  (Probably should have addressed both the installer and
uninstaller in the same bug but didn't.)

Comment 24

[[e-mail] (away - not reading bugmail)]

Comment on attachment 102546 [details] [diff] [review]
Additional Patch 2

makes sense :-)

r=jbetak

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 102546 [details] [diff] [review]
Additional Patch 2

sr=dveditz

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 102546 [details] [diff] [review]
Additional Patch 2

a=asa for checkin to 1.2beta (on behalf of drivers)

Comment 27

[Curt Patrick (gone)]

checked in

Comment 28

[Charles Rosendahl]

20021017 Build.

I see that the entry into add/remove programs is now created only when I install
as GREUser.

I take it that bug #173839 will remove the entry from the list once fixed by
either removing the GREUser entry from the registry and then the entry from the
control panel (if other apps are registered in the applist), or by removing the
entire installation if it is the last entry in the applist.