Roland Illig wrote:
> joerg@britannica.bec.de wrote:
> 
>> On Thu, Dec 01, 2005 at 07:14:03PM +0100, Roland Illig wrote:
>>
>>> - Any objections against this?
>>
>>
>>
>> No objection, but please try to commit it in one big swap. It saves the
>> bulk builders a bunch of time :-)
> 
> 
> After a one-day-marathon of going through all packages that had pkglint 
> warnings, this is the result:
> 
>     ftp://ftp.NetBSD.org/pub/pkgsrc/misc/rillig/pkgsrc.diff
>     ftp://ftp.NetBSD.org/pub/pkgsrc/misc/rillig/pkgsrc.diff.gz
> 
> Any volunteers to review it?

To make a review easier, I will outline some of the ideas leading to the 
changes, in order of appearance in the diff file.

===> Replacing ".ifdef" with ".if defined(...)"

This is generally useful because the parser of NetBSD's make(1) is 
sloppy in many areas. It accepts weird directives like ".elifndef", but 
doesn't understand them. Therefore I decided that it's best to only use 
the most basic features. Furthermore, the ".ifndef" directive isn't in 
such wide use as that of the C preprocessor, maybe because the visually 
appealing counterpart ".define" does not exist. Compare the following 
code snippets:

     #ifndef HAVE_FOO_H
     #define HAVE_FOO_H

and

     .ifndef HAVE_FOO_H
     HAVE_FOO_H=        # defined

The first one looks like a block, while the second one does not. So I 
didn't see the need to keep the ".ifndef".

===> Forcing many variables to be modified using only "+="

The whole pkgsrc system would be much more understandable if we had only 
variables that are modified using a very restricted set of operations. 
The operations I have in mind are:

     "?="    default value assignment
     "+="    appending to a list
     "="     simple assignment (should happen only once per variable)

That way, if you add something to a list you can be sure that it doesn't 
get removed later. Currently this is not guaranteed. Just have a look 
for the various places where CONFIGURE_ARGS is modified using the ":=" 
operator, which evaluates its right-hand-side and assigns it to its 
left-hand-side. So let's say you have the following lines:

     CPPFLAGS+=         -DFOO=1
     CONFIGURE_ARGS+=   CPPFLAGS=${CPPFLAGS:M*:Q}
     CONFIGURE_ARGS:=   ${CONFIGURE_ARGS:N--with-ncurses}
     CPPFLAGS+=         -I/usr/local/include

The last addition to CPPFLAGS won't make it into CONFIGURE_ARGS. Now 
imagine that these four assignments are all in individual files, spread 
around the whole pkgsrc tree. How do you find the cause that 
"-I/usr/local/include" is not included in the CPPFLAGS passed to the 
configure script?

For the GCC_REQD variable, the "+=" operator is required because there 
may be many sources requiring a special version of gcc. It wouldn't be 
helpful if one source could override all the others. The same goes for 
the BUILD_DEFS variable, which has also been assigned unconditionally in 
many cases, including ones in Makefile.common files. In these cases you 
cannot be sure at all what has happened before.

===> Platform triples

A platform triple has the form <OPSYS>-<OS_VERSION>-<MACHINE_ARCH>. Each 
of these components may be a shell globbing expression.

I had been curious which combinations of OPSYS, OS_VERSION and 
MACHINE_ARCH were used in pkgsrc, so I have written a pkglint check for 
it. As a result I noticed that in many cases, the platform triples were 
actually only pairs. To make pkgsrc more uniform, I decided to change 
the pairs into triples, so that a human, reading a package Makefile, 
will see that a triple is meant.

===> White-space at the beginning of lines

I often search for variables with "grep -wr ^MAKE_ENV /usr/pkgsrc", 
because by looking at many Makefiles, this seemed to be the way 
variables are declared: starting in line 1. Because there are only very 
few exceptions to this scheme, I had not expected variables that are 
declared with leading white-space. To make pkgsrc more "expected", I've 
added this check to pkglint.

===> The :Q and the :M*:Q operators for quoting variables

Currently pkgsrc cannot handle white-space. No, it really cannot. But 
I'd like it to be able to handle white-space. Maybe one day, when the 
GNU make(1) and the GNU autotools become mature, they will allow 
white-space in filenames, and how great would it be if it worked 
immediately with pkgsrc?

All jokes aside, we already have packages containing white-space and 
other special characters in their filenames. They already work if you're 
lucky, but please, please, please, don't try to embed white-space into 
your LOCALBASE variable. That does not work and will lead to undefined 
behavior, because even in the pkgsrc infrastructure, not enough time and 
knowledge has been invested for these kinds of things.

To make the situation better we need more knowledge about proper quoting 
and filename handling. Replacing the various ad-hoc quoting styles with 
the one and only that works in all cases is the first step into that 
direction. See http://www.NetBSD.org/Documentation/pkgsrc/makefile.html 
for further details, especially the :M*:Q operator.

===> Making DIST_SUBDIR independent of PKGREVISION

These two variables are completely unrelated, but as almost nobody knows 
that PKGNAME is modified internally in bsd.pkg.mk by appending the 
"nb${PKGREVISION}" string to it if PKGREVISION is non-empty and not "0", 
some people thought that choosing ${PKGNAME} as the value for 
DIST_SUBDIR was the right thing. They overlooked that PKGREVISION might 
change without the need to refetch the DISTFILES. Now these two 
variables are independent again. To not cause the DISTFILES to be 
downloaded again as a result of this change, I've set the DIST_SUBDIR to 
the current value of ${PKGNAME}, expressed as ${PKGNAME_NOREV}nb1 in the 
case of audio/festvox-aec.

===> Explicit PKGNAME (audio/mp3asm)

The version number in a PKGNAME should only contain digits and dots. As 
the DISTNAME contains a dash in the version number part, the PKGNAME has 
been defined explicitly.

===> Two-level relative paths (audio/mpg123-esound)

It just looks more uniform if every relative path starts with "../../".

===> Removing partial RCS tags from patch files

Up to now, a partial $NetBSD$ tag makes bsd.pkg.mk and bsd.pkg.patch.mk 
ignore the complete line when generating the checksum. Depending on the 
quality of the algorithms, other programs may have problems 
distinguishing partial RCS tags from complete ones, so the easiest thing 
is to avoid them as far as possible.

===> ".else if" (biology/rasmol)
===> ".ifdef (${OPSYS} == "SunOS")" (comms/lrzsz)

The parser of NetBSD's make(1) didn't even warn on these lines, so I had 
to add this check to pkglint. Obviously it has to be fixed.

===

So far for now. I'm preparing a diff that does not include the changes 
done by pkglint --autofix. This will be easier to review than the 28000 
lines the patch currently has. I'll add the remaining things later.

Roland