[PATCH] Stricter macro substitution syntax

Alexey Tourbin alexey.tourbin at gmail.com
Wed Jan 23 01:13:17 UTC 2013

On Tue, Jan 22, 2013 at 7:16 PM, Panu Matilainen
<pmatilai at laiskiainen.org> wrote:
> On 01/21/2013 10:14 PM, Alexey Tourbin wrote:
>> This change introduces a separate routine to parse for valid macro
>> names.  Valid macro names are either regular 3+ character identifiers,
>> or special names: "S", "P", "0", "#", "*", "**", macro options such as
>> "-o" and "-o*", and macro arguments such as "1".  Other names are not
>> valid.  This fixes a number of bugs seen earlier due to sloppy name
>> parsing: "%_libdir*" and "%01" were not expanded (these are now expanded
>> to e.g. "/usr/lib64*" and "<name>1", as expected).  This also fixes
>> bugs in as-is substitution: "%!foo" was expanded to "%foo", and likewise
>> "%!!!" was expanded to "%" (and to "%<garbage>" at EOL).
>> Also, bad names in %name and %{name...} substitutions are now handled
>> differently.  In %name form, the name is parsed tentatively; a silent
>> fall-back to as-is substitution is provisioned when no valid name can
>> be obtain.  In %{name...} form, a failure to obtain a valid name is now
>> a syntax error.  Furthermore, only 3 variants are syntactically valid:
>> %{name} proper, %{name:...}, and %{name ...}.  This renders invalid
>> ambiguous macro substitutions such as the one found in FC18 lvm2.spec:
>> Requires: util-linux >= %{util-linux_version}
>> error: Invalid macro syntax: %{util-linux_version}
> Nice. The garbage spotted by this in Fedora specs is ... scary.
> OTOH changes of this scale in the macro parser are a bit scary in themselves
> as its sooooo easy to break some obscure corner-case :) I think I'll try to
> give this a spin with some of the more macro-intensive packages like
> Fedora's kernel and font-packages first, but unless I find something
> unexpected breakage, consider it applied. On a related note, the test-suite
> could really use more thorough and more advanced test-cases to make these
> kind of changes less scary.

Right, such changes must be extensively tested, and their practical
consequences must be well (and early) understood. For this, I employ
the "corpus" approach - that is, preprocess all specfiles from FC18
and examine the differences.

Here is roughly how the process goes.

0) Extract all specfiles from FC18:
$ mkdir  /fc18-specs
$ for f in /fc18-src/?/*.src.rpm; do
rpm2cpio $f |(cd /fc18-specs; cpio -idmu --quiet '*.spec'); done

1) Preprocess:
$ mkdir /fc18-cpp1
$ for f in /fc18-specs/*.spec; do
rpmspec -P $f >/fc18-cpp1/${f##*/} 2>&1 ; done

2) Hack on rpmio/macro.c, then make(1).  New code can be injected with e.g.
$ export LD_LIBRARY_PATH=~/rpm/rpmio/.libs

3) Preprocess again:
$ mkdir /fc18-cpp2
$ for f in /fc18-specs/*.spec; do
rpmspec -P $f >/fc18-cpp2/${f##*/} 2>&1 ; done

4) Compare:
$ duff -ur --ignore-m='^Build[Rr]oot:' /fc18-cpp1 /fc18-cpp2

This way, _every_ change in _every_ specfile will be noticed.

> BTW, while the rules are obviously different between define vs expansion due
> to the built-in special macros, perhaps parseMacroName() function here could
> be extended to cover the %define/%global/%undefine macro name sanity checks
> as well (see my reply wrt the missing-whitespace patch).

Macros defined with %define always have sane names; this might betray
user intentions, though; specifically, when a sane name is not
followed by a whitespace.

More information about the Rpm-list mailing list