Re: [gentoo-portage-dev] Plugin framework

[Jason Stubbs]

On Sunday 13 November 2005 11:57, Brian Harring wrote:
> On Sat, Nov 12, 2005 at 11:33:06PM +0900, Jason Stubbs wrote:
> > I've had a go at creating a generic plugin framework for portage. The
> > attached patch contains:
> >
> > * plugins/__init__.py that does plugin searching and loading.
> > * plugins/cache/__init__.py which specifies what class cache plugins must
> >   derive from.
> > * cache/{anydbm,flat_hash,flat_list,metadata,sqlite}.py copied to
> >   plugins/cache/ with imports adjusted and classes renamed to match their
> >   filenames.
> > * portage.py edits to the config class to make use of the framework.
> >
> > Essentially, plugins.registry gives dict access to modules under
> > plugins/. To give an example, the plugins.cache.anydbm.anydbm class can
> > be accessed via plugins.repository["cache"]["anydbm"].
>
> Offhand... you're duplicating pythons "first found" for module
> namespace, which I'm not particularly much for, at least for a
> registry framework.
>
> If you're going to introduce a plugin registry, allow the plugins to
> exist wherever they want in python namespace, and have the registry
> entry point to it (preferably lifting code from 3.0 where possible
> also).

I'm not familiar with this. If it works better than what I've got now without 
having to hardcode what plugins are available, I'm all for it. Perhaps 
registry is the wrong word for what I was trying to do...

> > In loading, I'm iterating through sys.path and chopping that out from the
> > detected module's path. I didn't need to do this when I was first testing
> > the module loader, however. After integrating it with portage I was
> > getting "module not found" errors. If anybody knows why that is, I'll get
> > rid of that iteration. To be clear, the following doesn't work only when
> > running from portage (regardless of the path used):
> >
> > python -c '__import__("/usr/lib/portage/pym/cache")'
>
> Err... it's working for me. :)
>
> Missing __init__.py in cache, or...

The python call works but putting that line in _plugin_loader.__init__() 
failed.

> Meanwhile, couple of code comments, then final comments...
>
> > diff -uNr portage-orig/pym/plugins/__init__.py
> > portage-plugin-framework/pym/plugins/__init__.py ---
> > portage-orig/pym/plugins/__init__.py	1970-01-01 09:00:00.000000000 +0900
> > +++ portage-plugin-framework/pym/plugins/__init__.py	2005-11-12
> > 21:52:15.000000000 +0900 @@ -0,0 +1,80 @@
> > +class registry:
> > +
> > +	class _plugin_loader:
>
> Shoving the _pluging_loader class into the registry class namespace

Strange obsession with keeping the namespace empty.

> makes it a bit hard for derivatives of registry to be implemented.

I can't see any possible reason to want to derive from it. The point of it is 
that pulling external code in should be seamless. If it doesn't serve as is, 
I'd prefer that it be fixed rather than putting the functionality somewhere 
else.

> > +
> > +		def __init__(self, path):
> > +			try:
> > +				import sys
> > +				for prefix in sys.path:
> > +					if path.startswith(prefix):
>
> *cough*
> python -c'import sys;print "" in sys.path'
>
> That won't do what you're after...
> Questionable how well it's going to play with non-normalized paths
> also.

So that's why it was working from the command line! Actually, it is doing 
exactly what I was after. Path itself comes from __file__ which comes from a 
path that was originally derived from sys.path.

> > +						mod_name = path[len(prefix)+1:]
> > +						self._plugin = __import__(mod_name)
> > +						getattr(self._plugin, "plugin_class")
> > +						break
> > +			except (ImportError, AttributeError), e:
> > +				raise ImportError
>
> This leaves partially imported modules in sys.modules on import
> failures; if doing plugins, would suggest raiding portage.util.modules
> from 3.0 (already addressed it there).

Yep, I was aware of that... Didn't want to make the code too messy when the 
interface itself could be thrown out.

> > +			self._path = path
> > +			self._loaded = {}
> > +
> > +		def __getitem__(self, name):
>
> This is not thread safe (addressed in 3.0)

This is an interesting point. I wasn't thinking about thread-safety at all, 
but when should I be? Should everything be implemented with thread safety in 
mind? If not, at what point should the line be drawn?

> > +			if name in self._loaded:
> > +				return self._loaded[name]
> > +			try:
> > +				import os
> > +				plugin_path = os.path.join(self._path, name)
> > +				import sys
> > +				for prefix in sys.path:
> > +					if plugin_path.startswith(prefix):
> > +						plugin_name = plugin_path[len(prefix)+1:]
> > +						plugin = __import__(plugin_name)
> > +						break
> > +				plugin = getattr(plugin, name)
> > +				if issubclass(plugin, self._plugin.plugin_class):
> > +					self._loaded[name] = plugin
> > +					return plugin
> > +			except (ImportError, AttributeError), e:
> > +				pass
>
> Same thing here; plus it's redundant code, abuse a method...

Yep, it was much cleaner when I wrote it. Then after trying it, it didn't work 
so I incrementally modified until it did. I sent what was working before 
heading to bed. ;)

> > +			raise KeyError(name)
> > +
> > +		def keys(self):
> > +			import os
> > +			for name in os.listdir(self._path):
> > +				(name, ext) = os.path.splitext(name)
> > +				if name.startswith("_") or not ext.startswith(".py"):
> > +					continue
> > +				if name not in self._loaded:
> > +					try:
> > +						self[name]
> > +					except KeyError:
> > +						pass
> > +			keys = self._loaded.keys()
> > +			keys.sort()
> > +			return keys
> > +
> > +	def __init__(self):
> > +		import os
>
> import globally (you're using os in multiple methods)

Again, the weird empty namespace obsession.

$ python -c 'import plugins; print dir(plugins)'
['__builtins__', '__doc__', '__file__', '__name__', '__path__', 'registry']

There's something attractive about the above, but I'm not married to it.

> > +		self._path = os.path.dirname(__file__)
> > +		self._loaded = {}
> > +
> > +	def __getitem__(self, name):
> > +		if name not in self._loaded:
> > +			try:
> > +				import os
> > +				self._loaded[name] = self._plugin_loader(os.path.join(self._path,
> > name)) +			except ImportError, e:
> > +				raise KeyError, name
> > +		return self._loaded[name]
> > +
> > +	def keys(self):
> > +		import os
> > +		for name in os.listdir(self._path):
> > +			if name not in self._loaded:
> > +				try:
> > +					self[name]
> > +				except KeyError:
> > +					pass
> > +		keys = self._loaded.keys()
> > +		keys.sort()
>
> Why the sort?

Yep. Shouldn't be there. It comes from the sleepy misguided thought that the 
method would only be used by some user interface allowing the user to select 
what plugin they wanted to use...

> > +		return keys
> > +
> > +registry = registry()
>
> <snip cache and other stuff>
>
> > diff -uNr portage-orig/pym/portage.py
> > portage-plugin-framework/pym/portage.py ---
> > portage-orig/pym/portage.py	2005-11-12 21:53:05.000000000 +0900 +++
> > portage-plugin-framework/pym/portage.py	2005-11-12 21:52:15.000000000
> > +0900 @@ -1229,8 +1210,18 @@
> >  		self.virtuals = self.getvirtuals(root)
> >
> >  	def load_best_module(self,property_string):
> > -		best_mod =
> > best_from_dict(property_string,self.modules,self.module_priority)
> > -		return load_mod(best_mod)
> > +		for prio in self.module_priority:
> > +			if property_string not in self.modules[prio]:
> > +				continue
> > +			mod_split = self.modules[prio][property_string].split(".", 1)
> > +			if len(mod_split) == 2:
> > +				(mod_type, mod_name) = mod_split
> > +				#try:
> > +				return plugins.registry[mod_type][mod_name]
> > +				#except KeyError:
> > +				#	pass
> > +			writemsg("%s module %s could not be loaded.\n" % (property_string,
> > self.modules[prio][property_string])) +		raise KeyError(property_string)
> >
> >  	def lock(self):
> >  		self.locked = 1
>
> config.module_priority is _evil_ (yep, first I've noticed that gem).
> If a user specified cache backend can't be loaded, falling to a
> default is a great way to piss users off (that's a bad 'helpful'
> feature).  Meanwhile, back to your patch...

Actually, that was also my mistaken addition. The original code uses 
module_priority but only tries loading the first found. More
misguidedness. :(

> Offhand... what's the non-cache intention of this?  I'm not seeing the
> gain for stable aside from a more complex handling of cache plugins,
> and allowing for python to lose it's marbles in importing- case in
> point, while you're trying to do lookups in each sys.path for a plugin
> namespace, you're still going to have a l->r namespace pollution.

So, as I said before, the point is to unify plugin management. Instead of 
having the imports done wherever something can be plugged in, unify that and 
do it all in one place. The cache and elog plugin selection(s) come from user 
settings but emaint (and repoman whenever that happens (and possibly even 
emerge itself one day?)) needs to automatically detect what's available and 
use it.

--
Jason Stubbs


-- 
gentoo-portage-dev@g.o mailing list