Fix broken --version option (gwastro#4630)

* Fix broken --version option, add CI test, cleanup and simplify related code

* Fix bug preventing .so files from being found

* Fix missing import, typo, more codeclimate

* pycbc_coinc_time needs dqsegdb; deduplicate --help commands

pycbc/_version.py

@@ -13,70 +13,99 @@
 # You should have received a copy of the GNU 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.
+
 """
 This modules contains a function to provide an argparse action that reports
 extremely verbose version information for PyCBC, lal, and lalsimulation.
 """
 
-import os, sys
+import os
+import sys
+import glob
 import argparse
 import inspect
 import subprocess
 
+
 def print_link(library):
-    err_msg = "Could not execute runtime linker to determine\n" + \
-              "shared library paths for library:\n  " + library + "\n"
-    FNULL = open(os.devnull, 'w')
+    err_msg = (
+        "Could not execute runtime linker to determine\n"
+        f"shared library paths for library:\n  {library}\n"
+    )
     try:
-        link = subprocess.check_output(['ldd', library],
-                                       stderr=FNULL)
+        # Linux
+        link = subprocess.check_output(
+            ['ldd', library],
+            stderr=subprocess.DEVNULL,
+            text=True
+        )
     except OSError:
         try:
-            link = subprocess.check_output(['otool', '-L', library],
-                                           stderr=FNULL)
+            # macOS
+            link = subprocess.check_output(
+                ['otool', '-L', library],
+                stderr=subprocess.DEVNULL,
+                text=True
+            )
         except:
             link = err_msg
     except:
         link = err_msg
     return link
 
 
+def get_lal_info(module, lib_glob):
+    """Return a string reporting the version and runtime library information
+    for a LAL Python import.
+    """
+    module_path = inspect.getfile(module)
+    version_str = (
+        module.git_version.verbose_msg +
+        "\n\nImported from: " + module_path +
+        "\n\nRuntime libraries:\n"
+    )
+    possible_lib_paths = glob.glob(
+        os.path.join(os.path.dirname(module_path), lib_glob)
+    )
+    for lib_path in possible_lib_paths:
+        version_str += print_link(lib_path)
+    return version_str
+
+
 class Version(argparse.Action):
-    """ print the pycbc, lal and lalsimulation versions """
+    """Subclass of argparse.Action that prints version information for PyCBC,
+    LAL and LALSimulation.
+    """
     def __init__(self, nargs=0, **kw):
         super(Version, self).__init__(nargs=nargs, **kw)
 
-
     def __call__(self, parser, namespace, values, option_string=None):
-
         import pycbc
-        version_str="--- PyCBC Version --------------------------\n" + \
-            pycbc.version.git_verbose_msg + \
+
+        version_str = (
+            "--- PyCBC Version --------------------------\n" +
+            pycbc.version.git_verbose_msg +
             "\n\nImported from: " + inspect.getfile(pycbc)
+        )
 
         version_str += "\n\n--- LAL Version ----------------------------\n"
         try:
             import lal.git_version
-            lal_module = inspect.getfile(lal)
-            lal_library = os.path.join( os.path.dirname(lal_module),
-                '_lal.so')
-            version_str += lal.git_version.verbose_msg + \
-            "\n\nImported from: " + lal_module + \
-            "\n\nRuntime libraries:\n" + print_link(lal_library)
         except ImportError:
             version_str += "\nLAL not installed in environment\n"
+        else:
+            version_str += get_lal_info(lal, '_lal*.so')
 
         version_str += "\n\n--- LALSimulation Version-------------------\n"
         try:
             import lalsimulation.git_version
-            lalsimulation_module = inspect.getfile(lalsimulation)
-            lalsimulation_library = os.path.join( os.path.dirname(lalsimulation_module),
-                '_lalsimulation.so')
-            version_str += lalsimulation.git_version.verbose_msg + \
-            "\n\nImported from: " + lalsimulation_module + \
-            "\n\nRuntime libraries:\n" + print_link(lalsimulation_library)
         except ImportError:
             version_str += "\nLALSimulation not installed in environment\n"
+        else:
+            version_str += get_lal_info(lalsimulation, '_lalsimulation*.so')
 
         print(version_str)
         sys.exit(0)
+
+
+__all__ = ['Version']

pycbc/_version_helper.py

@@ -64,30 +64,29 @@ def call(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
     if p.returncode != 0 and on_error == 'raise':
         raise GitInvocationError('Failed to run "%s"' % " ".join(command))
 
-    out = out.decode('utf-8')
+    out = out.decode('utf-8').strip()
 
     if returncode:
-        return out.strip(), p.returncode
-    else:
-        return out.strip()
+        return out, p.returncode
+    return out
 
 
 def get_build_name(git_path='git'):
     """Find the username of the current builder
     """
-    name,retcode = call(('git', 'config', 'user.name'), returncode=True)
+    name, retcode = call(('git', 'config', 'user.name'), returncode=True)
     if retcode:
         name = "Unknown User"
-    email,retcode = call(('git', 'config', 'user.email'), returncode=True)
+    email, retcode = call(('git', 'config', 'user.email'), returncode=True)
     if retcode:
         email = ""
-    return "%s <%s>" % (name, email)
+    return f"{name} <{email}>"
 
 
 def get_build_date():
     """Returns the current datetime as the git build date
     """
-    return time.strftime('%Y-%m-%d %H:%M:%S +0000', time.gmtime())
+    return time.strftime(r'%Y-%m-%d %H:%M:%S +0000', time.gmtime())
 
 
 def get_last_commit(git_path='git'):
@@ -96,12 +95,15 @@ def get_last_commit(git_path='git'):
     Returns a tuple (hash, date, author name, author e-mail,
     committer name, committer e-mail).
     """
-    hash_, udate, aname, amail, cname, cmail = (
-        call((git_path, 'log', '-1',
-              '--pretty=format:%H,%ct,%an,%ae,%cn,%ce')).split(","))
-    date = time.strftime('%Y-%m-%d %H:%M:%S +0000', time.gmtime(float(udate)))
-    author = '%s <%s>' % (aname, amail)
-    committer = '%s <%s>' % (cname, cmail)
+    hash_, udate, aname, amail, cname, cmail = call((
+        git_path,
+        'log',
+        '-1',
+        r'--pretty=format:%H,%ct,%an,%ae,%cn,%ce'
+    )).split(",")
+    date = time.strftime(r'%Y-%m-%d %H:%M:%S +0000', time.gmtime(float(udate)))
+    author = f'{aname} <{amail}>'
+    committer = f'{cname} <{cmail}>'
     return hash_, date, author, committer
 
 
@@ -111,8 +113,7 @@ def get_git_branch(git_path='git'):
     branch_match = call((git_path, 'rev-parse', '--symbolic-full-name', 'HEAD'))
     if branch_match == "HEAD":
         return None
-    else:
-        return os.path.basename(branch_match)
+    return os.path.basename(branch_match)
 
 
 def get_git_tag(hash_, git_path='git'):
@@ -122,26 +123,26 @@ def get_git_tag(hash_, git_path='git'):
                         '--tags', hash_), returncode=True)
     if status == 0:
         return tag
-    else:
-        return None
+    return None
+
 
 def get_num_commits():
     return call(('git', 'rev-list', '--count', 'HEAD'))
 
+
 def get_git_status(git_path='git'):
     """Returns the state of the git working copy
     """
     status_output = subprocess.call((git_path, 'diff-files', '--quiet'))
     if status_output != 0:
         return 'UNCLEAN: Modified working tree'
-    else:
-        # check index for changes
-        status_output = subprocess.call((git_path, 'diff-index', '--cached',
-                                         '--quiet', 'HEAD'))
-        if status_output != 0:
-            return 'UNCLEAN: Modified index'
-        else:
-            return 'CLEAN: All modifications committed'
+    # check index for changes
+    status_output = subprocess.call((git_path, 'diff-index', '--cached',
+                                     '--quiet', 'HEAD'))
+    if status_output != 0:
+        return 'UNCLEAN: Modified index'
+    return 'CLEAN: All modifications committed'
+
 
 def determine_latest_release_version():
     """Query the git repository for the last released version of the code.
@@ -152,22 +153,21 @@ def determine_latest_release_version():
     tag_list = call((git_path, 'tag')).split('\n')
 
     # Reduce to only versions
-    tag_list = [t[1:] for t in tag_list if t.startswith('v')]
+    re_magic = re.compile(r"v\d+\.\d+\.\d+$")
+    tag_list = [t[1:] for t in tag_list if re_magic.match(t)]
 
-    # Determine if indeed a tag and store largest
+    # find latest version
     latest_version = None
     latest_version_string = None
-    re_magic = re.compile("\d+\.\d+\.\d+$")
     for tag in tag_list:
-        # Is this a version string
-        if re_magic.match(tag):
-            curr_version = distutils.version.StrictVersion(tag)
-            if latest_version is None or curr_version > latest_version:
-                latest_version = curr_version
-                latest_version_string = tag
+        curr_version = distutils.version.StrictVersion(tag)
+        if latest_version is None or curr_version > latest_version:
+            latest_version = curr_version
+            latest_version_string = tag
 
     return latest_version_string
 
+
 def generate_git_version_info():
     """Query the git repository information to generate a version module.
     """

tools/pycbc_test_suite.sh

@@ -31,7 +31,7 @@ fi
 if [ "$PYCBC_TEST_TYPE" = "help" ] || [ -z ${PYCBC_TEST_TYPE+x} ]; then
     # check that all executables that do not require
     # special environments can return a help message
-    for prog in `find ${PATH//:/ } -maxdepth 1 -name 'pycbc*' -print 2>/dev/null | egrep -v '(pycbc_live_nagios_monitor|pycbc_make_offline_grb_workflow|pycbc_mvsc_get_features|pycbc_upload_xml_to_gracedb|pycbc_coinc_time)'`
+    for prog in `find ${PATH//:/ } -maxdepth 1 -name 'pycbc*' -print 2>/dev/null | egrep -v '(pycbc_live_nagios_monitor|pycbc_make_offline_grb_workflow|pycbc_mvsc_get_features|pycbc_upload_xml_to_gracedb|pycbc_coinc_time)' | sort | uniq`
     do
         echo -e ">> [`date`] running $prog --help"
         $prog --help &> $LOG_FILE
@@ -45,6 +45,18 @@ if [ "$PYCBC_TEST_TYPE" = "help" ] || [ -z ${PYCBC_TEST_TYPE+x} ]; then
             echo -e "    Pass."
         fi
     done
+    # also check that --version works for one executable
+    echo -e ">> [`date`] running pycbc_inspiral --version"
+    pycbc_inspiral --version &> $LOG_FILE
+    if test $? -ne 0 ; then
+        RESULT=1
+        echo -e "    FAILED!"
+        echo -e "---------------------------------------------------------"
+        cat $LOG_FILE
+        echo -e "---------------------------------------------------------"
+    else
+        echo -e "    Pass."
+    fi
 fi
 
 if [ "$PYCBC_TEST_TYPE" = "search" ] || [ -z ${PYCBC_TEST_TYPE+x} ]; then