This is the mail archive of the
cygwin-apps@cygwin.com
mailing list for the Cygwin project.
RE: Pending patches for generic build script
- From: Igor Pechtchanski <pechtcha at cs dot nyu dot edu>
- To: alan dot miles at ieee dot org
- Cc: cygwin-apps at cygwin dot com
- Date: Tue, 10 Feb 2004 21:25:23 -0500 (EST)
- Subject: RE: Pending patches for generic build script
- References: <JLEBIHHBMBHBAFPAJLEFOENHFDAA.miles0201@cox.net>
- Reply-to: cygwin-apps at cygwin dot com
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