initial custom action library, "hello, world" quality

This code is adapted from the WiX custom action library. As a disclosure, I really hate the code quality. Nevertheless, it does provide an initial version to rewrite, something I expect will be faster than trying to deal with CA's from scratch.

I've cut down most of the WiX library cruft (the stuff in "dutil"). There's only six of the ~50 source files needed at all, and I've removed most of the code in those. The wca* files are its more specific CA library. I've left that largely intact, even though we'll end up stripping it down eventually. For now there are a number of debugging function in there we might find useful during development; I don't know yet.

As will be immediately apparent, the WiX team is mostly ex-Microsoft people, and they code like it. Most everything is declared 'extern "C"' so they can stay close the Windows calling conventions, so it's mostly really C code rather than C++. There's no RIAA, for example, even for some obvious stuff like some of the string allocation they use. There are also way too many global variables. Personally, I'd treat this code as FYI, rather than something to spend much time commenting on at this time.

So other than policy, why a review at all? The WiX source "adblockplusie.wxs" has all the modifications needed to link and bind a custom action and its DLL embodiment into the installer. That part works. This version of the installer starts up and immediately fails because the custom action returns an error code. The UI fails (as it should), and the log files show the details. That's the goal of this chunk of work, at that's done. The custom action itself is in "close_application.cpp". It's a mostly-unaltered copy of the corresponding WiX custom action; there's one line to make it always return an error. Nevertheless, it initializes itself correctly otherwise, including all the MSI-specific resources.

I want to commit this on a branch, not only because the installer fails immediately (which isn't, ahem, a shippable product), but also because I'm sure we'll need some experimentation with what UI we want. Not to mention how awful the code quality is.

[Eric, 2013-09-03 12:52:56]

The only file that is really worth review is "adblockplusie.wxs", whose code is
close to what we'll use once finished. All the C++ code is mostly-unaltered WiX
code, much of which needs to be rewritten.

The code for the custom action itself is in close_application.cpp, which has the
algorithm that WiX 'util:CloseApplication' uses, including WM_CLOSE. I've hardly
touched this code. Also of note is that the 'procutil' code (in two .cpp files)
has some ways of manipulating processes by Process ID, which might work better
for the engine.

Please read the review description, as well.

[Felix Dahlke, 2013-09-03 21:32:56]

To summarise, what we need this for is:

1. Ask the user before shutting engine/IE down - with a localised message
2. Actually terminate the process, not just send WM_CLOSE (strictly speaking
optional, adding a message loop to the engine is no big deal)
3. Start IE again right after the installation

Looking through the WiX docs, it actually looks like this is supported out of
the box.

1. The PromptToContinue option of CloseApplication seems to do this
(http://wixtoolset.org/documentation/manual/v3/xsd/util/..%5Cutil%5Ccloseappli...)
- the question is whether the message is translated/translateable
2. The TerminateProcess option of CloseApplication seems to do just this: kill
the process if closing failed
(http://wixtoolset.org/documentation/manual/v3/xsd/util/..%5Cutil%5Ccloseappli...)
- but I think falling back to reboot is actually fine. Looks like the task
manager sends WM_CLOSE before killing too, so we should probably handle WM_CLOSE
for graceful shutdown in the engine anyway.
3. They have docs for this, looks like we can use WixShellExec:
http://wixtoolset.org/documentation/manual/v3/howtos%5Cui_and_localization%5C...

Have you seen this, Eric? I haven't tried any of this, but it does look
promising.

[Felix Dahlke, 2013-09-05 11:52:00]

On 2013/09/03 21:32:56, Felix H. Dahlke wrote:
> To summarise, what we need this for is:
> 
> 1. Ask the user before shutting engine/IE down - with a localised message
> 2. Actually terminate the process, not just send WM_CLOSE (strictly speaking
> optional, adding a message loop to the engine is no big deal)
> 3. Start IE again right after the installation
> 
> Looking through the WiX docs, it actually looks like this is supported out of
> the box.
> 
> 1. The PromptToContinue option of CloseApplication seems to do this
>
(http://wixtoolset.org/documentation/manual/v3/xsd/util/..%255Cutil%255Cclosea...)
> - the question is whether the message is translated/translateable
> 2. The TerminateProcess option of CloseApplication seems to do just this: kill
> the process if closing failed
>
(http://wixtoolset.org/documentation/manual/v3/xsd/util/..%255Cutil%255Cclosea...)
> - but I think falling back to reboot is actually fine. Looks like the task
> manager sends WM_CLOSE before killing too, so we should probably handle
WM_CLOSE
> for graceful shutdown in the engine anyway.
> 3. They have docs for this, looks like we can use WixShellExec:
>
http://wixtoolset.org/documentation/manual/v3/howtos%255Cui_and_localization%...
> 
> Have you seen this, Eric? I haven't tried any of this, but it does look
> promising.

Have looked a bit into this, since I was curious why you didn't consider this so
far. Turns out PromptToContinue and TerminateProcess are new features in WiX 3.8
(current stable is 3.7)

However, I don't think we really need TerminateProcess. WM_CLOSE should work in
most cases, if we handle it in the engine which we should do anyway to support
graceful shutdown.

So PromptToContinue is the major missing thing here. The text is taken from the
Description attribute, so I think the only way to translate it is to have
multiple wxs files, which we could generate automatically. If we can't use this
(e.g. because we don't want to wait for WiX 3.8), we can show the question box
in the wrapper/IE before we even start the installation.

WixShellExec works pretty fine for starting IE after the installation, and
that's supported in 3.7.

So, I'm wondering if we actually need custom actions. Am I missing something?

[Eric, 2013-09-05 14:00:15]

On 2013/09/05 11:52:00, Felix H. Dahlke wrote:
> Have looked a bit into this, since I was curious why you didn't consider this
so
> far. Turns out PromptToContinue and TerminateProcess are new features in WiX
3.8
> (current stable is 3.7)

I've been developing on WiX 3.7. I just verified that PromptToContinue does,
indeed, not appear in the 3.7 code base, so it was indeed off my radar. Also,
you've been reading the new documentation, which is less than a month old (and
has new formatting). It appears that they've put up the documentation that's
more recent than the 3.7 release, but didn't bother to branch 3.8 features in
their version control so that they are presenting accurate documentation for the
current stable release. This seems typical of the WiX team, speaking from my
personal experience.

FYI. The Wayback Machine has the only documentation I saw until a couple of
weeks ago.

I've been assuming that we can't rely on a WiX release schedule for timeliness,
and that we would do better not to rely on pre-release code from these people.
(There's not even a beta release yet for 3.8, if the WiX news page is accurate,
although the download page has weekly builds.) We can revisit these assumptions,
if you like, but I'm hesitant to use anything other than an actual release.

> So, I'm wondering if we actually need custom actions. Am I missing something?

What I was seeing when experimenting with util:CloseApplication before was that
sending WM_CLOSE to IE was sometimes not reliable, in that IE didn't shut down
anywhere near immediately. It always seemed to honor the message, in that it got
itself into a shutting-down state, but in a few cases it took longer to shut
down than CloseApplication was waiting.

We'll have the same issue with shutting down the engine. If the engine is in the
middle of a list download and we ask it to terminate, we either need to (1)
abort the list download or (2) wait for it to finish. The code for (1) isn't
present today in the engine and the code for (2) isn't present in a WiX release
today.

Thus I was assuming that we'd need a CA to get the right thing done, in part by
controlling our own equivalent of PromptToContinue.

[Eric, 2013-09-05 14:01:02]

Hit the wrong button before pasting in the URL.

> FYI. The Wayback Machine has the only documentation I saw until a couple of
> weeks ago.

http://web.archive.org/web/20090827100222/http://wix.sourceforge.net/manual-w...

[Felix Dahlke, 2013-09-05 14:39:32]

On 2013/09/05 14:00:15, Eric wrote:
> FYI. The Wayback Machine has the only documentation I saw until a couple of
> weeks ago.

I didn't mean to say you didn't look at the docs, I suspected that they only put
it up recently.

> I've been assuming that we can't rely on a WiX release schedule for
timeliness,
> and that we would do better not to rely on pre-release code from these people.
> (There's not even a beta release yet for 3.8, if the WiX news page is
accurate,
> although the download page has weekly builds.) We can revisit these
assumptions,
> if you like, but I'm hesitant to use anything other than an actual release.

That's the question, yes. Since I think that the only feature we really _need_
is PromptToContinue, I thought of showing an upfront confirmation in the
wrapper/IE until we can use that. But I think 3.8 is worth a try, would simplify
things.

> > So, I'm wondering if we actually need custom actions. Am I missing
something?
> 
> What I was seeing when experimenting with util:CloseApplication before was
that
> sending WM_CLOSE to IE was sometimes not reliable, in that IE didn't shut down
> anywhere near immediately. It always seemed to honor the message, in that it
got
> itself into a shutting-down state, but in a few cases it took longer to shut
> down than CloseApplication was waiting.

WiX 3.8 has added the Timeout option for that, we can specify an arbitrary
timeout in seconds. But not in 3.7.

> Thus I was assuming that we'd need a CA to get the right thing done, in part
by
> controlling our own equivalent of PromptToContinue.

Yes, that would have been my preferred approach, as opposed to the workaround,
but if it's the only thing we'd need this for....

However, after talking to Wladimir today, I realised that we might need custom
actions for translating the MSI. Working with those MSTs seems awfully tricky.

So I'll review the relevant parts here, and then let's put it in a branch as you
recommended.

[Felix Dahlke, 2013-09-06 16:59:03]

Some remarks. I agree that we need this, if only for experimentation. I also
agree that we should put it into a branch.

How about we put the whole improved-installer endavour into a branch? e.g.
"improved-installer". It's all pretty interrelated.

http://codereview.adblockplus.org/11521026/diff/1/adblockplus.gyp
File adblockplus.gyp (right):

http://codereview.adblockplus.org/11521026/diff/1/adblockplus.gyp#newcode168
adblockplus.gyp:168: },
I'd rather see this custom action code elsewhere, not in src. Likewise, I'd
rather see the target in a separate GYP file.

How about something like this?

/installer/ca/ca.gyp
/installer/ca/src/abp_ca.cpp
...

Or maybe a fancier name:

/installer/abp_wix_util

Naming's up to you.

http://codereview.adblockplus.org/11521026/diff/1/installer/adblockplusie.wxs
File installer/adblockplusie.wxs (right):

http://codereview.adblockplus.org/11521026/diff/1/installer/adblockplusie.wxs...
installer/adblockplusie.wxs:92: Define and schedule a "Custom Action Type 1" to
ensure that IE is closed before installation.
This paragraph describes something that's pretty apparent in the WiX docs, and
it's rather obvious when reading the source IMO. The next paragraph makes sense,
"check" is non-obvious.

[Wladimir Palant, 2013-09-11 12:05:34]

Eric, are you seriously saying that a basic custom action requires 24 source
files, most of which have been taken over from an external project with an
incompatible source code license? A quick googling turned up this example for a
custom action implementation:
http://blogs.technet.com/b/alexshev/archive/2009/05/15/from-msi-to-wix-part-2...

[Felix Dahlke, 2013-10-16 08:21:07]

So it looks like we settled on creating a custom action library that's not based
on the WiX code. I guess this can be closed then?

[Eric, 2013-10-16 14:19:12]

On 2013/10/16 08:21:07, Felix H. Dahlke wrote:
> I guess this can be closed then?
Yep. What small bits will reappear (the .def file, for example) can easily go in
another review.