This is the mail archive of the cygwin-apps@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: gbs cleanup patch


On Fri, 15 Oct 2004, Schulman.Andrew wrote:

> As we discussed a day or two ago, here is a patch that cleans up the
> following aspects of the generic build script (today's CVS version):

Great, thanks, Andrew.  Would you mind attaching the patch next time,
though, as mailers screw up spacing and line wrapping...  Also, it's
missing a ChangeLog (what you wrote below doesn't quite qualify, although
it could be massaged into one).  Some more comments below.

> - Invokes the shell by /bin/sh -e.

Great.  Have you tested whether the -e flag gets propagated inside the
functions?

> - Removes the many superfluous &&'s,  ;'s, and \'s between commands and
> at end of lines.

Umm, actually that does change the semantics of the calls slightly (though
perhaps for the better).  In particular, code like

  if [ -f ${src_patch} ] ; then
    patch -Z -p0 < ${src_patch}
  fi

will, I believe, now fail if the 'patch' command fails, and it wouldn't
have before.  Just something to note.  Have you tested that the script
works properly in these cases?

> - Removes all of the $STATUS junk at the end.  There's no more need for
> this, since with -e the script will fail when any subcommand fails.

Ok, this is fine.

> - Replaces all instances of 'if [ ! -d xxx ] ; then mkdir -p xxx ; fi'
> with just 'mkdir -p xxx'.  The second form is equivalent but simpler.

Ah, I didn't realize that '-p' also means "no error if existing".  Good
catch! :-)

> - Adds -r to every invocation of xargs.  In every case this is desirable
> and will prevent errors in null cases.

Yup, this is something I was thinking of doing for a while.  Thanks.
Later we should also probably change all "find | xargs" combos to "find
-print0 | xargs -0".

> With /bin/sh -e, one has to be careful about trapping unwanted non-zero
> exit statuses.  The only place I found where this was an issue was in
> mkpatch(), because diff has an exit status of 1 if it finds differences.
> Because of this I added '|| true' after the diff command.  One could be
> fussier here and check for an exit status of 1 (which is okay) or 2
> (which is an error), but I didn't bother.  I removed all other instances
> of trailing 'true's from the script.

Hmm, you should've really added '|| true' everywhere there was a '; \' in
the original code to make this patch a simple transformation.  In
particular, the "find | xargs" combo sometimes returned false for no
reason, so we used to ignore its exit status (was it the lack of "-r"?).
Ditto for "install" -- neither the man nor the info page say anything
about the exit status, and I can't look at the code right now.  I don't
mean to nitpick, but whenever you chose to omit '|| true', you could have
probably added a comment saying why the original code was wrong.  Would
you mind doing that?

A comment about the exit status 1 vs. 2 for diff would also be helpful in
the source, in case we decide to do this in the future...

> I've tested the revised gbs by building three different packages, and
> tested each operation at least once.  I had no problems.

I assume all of them succeeded...  Did you test for failures too (i.e.,
that the script correctly stops if a command fails, etc, etc)?

A few other minor items:

- there were some space changes (blank lines deleted) that shouldn't have
  been (the blank line after "unpack()" is a particular one I'm thinking
  of)

- redirections from xargs were removed (perhaps most stderr messages were
  removed by the "-r" flag, but maybe not)

- you changed the "cd $dir && find ." to "find $dir" in some places, which
  I'd be more comfortable changing as a separate step, if at all

- in list(), you changed the files that are found (i.e., ".*" would have
  been ignored before, and won't be now), and added "sort".  Frankly, I'd
  prefer not to include extra improvements like that in a patch this
  large.  I'll be happy to commit them separately later.

- in the main arg processing loop, you've combined cases.  While it makes
  the code shorter somewhat, I don't see it as a readability improvement,
  and don't think that this patch should cover it -- a separate patch
  would be better.

Thanks again for the effort you've invested in this, and for putting up
with my nitpicking.
	Igor
-- 
				http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_		pechtcha@cs.nyu.edu
ZZZzz /,`.-'`'    -.  ;-;;,_		igor@watson.ibm.com
     |,4-  ) )-,_. ,\ (  `'-'		Igor Pechtchanski, Ph.D.
    '---''(_/--'  `-'\_) fL	a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"Happiness lies in being privileged to work hard for long hours in doing
whatever you think is worth doing."  -- Dr. Jubal Harshaw


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]