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: New patch for generic build script


Alan,

I was going to say that I'll review the patch later, and offer a few quick
comments, but it looks like I did a full patch review, so here goes:

(1)  WOW, this is a HUGE ChangeLog.  Wa-ay too detailed.  Most of this
stuff should go into comments in the script, if you feel like documenting
it at all and if it isn't self-documenting.  I also forgot to include a
link to the Cygwin ChangeLog guidelines (<http://cygwin.com/contrib.html>,
based on the GNU ChangeLog guidelines, to which there's a link on the
above page), so please review that.

Just to give you a feel for what would be the right level of detail,
here's an attempt to shorten this:

2004-02-24  Alan Miles  <alan.miles<at>ieee.org>

	* templates/generic-readme: Fix phrasing for better pattern
	matching.  Parameterize initial release version.
	* templates/generic-build-script: Add 'readmelist' argument that
	calls readmelist().  Add readmelist() to the 'all' sequence.
	Define constant strings for pattern matching.
	(readmelist): New function to update the README.
	(hintfiledata): New function to extract setup.hint data.
	(strip,mkpatch,depend): Add xargs option to prevent complaints
	when no executables are found.
	(install): Parameterize README filename.

Even that might be too detailed, but at least it's manageable. :-)  I may
have missed something, too.

(2) It's usually a good idea to submit independent changes as separate
patches.  For example, I'd do the xargs change separately.  I'm not sure
if the rest of the patch is acceptable as-is (pending further review), but
I'd suggest singling this change out and sending a separate short patch.

(3) Please, please set your linewrap at 80 columns (for both the patch and
the ChangeLog).  It might even be better for the ChangeLog to be wrapped
at 76 columns or so...

(4) I'd like to keep the order or the file entries (i.e., /etc/postinstall
at the end).  I also think it would be good to keep the "(this file)"
after the README entry.

(5) Frankly, I don't see the point in the first hunk of the readme patch.
You can just as well match all of the lines between 'Runtime requirements'
and '<other requirements.*>' (or 'Build requirements'...).

(6) I've been thinking that it might actually be better to have a
computer-readable README.in, and a separate 'gen_readme' script that would
produce a README from it.  The reasoning here is that the package build
script is invoked not only by the package maintainers, but also
(hopefully) every time someone builds a package from sources.  Do we
really want to regenerate the README for these people?  Besides, the
dependences are all going to be different, at the very least.
Alternatively, leave the functionality in the generic-build-script, but
don't call it by default (like signing).

(7) If the variable's value comes from the ldesc field, why not name it
LDESC?  You might want to give similar descriptive naming to other
variables.

(8) The "OTHERPORT" idea is wrong, wrong, wrong.  I mentioned this
earlier, but I guess I wasn't articulate enough.  OTHERPORT is actually a
whole change log for the previous ports.  There should be no reason to
regenerate it, and certainly no reason to stick the port notes information
into the script.  It might be ok to have the *latest* version's changes in
the script (after all, that's the version that this script builds), but
the others should be put into the readme only once.  Perhaps we should
table it and get the rest of the patch going.

In fact, let's do this a couple of patterns at a time.  Some things can be
easily extracted from other places, such as setup.hint, so for those it
makes sense to have auto-generation.  If the maintainer has to modify a
variable in the script to put some text into the README, why not put it
directly into the README and avoid the hassle?  If you think that having
only one file to maintain is easier, I doubt that's true -- eventually the
'one-size-fits-all' generic-build-script will become a maintenance
nightmare, especially with trying to pull in the latest changes from CVS.

So, please split your patch into multiple contained patches, each of which
has one independent part of this functionality.  We'll look at one patch
at a time, and decide what to do with them on a patch-by-patch basis,
rather than trying to get approval for one monster patch.

Ooh, boy, I just read all of the above...  I'm getting really verbose as
the night grows older.  Good thing I'm not writing ChangeLogs. :-)
In your words: now to bed...
	Igor
P.S. I'm too tired to snip out the relevant parts from the message below.
Apologies to all for a long quote.

On Tue, 24 Feb 2004, Alan Miles wrote:

> All,
>
> I am attaching a new release of the this patch to mitigate below.
>
> Igor, please review and advise. Note: I built against 1.19 for the
> generic-build-script and 1.7 for the generic readme.
>
> Change log entry:
>
> 2004-02-24  Alan Miles  <alan.miles<at>ieee.org>
>
>         * templates/generic-readme : Changed the runtime requirements string
>         from "<other requirements, e.g., libintl1>" to:
>         "<other Runtime requirements, e.g., libintl1>" allowing better text
>         replacement using the values generated by depend() function. The generic
>         build script will replace this string in its function "readmelist()".
>
>         * templates/generic-readme : Changed the build requirements string
>         from "<other requirements, e.g., gettext>" to:
>         "<other Build requirements, e.g., gettext>" allowing better text
>         replacement using values from the generic build script, which
>         will replace this string in its function "readmelist()".
>
>         * templates/generic-readme : Replaced the listed files under the
>         "Files included" section with the string "<THEFILES>". The generic
>         build script will replace the string "<THEFILES>" with a sorted list of
>         files which will comprise the binary distribution. The generic build
>         scripts "readmelist()" function does this.
>
>         * templates/generic-readme : Changed the port notes string
>         from "Other information" to:
>         "<Other Port information>" allowing better text
>         replacement using values from the generic build script, which
>         will replace this string in its function "readmelist()".
>
>         * templates/generic-readme : Changed the older release string
>         from "<PKG>-<older VER>-1" to:
>         "<PKG>-<older VER>-<older REL>" since the release number
>         may not be 1. The generic build script will analyse the setup.hint file
>         and set the values of "<older VER>" and "<older REL>", substituting these
>         values. If the script cannot find the setup.hint file, these values
>         become values of "<VER>" and "<REL>" respectively.Again the generic build
>         script's "readmelist()" and "hintfiledata()" functions do the substitutions
>         and extract the data from the setup.hint file.
>
>         * templates/generic-build-script : Added defaults and instructions to Package
>         maintainers regarding some new constants:
>         export SHORTDESC="<short description, 2 or 3 lines>"
>         export OTHERRUNTIME="  <other Runtime requirements, e.g., libintl1>"
>         export OTHERBUILD="  <other Build requirements, e.g., gettext>"
>         export OTHERPORT="<Other Port information>"
>         export OLDERVER="${VER}" # Set the default to the "current" package's version number
>         export OLDERREL="${REL}" # Set the default to the "current" package's release number
>         export MAINTNAME="<Your Name Here>"
>         export MAINTEMAIL="<your email here>"
>         The "readmelist()" function substitutes these values into the binary package README.
>         Note: Function "hintfiledata()" overrides the values of SHORTDESC, setting it to the
>         setup.hint file's ldesc value, and obtains the OLDERVER, OLDERREL values from the
>         setup.hint if it contains the "prev: " value
>
>         * templates/generic-build-script : Added function "hintfiledata()" which parses
>         and extracts the ldesc: value and prev: values, if the
>         "${srcdir}/CYGWIN-PATCHES/setup.hint" file exists, altering the SHORTDESC, OLDERVER,
>         OLDERREL values respectively.
>
>         * templates/generic-build-script : Added function "readmelist()" which examines
>         all the files in "${instdir}", sorting them and then replacing the "<THEFILES>" string
>         with the lines. This replacement exploits sed's "e command", which allows one to pipe
>         input from a shell command into pattern space. The command does the following replacements:
>         (a) string "<PKG>" becomes the value of "${PKG}"
>         (b) string "<VER>" becomes the value of "${VER}"
>         (c) string "<REL>" becomes the value of "${REL}"
>         (d) string "<package name>" becomes the value of "${PKG}"
>         (e) string "<short description, 2 or 3 lines>" becomes the value of "${SHORTDESC}"
>         (f) string "  <other Build requirements, e.g., gettext>" becomes the value of "${OTHERBUILD}"
>         (g) string "<Other Port information>" becomes the value of "${OTHERPORT}"
>         (h) string "<older VER>" becomes the value of "${OLDERVER}"
>         (i) string "<older REL>" becomes the value of "${OLDERREL}"
>         (j) string "<Your Name Here>" becomes the value of "${MAINTNAME}"
>         (k) string "<your email here>" becomes the value of "${MAINTMAIL}"
>         (l) string "  <other Runtime requirements, e.g., libintl1>" becomes the value of sorted
>             "depends()" function.
>
>         * templates/generic-build-script : Added the call to function "readmelist()" in the "all" list
>
>         * templates/generic-build-script : Fixed a problem in the functions "strip()", "mkpatch()",
>         and "depend()" when there are no dll's or exes (the find returns no files) - xargs would try to
>         execute strip or cygcheck/cygpath with no arguments and these commands would complain about
>         missing arguments. The fix is to add the -r parameter to xargs, which prevents xargs
>         from executing its parameters if there is no input.
>
>         * templates/generic-build-script : Altered references in install() from:
>         "${instdir}${prefix}/share/doc/Cygwin/${BASEPKG}.README" to:
>         "${PkgReadMe}" thus allowing only one definition of the README - "readmelist()" uses
>         "${PkgReadMe}"
>
>
> -----Original Message-----
> From: Alan Miles
> Sent: February 10, 2004 23:25
> To: cygwin-apps<at>cygwin<dot>com
> Subject: RE: Pending patches for generic build script
>
> Igor,
>
> Small attempt at humour here ...
>
> I guess I was just trying to provide a means to get the "listing" inside
> the readme, together was some variables like VER reducing the
> maintainers job.
>
> However, I will review your comments and suggestions etc.
>
> [snip]
> >> 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).
>
> I'll need to review the ChangeLog as I am not familiar with this piece
> (as suggested I need to "see packaging/ChangeLog in CVS for examples") -
> also back to the cygwin site to review submissions as well just to
> ensure I do this correctly ...
>
> >> 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.
>
> I guess I missed the point of what list() was to do - however, yes I agree
> it should be a different name
>
> >> 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%.
>
> I wasn't sure what %THEFILES% entries a submitter needed to be compliant
> with, so I left a few items in. It is late, and my brain isn't
> functioning well to ensure I understand your comment correctly, so I
> will review 2) when my brain is clearer - I think I now what you want,
> but I cannot reason it out correctly right now.
>
> >> 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>).
>
> I'll need to review this and make the corrections as needed ...
>
> >> 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.
>
> Sounds interesting and do-able - I am not familiar with Yaakov's patch
> [yet]...
>
> >> 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).
>
> Sounds interesting and do-able - since I have a copy of the upset script
> (which I use to build a setup.ini file for my custom installation to get
> installed through setup) I could look at this too. Obviously, I will need to
> ensure my upset script is the latest from CVS. Again brain challenged, so
> will review your comments when my brain is more functional...
>
> >>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.
>
> Some of this stylistic, but understood. I need to review mktemp (man mktemp)
> or the $$ items.
> Agreed I need additional comments, and the fix for LTS might help in
> readability as well.
>
> >> 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.
>
> Not scared off - if it does not yet pass muster then it needs fixing. I
> will wait a few days to ensure I get the latest CVS files. With this
> weekend coming up, I don't know how much time I will get but the delay
> will buy me some time.
>
> Now to bed ... have to go to work tomorrow ...
>
> Alan

-- 
				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]