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