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

Panu Matilainen pmatilai at laiskiainen.org
Sat Feb 9 13:03:56 UTC 2013

On 02/09/2013 06:16 AM, Alexey Tourbin wrote:
> On Fri, Feb 08, 2013 at 01:38:37PM +0200, Panu Matilainen wrote:
>>> 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.
> There's a valid case with missing whitespace found in specfiles:
> vym.spec:%global __requires_exclude%{?__requires_exclude:%__requires_exclude|}perl\\(BugzillaClient\\)
> What would be the warning/error message here?
> warning: Illegal macro name __requires_exclude%{?__requires_exclude:%__requires_exclude|}perl\\(BugzillaClient\\)
> Or should it trigger an existing error - ?
> error: Macro __requires_exclude%{?__requires_exclude:%__requires_exclude|}perl\\(BugzillaClient\\) has empty body
> So, you see, a problem here is that if you follow the parser rules, the
> warning can only refer to the fact that macro name is not followed by a
> whitespace.  Otherwise, if you indulge yourself into more liberties (in
> a desperate attempt to please users who write stuff LIKE THIS), you can
> end up with even more obscure messages.
> Technically, it would be best to clarify syntax.
> Current approach is:
> - parse for a word - that would be the name - and stop at the word
>    boundary.
> Arguably better approach would be:
> - split the first argument (by traditional argv/popt rules), and check
>    additional constraints for either name or name(args).
> The second approach is hampered by the fact that argument parsing must
> interfere with \ escapes - the first argument could be "foo\" then.
> There are actually to different syntax rules for %define.
> $ rpm --define 'regex \.txt$'   --eval %regex
> .txt$
> $ rpm --define 'regex {\.txt$}' --eval %regex
> \.txt$
> $
> This is another source of subtle bugs - people do not realize how many
> "layers" of backslashes are actually needed.  Representatives of another
> school of thought, should I say, believe that the amount of backslashes
> must be doubled yet again:
> /etc/rpm/macros.perl:%global __requires_exclude perl\\\\(VMS|perl\\\\(Win32
> Recently I've been pondering over a plausible solution to this problem,
> but perhaps it's too late to change anything.
>> 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.
> There is currently no way to return a "downright error" from doDefine,
> since it has to return the end of (backslash-concatenated) string.  So
> the error would only lead to a macro not being defined.  Or if you
> choose to turn it into a hard error, what would it do?  Perhaps, abort
> specfile parsing?  But specfiles are not the only place where bad macros
> come from.
> /etc/rpm/macros.mingw32:%mingw32_as              %{mingw32_target}-as
> /etc/rpm/macros.mingw32:%mingw32_c++             %{mingw32_target}-c++
> /etc/rpm/macros.mingw32:%mingw32_c++filt         %{mingw32_target}-c++filt
> /etc/rpm/macros.mingw32:%mingw32_dlltool         %{mingw32_target}-dlltool
> So, should it then abort macro file parsing, and hence terminate rpm
> startup routines?  If you turn it into an error, rpm won't even start!
> It's THAT BAD!

There are several pre-existing ways doDefine() can fail (illegal name, 
empty or unterminated body etc), for which the existing behavior is to 
not define the macro at all but it wont stop other processing.

> Given all these possibilities, I think that my original patch is the
> least controversial thing to do, at least for now.

Fair enough, applied. Issuing a warning on these cases is *vastly* 
better than the former silent "I bet you didn't expect THIS! <evil 
laughter>" behavior anyway, and can be tweaked later if necessary.


	- Panu -

More information about the Rpm-maint mailing list