Re: [gentoo-portage-dev] [PATCH] conf: Enable to set rsync extra opts per repository

[Brian Dolbec]

On Thu, 18 Jun 2015 23:32:27 +0200
Étienne Buira <etienne.buira@×××××.com> wrote:

> Hi, thank you for reviewing
>
> 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.
> >
> > On Wed, 17 Jun 2015 20:40:30 +0200
> > Étienne Buira <etienne.buira@×××××.com> wrote:
> >
> > > 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 = confs.get("SYNC")
> > >  				if v is not None:
> > >  					portdir_sync = v
> > > +				if 'PORTAGE_RSYNC_EXTRA_OPTS' in
> > > confs:
> > > +
> > > self['PORTAGE_RSYNC_EXTRA_OPTS'] =
> > > confs['PORTAGE_RSYNC_EXTRA_OPTS'] self["PORTDIR"] = portdir
> > >  			self["PORTDIR_OVERLAY"] = portdir_overlay
> >
> > Not sure why ebuild/config.py needs changes... will look at it more
>
> For the same reason the same thing is done about ['SYNC']: forwarding
> its value to RepoConfigLoader.__init__ settings argument.
>
> > > 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
> >
> >
> > This approach is not wanted, it means hard coding any sync module
> > options in the main loader that might be sync-type specific.
> >
> > 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.
>
> 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 = {
	'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.  

 I'll look into whether that will work.
I'll add your patch and see about moving things around.


>
> > 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.
>
> Not sure i understood what you meant, nor how that would look like,
> please explain if patchset is not oked.
>
> Regards.
>
>

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.


-- 
Brian Dolbec <dolsen>