On Sun, Jan 19, 2014 at 6:23 PM, Tom Wijsman <tom...@gentoo.org> wrote:
> On Sun, 19 Jan 2014 04:38:31 -0500
> Mike Frysinger <vap...@gentoo.org> wrote:
>
> > On Friday 17 January 2014 18:03:57 Tom Wijsman wrote:
> > > ---
> >
> > please shorten your commit summary and move more content to the body
>
> Right, I'm used to writing this on the command line; I'll try to look
> into setting up an editor or a GUI (gitg, ...) to do my commit messages.
>
> > > +getNonSystemArchiveDepends.archivers = {
> >
> > it is super weird to attach to the object like this. some might even
> > say wrong.
>
> It a function, this makes it a static function variable.
>
> What is weird here? It works properly, what would be wrong?
>
It is certainly weird (as we discussed on IRC.) I've never seen anyone do
it in any codebase I liked.
One of the problems is that it isn't immutable, so that earlier callers can
mess with later callers. That is not possible in vapier's proposal, as the
attributes are hidden in the function code and are not visible to callers.
> move it into the class definition.
> > def getNonSystemArchiveDepends(fetchlist, eapi):
> > ...
> >
> > ARCHIVERS = {
> > ...
> > }
>
> That makes it a non-static function variable? This is a regression.
>
I guess I am not seeing why it must be a static function variable. Can you
explain?
>
> > > + re.compile('.*\.7[zZ]$'):"app-arch/p7zip",
> >
> > regexes should always use raw strings. there should also be a space
> > after the colon in dicts. so you want:
> >
> > re.compile(r'.*\.7[zZ]$'): 'app-arch/p7zip',
>
> Raw strings are a solution when \ forms a problem, which is not the
> case here. The colon has no space after it in the rest of repoman near
> it, hence I left it out; is there a coding standard we're trying to
> respect here? Can you refer it to me?
>
For the colon's in dicts thing:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace
>
> > > + re.compile('.*\.lzma$'):"app-arch/lzma-utils",
> > xz-utils, not lzma-utils
>
> http://dev.gentoo.org/~ulm/pms/5/pms.html
>
> Please see "unpack" in PMS "11.3.3.13 Misc Commands", quote:
>
> "lzma-compressed files (*.lzma). Ebuilds must ensure that LZMA Utils is
> installed."
>
> The TODO comment that had a PMS reference was near it before, but with
> the refactor it has since moved; I'll put back the reference to it.
>
> > > + re.compile('.*\.(bz2?|tbz2)$'):"app-arch/bzip2",
> > > +
> > > re.compile('.*\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?$'):"app-arch/tar",
> > > + re.compile('.*\.(gz|tar\.Z|t[bg]z|[zZ])$'):"app-arch/gzip",
> > > + re.compile('.*\.tar.xz$'):"app-arch/tar",
> >
> > requiring people list gzip, tar, and bzip2 is a significant policy
> > change (which i'm inclined to say is wrong). it needs discussion on
> > the dev mailing list first.
>
> Please see "unpack" in PMS "11.3.3.13 Misc Commands", example quote:
>
> "gzip-compressed tar files (*.tar.gz, *.tgz, *.tar.Z, *.tbz). Ebuilds
> must ensure that GNU gzip and GNU tar are installed."
>
> To spare on mail length, I don't quote the rest of that enumeration.
>
The @system set in gentoo will ensure these are installed. I understand the
wording of PMS (as the dependencies should be expressed somewhere) but in
general we prefer to do that in @system. For the same reason all packages
do not depend on glibc, or the compiler, or anything else.
>
> > > +def checkArchiveDepends(atoms, catpkg, relative_path, \
> > > + system_set_atoms, needed_unpack_depends, stats, fails):
> >
> > you don't need the \ there because you have paren already to gather
> > things.
> >
> > > + for entry in needed_unpack_depends[catpkg]:
> > > + if entry not in [atom.cp for atom in atoms if
> > > atom != "||"]:
> > > + stats['unpack.' + mytype + '.missing'] += 1
> > > + fails['unpack.' + mytype +
> > > '.missing'].append( \
> > > + relative_path + ": %s is missing
> > > in %s" % \
> > > + (entry, mytype))
> >
> > i know existing style doesn't follow it, but imo we should avoid
> > string concatenation. it makes the code harder to read imo.
> >
> > key = 'unpack.%s.missing' % mytype
> > stats[key] += 1
> > fails[key].append(...)
> >
> > > +def eapi_has_xz_utils(eapi):
> > > + return eapi not in ("0", "1", "2")
> >
> > i would use "eapi_supports_xz_archives" unless there's a pre-existing
> > style -mike
>
> Good idea, I'll take these last ones into account in the next version.
>
> Thank you everyone for the reviews so far.
>
> --
> With kind regards,
>
> Tom Wijsman (TomWij)
> Gentoo Developer
>
> E-mail address : tom...@gentoo.org
> GPG Public Key : 6D34E57D
> GPG Fingerprint : C165 AF18 AB4C 400B C3D2 ABF0 95B2 1FCD 6D34 E57D
>
