Re: [gentoo-portage-dev] [PATCH 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).
On Sun, 19 Jan 2014 18:43:46 -0800
Alec Warner anta...@gentoo.org wrote:
It is certainly weird (as we discussed on IRC.) I've never seen
anyone do it in any codebase I liked.
My backlog was limited so I didn't catch that discussion, feel free to
share the log; I've since increased it. There's a lot more talk than my
defaults on IRC (as well as here on the mailing list). :)
On a side note, I liked seems a too subjective way to review patches.
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.
True, but do you have a better suggestion? (Not the one below)
From a quick lookup Python seems to not really provide a clean
immutable solution here; one option would be to use a frozenset, but
then one has to make classes to put into that (which are still
mutable). That is a misuse for what could just be a dictionary.
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?
Because you would call re.compile for each time that function is
called; while the most recent compiled versions of re.compile are
cached, I still do not see a reason for this variable not to be static.
For the colon's in dicts thing:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace
Okay, I will follow those guidelines.
The @system set in gentoo will ensure these are installed.
You can compare @system against the PMS and you will note that entries
are missing in @system; the @system set only covers the most popular
ones, the rest is left up to the maintainer to add to the ebuild. Thus
this enumerates all of them; as the @system set can change in the
future, we need to make the code future proof hence the @system check.
It is possible for such atom to get removed from @system later.
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.
Things that are in @system are not complained about by this code:
if format not in system_set_atoms:
--
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
signature.asc
Description: PGP signature
Re: [gentoo-portage-dev] [PATCH 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).
On Friday 17 January 2014 18:03:57 Tom Wijsman wrote:
---
please shorten your commit summary and move more content to the body
+getNonSystemArchiveDepends.archivers = {
it is super weird to attach to the object like this. some might even say
wrong. move it into the class definition.
def getNonSystemArchiveDepends(fetchlist, eapi):
...
ARCHIVERS = {
...
}
+ 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',
+ re.compile('.*\.lzma$'):app-arch/lzma-utils,
xz-utils, not lzma-utils
+ 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.
+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
signature.asc
Description: This is a digitally signed message part.
Re: [gentoo-portage-dev] [PATCH 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).
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
