[PATCH 0/7] Various Python cleanups.
Patch #7 depends on: https://www.redhat.com/archives/libguestfs/2020-January/msg00035.html No, Python < 3 support is not dropped yet, however it will be easier after this series. Pino Toscano (7): build: enforce a minimum Python version python: drop code for Python < 2.5 python: assume support for Capsules python: remove compile time check for PyString_AsString python: replace guestfs_int_py_fromstringsize in Py2 branches python: stop including config.h python: stop bundling config.h in dist archives .gitignore | 1 - generator/python.ml | 32 ++------------------------------ m4/guestfs-python.m4 | 21 +++++---------------- python/MANIFEST.in | 1 - python/Makefile.am | 5 ----- python/handle.c | 8 +++----- 6 files changed, 10 insertions(+), 58 deletions(-) -- 2.24.1
In case the Python bindings are enabled, enforce a minimum version. Set this minimum version to 2.7, as it has been de-facto for a while now. --- m4/guestfs-python.m4 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/m4/guestfs-python.m4 b/m4/guestfs-python.m4 index befa9b102..1d7849894 100644 --- a/m4/guestfs-python.m4 +++ b/m4/guestfs-python.m4 @@ -19,6 +19,8 @@ dnl Check for Python (optional, for Python bindings). PYTHON_PREFIX= PYTHON_VERSION= PYTHON_INSTALLDIR= +PYTHON_REQ_MAJOR=2 +PYTHON_REQ_MINOR=7 AC_ARG_ENABLE([python], AS_HELP_STRING([--disable-python], [disable Python language bindings]), @@ -32,6 +34,9 @@ AS_IF([test "x$enable_python" != "xno"],[ PYTHON_VERSION_MAJOR=`$PYTHON -c "import sys; print (sys.version_info@<:@0@:>@)"` PYTHON_VERSION_MINOR=`$PYTHON -c "import sys; print (sys.version_info@<:@1@:>@)"` PYTHON_VERSION="$PYTHON_VERSION_MAJOR.$PYTHON_VERSION_MINOR" + AS_IF([test "$PYTHON_VERSION_MAJOR" -lt $PYTHON_REQ_MAJOR || ( test "$PYTHON_VERSION_MAJOR" -eq $PYTHON_REQ_MAJOR && test "$PYTHON_VERSION_MINOR" -lt $PYTHON_REQ_MINOR )],[ + AC_MSG_ERROR([found Python $PYTHON_VERSION, while Python $PYTHON_REQ_MAJOR.$PYTHON_REQ_MINOR is required]) + ]) AC_MSG_RESULT([$PYTHON_VERSION]) # Debian: python-2.7.pc, python-3.2.pc -- 2.24.1
Python >= 2.7 is required. --- generator/python.ml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/generator/python.ml b/generator/python.ml index 821c66755..750d9c675 100644 --- a/generator/python.ml +++ b/generator/python.ml @@ -51,12 +51,6 @@ let rec generate_python_actions_h () = #include <Python.h> #pragma GCC diagnostic pop -#if PY_VERSION_HEX < 0x02050000 -typedef int Py_ssize_t; -#define PY_SSIZE_T_MAX INT_MAX -#define PY_SSIZE_T_MIN INT_MIN -#endif - #ifndef HAVE_PYCAPSULE_NEW typedef struct { PyObject_HEAD -- 2.24.1
They were introduced in Python 2.7 and 3.1, so we can assume they exist (as Python 2.7 is the minimum version required, and 3.0 is long gone). --- generator/python.ml | 16 ---------------- m4/guestfs-python.m4 | 4 ---- 2 files changed, 20 deletions(-) diff --git a/generator/python.ml b/generator/python.ml index 750d9c675..2a1a48fc0 100644 --- a/generator/python.ml +++ b/generator/python.ml @@ -51,35 +51,19 @@ let rec generate_python_actions_h () = #include <Python.h> #pragma GCC diagnostic pop -#ifndef HAVE_PYCAPSULE_NEW -typedef struct { - PyObject_HEAD - guestfs_h *g; -} Pyguestfs_Object; -#endif - static inline guestfs_h * get_handle (PyObject *obj) { assert (obj); assert (obj != Py_None); -#ifndef HAVE_PYCAPSULE_NEW - return ((Pyguestfs_Object *) obj)->g; -#else return (guestfs_h*) PyCapsule_GetPointer(obj, \"guestfs_h\"); -#endif } static inline PyObject * put_handle (guestfs_h *g) { assert (g); -#ifndef HAVE_PYCAPSULE_NEW - return - PyCObject_FromVoidPtrAndDesc ((void *) g, (char *) \"guestfs_h\", NULL); -#else return PyCapsule_New ((void *) g, \"guestfs_h\", NULL); -#endif } extern void guestfs_int_py_extend_module (PyObject *module); diff --git a/m4/guestfs-python.m4 b/m4/guestfs-python.m4 index 1d7849894..35dc19fa6 100644 --- a/m4/guestfs-python.m4 +++ b/m4/guestfs-python.m4 @@ -92,10 +92,6 @@ AS_IF([test "x$enable_python" != "xno"],[ PYTHON_BLDLIBRARY=`$PYTHON -c "import distutils.sysconfig; \ print (distutils.sysconfig.get_config_var('BLDLIBRARY'))"` - AC_CHECK_LIB([c],[PyCapsule_New], - [AC_DEFINE([HAVE_PYCAPSULE_NEW],1, - [Found PyCapsule_New in libpython.])], - [],[$PYTHON_BLDLIBRARY]) AC_CHECK_LIB([c],[PyString_AsString], [AC_DEFINE([HAVE_PYSTRING_ASSTRING],1, [Found PyString_AsString in libpython.])], -- 2.24.1
The PyString API is specific to Python < 3, replaced by PyBytes and PyUnicode in Python 3+; hence, drop the compile time check, and use the right API depending on the Python version. --- m4/guestfs-python.m4 | 12 ------------ python/handle.c | 6 +++--- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/m4/guestfs-python.m4 b/m4/guestfs-python.m4 index 35dc19fa6..ac857915f 100644 --- a/m4/guestfs-python.m4 +++ b/m4/guestfs-python.m4 @@ -86,18 +86,6 @@ AS_IF([test "x$enable_python" != "xno"],[ PYTHON_EXT_SUFFIX=$python_ext_suffix fi AC_MSG_RESULT([$PYTHON_EXT_SUFFIX]) - - dnl Look for some optional symbols in libpython. - old_LIBS="$LIBS" - - PYTHON_BLDLIBRARY=`$PYTHON -c "import distutils.sysconfig; \ - print (distutils.sysconfig.get_config_var('BLDLIBRARY'))"` - AC_CHECK_LIB([c],[PyString_AsString], - [AC_DEFINE([HAVE_PYSTRING_ASSTRING],1, - [Found PyString_AsString in libpython.])], - [],[$PYTHON_BLDLIBRARY]) - - LIBS="$old_LIBS" fi AC_SUBST(PYTHON_PREFIX) diff --git a/python/handle.c b/python/handle.c index ccf6eb643..21077bba9 100644 --- a/python/handle.c +++ b/python/handle.c @@ -382,7 +382,7 @@ guestfs_int_py_put_table (char * const * const argv) PyObject * guestfs_int_py_fromstring (const char *str) { -#ifdef HAVE_PYSTRING_ASSTRING +#if PY_MAJOR_VERSION < 3 return PyString_FromString (str); #else return PyUnicode_FromString (str); @@ -392,7 +392,7 @@ guestfs_int_py_fromstring (const char *str) PyObject * guestfs_int_py_fromstringsize (const char *str, size_t size) { -#ifdef HAVE_PYSTRING_ASSTRING +#if PY_MAJOR_VERSION < 3 return PyString_FromStringAndSize (str, size); #else return PyUnicode_FromStringAndSize (str, size); @@ -402,7 +402,7 @@ guestfs_int_py_fromstringsize (const char *str, size_t size) char * guestfs_int_py_asstring (PyObject *obj) { -#ifdef HAVE_PYSTRING_ASSTRING +#if PY_MAJOR_VERSION < 3 return PyString_AsString (obj); #else PyObject *bytes = PyUnicode_AsUTF8String (obj); -- 2.24.1
In Python 2 code branches use PyString_FromStringAndSize directly instead of the guestfs_int_py_fromstringsize wrapper (which calls PyString_FromStringAndSize anyway on Python < 3). --- generator/python.ml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/generator/python.ml b/generator/python.ml index 2a1a48fc0..0e1ed20d8 100644 --- a/generator/python.ml +++ b/generator/python.ml @@ -178,7 +178,7 @@ and generate_python_structs () = pr " value = PyBytes_FromStringAndSize (%s->%s, %s->%s_len);\n" typ name typ name; pr "#else\n"; - pr " value = guestfs_int_py_fromstringsize (%s->%s, %s->%s_len);\n" + pr " value = PyString_FromStringAndSize (%s->%s, %s->%s_len);\n" typ name typ name; pr "#endif\n"; pr " if (value == NULL)\n"; @@ -496,7 +496,7 @@ and generate_python_actions actions () = pr "#if PY_MAJOR_VERSION >= 3\n"; pr " py_r = PyBytes_FromStringAndSize (r, size);\n"; pr "#else\n"; - pr " py_r = guestfs_int_py_fromstringsize (r, size);\n"; + pr " py_r = PyString_FromStringAndSize (r, size);\n"; pr "#endif\n"; pr " free (r);\n"; pr " if (py_r == NULL) goto out;\n"; -- 2.24.1
All good up to this patch (#5). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
There is nothing in the Python bindings that require results from configure, and gnulib is not used already. --- generator/python.ml | 6 ------ python/handle.c | 2 -- 2 files changed, 8 deletions(-) diff --git a/generator/python.ml b/generator/python.ml index 0e1ed20d8..fd297321e 100644 --- a/generator/python.ml +++ b/generator/python.ml @@ -119,8 +119,6 @@ and generate_python_structs () = generate_header CStyle LGPLv2plus; pr "\ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <assert.h> @@ -251,8 +249,6 @@ and generate_python_actions actions () = generate_header CStyle LGPLv2plus; pr "\ -#include <config.h> - /* It is safe to call deprecated functions from this file. */ #define GUESTFS_NO_WARN_DEPRECATED #undef GUESTFS_NO_DEPRECATED @@ -542,8 +538,6 @@ and generate_python_module () = generate_header CStyle LGPLv2plus; pr "\ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <assert.h> diff --git a/python/handle.c b/python/handle.c index 21077bba9..8200adb52 100644 --- a/python/handle.c +++ b/python/handle.c @@ -22,8 +22,6 @@ * F<python/actions-*.c>). */ -#include <config.h> - #include <stdio.h> #include <stdlib.h> -- 2.24.1
On Thu, Jan 09, 2020 at 06:37:49PM +0100, Pino Toscano wrote:
There is nothing in the Python bindings that require results from configure, and gnulib is not used already. --- generator/python.ml | 6 ------ python/handle.c | 2 -- 2 files changed, 8 deletions(-)
I guess the motivation is because we cannot create this header correctly from PyPi so it'll be wrong there? I still think we might need this header when compiling libguestfs Python bindings in the normal (not PyPi) case, even if it happens to work right now. Could we instead defend these with #ifdef HAVE_CONFIG_H? That way the header shouldn't be used for PyPi and shouldn't need to be bundled (so patch #7 would be OK). Rich.
diff --git a/generator/python.ml b/generator/python.ml index 0e1ed20d8..fd297321e 100644 --- a/generator/python.ml +++ b/generator/python.ml @@ -119,8 +119,6 @@ and generate_python_structs () = generate_header CStyle LGPLv2plus;
pr "\ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <assert.h> @@ -251,8 +249,6 @@ and generate_python_actions actions () = generate_header CStyle LGPLv2plus;
pr "\ -#include <config.h> - /* It is safe to call deprecated functions from this file. */ #define GUESTFS_NO_WARN_DEPRECATED #undef GUESTFS_NO_DEPRECATED @@ -542,8 +538,6 @@ and generate_python_module () = generate_header CStyle LGPLv2plus;
pr "\ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <assert.h> diff --git a/python/handle.c b/python/handle.c index 21077bba9..8200adb52 100644 --- a/python/handle.c +++ b/python/handle.c @@ -22,8 +22,6 @@ * F<python/actions-*.c>). */
-#include <config.h> - #include <stdio.h> #include <stdlib.h>
-- 2.24.1
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Friday, 10 January 2020 23:13:35 CET Richard W.M. Jones wrote:
On Thu, Jan 09, 2020 at 06:37:49PM +0100, Pino Toscano wrote:
There is nothing in the Python bindings that require results from configure, and gnulib is not used already. --- generator/python.ml | 6 ------ python/handle.c | 2 -- 2 files changed, 8 deletions(-)
I guess the motivation is because we cannot create this header correctly from PyPi so it'll be wrong there?
That is the thing: config.h is not created at all when building the Python binding (either as part of libguestfs, or when building the standalone tar.gz dist), but instead the one from libguestfs is used.
I still think we might need this header when compiling libguestfs Python bindings in the normal (not PyPi) case, even if it happens to work right now.
Actually, it happens to _not_ work right now: https://bugzilla.redhat.com/show_bug.cgi?id=1627964 the libguestfs config.h is shipped in the tarball, and the content of that file depends on the way the binding was configured from the libguestfs build that created it. So in case of that bug, the tarball was created with Python 2. And while patches #3, #4, and #5 should fix the majority of the visible cases of this issue, there are still potential issues: config.h defines a series of macros (_GNU_SOURCE, __EXTENSIONS__, etc) that change the way things are built, and strictly depend on the platform.
Could we instead defend these with #ifdef HAVE_CONFIG_H? That way the header shouldn't be used for PyPi and shouldn't need to be bundled (so patch #7 would be OK).
I'll reverse the question: if building the standalone tarball works fine with no config.h, why shouldn't the build-in-libguestfs do the same? The bindings do not use gnulib and use only few system headers and APIs, and nothing of the config.h content really matters (the closest thing is HAVE_PYTHON, which is obviously true...). In the end, it is just a binding that wraps the C API for a different language (Python in this case), so it definitely _ought to_ not depend on the way libguestfs itself was configured/built. Moreover: the same situation applies to the other bindings too (those with C parts, of course). -- Pino Toscano
On Mon, Jan 13, 2020 at 10:35:53AM +0100, Pino Toscano wrote:
On Friday, 10 January 2020 23:13:35 CET Richard W.M. Jones wrote:
On Thu, Jan 09, 2020 at 06:37:49PM +0100, Pino Toscano wrote:
There is nothing in the Python bindings that require results from configure, and gnulib is not used already. --- generator/python.ml | 6 ------ python/handle.c | 2 -- 2 files changed, 8 deletions(-)
I guess the motivation is because we cannot create this header correctly from PyPi so it'll be wrong there?
That is the thing: config.h is not created at all when building the Python binding (either as part of libguestfs, or when building the standalone tar.gz dist), but instead the one from libguestfs is used.
So for clear terminology let's call these situations: - libguestfs build: Python bindings are built normally as part of libguestfs - PyPi build: Python bindings are built from the python-specific tar gz file For the libguestfs build case we create config.h from ./configure. For the PyPi build case we bundle config.h (which is wrong - not arguing with that). Because in the PyPi case there is no real equivalent of ./configure we cannot create config.h properly.
I still think we might need this header when compiling libguestfs Python bindings in the normal (not PyPi) case, even if it happens to work right now.
Actually, it happens to _not_ work right now: https://bugzilla.redhat.com/show_bug.cgi?id=1627964 the libguestfs config.h is shipped in the tarball, and the content of that file depends on the way the binding was configured from the libguestfs build that created it.
Yup, this is wrong, not arguing there.
So in case of that bug, the tarball was created with Python 2. And while patches #3, #4, and #5 should fix the majority of the visible cases of this issue, there are still potential issues: config.h defines a series of macros (_GNU_SOURCE, __EXTENSIONS__, etc) that change the way things are built, and strictly depend on the platform.
Could we instead defend these with #ifdef HAVE_CONFIG_H? That way the header shouldn't be used for PyPi and shouldn't need to be bundled (so patch #7 would be OK).
I'll reverse the question: if building the standalone tarball works fine with no config.h, why shouldn't the build-in-libguestfs do the same?
The Python bindings probably don't need the contents of config.h at the moment, but that isn't to say that config.h might not be necessary on some more obscure platform either now or in the future. In the libguestfs build case using HAVE_CONFIG_H would ensure this keeps working, while also preventing us from needing to bundle the incorrect config.h for the PyPi build case. I think it achieves what you're trying to do (fix the PyPi case) with least risk.
The bindings do not use gnulib and use only few system headers and APIs, and nothing of the config.h content really matters (the closest thing is HAVE_PYTHON, which is obviously true...). In the end, it is just a binding that wraps the C API for a different language (Python in this case), so it definitely _ought to_ not depend on the way libguestfs itself was configured/built.
Moreover: the same situation applies to the other bindings too (those with C parts, of course).
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Tuesday, 14 January 2020 10:56:03 CET Richard W.M. Jones wrote:
On Mon, Jan 13, 2020 at 10:35:53AM +0100, Pino Toscano wrote:
On Friday, 10 January 2020 23:13:35 CET Richard W.M. Jones wrote:
On Thu, Jan 09, 2020 at 06:37:49PM +0100, Pino Toscano wrote:
There is nothing in the Python bindings that require results from configure, and gnulib is not used already. --- generator/python.ml | 6 ------ python/handle.c | 2 -- 2 files changed, 8 deletions(-)
I guess the motivation is because we cannot create this header correctly from PyPi so it'll be wrong there?
That is the thing: config.h is not created at all when building the Python binding (either as part of libguestfs, or when building the standalone tar.gz dist), but instead the one from libguestfs is used.
So for clear terminology let's call these situations:
- libguestfs build: Python bindings are built normally as part of libguestfs
- PyPi build: Python bindings are built from the python-specific tar gz file
For the libguestfs build case we create config.h from ./configure.
For the PyPi build case we bundle config.h (which is wrong - not arguing with that). Because in the PyPi case there is no real equivalent of ./configure we cannot create config.h properly.
I still think we might need this header when compiling libguestfs Python bindings in the normal (not PyPi) case, even if it happens to work right now.
Actually, it happens to _not_ work right now: https://bugzilla.redhat.com/show_bug.cgi?id=1627964 the libguestfs config.h is shipped in the tarball, and the content of that file depends on the way the binding was configured from the libguestfs build that created it.
Yup, this is wrong, not arguing there.
So in case of that bug, the tarball was created with Python 2. And while patches #3, #4, and #5 should fix the majority of the visible cases of this issue, there are still potential issues: config.h defines a series of macros (_GNU_SOURCE, __EXTENSIONS__, etc) that change the way things are built, and strictly depend on the platform.
Could we instead defend these with #ifdef HAVE_CONFIG_H? That way the header shouldn't be used for PyPi and shouldn't need to be bundled (so patch #7 would be OK).
I'll reverse the question: if building the standalone tarball works fine with no config.h, why shouldn't the build-in-libguestfs do the same?
The Python bindings probably don't need the contents of config.h at the moment, but that isn't to say that config.h might not be necessary on some more obscure platform either now or in the future.
Right, although how? The API used is really minimal, and IMHO the risk of getting into this situation is almost nil.
In the libguestfs build case using HAVE_CONFIG_H would ensure this keeps working, while also preventing us from needing to bundle the incorrect config.h for the PyPi build case. I think it achieves what you're trying to do (fix the PyPi case) with least risk.
I thought about this, however it has the drawback that the libguestfs build and PyPi build would behave differently (see the content of config.h), and thus potentially creating issues for the PyPi build, which is tested less. -- Pino Toscano
It is not needed anymore now. --- .gitignore | 1 - python/MANIFEST.in | 1 - python/Makefile.am | 5 ----- 3 files changed, 7 deletions(-) diff --git a/.gitignore b/.gitignore index 1abb1c8b7..9b7cd85d2 100644 --- a/.gitignore +++ b/.gitignore @@ -451,7 +451,6 @@ Makefile.in /python/actions.h /python/bindtests.py /python/build -/python/config.h /python/dist /python/examples/guestfs-python.3 /python/examples/stamp-guestfs-python.pod diff --git a/python/MANIFEST.in b/python/MANIFEST.in index 979f99233..f0081a5b5 100644 --- a/python/MANIFEST.in +++ b/python/MANIFEST.in @@ -16,5 +16,4 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. include actions.h -include config.h include guestfs-stringlists-utils.h diff --git a/python/Makefile.am b/python/Makefile.am index b470d2df2..f0a4b55bd 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -97,14 +97,10 @@ setup-install: setup.py stamp-extra-files # Python's crappy MANIFEST file cannot graft single files, so we have # to hard-link any extra files we need into the local directory. stamp-extra-files: \ - config.h \ guestfs-stringlists-utils.h \ stringlists-utils.c touch $@ -config.h: - ln ../config.h $@ - guestfs-stringlists-utils.h: ln $(top_srcdir)/common/utils/guestfs-stringlists-utils.h $@ @@ -128,7 +124,6 @@ CLEANFILES += \ *.pyc \ examples/*~ examples/*.pyc \ t/*~ t/*.pyc \ - config.h \ guestfs-stringlists-utils.h \ stamp-extra-files \ stringlists-utils.c -- 2.24.1
participants (2)
-
Pino Toscano -
Richard W.M. Jones