[Rpm-maint] [PATCH] Warn when whitespace is missing before macro body

[Panu Matilainen]

Moving this to rpm-maint, for those who missed the initial discussion on 
rpm-list the start of the thread is here:
http://lists.rpm.org/pipermail/rpm-list/2013-January/001360.html

On 01/23/2013 02:32 AM, Alexey Tourbin wrote:
> On Tue, Jan 22, 2013 at 5:14 PM, Panu Matilainen
> <pmatilai at laiskiainen.org> wrote:
>> On 01/21/2013 08:43 PM, Alexey Tourbin wrote:
>>
>> Hi!

At long last getting back to this, now that the ghost episode is 
hopefully dealt with...

>>
>>> This will now issue a warning when macro definition is possibly
>>> incorrect or ambigous, such as the one found in FC18 lvm2.spec:
>>>
>>> %define util-linux_version 2.22.1
>>> warning: Macro %util needs whitespace before body
>>
>> Hmm. I like the idea, but the message is obscure (as of course is the
>> existing behavior). I'd be ready to bet the first reaction of packagers
>> seeing that would be filing "rpm macro parser has gone crazy" bugs instead
>> of fixing their macro names :)
>
> This warning message is modeled after gcc:
>
> $ cat test.c
> #define X-1
> #define Y(x)-x
> $ gcc -c test.c
> test.c:1:10: warning: missing whitespace after the macro name [enabled
> by default]
> $
>
> So gcc warns only about macros without arguments, and hence in
> "#define X-1", whitespace is missing "after the macro name". Although
> macros with argument list, such as "#define Y(x)-x", might seem less
> ambiguous, requiring a whitespace is still a good idea from even
> purely syntactic point of view; and hence "after the macro name"
> becomes "before body". Perhaps a better wording is possible.

Looks like I shot the messenger here, sorry about that... The warning 
reflects rpm's behavior (and a warning is a vast improvement over 
current behavior) but its rpm's behavior that is IMO plain nonsensical 
here. Things might be different in the gcc context but then again, gcc 
isn't exactly known for its clear and helpful error messages either :)

>> I think it'd be better just to sanity check the name at define/undefine time
>> instead, eg change the COPYNAME() macro into a function which validates the
>> whole name as it goes and errors out on invalid characters, or something to
>> that effect.
>
> Let's make sure that we understand that the macro
> %define util-linux_version 2.22.1
> defines the macro "util" with the value "-linux_version 2.22.1".  My
> patch (and subsequent patches) do not change this fact, it only tries
> to point out the the meaning is probably unintended.

Yes, understood. I just dont think anybody would ever *intentionally* 
define "util" macro that way, and users are going to have easier time 
understanding an "illegal macro name" message than the actual behavior 
of the parser.

That's what I meant with the "obscure message": going from silence to 
warning about whitespace in such construct, I think people are far more 
likely to see this as a new bug in the parser ("why on earth would it 
interpret it that way?") rather than their own error.

The other patch to make macro substitution syntax stricter does just 
that for attempts to *use* such macro names which will catch and better 
explain the issue to users, but there are various cases where specs 
define macros that are not used in the spec (eg to override some rpm 
behavior via a macro), and "illegal macro name" error would drive the 
point across more clearly than (the currently technically correct) 
message about whitespace.

So... while I can apply this, I would rather see a patch to make the 
parser consider it a downright error of an illegal macro name instead.
Let me know if you're willing to have a look at doing that now'ish, 
otherwise I guess I'll just apply it as is because it *is* a definite 
improvement anyway and it's not like it couldn't be changed later.

	- Panu -