Update Python3 behaviour to use the standard import mechanism during discovery

[stub42]

My charm needs to import modules from other layers. For example, reactive/postgresql/replication.py needs to import reactive/leadership.py. Per Issue #31, relative imports don't work at the moment so 'from .. import leadership' doesn't work. And I can't use absolute imports, because neither CHARM_DIR nor CHARM_DIR/reactive are on the PYTHON_PATH. And there are other glitches with the current discovery implementation, such as Issue #33 and Issue #48 .

This branch replaces the import mechanism under Python3 with one addressing all these concerns. sys.path is adjusted so discovered modules can do imports as they expect (both relative and absolute). The standard import method is used (via importlib.import_module), and the modules imported into either the 'reactive' or 'relations' package namespace. So in my original example, both 'from .. import leadership' and 'from reactive import leadership' work.

I left the old implementation in place for Python2. Is it worth tearing out Python2 support completely, or is it still worth keeping around?

[stub42]

Lack of absolute imports is making the apt and leadership layers difficult to use, and is blocking the coordinator layer from being announced. I might be able to work around this if we can't do absolute imports for some reason, by shuffling the API from reactive/leadership.py to lib/reactive/leadership.py (leaving the handlers and bootstrap where it is). I didn't twig earlier as I had been using non-standard hooks that added $CHARM_DIR to sys.path.

[stub42]

Ha. The symlink hack works (cd lib; ln -s ../reactive .), so I have a work around. Spoke too soon :-P

[stub42]

Shaved hair

[stub42]

I've worked out a solid work around, which I'm successfully using in the PostgreSQL charm allowing me to split things into more manageable chunks. I've also moved the bulk of the apt and leadership layers into lib/charms so they no longer need this.

I still think it is worth fixing though, to fix the other import glitches and avoid the work around for charms that wish to import from one reactive/*.py into another.

[freeekanayaka]

I've been caught by the same issues.

In absence of regular import mechanism at discover() level, in my case it's even harder to put in place workarounds than with postgres, because I have to import from paths that contain dashes. For example consider the following charm-built layout:

./hooks/
./hooks/relations/
./hooks/relations/jenkins-slave/
./hooks/relations/jenkins-slave/requires.py
./hooks/relations/jenkins-slave/lib/master.py

./hooks/relations/jenkins-slave/unit_tests/test_master.py

In such situation it turns out that:

1. From withing requires.py it's not possible to import utilities living in lib/master.py
2. When discover() scans the tree, it will try to import test_master.py, which fails to import not only because of 1., but also because it contains test-only dependencies that are not declared in wheehouse.txt (since they are not needed at runtime).

[freeekanayaka]

Stuart and I discussed the issue with dash-containing interfaces on IRC and agreed that a reasonable solution is to convert dashes to underscores when copying the files to ./reactive, so the tree becomes a regular Python code tree.

Plus, discover() should skip test files (I guess "anything matching test_*.py" is a good enough euristics).

I think this is an important fix, since this issue hinders proper code factoring in reactive-based charms. If nobody is availale to implement the proposed changes in Stuart's branch, please let me know and I'll help.

[stub42]

importlib.import_module() is quite happy importing modules and packages with hyphens in the name, so interface names containing hyphens need not block this branch from landing.

Getting discover() to ignore tests and switching from hooks/relations/interface-name to hooks/relations/interface_name can be done in two separate branches (and are probably two separate issues).

This branch would still improve Free's situation, as test_master.py could use relative imports to avoid the hypen in the package name or """jenkins_slave = importlib.import_module('relations.jenkins-slave')""" if an absolute path is needed for some reason.

[freeekanayaka]

Thanks for checking Stuart! Makes all sense. Hope this will be reviewed and merged.

[stub42]

Another thing this fixes: it allows pdb to set breakpoints in reactive handlers.

[stub42]

More tests demonstrating imports really happening (no mocks), and handling of paths containing hyphens (common in interface names).

[simonklb]

I encountered the same issue when trying to do relative imports. Getting this fix in would be great!