[Rpm-maint] [PATCH] Check for undefined macros

[Alexey Tourbin]

On Sun, Jan 27, 2013 at 05:27:09AM +0000, Alexey Tourbin wrote:
> This change introduces a generalized routine rpmExpandMacros() to expand
> macros on behalf of user input, with improved diagnostic facilities.
> In particular, one of its arguments is a callback function which is
> called whenever an undefined macro is encountered in user input.
> 
> When specfile parser calls this routine, it supplies a custom function
> to handle undefined macros.  This function is keenly aware of specfile
> syntax: valid specfile tokens, such as %prep and %setup, when found in
> the right context, are not treated as undefined macros.  Otherwise,
> there are 3 possibilities.
> - Undefined macros in preamble and %pre/%post/... scriptlets raise
>   an error, unless a non-build parse is performed; this is to prevent
>   malformed packages from being built.
> - In other cases, a warning is issued.
> - Some tokens are permitted beyond their context (e.g. %ghost in
>   %changelog), to comply with traditional/sloppy usage.  Also, undefined
>   macros found in comments normally do not produce a warning.

	Hello,

At last I have estimated some consequences of the "undefined macro" patch.
This is not full coverage results, since that would require full Fedora
rebuild.  Nevertheless, the testing I've conducted is fairly extensive.
Below I discuss some typical and particular cases.

	TL;DR

Prohibiting undefined macros in preamble and scriptlets is probably the right
thing to do.  Warnings are also quite useful (in other specfile sections).
A lot of bugs are now unveiled.

	SLOPPY USE OF GLOBAL

The most common source of "undefined macro" diagnostics is the misuse of %global.
Unlike %define, %global evaluates its body twice.  Thus, if a specfile contains
something like this:

%global mozappdir %{_libdir}/%{name}
Name: xulrunner

The body of %global is first evaluated even before the macro %{name} is
implicitly defined with the Name tag.  This sloppy use of %global in preamble
is so pervasive that there's a special case in my patch which downgrades this
error to a warning.  (When the macro %{mozappdir} gets called/substituted, the
%{name} then will get a second chance to expand properly.)  Specfile authors
should generally use %define instead of %global.

Likewise, when the macro %{?perl_default_subpackage_tests} is used in specfile
preamble, this will now raise an error due to undefined macro %buildsubdir.
This is because %buildsubdir only gets defined after %setup.  Unfortunately,
the error cannot be recovered, since the macro also messes with %expand (which
subverts the special case for %global).  Fortunately, the macro can be easily
fixed.

	PREAMBLE IS INTOLERANT

Undefined macros in preamble will now raise an error (unless a non-build parse
is requested - see a note for mock developers below).  This is a strong
measure, but it does address a myriad of bad problems, such as resulting in
malformed packages.

Here is a few simple examples - a macro was never defined:
PyQt.spec: BuildRequires: %{qt3}-devel >= %{qt3version}
Thunar.spec: Requires: exo-devel >= %{exoversion}
gedit-code-assistance.spec: Requires: gedit-devel >= %{gtk_version}
perl-LWP-UserAgent-Determined.spec: URL: http://search.cpan.org/dist/%{pkg_name}/
rubygem-main.spec: Provides: rubygem(%{gemname}) = %{version}
xfce4-cellmodem-plugin.spec: Requires: xfce4-panel >= %{panelversion}

Unless Epoch tag is provided first, the %{epoch} will raise an error - that is,
there is no magic fall back to zero on a macro level:
maven-shared.spec: Provides:  %{name}-file-management-javadoc = %{epoch}:%{file_management_version}-%{release}
maven-shared.spec: Obsoletes: %{name}-file-management-javadoc < %{epoch}:%{file_management_version}-%{release}

A more sophisticated problem is found in gdb.spec:

%if "%{scl}" == "devtoolset-1.1"
Obsoletes: devtoolset-1.0-%{pkg_name}
%endif

Here specfile author relies on the fact that, if scl macro is not defined, a
fall back to as-is substitution will implicitly do the right thing (since the
string equality will not hold).  This is no longer permitted - not in preamble.

A similar case is found in tmda.spec and in a few other specfiles:

%if 0%{?rhel} && "%rhel" < "5"
...

Here, the first test for macro existence does not somehow legalize the second
substitution (this is because, although the && operator does perform a
short-circuit evaluation, it has nothing to do with macro expansion).

Similarly, there are quite a few bugs which come in conjunction with dependency
filtering.  For example, in saslwrapper.spec the following construction can be
observed:

%{?filter_setup:
%filter_provides_in %{python_sitearch}/.*\.so$
%if 0%{?fedora} < 17
%filter_provides_in %{ruby_sitearch}/.*\.so$
%else
%filter_provides_in %{ruby_vendorarchdir}/.*\.so$
%endif
%filter_setup
}

Unfortunately, %if does not work as expected within multiline macro
substitution like this.  What happens is that first, the whole macro invocation
is read until the matching '}'.  Second, the substitution text gets expanded and
substituted as a whole.  It is only third that the logical line chunking is
performed.  Thus, when %if line gets processed, all sandwiched lines have long
been expanded.  Hence, contrary to the author's intention, both ruby_sitearch
and ruby_vendorarchdir are required to be defined.

By the way, this whole "dependency filtering" stuff looks pretty ugly,
and needs to be cleaned up.  Look.  If every package gets engaged into
dependency filtering, then there is simply something wrong with dependencies.
But I digress - that's another story for another day.

	SCRIPTLETS ARE UNFORGIVING TOO

Since undefined macros in scriptlets usually lead to problems during
installation, these are outlawed and will raise an error, too.  However,
comments are exempt from the check; but, like in the following case, files tags
are not:

fmt-ptrn.spec:
%postun
/sbin/ldconfig
%doc	AUTHORS COPYING ChangeLog INSTALL NEWS README FAQ 

Here, %doc is treated as undefined macro in %postun scriptlet, and raises and
error.  Otherwise, the %doc line will be part of a scriptlet, and during
installation the shell will die with some obscure mention of job control.

usbmuxd.spec:
%preun
%systemd_preun usbmuxd.service
%postun/
sbin/ldconfig
%systemd_postun_with_restart usbmuxd.service 

Here, since %postun/ does not properly start a section, it is treated as
undefined macro %postun followed by a slash, and raises an error.  Otherwise,
%postun/ is treated as part of %preun scriptlet.

A similar case is found icon-naming-utils.spec:

%prep:
%setup -q
%patch0 -p1 -b .paths

In this case, %prep, %setup, and %patch are parsed on behalf of %description.

timeline.spec:
%postun
touch --no-create %{_datadir}/icons/hicolor || :
%posttran
if [ -x %{_bindir}/gtk-update-icon-cache ]; then
 %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
fi

Here, %posttran is invalid token, resulting in broken %postun.

util-vserver.spec:
%post legacy
for i in %v_services; do
	chkconfig --add v_$i
done

Macro %v_services was never defined. This ultimately results in bad
chkconfig invocation.

The following is just about the only valid case of undefined macro in a scriptlet,
which will now be outlawed:

glusterfs.spec:
%post server
pidof -c -o %PPID -x glusterd &> /dev/null

The command pidof(1) has special syntax %PPID for parent pid.
Specfile author needs to escape it as %%PPID.

	TYPOS

Typos are numerous, here is only a few examples.

toped.spec: CPPFLAGS=%{optflag} 
(optflags in singular)

gnustep-back.spec: BuildRoot: %{_tmpath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
(_tmppath misspelled)

mozilla-adblockplus.spec: install -Dpm 644 chrome/content/errors.html %{bulidroot}%{inst_dir}/chrome/content/errors.html
(buildroot misspelled)

eurephia.spec: BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}{?betatag:_%{betatag}}-root-%(%{__id_u} -n)
(missing % before the macro test)

perl-Smart-Comments.spec: perl -pi -e 's|^#!perl -T|#!%{_perl}|' t/*
(that would be __perl, not _perl... why do they want to fix t/* tests anyway?
these are normally not launched via shebang)

	PREFIX IS NOT WHAT YOU THINK

Macro named %prefix only gets defined along with the Prefix tag (which
indicates a relocatable content).  The meaning such as /usr is associated with
the %_prefix macro.  In my opinion, it is better to spell /usr explicitly.

A few examples of this bug:
psgml.spec: prefix=$RPM_BUILD_ROOT/%{prefix} \
tilda.spec: sed -i 's|%{prefix}/share|%{_datadir}/%{name}|g' src/Makefile.in
vanessa_adt.spec: mkdir -p %{buildroot}/{etc,%{prefix}/{lib,bin,doc}}

Even glibc.spec is affected:
grep '%{_prefix}/share' < rpm.filelist | \
  grep -v -e '%{_prefix}/share/zoneinfo' -e '%%dir %{prefix}/share' \
       >> common.filelist 

	MORE THINKOS

warning: wesnoth.spec:124: Undefined macro %RPM_BUILD_ROOT
(macro vs shell variable)

libyubikey.spec: BuildRoot: %{mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX}
(macro substitution vs shell command substitution)

ocaml-zip.spec: description = "%{description}"
(no macro named description after the %description section)

xnoise.spec: rm -rf $RPM_BUILD_ROOT%{_share}/locale/default
(when you mean /usr/share, you're better off spelling it as /usr/share)

zikula.spec: rm -f %{buildroot}/%{datadir}/%{name}/includes/.htaccess
(likewise, no macro datadir)

And this leads me to the point that...

	PEOPLE SHOULD USE RM, NOT 'RM -F'

epic.spec: rm -rf $RPM_BUILD_DIR/ircii-EPIC%{prog_version}
(prog_version was never defined; the author should have used rm, not 'rm -f')

bless.spec: rm -f $RPM_BUILD_ROOT%{docdir}/%{name}/INSTALL
(people should use 'rm' instead of 'rm -f'; %docdir is not a macro but is
a special directive that can be only used in %files section, much like %dir)

gcc.spec: rm -f %{buildroot}%{mandir}/man3/ffi*
(people should use 'rm' instead of 'rm -f'; there is no mandir macro)

selinux-policy.spec: rm -f %{buildroot}/%{_sysconfigdir}/selinux/%1/modules/active/policy.kern
(people should use... oh well... there is no marco _sysconfIGdir).

	A NOTE FOR MOCK DEVELOPERS

Mock uses 'rpmbuild -bs --nodeps' in order to build src.rpm package tentatively
and to query its build dependencies.  Do downgrade all "undefined macro" errors
to warnings, the command now has to be supplemented with --force.  This will
also disable source downloading, and otherwise a fully functional src.rpm package
will be obtained.  Note that --force must not be used other than for tentative
builds of src.rpm packages - there are known pitfalls in using --force for
actual builds.