rpm-software-management / rpm

The RPM package manager
http://rpm.org
Other
506 stars 366 forks source link

rpmbuild should bail out as soon as recursive macro expansion is detected #145

Open jnpkrn opened 7 years ago

jnpkrn commented 7 years ago

Currently:

$ cat newpackage.spec 
%define __os_install_post %(echo '%{__os_install_post}' \
       | sed -e 's!/usr/lib[^[:space:]]*/brp-python-bytecompile[[:space:]].*$!!g')

Name:           foo
Version:        1
Release:        1%{?dist}
Summary:        bar

License:        GPLv2
URL:            http://localhost
Source:         newpackage.spec

%description
Foo bar

%install
$ rpmbuild -bb -v -D "_srcdir $(pwd)" newpackage.spec 
error: Too many levels of recursion in macro expansion. It is likely caused by recursive macro declaration.
Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.cIXCaT
+ umask 022
+ cd /home/mock/rpmbuild/BUILD
+ '[' /home/mock/rpmbuild/BUILDROOT/foo-1-1.fc22.x86_64 '!=' / ']'
+ rm -rf /home/mock/rpmbuild/BUILDROOT/foo-1-1.fc22.x86_64
++ dirname /home/mock/rpmbuild/BUILDROOT/foo-1-1.fc22.x86_64
+ mkdir -p /home/mock/rpmbuild/BUILDROOT
+ mkdir /home/mock/rpmbuild/BUILDROOT/foo-1-1.fc22.x86_64
+ /usr/lib/rpm/check-buildroot
Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/mock/rpmbuild/BUILDROOT/foo-1-1.fc22.x86_64
Executing(%clean): /bin/sh -e /var/tmp/rpm-tmp.E2B2uY
+ umask 022
+ cd /home/mock/rpmbuild/BUILD
+ /usr/bin/rm -rf /home/mock/rpmbuild/BUILDROOT/foo-1-1.fc22.x86_64
+ exit 0
$ echo $?
0

So the process doesn't fail and continues despite an internal error with undesired consequences. It would be expected that the build fails instead, not masking any subtle issues.

jnpkrn commented 7 years ago

See also: https://github.com/ClusterLabs/pacemaker/pull/1218

pmatilai commented 7 years ago

Yup. It's in one of those > 100 macro expansions within rpm which have no way of returning an error, eg rpmExpand() has no way of indicating an error as it is, and there's no other API that it can be directly replaced with in all cases.

Which is not to say it shouldn't be fixed but it's far more laborous than just adding a missing check for an error code someplace.

ffesti commented 4 years ago

So this is basically waiting on a large scale refactoring effort of the macro handling.

pmatilai commented 4 years ago

That, or some less intrusive way of stopping rpm from macro errors after they occurrer. rpmMacroWasError() that gives the return code of previous expansion at couple of strategic locations could perhaps be used to home in on the ballpark of last failed expansion without changing all of those rpmExpands().