fixes #62

[EliAndrewC]

I think I've left enough comments on #62 to explain everything but I'll leave comments on anything that seems noteworthy.

[EliAndrewC]

I'll note that none of this will actually work until rpctools is ported, since that relies on some local changes I made inside my virtualenv. So no one should merge this until @ftobia has a chance to tackle that. I'll just pull from this branch to get started on MAGFest stuff in the meantime.

[robdennis]

is it surprising then that the build passes?

[robdennis]

updating the travis.yml in this build

[robdennis]

this errors on python 3 as expected, though 3.2 seems to have hung instead of just failing. Which is odd.

[EliAndrewC]

I'd expect the build to pass on Python 2, since nothing that I changed should break anything in Python 2 (except ldap auth, which is not tested and thus won't break the build for being broken),

Python 3 will fail to finish importing Sideboard because it will error on rpctools. Assuming that it doesn't fail before then for some reason.

[ftobia]

Looks like the Travis build for Python 3 is indeed failing b/c of rpctools. Lucky for you I made rpctools import without dying horribly on Python 3: https://github.com/appliedsec/rpctools/pull/4

[robdennis]

as a sanity check, should the tests still fail on python3 if I'm using the latest checkin from @ftobia's rpctools in my virtualenv?

I'm getting this:

sideboard/internal/imports.py:166: in get_plugin_venv_package_directories
>       assert os.path.exists(paths[0]), 'plugin site-packages path "{}" does not exist'.format(paths[0])
E       AssertionError: plugin site-packages path "/Users/rad3/development/sideboard/plugins/__pycache__/env/lib/python3.4/site-packages" does not exist

apparently because it finds __pycache__ and think it's a plugin:

(env)jane-air:sideboard rad3$ ls plugins/
__pycache__ conftest.py conftest.pyc

the py3 branch could specifically call out frank's checkin using "-e"

[EliAndrewC]

Interesting. I wonder why that doesn't happen on my machine. I don't have a __pycache__ in the plugins directory even after running py.test so maybe there's some kind of platform-dependent thing going on. Either way, a good general rule might be that any directory in the plugins directory starting with a . or _ should not be treated as a plugin, so I'll go ahead and make that change and then update this pull request.

[EliAndrewC]

Ok, so the bad news is that the import hook thing is a bust. There's just a fundamental assumption built into the import system that after you import something, it will be in sys.modules, which is a documented postcondition that is required to be part of any import hook. Fair enough. If Sideboard is successful enough then we can lobby to get this changed in a later version.

So I have an actual solution which is like 10 lines of code which seems to fix everything, and I would like to solicit snap opinions from @robdennis and @ftobia about how terrible it is.

Our real problem is that in Python 3, there's an importlib.import_module function which bypasses the __import__ builtin. So we can override the __import__ builtin and still have that still work for regular import statements, but if someone calls importlib.import_module then it won't be respected.

We can't just replace importlib.import_module because some third party module might have done a from importlib import import_module and then the original would be used.

Fortunately, import_module does very little and just dispatches to an undocumented function called _gcd_import, so if we replace _gcd_import then everything works:

 from importlib import _bootstrap
@wraps(_bootstrap._gcd_import)
def _new_gcd_import(name, package=None, level=0):
    if level > 0:
        name = _bootstrap._resolve_name(name, package, level)
    with _import_lock:
        return __import__(name, fromlist='*')
_bootstrap._gcd_import = _new_gcd_import

The upside is that this is really simple and works great. The downside is that we're relying on an undocumented implementation detail. I'm inclined to go with it and see if we can come up with a better solution in the future, but I'd love to get an awfulness gut check from @robdennis and @ftobia.

[EliAndrewC]

Hmm, it occurs to me that there is a somewhat safer way to do this, which is to override importlib.import_module as literally the very first thing we do, before we import any plugins. Our overridden version would basically be implemented as

if plugins_not_imported_yet:
    return original_import_module(...)
else:
    return __import__(...)

That seems somewhat more robust, and I'd expect it to even work on older and newer versions of Python 3.x even if they change the original importlib documentation.

(One thing that is clearly true is that no matter what we do will probably break horribly if a Sideboard plugin also implements its own import hooks. That's just not supported for now, although in time we could swap out e.g. sys.meta_path the way we swap out sys.path and sys.modules.)

[EliAndrewC]

Ok, so I think I finally have this working! (Bleh.)

I tried overriding sys.modules, but much like on Python 2 it must be an actual Python dict (not a MutableMapping and not even a subclass of dict). So that was a no go.

Monkeypatching importlib.import_module eventually worked great, though my first few implementations didn't work out because calling __import__ from within the import_module function led to a situation where changes to sys.path and sys.module got swapped out before they should have. I tried out three different implementations to address this and settled on the one that I liked best.

So this is all good news overall, things seem to be working okay, so I just need to fix the unit tests to match the new internal API, which I can probably get done tomorrow, and then I'll update this pull request.

For now, the MAGFest code is happily running under Sideboard, and the MAGFest unit tests actually pass. Admittedly, there aren't very many of them so that's not a big deal, but the site seems to be working okay, which is a great sign.

[EliAndrewC]

Since @ftobia has finished porting rpctools to run on Python 3, I'm going to go ahead and push that out as a new release, then update requirements.txt to say rpctools==0.2 and then as soon as the tests pass on travis, I'll merge this into master.

LDAP authentication will still be commented out, but the unit tests don't require it to pass, so I'll just go with it for now, because I want to be able to make other pull requests for other features without having to have a ton of branches which rely on this branch. Hopefully @robdennis can complete that ticket soon, but since it's not blocking me on my MAGFest work we can go without LDAP working in master for awhile.

As a side note, I learned over the weekend that the "Files Changed" tab in a pull request shows a single combined diff of every file instead of showing the commits separately. So in the future I don't think we need to bother rebasing to merge commits into a single commit.

[ftobia]

I love the "Files Changed" tab, and I use it almost exclusively.

I'm glad to hear that my rpctools work is moving the ball forward over here.

[EliAndrewC]

This passed on Python 2.7 and 3.3 and 3.4 but timed out on 3.2 from the tests taking too long, so I'm just merging.