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


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



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