[Rpm-maint] [PATCH] Check for undefined macros

[Alexey Tourbin]

On Wed, Feb 06, 2013 at 07:09:00AM +0400, Dmitry V. Levin wrote:
> On Sun, Jan 27, 2013 at 05:27:09AM +0000, Alexey Tourbin wrote:
> > This change introduces a generalized routine rpmExpandMacros() to expand
> > macros on behalf of user input, with improved diagnostic facilities.
> > In particular, one of its arguments is a callback function which is
> > called whenever an undefined macro is encountered in user input.
> > 
> > When specfile parser calls this routine, it supplies a custom function
> > to handle undefined macros.  This function is keenly aware of specfile
> > syntax: valid specfile tokens, such as %prep and %setup, when found in
> > the right context, are not treated as undefined macros.  Otherwise,
> > there are 3 possibilities.
> > - Undefined macros in preamble and %pre/%post/... scriptlets raise
> >   an error, unless a non-build parse is performed; this is to prevent
> >   malformed packages from being built.
> > - In other cases, a warning is issued.
> > - Some tokens are permitted beyond their context (e.g. %ghost in
> >   %changelog), to comply with traditional/sloppy usage.  Also, undefined
> >   macros found in comments normally do not produce a warning.
> 
> The idea is certainly not new, it was implemented in ALT Linux in October
> of 2005, actually.  This feature proved to be useful in catching subtle
> specfile bugs caused by undefined macros.  Said that, one should

The idea is not new, but the implementation has been much refined, and
is now more sensitive to specfile syntax.  E.g. it is now possible to
identify subtle bugs like "%prep:" not starting %prep section properly
(this is actually found in FC18 icon-naming-utils.spec - and I wonder
how they actually managed to build the package).  Also, one can actually
subvert the logic in the handler, but this requires some imagination.
For example, one can use something like %{expand:%po}%{expand:st} to
start %post section.  Unless deliberate tricks like this are attempted,
I believe the check is pretty robust.

Here is another similar case which would go unnoticed with older
implementation:

$ grep '^%description(' *.spec
trafshow.spec:%description(ru)
$

With new implementation, a warning is issued:
warning: trafshow.spec:31: Undefined macro %description

> understand that a change that results to some previously acceptable
> specfiles being rejected is a policy enforcement.  Whether it is a good
> idea to impose this specfile check without some grace period during which
> newly recognized specfile errors are reported but not yet raise errors
> depends on your user base, percentage of specfiles affected by the change,
> how long it would take to fix them all, etc.  In ALT Linux, we chosen "no
> grace period" option and introduced %_allow_undefined_macros macro
> allowing users to relax the check.  Of course, your mileage may vary.

I've posted an analysis of how FC18 specfiles are affected by this change.
It does not include statistics in terms of percents, but it does show that
indeed many bugs can be identified, and also discusses a few cases when
valid packages needs to be adjusted - such as no short-circuit expansion
in conditionals and 'pidof %PPID' in scriptlets.

> Finally, I'd like to thank Alexey for bringing this feature to a wider
> audience.

I am perhaps left to take reciprocal attitude towards Dmitry and thank
him for first implementing this feature back in 2005.