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: "Alan Miles" <miles0201 at cox dot net>
- To: <cygwin-apps at cygwin dot com>
- Date: Tue, 10 Feb 2004 23:25:14 -0600
- Subject: RE: Pending patches for generic build script
- Reply-to: <alan dot miles at ieee dot org>
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