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: Pending patches for generic build script


On Tue, 10 Feb 2004, Alan Miles wrote:

> Igor,
>
> Thank-you for the update and patch consideration, and sorry for the
> multiple postings - I thought my patches kept getting rejected [maybe
> some meanness ... :) ] since I did not hear back from anyone after your
> initial review. However, I could not figure out why it may be rejected
> so I kept on re-submitting them in the hopes that I would get a response
> from someone ... which I did, via the list, when somebody (can't
> remember who) stated that they are back-logged on these patches.

Alan,

That must've been Chuck.  Given that he's very busy, I'm temporarily
maintaining the packaging templates, and just getting to the patches now.
In fact, I have a few questions/comments about your patch (below).

> >> Alan, all 3 submissions from you are essentially the same.  I'm going
> >> to go with the latest one (dated 20040109[3]).  What I said about
> >> ChangeLogs above applies here too.
>
> This statement is absolutely correct!
>
> Alan

Umm, there are two statements above.  The second is actually a question
about ChangeLogs.  It refers to the following two sentences from earlier
in the message:

> I'll use this message as a sort of a ChangeLog -- please let me know if
> you'd rather construct your own ChangeLog entries.

and later

> Should I use the accompanying message for the ChangeLog?
>
> [3] http://cygwin.com/ml/cygwin-apps/2004-01/msg00042.html

You haven't answered this (frankly, it's kind of hard to reconstruct a
ChangeLog entry from your message anyway).  It would be great if you
recreated the patch against the latest CVS, taking my comments below into
account, and resubmitted it with a ChangeLog (see packaging/ChangeLog in
CVS for examples).

Ok, now for the patch comments:

1) I'm a bit wary of modifying "list()" to update the README.  I'd much
   rather have a separate function that does this.

2) If you're replacing the file list, why not do the whole thing?  Also,
   you could try sorting them in the order of the README (by playing with
   the order of paths supplied to 'find').  Alternatively, you'll need to
   filter out the files that *are* present as templates in the README from
   %THEFILES%.

3) What you're doing with %NEW_VER% is completely wrong.  Technically,
   %NEW_VER% should always be equal to %VER%-%REL% (since it is the latest
   version that you're packaging).  Also, you should leave the change
   history ("version %VER%" in your patch) well enough alone -- it should
   be static.  I suggest taking it out of your patch for now and tackling
   it later (see <5>).

4) [Optional] I just realized that most people put the same text as
   project description as that in the "ldesc:" field of setup.hint (which
   should be in CYGWIN-PATCHES).  It would be great if it could be
   extracted and placed into the README.  The setup.hint itself could also
   be verified using the "depend()" function in Yaakov's patch.  That same
   function could also be used to generate run-time dependences in the
   README.

5) [Optional] One of the things that a package maintainer may forget is to
   update the README with the porting notes for the latest version.  If
   there was a mechanism for checking that the latest (%VER%-%REL%)
   version's porting notes are present in the section, and that version
   numbers in that section monotonically decrease, that would be helpful.
   The logic for comparing version numbers can be borrowed from the
   "upset" script.  Also, if the porting notes for %VER%-%REL% *aren't*
   present, the script could insert a placeholder and then pop up the
   README in an editor (even with the cursor at the correct line).

6) Some *minor* nits about the patch:
   - "ThePackageReadmeFile", while a long and descriptive variable name,
     doesn't follow the variable name conventions in the surrounding code
     (which calls for something like "readmefile");
   - Similarly, the rest of the script uses a "'then' on the same line as
     'if'" convention.  It would be good if it were followed.
   - I know the script is not reentrant, and that it's unlikely that
     someone will have a %PKG%-%VER%.README in their /tmp, but still, for
     peace of mind, could you please use "mktemp"?  Or, if you don't want
     to add another dependence, at least use "$$" in the filename?
   - Some of your sed expressions suffer from LTS ("Leaning Toothpick
     Syndrome"), which is easily fixable by using an alternate separator
     character (e.g., "#").  You do use it in some places, but not
     everywhere it's needed.
   - Please don't indent the line continuation backslash all the way to
     the end -- one space is quite enough.  Ditto for '&&'.  Redirection,
     on the other hand, can be outdented to be a couple of spaces past the
     sed command, with the '>' on the same line as the output file.
   - One sed expression for everything is neat, but rather hard to read.
     It would help if you used comments.  Also, a consistent separator
     character for the 's' command would be great.

Well, hopefully this doesn't scare you off -- this seems like useful
functionality.  Oh, and you might want to wait a bit before working on the
patch -- I'm planning to make some changes to the generic-build-script in
the next few days that will make it easier to implement similar
functionality.
	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!

"I have since come to realize that being between your mentor and his route
to the bathroom is a major career booster."  -- Patrick Naughton


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