From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id 7618E138247 for ; Mon, 20 Jan 2014 02:24:54 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 37EF8E0D41; Mon, 20 Jan 2014 02:24:52 +0000 (UTC) Received: from albert.telenet-ops.be (albert.telenet-ops.be [195.130.137.90]) by pigeon.gentoo.org (Postfix) with ESMTP id 6C099E0D2D for ; Mon, 20 Jan 2014 02:24:51 +0000 (UTC) Received: from TOMWIJ-GENTOO ([94.226.55.127]) by albert.telenet-ops.be with bizsmtp id G2Qq1n00N2khLEN062QqyP; Mon, 20 Jan 2014 03:24:50 +0100 Date: Mon, 20 Jan 2014 03:23:48 +0100 From: Tom Wijsman To: vapier@gentoo.org Cc: gentoo-portage-dev@lists.gentoo.org Subject: 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). Message-ID: <20140120032348.429ed4d3@TOMWIJ-GENTOO> In-Reply-To: <201401190438.32666.vapier@gentoo.org> References: <1389830840-25848-2-git-send-email-tomwij@gentoo.org> <1389999837-16516-1-git-send-email-tomwij@gentoo.org> <201401190438.32666.vapier@gentoo.org> X-Mailer: Claws Mail 3.9.0 (GTK+ 2.24.22; x86_64-pc-linux-gnu) Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-portage-dev@lists.gentoo.org Reply-to: gentoo-portage-dev@lists.gentoo.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=PGP-SHA1; boundary="Sig_/ZdyZdcggeMYI.qiKGwAMvww"; protocol="application/pgp-signature" X-Archives-Salt: 03c0c589-efd2-496a-88e5-ad2c0aaf29c1 X-Archives-Hash: 75a8a0bb705edab2cada19c17fe321e2 --Sig_/ZdyZdcggeMYI.qiKGwAMvww Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Sun, 19 Jan 2014 04:38:31 -0500 Mike Frysinger wrote: > On Friday 17 January 2014 18:03:57 Tom Wijsman wrote: > > --- >=20 > 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. =20 > > +getNonSystemArchiveDepends.archivers =3D { >=20 > it is super weird to attach to the object like this. some might even > say wrong. =20 It a function, this makes it a static function variable. What is weird here? It works properly, what would be wrong? > move it into the class definition. > def getNonSystemArchiveDepends(fetchlist, eapi): > ... >=20 > ARCHIVERS =3D { > ... > } That makes it a non-static function variable? This is a regression. > > + re.compile('.*\.7[zZ]$'):"app-arch/p7zip", >=20 > regexes should always use raw strings. there should also be a space > after the colon in dicts. so you want: >=20 > 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? > > + 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", >=20 > 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. > > +def checkArchiveDepends(atoms, catpkg, relative_path, \ > > + system_set_atoms, needed_unpack_depends, stats, fails): >=20 > you don't need the \ there because you have paren already to gather > things. >=20 > > + for entry in needed_unpack_depends[catpkg]: > > + if entry not in [atom.cp for atom in atoms if > > atom !=3D "||"]: > > + stats['unpack.' + mytype + '.missing'] +=3D 1 > > + fails['unpack.' + mytype + > > '.missing'].append( \ > > + relative_path + ": %s is missing > > in %s" % \ > > + (entry, mytype)) >=20 > i know existing style doesn't follow it, but imo we should avoid > string concatenation. it makes the code harder to read imo. >=20 > key =3D 'unpack.%s.missing' % mytype > stats[key] +=3D 1 > fails[key].append(...) >=20 > > +def eapi_has_xz_utils(eapi): > > + return eapi not in ("0", "1", "2") >=20 > 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. --=20 With kind regards, Tom Wijsman (TomWij) Gentoo Developer E-mail address : TomWij@gentoo.org GPG Public Key : 6D34E57D GPG Fingerprint : C165 AF18 AB4C 400B C3D2 ABF0 95B2 1FCD 6D34 E57D --Sig_/ZdyZdcggeMYI.qiKGwAMvww Content-Type: application/pgp-signature; name=signature.asc Content-Disposition: attachment; filename=signature.asc -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQEcBAEBAgAGBQJS3Ii0AAoJEJWyH81tNOV9oE8H/R2a6OXJ2QVx7p+/cpHnOuOB J7b7N3quciuDZbHyjsBcaHpRH26vLiW/VT1hhBQoYoJOteutr7a8nvWZ0EpXjzNa 5fqDE8pwi7bO+0ICJXyWpz+gmo5k63+qQTwKnktX+khNzsPMZpHEmFQi4yU1dLZV xssX8CdUEGUSHR6w/yXBsZmPy3m8v9UGV1GaE/QLItJBUciSZGY3/yZf2yuB7LUG KX43XRZwxV4v3Hjg5ado+LXLkH3icOanW1WOd/lgDBv3SoJg6Nmkv/PDcFHpRlN4 AHoNKiYlFOgFIYBaV+zU8i4dVyNk6wetO2bEBsV/3MeJCbmjQBjahl+8EefZMB4= =oVQY -----END PGP SIGNATURE----- --Sig_/ZdyZdcggeMYI.qiKGwAMvww--