[Rpm-maint] [PATCH] Warn when whitespace is missing before macro body
pmatilai at laiskiainen.org
Fri Feb 8 11:38:37 UTC 2013
Moving this to rpm-maint, for those who missed the initial discussion on
rpm-list the start of the thread is here:
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:
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 -
More information about the Rpm-maint