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/ |\ _,,,---,,_ [EMAIL PROTECTED] ZZZzz /,`.-'`' -. ;-;;,_ [EMAIL PROTECTED] |,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
