Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C extension version mismatch should be detected at import time #564

Closed
indygreg opened this issue Dec 29, 2014 · 6 comments
Closed

C extension version mismatch should be detected at import time #564

indygreg opened this issue Dec 29, 2014 · 6 comments

Comments

@indygreg
Copy link

We use psutil as part of the Firefox build system to record system resource usage. We have a kind of hacky virtualenv setup. Long story short, our local copy of psutil is added to sys.path and imported before the psutil C extension may be built (behavior is completely wrong, I know). If there are no traces of psutil on the system, psutil raises due to failure to import the C extension. That's expected.

However, if psutil is installed at the system level (apt-get install python-psutil for example), we get in a scenario where our local psutil .py files import the system Python's psutil C extension. If the version of our local psutil and the C extension differ, we likely get a run-time failure when accessing certain psutil APIs. In our case, we get a AttributeError: 'module' object has no attribute 'linux_sysinfo' when calling psutil.virtual_memory(). See more at https://bugzilla.mozilla.org/show_bug.cgi?id=1116194.

While our behavior is very wrong and we deserve to get failures, I think there is room for psutil's behavior to be more robust.

I think psutil should perform some kind of verification that the imported C extension comes from the same version of psutil that the .py files are running. This verification should be performed immediately after importing the C extension. If there is a version mismatch, I believe psutil should raise an ImportError. I believe failing fast (at import time) is better than letting psutil linger in an unknown inconsistent state, only to be discovered by the first consumer of psutil APIs.

@giampaolo
Copy link
Owner

I'm not sure what you're suggesting here. What kind of verification do you think psutil should do? Can you provide a code sample?

@indygreg
Copy link
Author

I think there should be some kind of version identifier embedded within both the compiled C extension and the .py package. That could be the psutil version number or even some randomly generated string. (Although the latter may complicate local development.) Essentially:

import psutil._pslinux as _psplatform
...
if _psplatform.version != __version__:
    raise ImportError('psutil C extension does not match source version: %s != %s' % (_psplatform.version, __version__)

I thought about coding up a patch, but I don't know Python C extensions. I could probably cobble something together if you say you'll accept the code.

@giampaolo
Copy link
Owner

Ah I see what you mean now. I don't think that is a good idea.
It would require maintaining a hard-coded version tuple in each C extension module, which is a risky extra effort because it's easy to forget to update all the tuples on every new release.
Also, the existing users will have an old version of psutil where such a tuple does not exist, which would turn our code into something like this:

 if not hasattr(_psplatform, "version") or _psplatform.version != __version__:
      ....

To me that looks too hackish and probably not robust enough (to say one, it's not testable).
The use case you describe is pretty twisted, in fact I've never bumped into something like this.
What you say is going on is that "import psutil" imports the more recent *.py modules (installed via virtualenv+pip) but the old *.c extension modules installed at system level (installed via apt).
That clearly means there's something wrong with the virtualenv or someone played with sys.path in ways he/she was not supposed to. I can't think of any robust way to detect such a messed up environment upfront (at import time).
I would suggest to either fix your build system or in case that's too much work include a routine which checks the scenario you describe into your own code (inspect psutil.__path__ perhaps?) rather than expecting psutil to perform such a check.

@indygreg
Copy link
Author

indygreg commented Jan 1, 2015

I concede that my particular use case is far from robust w.r.t. Python packaging and that I'm partially at fault for falling into this trap. However, basic run-time detection for compatibility is a good idea and I think having something basic in psutil's implementation is warranted. Other scenarios where you can fall into this trap include when you've obtained the psutil source code and you encounter an error compiling the C extensions but you still have the .py files installed and the C extensions from your system install are utilized. Packaging is complicated. This stuff does happen.

There is precedence in other Python C extensions for this. Here's PIL's implementation: python-pillow/Pillow@088c752 As you can see, it isn't that much work.

Also, you don't need to hard-code the version tuple in each C extension module: you can #define it in a shared file and #include that from each C extension module. With a little setup.py magic, it might even be possible to auto-generate that shared file from whatever version is present in setup.py or a __version__ variable in another file.

Tell me you'll accept the pull request and I'll write the code.

@giampaolo
Copy link
Owner

OK, you convinced me. I realized we can 'pass' the psutil version to the underlying C module from setup.py via a macro. As such we can avoid redefining the psutil version in each C module.
With this you should now get this error at import time:

ImportError: version conflict: '/home/giampaolo/.local/lib/python2.7/site-packages/psutil-2.2.0-py2.7-linux-x86_64.egg/_psutil_linux.so' C extension module was built for another version of psutil (different than 2.2.0)

@indygreg
Copy link
Author

indygreg commented Jan 6, 2015

Thank you very much for implementing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants