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

[Allan McRae]

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.

A