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 BFD8A138D0B for ; Mon, 6 Jul 2015 18:28:49 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 198A0E099E; Mon, 6 Jul 2015 18:28:41 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 70D22E0999 for ; Mon, 6 Jul 2015 18:28:40 +0000 (UTC) Received: from big_daddy.dol-sen.ca (S010634bdfa9ecf80.vc.shawcable.net [96.49.31.57]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id 9E23C340BDE for ; Mon, 6 Jul 2015 18:28:39 +0000 (UTC) Date: Mon, 6 Jul 2015 11:28:36 -0700 From: Brian Dolbec To: gentoo-portage-dev@lists.gentoo.org Subject: Re: [gentoo-portage-dev] [PATCH] conf: Enable to set rsync extra opts per repository Message-ID: <20150706112836.4e012276.dolsen@gentoo.org> In-Reply-To: <20150618213227.GA17153@rcKGHUlyQfVFW> References: <20150617184030.GC26811@rcKGHUlyQfVFW> <20150617133217.303b3b68.dolsen@gentoo.org> <20150618213227.GA17153@rcKGHUlyQfVFW> Organization: Gentoo 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: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Archives-Salt: 95c852e6-567c-4c8a-acf1-a1006873551f X-Archives-Hash: 155a9c6b90e65df0a73f017bfaf3e388 On Thu, 18 Jun 2015 23:32:27 +0200 =C3=89tienne Buira wrote: > Hi, thank you for reviewing >=20 > On Wed, Jun 17, 2015 at 01:32:17PM -0700, Brian Dolbec wrote: > > Be aware that I have not read over the diff very much yet. =20 > >=20 > > On Wed, 17 Jun 2015 20:40:30 +0200 > > =C3=89tienne Buira wrote: > >=20 > > > diff --git a/pym/portage/package/ebuild/config.py > > > b/pym/portage/package/ebuild/config.py index 3a4007b..08db363 > > > 100644 --- a/pym/portage/package/ebuild/config.py > > > +++ b/pym/portage/package/ebuild/config.py > > > @@ -515,6 +515,8 @@ class config(object): > > > v =3D confs.get("SYNC") > > > if v is not None: > > > portdir_sync =3D v > > > + if 'PORTAGE_RSYNC_EXTRA_OPTS' in > > > confs: > > > + > > > self['PORTAGE_RSYNC_EXTRA_OPTS'] =3D > > > confs['PORTAGE_RSYNC_EXTRA_OPTS'] self["PORTDIR"] =3D portdir > > > self["PORTDIR_OVERLAY"] =3D portdir_overlay > >=20 > > Not sure why ebuild/config.py needs changes... will look at it more >=20 > For the same reason the same thing is done about ['SYNC']: forwarding > its value to RepoConfigLoader.__init__ settings argument. >=20 > > > diff --git a/pym/portage/repository/config.py > > > b/pym/portage/repository/config.py index b7c969d..196b87a 100644 > > > --- a/pym/portage/repository/config.py > > > +++ b/pym/portage/repository/config.py > >=20 > >=20 > > This approach is not wanted, it means hard coding any sync module > > options in the main loader that might be sync-type specific. > >=20 > > We want to make it generic so any arbitrary sync-type options (more > > than the base required options) can be added for any module without > > the need to change this. Those options are not used anywhere else > > other than the sync module. >=20 > Ok, patchset follows. Sorry, it's taken so long to review these ones. There has been a lot going on recently. This is much better, I like the way you've added them for the most part. But, you added them to the actual sync module class. By doing that, it forces portage to load the sync module even if it is not going to perform any sync action. Portage's code requires that the configs be loaded for all operations, so it reads them on initialization. That is the reason I put the config validation code handling in the sync module's module_spec. It is small, lightweight, and only provides the info needed. Each sync module's __init__.py contains only the information about the sync module for the plugin system to use and any config validation code needed. The module_specific_options should be defined here. That way none of the actual sync code is loaded until a sync operation is performed. This both speeds up initialization of portage and reduces it's memory needs slightly. hmm, this is the cvs module's module_spec, I was thinking we might be able to add the module_specific_opts there after the validate_config definition, like so: module_spec =3D { 'name': 'cvs', 'description': doc, 'provides':{ 'cvs-module': { 'name': "cvs", 'class': "CVSSync", 'description': doc, 'functions': ['sync', 'new', 'exists'], 'func_desc': { 'sync': 'Performs a cvs up on the repository', 'new': 'Creates the new repository at the specified location', 'exists': 'Returns a boolean of whether the specified dir ' + 'exists and is a valid CVS repository', }, 'validate_config': CheckCVSConfig, 'module_specific_opts': None, } } } Also, the config validation code should probably be exteneded for any module specific options as well. =20 I'll look into whether that will work. I'll add your patch and see about moving things around. >=20 > > One method might be to replace the copying of the configparser > > options into python variables. Instead change it to look for the > > option in the configparser instance. And only maintain some base > > options or functions which pull from the config instance. >=20 > Not sure i understood what you meant, nor how that would look like, > please explain if patchset is not oked. >=20 > Regards. > =20 >=20 Don't worry about this, this change will require a complete re-do of the portage/repository/config code. I like your approach to handle this for the most part. It will be the easiest to do with the code that is in place now. --=20 Brian Dolbec