rpmbuild fails on nested user-defined %if* macros

[Mike Miller]

On Tue, Oct 02, 2012 at 09:48:50AM +0300, Panu Matilainen wrote:
> On 10/02/2012 05:44 AM, Mike Miller wrote:
> >Hi, a regression has come up between rpmbuild 4.9 and 4.10 and I want to
> >know if this was intentional or not. Here is an artificial test-case
> >spec file that illustrates the regression:
> >
> >%define if_some_value() %{expand:%if "%{_some_value}" == "%1" || "%{_some_value}" == "%2"}
> >Summary: Test case spec file for LP #1058378
> >Name: test-pkg
> >Version: 1.0.0
> >Release: 1
> >License: GPL
> >Group: Development/Languages
> >%description
> >Test case spec file for LP #1058378
> >%prep
> >%if_some_value foo bar
> >echo first case
> >%else
> >%if_some_value shibboleet
> >echo second case
> >%else
> >echo third case
> >%endif
> >%endif
> >
> >If the first conditional is false, there is no problem. If the first
> >conditional is true (e.g. rpmbuild -D"_some_value foo"), rpmbuild 4.9
> >succeeds but 4.10 fails with
> >
> >error: /home/mike/lp-1058378.spec:19: Got a %endif with no %if
> >
> >So it looks to me like user-defined %if* macros are supported, but not
> >if one appears in the false branch of a conditional. I think this commit
> >
> >http://rpm.org/gitweb?p=rpm.git;a=commitdiff;h=3a102207154bc9e695d8875d6f01fd98fc8783c7
> >
> >is responsible for the change in behavior that produces this error, but
> >I'm not sure if the intention was to continue supporting such
> >user-defined %if* macros or not. Can someone clarify? Thanks.
> >
> >Original bug report: https://bugs.launchpad.net/bugs/1058378
> >
> 
> Hum, interesting case.
> 
> As a general rule, macros are not supposed to be expanded in a false
> branch of an %if, but the former implementation gets confused enough
> to let it through as *both* %if and a macro, expanding the macro
> when it technically shouldn't.
> 
> This and other similar mixups is pretty much exactly what the commit
> you mention was intended to fix, and it might be hard to convince me
> to revert that as the same can be achieved without abusing the spec
> parser.

Thanks Panu, that is what I suspected. When I was looking into this it
started to seem like an accident of the parser that this technique was
exploiting. What I was looking for was a confirmation from a dev that
the new behavior is correct and that there is a preferred way to do
this, as you suggest here:

> Simply move the "%if" out of the macro:
> 
> %define some_value() %{expand:"%{_some_value}" == "%1" ||
> "%{_some_value}" == "%2"}
> 
> %if %some_value foo
> ...
> %endif
> 
> The careful reader might notice that this too actually circumvents
> the "macros should not be expanded in false branch" rule: the
> arguments to %if get always expanded, false branch or not.

And that is what I've seen done more commonly in e.g. RHEL and Fedora
spec files that I'm familiar with.

To put this in some context (and to help you in case you get similar
questions) I'm responding to a bug report about failing to build a srpm
from the STLinux distribution (www.stlinux.com) which I had not heard of
until I saw the report. Their rpm configuration uses a couple of these
%if_* style macros throughout their distribution.

You've confirmed that this change was intentional, I'm going to
recommend that the reporter submit a request to STLinux to rework their
rpm configuration as you describe above to make it compatible and see if
that works for everyone.

-- 
mike