Re: Patch for unattended setup (updated)

[Brian Dessent <brian at dessent dot net>]

"Dr. Frank Lee" wrote:

> I attach a re-working of my previous patch to setup.exe to support the '-p
> package1[,package2[...]]' command line option. This patch also intercepts
> all
> message boxes and returns some default values to the calling function for
> unattended installations. A patched setup.exe will return
> IDS_REBOOT_REQUIRED
> (=118) if a reboot seems desirable.
> 
> This patch applies to setup version 2.578 (the most recent I could
> compile).
> 
> (http://www.sp.phy.cam.ac.uk/~rl201/cygwin-setup-unattended-patch.diff
> contains
> this patch and http://www.sp.phy.cam.ac.uk/~rl201/setup-patched.exe the
> resulting executable - usual caveats about running such binaries apply!)

Sorry but there are a number of problems with this.

1. Please don't send a patch with CRLF line endings when the file has LF
line endings.

2. Patches need to be against current CVS, not an 8 month old version. 
For example, CVS setup has already had a -p option (for specifying proxy
options) for several months, so you need to pick a different name.  Or
stated differently: if, when applied to HEAD, your patch won't compile
then it's a serious problem with the patch that needs fixing, since it
can only be applied to HEAD.  I just uploaded a 2.588 snapshot yesterday
so I know that HEAD builds fine.

3. You need to follow the the GNU coding style, e.g. opening brace on
its own line, 2 space indents, space before '(' and on both sides of
'='.  I know that not all of setup is consistent this way, but it
doesn't help to make the problem worse by introducing more.

4. There is no ChangeLog entry.

5. This looks like a messy work in progress; hunks like these are
unacceptable:

-static bool rebootneeded;
+// RFL static bool rebootneeded;

+// RFL
+#include "state.h"
+

+      //case MB_CANCELTRYCONTINUE:
+      //  return IDCONTINUE;
+      //  break;

+  // RFL suspects this code should really be in the _custom_MessageBox
routine but code
+  // placed there doesn't get executed (for the 'Files in-use have been
replaced' message,
+  // at least.
+  if (unattended_mode) {

In other words, if something needs to be removed, it should be removed
not commented out.  Also, patches should not have marker/vanity
comments.

Brian