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

[Alexey Tourbin]

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!

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