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 28CC0138247 for ; Mon, 20 Jan 2014 02:43:50 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 89ADCE0D2D; Mon, 20 Jan 2014 02:43:48 +0000 (UTC) Received: from mail-wg0-f53.google.com (mail-wg0-f53.google.com [74.125.82.53]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id B4718E0D1A for ; Mon, 20 Jan 2014 02:43:47 +0000 (UTC) Received: by mail-wg0-f53.google.com with SMTP id y10so6428985wgg.20 for ; Sun, 19 Jan 2014 18:43:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to:content-type; bh=Q72Y9SL9YzzQNdajPsU9MHFLUUNBBJeGvbkcm2ozkdc=; b=FFhJJqansNAb+I6jtsryV718pUAhjtDKTLlcBNOCbGwbl6lW23yfdSNdeH0Ana0ii+ dPVEZzogn19TlE6cyTPEbkEGFuS7aMR6ucPefus7yCzWpnVOJ0kNp8+FiZPmiS4SZMTD Y19um9hXF365hrnrd9pJLPu3jEO7EPfloEpugC1q9Uu4BDPP0y8L8vl+oJtTQO2ANk/t opy1avm7ZWT0rtO0xViqSRRcvqNy3SFjAd0W5fjNXCAPlffxY6l1ZTJHnQqY9Mt/nzmG ETVT9OuKi80dgEmK6R4Mb2pvyug3meXfTt02YaxYac3UTjNqf8zycUJrEZFCOWzRpVts 9DNQ== X-Gm-Message-State: ALoCoQkbDnCLVjmb6EJpsLcYP8oCvoSV2wQFekp7DjPIPpOaZQGJoMoKptc5sol//kp9sGK4gP3B 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 X-Received: by 10.180.99.100 with SMTP id ep4mr744148wib.31.1390185826381; Sun, 19 Jan 2014 18:43:46 -0800 (PST) Sender: antarus@scriptkitty.com Received: by 10.216.170.129 with HTTP; Sun, 19 Jan 2014 18:43:46 -0800 (PST) X-Originating-IP: [173.8.165.226] In-Reply-To: <20140120032348.429ed4d3@TOMWIJ-GENTOO> References: <1389830840-25848-2-git-send-email-tomwij@gentoo.org> <1389999837-16516-1-git-send-email-tomwij@gentoo.org> <201401190438.32666.vapier@gentoo.org> <20140120032348.429ed4d3@TOMWIJ-GENTOO> Date: Sun, 19 Jan 2014 18:43:46 -0800 X-Google-Sender-Auth: cl74DOlrz7XpNNjjI433aE11mJg Message-ID: 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). From: Alec Warner To: gentoo-portage-dev@lists.gentoo.org Content-Type: multipart/alternative; boundary=f46d04428e5c5a852b04f05ddc08 X-Archives-Salt: 0cf27e6b-bec6-4b7e-a444-8b5bf89eb18f X-Archives-Hash: fa693ad2dd2fe762233a9cbb1d7e7076 --f46d04428e5c5a852b04f05ddc08 Content-Type: text/plain; charset=UTF-8 On Sun, Jan 19, 2014 at 6:23 PM, Tom Wijsman wrote: > On Sun, 19 Jan 2014 04:38:31 -0500 > Mike Frysinger 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 : TomWij@gentoo.org > GPG Public Key : 6D34E57D > GPG Fingerprint : C165 AF18 AB4C 400B C3D2 ABF0 95B2 1FCD 6D34 E57D > --f46d04428e5c5a852b04f05ddc08 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On S= un, Jan 19, 2014 at 6:23 PM, Tom Wijsman <TomWij@gentoo.org>= wrote:
On Sun, 19 Jan 2014 04:38:31 -0500
Mike Frysinger <vapier@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 =3D {
>
> it is super weird to attach to the object like this. =C2=A0some 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 ca= llers can mess with later callers. That is not possible in vapier's pro= posal, as the attributes are hidden in the function code and are not visibl= e to callers.

> move it into the class definition.
> def getNonSystemArchiveDepends(fetchlist, eapi):
> =C2=A0 =C2=A0 =C2=A0 ...
>
> =C2=A0 =C2=A0 =C2=A0 ARCHIVERS =3D {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ...
> =C2=A0 =C2=A0 =C2=A0 }

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?
=C2=A0

> > + =C2=A0 re.compile('.*\.7[zZ]$'):"app-arch/p7zip&qu= ot;,
>
> regexes should always use raw strings. =C2=A0there should also be a sp= ace
> after the colon in dicts. =C2=A0so you want:
>
> =C2=A0 =C2=A0 =C2=A0 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?


=C2=A0

> > + =C2=A0 re.compile('.*\.lzma$'):"app-arch/lzma-util= s",
> 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", q= uote:

"lzma-compressed files (*.lzma). Ebuilds must ensure that LZMA Utils i= s
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.

> > + =C2=A0 re.compile('.*\.(bz2?|tbz2)$'):"app-arch/bz= ip2",
> > +
> > re.compile('.*\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?$'):&qu= ot;app-arch/tar",
> > + =C2=A0 re.compile('.*\.(gz|tar\.Z|t[bg]z|[zZ])$'):"= ;app-arch/gzip",
> > + =C2=A0 re.compile('.*\.tar.xz$'):"app-arch/tar&quo= t;,
>
> requiring people list gzip, tar, and bzip2 is a significant policy
> change (which i'm inclined to say is wrong). =C2=A0it needs discus= sion on
> the dev mailing list first.

Please see "unpack" in PMS "11.3.3.13 Misc Commands&qu= ot;, example quote:

"gzip-compressed tar files (*.tar.gz, *.tgz, *.tar.Z, *.tbz). Ebuilds<= br> 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 the= se are installed. I understand the wording of PMS (as the dependencies shou= ld 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, o= r anything else.
=C2=A0

> > +def checkArchiveDepends(atoms, catpkg, relative_path, \
> > + =C2=A0 system_set_atoms, needed_unpack_depends, stats, fails):<= br> >
> you don't need the \ there because you have paren already to gathe= r
> things.
>
> > + =C2=A0 for entry in needed_unpack_depends[catpkg]:
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if entry not in [atom.cp for= atom in atoms if
> > atom !=3D "||"]:
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = stats['unpack.' + mytype + '.missing'] +=3D 1
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = fails['unpack.' + mytype +
> > '.missing'].append( \
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 relative_path + ": %s is missing
> > in %s" % \
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 (entry, mytype))
>
> i know existing style doesn't follow it, but imo we should avoid > string concatenation. =C2=A0it makes the code harder to read imo.
>
> =C2=A0 =C2=A0 =C2=A0 key =3D 'unpack.%s.missing' % mytype
> =C2=A0 =C2=A0 =C2=A0 stats[key] +=3D 1
> =C2=A0 =C2=A0 =C2=A0 fails[key].append(...)
>
> > +def eapi_has_xz_utils(eapi):
> > + =C2=A0 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 ver= sion.

Thank you everyone for the reviews so far.

--
With kind regards,

Tom Wijsman (TomWij)
Gentoo Developer

E-mail address =C2=A0: TomWij@gentoo.o= rg
GPG Public Key =C2=A0: 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2 =C2=A0ABF0 95B2 1FCD 6D34 E57D

--f46d04428e5c5a852b04f05ddc08--