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

Alexey Tourbin alexey.tourbin at gmail.com
Sat Feb 9 04:16:33 UTC 2013

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
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
$ rpm --define 'regex {\.txt$}' --eval %regex

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!

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

More information about the Rpm-maint mailing list