GNOME Bugzilla – Bug 742349
Implement logic in Python instead of using CPython's API
Last modified: 2015-01-20 09:25:07 UTC
Instead of implementing all of the loader's logic using CPython's API we should implement it in Python itself using internal.py.
Created attachment 293829 [details] [review] Shorten the internal Python filename Using peas-plugin-loader-python-internal.py is just too long. --- This is useful for coming patches.
Created attachment 293830 [details] [review] Move internal Python code to another file All code that deals with internal.py is now in peas-python-internal.c instead of cluttering the plugin loader code.
Created attachment 293833 [details] [review] Remove overly verbose warnings in the Python loader Instead of very specific and still confusing messages we use g_return_*() to give us an idea about what went wrong while initializing. This only affects the internal Python initialization code.
Created attachment 293834 [details] [review] Use Python to implement the plugin loader's logic This allows us to avoid the CPython API and have a more understandable implementation.
Created attachment 293835 [details] [review] Add verbose Python plugin loader warnings Supports formatting Python objects and automatically includes the current Python exception in the message if there is one. This is great for the test suite which fails on the first uncaught warning, thus the Python exception is normally not printed. --- This sucks, but there isn't an easier way to turn a Python exception into a string.
Created attachment 294783 [details] [review] Compile internal Python source during build Otherwise syntax errors aren't noticed until the loader is initialized at runtime.
Created attachment 294784 [details] [review] Shorten Python plugin loader filenames Using "peas-plugin-loader-python" as a prefix is just too long.
Created attachment 294785 [details] [review] Move internal Python code to another file All code that deals with internal.py is now in peas-python-internal.c instead of cluttering the plugin loader code.
Created attachment 294786 [details] [review] Remove overly verbose warnings in the Python loader Instead of very specific and still confusing messages we use g_return_*() to give us an idea about what went wrong while initializing. This only affects the internal Python initialization code.
Created attachment 294787 [details] [review] Use Python to implement the plugin loader's logic This allows us to avoid the CPython API and have a more understandable implementation.
Created attachment 294788 [details] [review] Add verbose Python plugin loader warnings Supports formatting Python objects and automatically includes the current Python exception in the message if there is one. This is great for the test suite which fails on the first uncaught warning, thus the Python exception is normally not printed.
Created attachment 294952 [details] [review] Use Python to implement the plugin loader's logic v3 This allows us to avoid the CPython API and have a more understandable implementation. ---- Just a few tweaks for the next patch.
Created attachment 294953 [details] [review] Add verbose Python plugin loader warnings v3 Use a Python hook to implement the call logic. This allows use to have verbose error messages and include the exception traceback. This also adds format_plugin_exception() which removes all internal Python frames from the traceback. ---- This is a complete rework of the original patch, all of the work is now done in Python. This is of course the goal of this patch set and avoids adding ~200 lines of C to do an incomplete version of this path.
Review of attachment 294783 [details] [review]: Looks good, see the note though. ::: loaders/python/peas-python-compile.py @@ +4,3 @@ +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Library General Public License as published by old license? you should use the Lesser one?
Review of attachment 294784 [details] [review]: Sure
Review of attachment 294785 [details] [review]: Looks good. About the license, maybe make another commit changing all the licenses? ::: loaders/python/peas-plugin-loader-python.c @@ +620,3 @@ { state = PyGILState_Ensure (); + g_clear_pointer (&priv->internal, checking already for priv->internal != NULL, just call the free method directly? or you want to safe the extra line to set it to NULL? ::: loaders/python/peas-python-internal.c @@ +6,3 @@ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Library General Public License as published by lesser? @@ +17,3 @@ + * You should have received a copy of the GNU Library General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. also this is not used anymore afaik, it should point to the web page or something if I recall correct
Review of attachment 294786 [details] [review]: See the comment. ::: loaders/python/peas-python-internal.c @@ -123,3 @@ - "failed to run internal Python code"); - - Py_DECREF (globals); you are removing this decref, is this right? if yes please put it in a different commit
Review of attachment 294952 [details] [review]: Overlooked at it and seems fine to me. Cool stuff.
Review of attachment 294953 [details] [review]: This should be split in 2 patches but ok
(In reply to comment #14) > Review of attachment 294783 [details] [review]: > > Looks good, see the note though. > > ::: loaders/python/peas-python-compile.py > @@ +4,3 @@ > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU Library General Public License as published by > > old license? you should use the Lesser one? Huh, seems when the license was updated we used 2.0 not 2.1, will update in another bug. (In reply to comment #16) > Review of attachment 294785 [details] [review]: > > Looks good. About the license, maybe make another commit changing all the > licenses? > > ::: loaders/python/peas-plugin-loader-python.c > @@ +620,3 @@ > { > state = PyGILState_Ensure (); > + g_clear_pointer (&priv->internal, > > checking already for priv->internal != NULL, just call the free method > directly? or you want to safe the extra line to set it to NULL? Fixed. (In reply to comment #17) > Review of attachment 294786 [details] [review]: > > See the comment. > > ::: loaders/python/peas-python-internal.c > @@ -123,3 @@ > - "failed to run internal Python code"); > - > - Py_DECREF (globals); > > you are removing this decref, is this right? if yes please put it in a > different commit Nope, just fallout from trying to split all of the changes. Fixed. (In reply to comment #19) > Review of attachment 294953 [details] [review]: > > This should be split in 2 patches but ok Done.
Created attachment 294960 [details] [review] Add verbose Python plugin loader warnings v4 Use a Python hook to implement the call logic. This allows us to have verbose error messages and include the exception traceback.
Created attachment 294961 [details] [review] Do not reveal Python loader internals in tracebacks This adds format_plugin_exception() which removes all internal Python frames from the traceback.
Review of attachment 294960 [details] [review]: Looks good.
Review of attachment 294961 [details] [review]: OK
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.