[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