[PATCH] Two fixes for setup postinstall handling.

[PATCH] Two fixes for setup postinstall handling.

[Dave Korn]

[-- Attachment #1: Type: text/plain, Size: 2215 bytes --]



    Hi all,

  In case it's controversial, I thought I'd post these patches before
committing anything.  The two hunks I've included in one patchfile here are
in fact separate and orthogonal, I just posted them in one file for
convenience.

  The first hunk finds if bash is in the list of packages that have scripts
to run and swaps it with the first entry if so.  You may feel it's crude,
but it's also does-what-it-says-on-the-tin, and it's sane: we can't possibly
run postinstall scripts without a working shell, the postinstall script for
the shell is what guarantees we have what counts as "a working shell" for
that purpose, so why not just deliberately say "Hey, we have to do this
first and we know we have to do it first so let's just do it first"?

  The second hunk checks for the return code from bash that means 'bad
interpreter', i.e. when it has been unable to invoke the executable listed
after a shebang.  We can't know in the general case of an error during
execution whether the script run and/or how close it got to completion
before it failed, but in this one specific case we can know for absolute
certain that it didn't run not even a little bit, so let's do ourselves a
favour and not mark it done.  I think that means that people who have
failures could just re-run and click straight through setup.exe and get the
scripts to run without having to set everything to "Reinstall" as is
currently the case.  I'm not mad keen on a hardcoded 126 in there, but AFAI
could find out there's no public header with bash error codes in it -
someone please correct me if they know better.

  The first hunk could be obviated by doing away with postinstall scripts
for bash, but it a) is perhaps an easier change than respinning a fresh bash
release, and b) is defensive programming - it's always going to be the case
that, if there are any scripts needed to install the shell, they should be
the very first thing we do, and this means that should a situation ever
arise in the future where we find ourselves needing bash postinstall
scripts, they will just work and we won't regress to the situation we find
ourselves in now.



    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....

[-- Attachment #2: setup-postinstall-patch.diff --]
[-- Type: application/octet-stream, Size: 2060 bytes --]

? ChangeLog.new
? libgpg-error/autom4te.cache
Index: postinstall.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/postinstall.cc,v
retrieving revision 2.21
diff -p -u -r2.21 postinstall.cc
--- postinstall.cc	16 Apr 2006 22:26:44 -0000	2.21
+++ postinstall.cc	10 Aug 2008 13:01:38 -0000
@@ -107,6 +107,24 @@ do_postinstall_thread (HINSTANCE h, HWND
 	packages.push_back(&pkg);
       ++i;
     }
+
+  // Hard-coding this isn't great, but the shell is a special case.
+  for (i = packages.begin (); i != packages.end (); ++i)
+    {
+      if ((*i)->name == "bash")
+	{
+	  log (LOG_PLAIN) << "Found bash, swapping to first position." << endLog;
+	  // Found it!  Exchange it with first.
+	  if (i != packages.begin ())
+	    {
+	      packagemeta *temp = *i;
+	      *i = *packages.begin ();
+	      *packages.begin () = temp;
+	    }
+	    break;
+	}
+    }
+
   int numpkg = packages.size() + 1;
   int k = 0;
   for (i = packages.begin (); i != packages.end (); ++i)
Index: script.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/script.cc,v
retrieving revision 2.27
diff -p -u -r2.27 script.cc
--- script.cc	28 Feb 2007 00:55:04 -0000	2.27
+++ script.cc	10 Aug 2008 13:01:38 -0000
@@ -238,12 +238,16 @@ Script::run() const
 
   if (retval)
     log(LOG_PLAIN) << "abnormal exit: exit code=" << retval << endLog;
+
+  /* if the script failed with 'bad interpreter', leave it not done.  */
+  if (retval != 126)
+    {
+      /* if file exists then delete it otherwise just ignore no file error */
+      io_stream::remove ("cygfile://" + scriptName + ".done");
 
-  /* if file exists then delete it otherwise just ignore no file error */
-  io_stream::remove ("cygfile://" + scriptName + ".done");
-
-  io_stream::move ("cygfile://" + scriptName,
+      io_stream::move ("cygfile://" + scriptName,
                    "cygfile://" + scriptName + ".done");
+    }
 
   return retval;
 }

Re: [PATCH] Two fixes for setup postinstall handling.

[Christopher Faylor]

On Sun, Aug 10, 2008 at 02:21:48PM +0100, Dave Korn wrote:
>In case it's controversial, I thought I'd post these patches before
>committing anything.  The two hunks I've included in one patchfile here
>are in fact separate and orthogonal, I just posted them in one file for
>convenience.
>
>The first hunk finds if bash is in the list of packages that have
>scripts to run and swaps it with the first entry if so.  You may feel
>it's crude, but it's also does-what-it-says-on-the-tin, and it's sane:
>we can't possibly run postinstall scripts without a working shell, the
>postinstall script for the shell is what guarantees we have what counts
>as "a working shell" for that purpose, so why not just deliberately say
>"Hey, we have to do this first and we know we have to do it first so
>let's just do it first"?

If this is necessary, I don't like the idea of special casing bash.
Can't we add a keyword to setup.ini so this could be configurable?

>  The second hunk checks for the return code from bash that means 'bad
>interpreter', i.e. when it has been unable to invoke the executable listed
>after a shebang.  We can't know in the general case of an error during
>execution whether the script run and/or how close it got to completion
>before it failed, but in this one specific case we can know for absolute
>certain that it didn't run not even a little bit, so let's do ourselves a
>favour and not mark it done.  I think that means that people who have
>failures could just re-run and click straight through setup.exe and get the
>scripts to run without having to set everything to "Reinstall" as is
>currently the case.  I'm not mad keen on a hardcoded 126 in there, but AFAI
>could find out there's no public header with bash error codes in it -
>someone please correct me if they know better.
>
>  The first hunk could be obviated by doing away with postinstall scripts
>for bash, but it a) is perhaps an easier change than respinning a fresh bash
>release,

?  How is patching, reviewing the patch, rebuilding, and releasing setup.exe 
easier than Eric releasing a new version of bash which just installs sh.exe
unconditionally?

>b) is defensive programming - it's always going to be the case that, if
>there are any scripts needed to install the shell, they should be the
>very first thing we do, and this means that should a situation ever
>arise in the future where we find ourselves needing bash postinstall
>scripts, they will just work and we won't regress to the situation we
>find ourselves in now.

Sorry but I'm not yet convinced that this is necessary.

cgf

RE: [PATCH] Two fixes for setup postinstall handling.

[Dave Korn]

Christopher Faylor wrote on 10 August 2008 16:30:

> On Sun, Aug 10, 2008 at 02:21:48PM +0100, Dave Korn wrote:

>> The first hunk finds if bash is in the list of packages that have
>> scripts to run and swaps it with the first entry if so.

> If this is necessary, I don't like the idea of special casing bash.
> Can't we add a keyword to setup.ini so this could be configurable?

  Sure.  Or we could make it automagic by looking out for sh.exe when it
goes by - using the same string we know we're going to be using to invoke
the scripts later?

>>  The first hunk could be obviated by doing away with postinstall scripts
>> for bash, but it a) is perhaps an easier change than respinning a fresh
>> bash release,
> 
> ?  How is patching, reviewing the patch, rebuilding, and releasing
> setup.exe easier than Eric releasing a new version of bash which just
> installs sh.exe unconditionally?

  Depends how busy Eric is.

>> b) is defensive programming - it's always going to be the case that, if
>> there are any scripts needed to install the shell, they should be the
>> very first thing we do, and this means that should a situation ever
>> arise in the future where we find ourselves needing bash postinstall
>> scripts, they will just work and we won't regress to the situation we
>> find ourselves in now.
> 
> Sorry but I'm not yet convinced that this is necessary.

  Well, it's still "just plain wrong" to rename a script to ".done" when you
know for absolute certain that it hasn't been.


    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....

Re: [PATCH] Two fixes for setup postinstall handling.

[Brian Dessent]

Christopher Faylor wrote:

> If this is necessary, I don't like the idea of special casing bash.
> Can't we add a keyword to setup.ini so this could be configurable?

I don't like this either.  I feel it's not necessary.  If we just fix
the setup.hints for any packages that have postinstalls but don't list
bash in "requires:" then this problem goes away, and it is a fix that
works with any version of setup without requiring any user upgrades.

Brian

RE: [PATCH] Two fixes for setup postinstall handling.

[Dave Korn]

Brian Dessent wrote on 10 August 2008 20:51:

> Christopher Faylor wrote:
> 
>> If this is necessary, I don't like the idea of special casing bash.
>> Can't we add a keyword to setup.ini so this could be configurable?
> 
> I don't like this either.  I feel it's not necessary.  If we just fix
> the setup.hints for any packages that have postinstalls but don't list
> bash in "requires:" then this problem goes away, 

  Until next time.  Then we have to go through this all over again.

> and it is a fix that
> works with any version of setup without requiring any user upgrades.

  I did a crude estimate.

  I used the package-grep feature to find all packages which unpack any
files into etc/postinstall.  Then I fetched their setup hints and grepped
the requires line for bash.

  This is subject to false positives, as it may be an old version (package
grep returns all versions) that had the postinstall script and setup.hint
correctly now doesn't mention bash because the newer package versions lose
the postinstall.  Hoever, as a ball park figure:

  30 packages have it right.  202 have it wrong.

  This suggests to me that relying on a manual system is not working.

    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....

RE: [PATCH] Two fixes for setup postinstall handling.

[Dave Korn]

Dave Korn wrote on 10 August 2008 22:52:

> Brian Dessent wrote on 10 August 2008 20:51:
> 
>> Christopher Faylor wrote:
>> 
>>> Can't we add a keyword to setup.ini so this could be configurable?
>> 
>> ... just fix the setup.hints for any packages that have postinstalls 
>> but don't list bash in "requires:" 

>   This suggests to me that relying on a manual system is not working.

  Of course, we could add bash to the requires line at the server end in
gensetupini or whatever upset is called these days, rather than adding in
the dependency at the setup.exe client end.

    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....

Re: [PATCH] Two fixes for setup postinstall handling.

[Brian Dessent]

Dave Korn wrote:

>   30 packages have it right.  202 have it wrong.
> 
>   This suggests to me that relying on a manual system is not working.

Point taken.  However in the vast majority of those cases the
topological order is not significantly adversely affected, it's only a
select few (or one?) packages that are at issue.   As a short term fix
if we can correct those errant few that do actually cause a scratch
install to go off the rails I think we should do that since it's such an
easy fix that requires no releasing or upgrading anything.

For the long term, we can add special casing for one special package, or
we can transition to a world where that one package does not need
special treatment; or we can do both.  In either case there is a time
delay: in the former, it is users continuing to use older versions of
setup, in the latter it's that we cannot demand or expect immediate
maintainer action on Eric's part.

So I suppose my point then is that we should do the short term fix
regardless.

Brian

Re: [PATCH] Two fixes for setup postinstall handling.

[Christopher Faylor]

On Sun, Aug 10, 2008 at 07:41:03PM +0100, Dave Korn wrote:
>Christopher Faylor wrote on 10 August 2008 16:30:
>
>> On Sun, Aug 10, 2008 at 02:21:48PM +0100, Dave Korn wrote:
>
>>> The first hunk finds if bash is in the list of packages that have
>>> scripts to run and swaps it with the first entry if so.
>
>> If this is necessary, I don't like the idea of special casing bash.
>> Can't we add a keyword to setup.ini so this could be configurable?
>
>Sure.  Or we could make it automagic by looking out for sh.exe when it
>goes by - using the same string we know we're going to be using to
>invoke the scripts later?

My point is that I don't like hardcoding the string "sh.exe" into
setup.exe.

>>Sorry but I'm not yet convinced that this is necessary.
>
>Well, it's still "just plain wrong" to rename a script to ".done" when
>you know for absolute certain that it hasn't been.

No argument there.  Maybe that part of the change needs to go in
separately since it sounds like a genuine bug fix.

Btw, I'm happy that you're thinking about this problem.  Thank you.

cgf

Re: [PATCH] Two fixes for setup postinstall handling.

[Christopher Faylor]

On Sun, Aug 10, 2008 at 03:04:36PM -0700, Brian Dessent wrote:
>Dave Korn wrote:
>
>>   30 packages have it right.  202 have it wrong.
>> 
>>   This suggests to me that relying on a manual system is not working.
>
>Point taken.  However in the vast majority of those cases the
>topological order is not significantly adversely affected, it's only a
>select few (or one?) packages that are at issue.

If the one is _update-info-dir then it already relies on bash and I
changed the postinstall script to use /bin/bash rather than /bin/sh.

>As a short term fix if we can correct those errant few that do actually
>cause a scratch install to go off the rails I think we should do that
>since it's such an easy fix that requires no releasing or upgrading
>anything.

Yes, especially since these are real problems that, IMO, really should
be fixed.

I keep hoping that I will eventually fix my package lint script and move
towards a staged implementation for installing packages such that
packages don't show up in the release directory until they have been
verified ok.  I did start working on this a while ago but it's time to
finish it.

cgf

Re: [PATCH] Two fixes for setup postinstall handling.

[Brian Dessent]

Christopher Faylor wrote:

> If the one is _update-info-dir then it already relies on bash and I
> changed the postinstall script to use /bin/bash rather than /bin/sh.

I thought the other thread concluded it was terminfo whose setup.hint
needed a "requires: bash".

Brian

Re: [PATCH] Two fixes for setup postinstall handling.

[Christopher Faylor]

On Sun, Aug 10, 2008 at 04:35:37PM -0700, Brian Dessent wrote:
>Christopher Faylor wrote:
>
>> If the one is _update-info-dir then it already relies on bash and I
>> changed the postinstall script to use /bin/bash rather than /bin/sh.
>
>I thought the other thread concluded it was terminfo whose setup.hint
>needed a "requires: bash".

Ah, nevermind.

cgf

(who's going crazy trying to locate the nascent package lint script he
wrote in May)