[Rpm-maint] Strange test in doPatch/doUntar

[Jean Delvare]

Hi Panu,

On Wed, 28 Jan 2015 17:24:00 +0200, Panu Matilainen wrote:
> On 01/28/2015 05:08 PM, Jean Delvare wrote:
> > Hi all,
> >
> > While working on the rpmbuild performance patch I send last week, I
> > noticed something curious in file build/parsePrep.c, functions doPatch
> > and doUntar. These two functions look quite similar in the way they are
> > called and what they do. However they have different conditions for
> > early exit. doPatch has:
> >
> >     /* On non-build parse's, file cannot be stat'd or read. */
> >      if ((spec->flags & RPMSPEC_FORCE) || rpmFileIsCompressed(fn, &compressed) || checkOwners(fn)) goto exit;
> >
> > while doUntar has:
> >
> >      /* XXX On non-build parse's, file cannot be stat'd or read */
> >      if (!(spec->flags & RPMSPEC_FORCE) && (rpmFileIsCompressed(fn, &compressed) || checkOwners(fn))) {
> > 	goto exit;
> >      }
> >
> > While I will gladly admit my ignorance regarding what a "non-build
> > parse" is, I find the difference in logic rather suspect. Can anyone
> > more familiar with the code double check if one of these conditions is
> > incorrect?
> 
> Non-build parse is a spec query basically, such as 'rpmspec -q foo.spec' 
> or using the API.

Oh, I didn't know about rpmspec, thanks for the pointer.

> > Looking at the git history, I can see that the conditions were similar
> > up to this commit which changed the condition in doPatch:
> >
> > commit 54b027535f95fea9088f2cd12ed765fddb20c5c1
> > Author: Panu Matilainen
> > Date:   Wed Mar 18 14:53:34 2009 +0200
> >
> >      Don't try to parse %patch on spec query (rhbz#487855)
> >
> > So I would bet that the condition in doUntar is incorrect and should be
> > fixed the same way?
> 
> This is rpm, forget about logic ;)
> 
> IIRC the answer is that both cases are behaving incorrectly or at least 
> sub-optimally: you'd want patches and tarballs looked at even on 
> non-build parse IF they exist locally. RPMSPEC_FORCE presence should 
> only cause it to return with non-error if the files are not locally 
> present. The current difference comes from patches being handed to 
> macros but tar-case being handled in C.

OK, if everything is as it should then alright, I can move this concern
outside of my mind ;)

Thanks for your fast answer,
-- 
Jean Delvare
SUSE L3 Support