Re: [gentoo-portage-dev] Plugin framework

[Brian Harring]

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).


> 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...

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 
makes it a bit hard for derivatives of registry to be implemented.


> +
> +		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.

> +						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).

> +			self._path = path
> +			self._loaded = {}
> +
> +		def __getitem__(self, name):

This is not thread safe (addressed in 3.0)

> +			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...

> +			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)

> +		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?

> +		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...

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.

Python re-uses previously imported modules; cache namespace (fex) is 
going to default to portage's bundled version, regardless of any 
modules in different sys.path trying to define their own cache module.  
Python will just reuse the previous found module... not very flexible.
~harring