On Sun, Oct 19, 2014 at 09:28:12PM +1000, Allan McRae wrote: > Having been forced to watch this by my daughter, I still do not > understand the appeal of ponies... >
I don't even care about the show anymore, I just like using ponies for test data (*especially* at work). > > Having not even looked at your patch at this stage... Do we detect when > this fails? Could we detect it at the .PKGINFO construction stage and > print a warning? > Not sure if this is a typo. There won't be a .PKGINFO built at the same time as a .SRCINFO so we can't diff them in any way. We could possibly detect it at .SRCINFO construction time for some subset of the failures. My take on parsing problems: There's a bunch of unit tests which highlight known features and deficiencies (see the expectedFailure decorated methods): https://github.com/falconindy/pkgbuild-introspection/blob/master/test/testcases.py There's probably others. I think only HandlesShellVarInPackageAttr is worth putting effort into fixing/detecting. We can detect *some* cases of it by calling out empty attributes. As for the rest -- we can largely "fix" these by shaming people and asking for patches and/or more tests. Speaking of tests, I have to remind you about this patch (or at least the idea of this patch): https://lists.archlinux.org/pipermail/pacman-dev/2014-August/019261.html Merging something like that would allow me to at least move the existing tests that I've written against this code into the pacman repo. We'd then be in a much better position to accept patches against the SRCINFO generation code and be less afraid of regressions. > This was something that had been flagged as potential for inclusion > anyway. And I want this in for what I have planned post release... > > My usual test PKGBUILD: > > PKGBUILD: > pkgname='foobar' > pkgver=1 > pkgrel=1 > arch=('i686' 'x86_64') > source=('tmp.txt') > source_x86_64+=('foo.bar') > md5sums=('d41d8cd98f00b204e9800998ecf8427e') > md5sums_x86_64=('d41d8cd98f00b204e9800998ecf8427e') > > .SRCINFO > # Generated by makepkg 4.1.2-468-gf74e > # Sun Oct 19 11:22:00 UTC 2014 > pkgbase = foobar > pkgver = 1 > pkgrel = 1 > epoch = 0 > arch = i686 > arch = x86_64 > checkdepends = > makedepends = > depends = > optdepends = > provides = > conflicts = > replaces = > source = tmp.txt > md5sums = d41d8cd98f00b204e9800998ecf8427e > source_x86_64 = foo.bar > > pkgname = foobar > > So there are some bugs: > - epoch = 0 (should be not printed) I didn't see this in mkaurball because it operates in a much cleaner environment. makepkg explicitly does this: # set defaults if they weren't specified in buildfile pkgbase=${pkgbase:-${pkgname[0]}} epoch=${epoch:-0} basever=$(get_full_version) ...which is where an epoch of 0 comes from. I'm sure I can fix this, but I'm punting it to another patch. > - Lots of empty values Porting error -- just needed to localize a var in extract_global_var. > - No md5sum_x86_64 Missing from the arch-specific list. An oversight that exists in mkaurball, too! > > Otherwise, all good. Small query: > > > > + > > +srcinfo_write_attr() { > > + # $1: attr name > > + # $2: attr values > > + > > + local attrname=$1 attrvalues=("${@:2}") > > + > > + # normalize whitespace, strip leading and trailing > > + attrvalues=("${attrvalues[@]//+([[:space:]])/ }") > > + attrvalues=("${attrvalues[@]#[[:space:]]}") > > + attrvalues=("${attrvalues[@]%[[:space:]]}") > > Make all whitespace a single space then strip whitespace from start and > finish? What is the first step for? We do something similar for .PKGINFO (but without steps 2 and 3 which we should probably do there, too). Consider some busted ass shit like this: pkgdesc=" ponies of the world unite!!! " Step 1 normalizes the whitespace, yielding this: pkgdesc=" ponies of the world unite!!! " Steps 2/3 get rid of the remaining leading/trailing. I've thought about making this a lint error, but I think it's a bit too esoteric. Let's just clean it up for the user. d
