Re: [PATCH] Remove python binding

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/22/2013 09:59 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> 
> The python binding now lives in
> 
>   http://libvirt.org/git/libvirt-python.git
> 
> that repo also provides an RPM which is upgrade compatible
> with the old libvirt-python sub-RPM.
> 
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
>  .gitignore                                         |   11 -
>  Makefile.am                                        |    7 +-
>  autobuild.sh                                       |    6 +-
>  cfg.mk                                             |    7 +-
>  configure.ac                                       |   86 -

These changes are important...

>  libvirt.spec.in                                    |   36 +-
>  m4/virt-compile-warnings.m4                        |    8 -
>  mingw-libvirt.spec.in                              |    1 -

and these...

>  run.in                                             |    9 -

and this (hmm, do we need a ./run counterpart added to libvirt-python to
make it easy to import a built but uninstalled python bindings for
testing purposes?)

>  37 files changed, 7 insertions(+), 13369 deletions(-)

The rest of the patch is mechanical 'git rm'; it might have been nice to
use git's -O/path/to/file to order the patch to float the interesting
parts up top and trim the rest of the email.

grepping for python still had hits after your patch; here's my analysis:

ChangeLog-old - good to keep; we aren't changing details of historical
releases
bootstrap.conf - unsure: Does bootstrap.conf still need to worry about
finding python-config, or is that a thing of the past?
cfg.mk - keep; we still have .py files for docs
docs/Makefile.am
docs/apibuild.py
docs/apps.html.in
docs/bindings.html.in
docs/errors.html.in
docs/hooks.html.in
docs/index.py
docs/library.xen
docs/news.html.in
docs/python.html.in
docs/sitemap.html.in
docs/testsuites.html.in
docs/windows.html.in - good to keep all docs stuff for doc generation
libvirt.spec.in - good to keep; only mentions the historical %changelog
src/esx/esx_vi_generator.py
src/hyperv/hyperv_wmi_generator.py
src/util/virconf.c
src/util/virkeycode-mapgen.py - good to keep all the C code generators


We also need to scrub the code base for references to 1.1.5 and instead
refer to 1.2.0.

> diff --git a/.gitignore b/.gitignore
> index 6b024e7..faabd33 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -91,17 +91,6 @@
>  /mkinstalldirs
>  /po/*
>  /proxy/
> -/python/generated.stamp
> -/python/generator.py.stamp
> -/python/libvirt-export.c
> -/python/libvirt-lxc-export.c
> -/python/libvirt-lxc.[ch]
> -/python/libvirt-qemu-export.c
> -/python/libvirt-qemu.[ch]
> -/python/libvirt.[ch]
> -/python/libvirt.py
> -/python/libvirt_lxc.py
> -/python/libvirt_qemu.py

Oddly enough, removing these lines will mean that someone with an
incremental tree that likes to switch branches between master and any
earlier branch (say v1.0.5-maint) will now see git complaining about
untracked files.  I personally tend to avoid removing lines from
.gitignore (if we've ever ignored a file in the past, then branch
switching could leave the file around to still be ignored); but I won't
complain too much if you make the deletion (I can always re-add the
lines in my .git/info/exclude for my own environment).

> +++ b/Makefile.am

> @@ -74,8 +73,6 @@ check-local: all tests
>  
>  tests:
>  	@(cd docs/examples ; $(MAKE) MAKEFLAGS+=--silent tests)
> -	@(if [ "$(pythondir)" != "" ] ; then cd python ; \
> -	  $(MAKE) MAKEFLAGS+=--silent tests ; fi)

Did configure.ac clean up the setting of $(pythondir)?
/me looks some more
Wow - we never set $(pythondir) anywhere pre-patch, so it only ever did
something if you invoked 'make pythondir=...', and since autobuild
wasn't doing it, I think this was dead code.

> +++ b/autobuild.sh
> @@ -86,8 +86,7 @@ if test -x /usr/bin/i686-w64-mingw32-gcc ; then
>      --prefix="$AUTOBUILD_INSTALL_ROOT/i686-w64-mingw32/sys-root/mingw" \
>      --enable-expensive-tests \
>      --enable-werror \
> -    --without-libvirtd \
> -    --without-python
> +    --without-libvirtd

Hmm.  Was --without-python disabling _just_ the python bindings, or was
it _also_ disabling our attempts to use $(PYTHON) for building things in
our docs/ subtree?  We may STILL want to keep this setting for
controlling whether to attempt to build generated docs (vs. using the
pre-generated version shipped in the tarball).

> +++ b/configure.ac
> @@ -2012,83 +2012,6 @@ fi
>  AM_CONDITIONAL([WITH_HYPERV], [test "$with_hyperv" = "yes"])
>  
>  

> -        AC_MSG_NOTICE(Found python in $with_python/bin/python)
> -        PYTHON="$with_python/bin/python"
> -        with_python=yes
> -    else
> -        if test -x "$with_python"
> -        then
> -            AC_MSG_NOTICE(Found python in $with_python)
> -            PYTHON="$with_python"
> -            with_python=yes
> -        else
> -            if test -x "$PYTHON"
> -            then
> -                AC_MSG_NOTICE(Found python in environment PYTHON=$PYTHON)
> -                with_python=yes
> -            fi
> -        fi
> -    fi
> -
> -    if test "$with_python" = "yes" ; then
> -        AM_PATH_PYTHON(,, [:])

Ouch.  Deleting this may break uses of $(PYTHON) when building our docs.

> -
> -        if test "$PYTHON" != : ; then
> -            PYTHON_CONFIG="$PYTHON-config"

Meanwhile, this part is probably safe to drop, which goes back to my
question on getting rid of the check for python-config in bootstrap.conf.

> -            fi
> -        else
> -            AC_MSG_ERROR([You must install python to build Python bindings])
> -        fi
> -    else
> -        AC_MSG_NOTICE([Could not find python in $with_python, disabling bindings])
> -        with_python=no
> -    fi
> -fi

So it looks like pre-patch, --without-python attempted to disable only
the bindings, but may have also accidentally disabled the setting of
$(PYTHON)...

> -AM_CONDITIONAL([WITH_PYTHON], [test "$with_python" = "yes"])
> -AC_SUBST([PYTHON_VERSION])
> -AC_SUBST([PYTHON_INCLUDES])
> -
>  dnl Allow perl overrides
>  AC_PATH_PROG([PERL], [perl])

...maybe all we need is a simple AC_PATH_PROG([PYTHON], [python]) for
the sake of still using $(PYTHON) during doc generation?  And some of
this goes back to whether deleting --without-python from autobuild makes
sense.

> --- a/examples/domain-events/events-python/event-test.py

Giant snip :)

> +++ b/libvirt.spec.in
> @@ -126,7 +126,6 @@
>  %define with_libssh2       0%{!?_without_libssh2:0}
>  
>  # Non-server/HV driver defaults which are always enabled
> -%define with_python        0%{!?_without_python:1}
>  %define with_sasl          0%{!?_without_sasl:1}
>  
>  
> @@ -425,7 +424,6 @@ BuildRequires: gettext-devel
>  BuildRequires: libtool
>  BuildRequires: /usr/bin/pod2man
>  %endif
> -BuildRequires: python-devel

Ouch.  Don't we still need a buildreq on 'python' for doc generation
purposes, possibly conditionally based on whether we have patches
applied to a downstream rpm that warrant doc regeneration?

> -%if %{with_python}
> -%package python
> -Summary: Python bindings for the libvirt library
> -Group: Development/Libraries
> -Requires: %{name}-client = %{version}-%{release}

All of this transferred over to the libvirt-python.git, so good to see
it go here.

> -%if ! %{with_python}
> -    %define _without_python --without-python
> -%endif
> -
>  %if ! %{with_libvirtd}
>      %define _without_libvirtd --without-libvirtd
>  %endif
> @@ -1378,7 +1359,6 @@ of recent versions of Linux (and other OSes).
>             %{?_without_sasl} \
>             %{?_without_avahi} \
>             %{?_without_polkit} \
> -           %{?_without_python} \

Same question as in autobuild on whether --without-python still matters
for suppressing doc regeneration, or whether it was intended only for
suppressing bindings.

> --- a/python/Makefile.am
> +++ /dev/null

Another giant snip :)

> +++ b/run.in
> @@ -58,15 +58,6 @@ export LIBVIRT_LOCK_MANAGER_PLUGIN_DIR="$b/src/.libs"
>  export VIRTLOCKD_PATH="$b/src/virtlockd"
>  export LIBVIRTD_PATH="$b/daemon/libvirtd"
>  
> -# For Python.
> -export PYTHON=@PYTHON@
> -if [ -z "$PYTHONPATH" ]; then
> -    PYTHONPATH="$b/python:$b/python/.libs"
> -else
> -    PYTHONPATH="$b/python:$b/python/.libs:$PYTHONPATH"
> -fi
> -export PYTHONPATH

Fine.

If you can answer the comments I raised above, and if everyone is okay
with your libvirt-python.git patches, then I'm okay with this patch.
Let's get it polished and in before freeze!

I also just kicked off a 'make distcheck' and will report back later if
I saw anything fishy that needs fixing.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]