Re: [PATCH] Warn when whitespace is missing before macro body

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 22, 2013 at 5:14 PM, Panu Matilainen
<pmatilai@xxxxxxxxxxxxxxx> wrote:
> On 01/21/2013 08:43 PM, Alexey Tourbin wrote:
>
> Hi!
>
>> 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.

> 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.

By the way, this lvm2.spec case results in a bug:

1) The dependency in device-mapper (a lvm2.spec subpackage) goes wrong:
$ rpm -qR device-mapper | grep %
util-linux >= %{util-linux_version}
This is because, contrary to %define, macro named "util-linux_version"
was actually looked up.
2) Furthermore, the dependency is satisfied!
3) FC18 release process does not spot the bug (and other similar bugs).

> P.S. In general, rpm-maint@xxxxxxxxxxxxx is the more appropriate forum for
> patch submissions, this is more of a user-oriented list.

Okay, I'll send subsequent patches to rpm-maint. What I'd like to do
is to improve macro expansion so that most of the related bugs can be
identified automatically. On the other hand, traditional/sloppy usage
should probably be preserved (and so stray '%' will fall back to as-is
substitution instead of raising an error). This first series of
patches only deals with syntax. The second big topic is to implement a
check for unexpanded macros (e.g. due to typos). This requires some
intricate interplay between specfile parser and macro expansion.
_______________________________________________
Rpm-list mailing list
Rpm-list@xxxxxxxxxxxxx
http://lists.rpm.org/mailman/listinfo/rpm-list


[Index of Archives]     [RPM Ecosystem]     [Linux Kernel]     [Red Hat Install]     [PAM]     [Red Hat Watch]     [Red Hat Development]     [Red Hat]     [Gimp]     [Yosemite News]     [IETF Discussion]

  Powered by Linux