[pacman-dev] [PATCH 1/2] Refactor check_sanity, to give it some sanity of its own

[Dave Reisner]

On Mon, Aug 04, 2014 at 03:48:38PM +1000, Allan McRae wrote:
> On 03/08/14 23:43, Allan McRae wrote:
> > On 03/08/14 22:54, Dave Reisner wrote:
> >> On Sun, Aug 03, 2014 at 06:39:54PM +1000, Allan McRae wrote:
> >>> On 03/08/14 01:54, Dave Reisner wrote:
> >>>> Break apart each of the blocks into their own separate functions. And,
> >>>> instead of the hand crafted eval statements, reuse the logic from
> >>>> pkgbuild-introspection[0] to abstract away the complexities of parsing
> >>>> bash.
> >>>>
> >>>> This commit fixes at least 3 bugs in check_sanity, and one other:
> >>>>
> >>>
> >>> Can you clarify these bugs for me?
> >>>
> >>>> 1) The wrong variable is shown for the error which would be thrown
> >>>> when, e.g.  pkgname=('foopkg' 'bar at pkg')
> >>>
> >>> What error?  This pkgname actually works here...
> >>>
> >>
> >> Sorry, I found this error my static analysis, and then randomly chose a
> >> name which turns out to be valid (I recall we made an exception for @).
> >> If you have something like pkgname=('foopkg' 'bar^pkg') you'll get the
> >> error:
> >>
> >> ==> ERROR: pkgname contains invalid characters: ''
> >>
> >> Whereas, if you use pkgname=('bar^pkg' 'foopkg'), or apply this patch,
> >> you'll see:
> >>
> >> ==> ERROR: pkgname contains invalid characters: '^'
> >>
> >>>> 2) The "arch" variable is not sanity checked when the PKGBUILD has
> >>>> package_$pkgname() instead of package().
> >>>
> >>> Do you mean with an override in package_$pkgname()?
> >>>
> >>
> >> Actually, it seems to be the case that arch is never sanity checked at
> >> all for a singular package -- has nothing to do with how the package
> >> function is named. e.g.:
> >>
> >>   pkgname=foo
> >>   arch=('i686' 'x86_64')
> >>   ...
> >>
> >>   package_foo() {  # or name this package()
> >>     arch=('invalid')
> >>
> >>     ...
> >>   }
> >>
> >> This should explode, but doesn't, and happily makes a package for
> >> x86_64 (since my host is x86_64). After patch:
> >>
> >> ==> ERROR: foo is not available for the 'x86_64' architecture.
> > 
> > Ah... I see that check_sanity tested if the number of packages > 1
> > because at that stage we have not tested whether package_foo() or
> > package() is used.    And now also see we can override stuff in
> > package(), so the #packages > 1 check was useless.
> > 
> > I'll take a look at the patch itself tomorrow.
> > 
> 
> Patch looks good.  Apart from the removing pkver/pkgrel/epoch override
> checks.  I'd suggest just removing this first (it is a trivial patch,
> remove it from the override array, adjust get_full_version, and
> check_sanity), and then adjusting this patch.

Will do.

> I also assume this fixes FS#40361.  Can you add a note of that in the
> commit message.
> 

Yes it should. I'll update the commit message and rebase my branch.