[PATCH] Move Anaconda version detection from isys to Python code
Anne Mulhern
amulhern at redhat.com
Tue May 20 12:45:01 UTC 2014
----- Original Message -----
> From: "Martin Kolman" <mkolman at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Tuesday, May 20, 2014 8:00:24 AM
> Subject: Re: [PATCH] Move Anaconda version detection from isys to Python code
>
> On Mon, 2014-05-19 at 14:35 -0400, Anne Mulhern wrote:
> >
> >
> >
> > ----- Original Message -----
> > > From: "Martin Kolman" <mkolman at redhat.com>
> > > To: anaconda-patches at lists.fedorahosted.org
> > > Sent: Monday, May 19, 2014 12:56:52 PM
> > > Subject: [PATCH] Move Anaconda version detection from isys to Python code
> > >
> > > There is now pyanaconda/version.py.in module with a __version__
> > > variable, which automatically gets the Anaconda version from
> > > automake during build.
> > >
> > > As a side effect, the absence of compiled isys no longer crashes
> > > "anaconda --help". Also "anaconda --version" on freshly
> > > cloned/unpackaged Anaconda now correctly returns "unknown version".
> > >
> > > Signed-off-by: Martin Kolman <mkolman at redhat.com>
> > > ---
> > > anaconda | 12 ++++++++----
> > > configure.ac | 1 +
> > > pyanaconda/exception.py | 10 ++++++++--
> > > pyanaconda/isys/Makefile.am | 4 +---
> > > pyanaconda/isys/__init__.py | 3 ---
> > > pyanaconda/isys/isys.c | 6 ------
> > > pyanaconda/version.py.in | 1 +
> > > 7 files changed, 19 insertions(+), 18 deletions(-)
> > > create mode 100644 pyanaconda/version.py.in
> > >
> > > diff --git a/anaconda b/anaconda
> > > index 2cb014e..99ed511 100755
> > > --- a/anaconda
> > > +++ b/anaconda
> > > @@ -252,10 +252,14 @@ def symlink_updates(dest_dir, update_dir):
> > > os.symlink(dest_path, update_path)
> > >
> > > def getAnacondaVersion():
> > > - # Using _isys here so we don't drag in the logging stuff, which is
> > > always
> > > - # complicated.
> > > - from pyanaconda import _isys
> > > - return _isys.getAnacondaVersion()
> > > + # we are importing the version module directly so that we don't drag
> > > in
> > > any
> > > + # non-necessary stuff; we also need to handle the possibility of the
> > > + # import itself failing
> > > + try:
> > > + from pyanaconda import version
> > > + return version.__version__
> > > + except ImportError:
> > > + return "unknown version"
> > >
> > > def parseArguments(argv=None, boot_cmdline=None):
> > > from pyanaconda.anaconda_argparse import AnacondaArgumentParser
> > > diff --git a/configure.ac b/configure.ac
> > > index b262fe4..0d0a264 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -204,6 +204,7 @@ AC_CONFIG_FILES([Makefile
> > > po/Makefile.in
> > > scripts/Makefile
> > > pyanaconda/Makefile
> > > + pyanaconda/version.py
> > > pyanaconda/isys/Makefile
> > > pyanaconda/packaging/Makefile
> > > pyanaconda/ui/Makefile
> > > diff --git a/pyanaconda/exception.py b/pyanaconda/exception.py
> > > index 7ce9026..44c131b 100644
> > > --- a/pyanaconda/exception.py
> > > +++ b/pyanaconda/exception.py
> > > @@ -24,7 +24,7 @@
> > > from meh import Config
> > > from meh.handler import ExceptionHandler
> > > from meh.dump import ReverseExceptionDump
> > > -from pyanaconda import isys, iutil, kickstart
> > > +from pyanaconda import iutil, kickstart
> > > import sys
> > > import os
> > > import shutil
> > > @@ -227,8 +227,14 @@ def initExceptionHandling(anaconda):
> > > if anaconda.opts and anaconda.opts.ksfile:
> > > fileList.extend([anaconda.opts.ksfile])
> > >
> > > + try:
> > > + from pyanaconda import version
> > > + anaconda_version = version.__version__
> > > + except ImportError:
> > > + anaconda_version = "unknown version"
> > > +
> > > conf = Config(programName="anaconda",
> > > - programVersion=isys.getAnacondaVersion(),
> > > + programVersion=anaconda_version,
> > > programArch=os.uname()[4],
> > > attrSkipList=["_intf._actions",
> > > "_intf._currentAction._xklwrapper",
> > > diff --git a/pyanaconda/isys/Makefile.am b/pyanaconda/isys/Makefile.am
> > > index bd3e36f..c787f60 100644
> > > --- a/pyanaconda/isys/Makefile.am
> > > +++ b/pyanaconda/isys/Makefile.am
> > > @@ -21,13 +21,11 @@ pkgpyexecdir = $(pyexecdir)/py$(PACKAGE_NAME)
> > >
> > > dist_noinst_HEADERS = $(srcdir)/*.h
> > >
> > > -ISYS_CFLAGS =
> > > -DVERSION_RELEASE='"$(PACKAGE_VERSION)-$(PACKAGE_RELEASE)"'
> > > -
> > > isysdir = $(pkgpyexecdir)/isys
> > > isys_PYTHON = $(srcdir)/*.py
> > >
> > > pkgpyexec_LTLIBRARIES = _isys.la
> > > -_isys_la_CFLAGS = $(PYTHON_CFLAGS) $(ISYS_CFLAGS)
> > > +_isys_la_CFLAGS = $(PYTHON_CFLAGS)
> > > _isys_la_LDFLAGS = -module -avoid-version
> > > _isys_la_LIBADD = $(PYTHON_LIBS)
> > > _isys_la_SOURCES = isys.c
> > > diff --git a/pyanaconda/isys/__init__.py b/pyanaconda/isys/__init__.py
> > > index c985b3e..aa8c978 100644
> > > --- a/pyanaconda/isys/__init__.py
> > > +++ b/pyanaconda/isys/__init__.py
> > > @@ -106,9 +106,6 @@ def isLpaeAvailable():
> > >
> > > return False
> > >
> > > -def getAnacondaVersion():
> > > - return _isys.getAnacondaVersion()
> > > -
> > > def set_system_time(secs):
> > > """
> > > Set system time to time given as a number of seconds since the
> > > Epoch.
> > > diff --git a/pyanaconda/isys/isys.c b/pyanaconda/isys/isys.c
> > > index c9a7ff5..23fb77f 100644
> > > --- a/pyanaconda/isys/isys.c
> > > +++ b/pyanaconda/isys/isys.c
> > > @@ -31,13 +31,11 @@
> > >
> > > static PyObject * doSync(PyObject * s, PyObject * args);
> > > static PyObject * doSegvHandler(PyObject *s, PyObject *args);
> > > -static PyObject * doGetAnacondaVersion(PyObject * s, PyObject * args);
> > > static PyObject * doSetSystemTime(PyObject *s, PyObject *args);
> > >
> > > static PyMethodDef isysModuleMethods[] = {
> > > { "sync", (PyCFunction) doSync, METH_VARARGS, NULL},
> > > { "handleSegv", (PyCFunction) doSegvHandler, METH_VARARGS, NULL },
> > > - { "getAnacondaVersion", (PyCFunction) doGetAnacondaVersion,
> > > METH_VARARGS, NULL },
> > > { "set_system_time", (PyCFunction) doSetSystemTime, METH_VARARGS,
> > > NULL},
> > > { NULL, NULL, 0, NULL }
> > > } ;
> > > @@ -76,10 +74,6 @@ static PyObject * doSegvHandler(PyObject *s, PyObject
> > > *args) {
> > > exit(1);
> > > }
> > >
> > > -static PyObject * doGetAnacondaVersion(PyObject * s, PyObject * args) {
> > > - return Py_BuildValue("s", VERSION_RELEASE);
> > > -}
> > > -
> > > static PyObject * doSetSystemTime(PyObject *s, PyObject *args) {
> > > struct timeval tv;
> > > tv.tv_usec = 0;
> > > diff --git a/pyanaconda/version.py.in b/pyanaconda/version.py.in
> > > new file mode 100644
> > > index 0000000..8b5161a
> > > --- /dev/null
> > > +++ b/pyanaconda/version.py.in
> > > @@ -0,0 +1 @@
> > > +__version__ = "@PACKAGE_VERSION at -@PACKAGE_RELEASE@"
> > > --
> > > 1.9.0
> > >
> > > _______________________________________________
> > > anaconda-patches mailing list
> > > anaconda-patches at lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > >
> >
> > It might be
> > better to check for the existance of the version.py file, rather than
> > whether
> > or not an ImportError was raised on importing it, in determining the
> > existance
> > of the version since ImportErrors can be raised for lots of reasons.
> Yeah, that sounds cleaner, I'll use it instead of catching the import
> error. Also with this in place, any import error would be an indication
> our build process is broken as the version.py file has been incorrectly
> generated.
>
> > Also, I
> > prefer None to 'unknown version'.
> >
> > - mulhern
> >
> >
> > _______________________________________________
> > anaconda-patches mailing list
> > anaconda-patches at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
>
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
I've got yet another remark.
We know that you put getAnacondaVersion() in anaconda itself to minimize imports
at that stage. You put the duplicated chunk of code in pyanaconda/exceptions.py
because you can't call the anaconda.getAnacondaVersion() method there.
I think it would be better to put an identical method or perhaps a property in
pyanaconda.anaconda.Anaconda, and use that method in pyanaconda/exceptions.py.
It seems appropriate for the Anaconda class to be able to figure out the anaconda
version somehow.
If you do that, then each method should have a comment that emphasizes the
reasons for the duplication.
- mulhern
More information about the anaconda-patches
mailing list