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

[Dave Reisner]

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.

> > 3) Avoid linting pkgrel/epoch in package functions (because we don't
> > actually support these overrides).
> 
> As said in my previous mail, I vote to get rid of overriding
> pkg{ver,rel} and epoch.
> 

I'm fine with this. It doesn't make a whole lot of sense to me, and no
packages in the Arch repos currently override pkgver/pkgrel/epoch in
package functions.

> > 4) The "arch" variable is leaked when processing arch overrides in
> > packages. For example:
> > 
> >   pkgname=(foo libfoo)
> >   arch=('i686' 'x86_64')
> >   ....
> > 
> >   package_foo() {
> >     arch=('any')
> >     :
> >   }
> > 
> >   package_libfoo() {
> >     :
> >   }
> > 
> > This leaks arch=('any') into package_libfoo. Reversing the order of
> > pkgname will mask this bug.
> > 
> 
> Huh? This works for me:
> 
> foo-1-1-any.pkg.tar.xz
> libfoo-1-1-x86_64.pkg.tar.xz

I swear I saw this, but I can't replicate the problem anymore. The
backup/restore variable logic seems relatively sane, too.