* Re: [PATCH] Bug 22722 - Make fedabidiff and its tests support both python 3 and 2
2018-01-01 0:00 ` Chenxiong Qi
@ 2018-01-01 0:00 ` Dodji Seketeli
0 siblings, 0 replies; 4+ messages in thread
From: Dodji Seketeli @ 2018-01-01 0:00 UTC (permalink / raw)
To: Chenxiong Qi; +Cc: libabigail
[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]
Hello Chenxiong,
Chenxiong Qi <qcxhome@gmail.com> a écrit:
> Hi Dodji,
>
> My patch is updated. Please take another look. This time it also
> contains a few replacement from tab to spaces which are detected
> during running in Python 3.
Thanks!
So I reviewed it, found it great and so I committed it!
I just made some changes in the autotool-related parts to make it so
that the logic is to "enable fedabpkgdiff to run on python3". Not just
the tests. So the option is now --enable-python3. When that option is
fired, if python3 is found, then we enable the tests to run on python3
too.
I also made a change to test for the presence of required python modules
using either python2 or python3, depending on the version of python that
were selected. Because without my change, only python2 was being used
to check for the presence of the python modules required by
fedabipkgdiff. Doing that I realized, for instance, that the package
urlparse was renamed in python3 as urllib.parse. So I handled that too.
On a more cosmetic note, I renamed some automake conditionnal variables
to make them look like the other ones we already have in place.
I also updated the summary that is displayed at the end of the configure
script to make it mention the path to the python binary we are using and
if we are using python3 or not.
So below is the patch I committed.
Thank you so much for the time, effort and dedication you are showing.
It really is appreciated.
Cheers,
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The committed patch --]
[-- Type: text/x-patch, Size: 22714 bytes --]
From 974d6b8745c425e30ca6aafa92338ccfacfededb Mon Sep 17 00:00:00 2001
From: Chenxiong Qi <cqi@redhat.com>
Date: Sun, 25 Mar 2018 15:34:59 +0800
Subject: [PATCH] Bug 22722 - Make fedabipkgdiff and its tests support both
python 3 and 2
This patch makes fedabipkgdiff Python 3 compatible. All tests written
in Python are updated and compatible with Python 3 as well.
The patch looks for a Python 3 interperter. If it finds one then it
runs the tests using that interpreter. Otherwise it just tries to use
the Python 2 interpreter.
This behaviour can be disabled by the new --disable-python3 option.
* configure.ac: Add new option --enable-python3. Add new
test runner file tests/runtestdefaultsupprs-py3 and
tests/runtestfedabipkgdiffpy3.py. Add required six Python module.
* tests/Makefile.am: Add new test files
tests/runtestdefaultsupprspy3.py and
tests/runtestfedabipkgdiffpy3.py accordingly.
* tests/mockfedabipkgdiff.in: Convert print statement to
six.print_. Replace call to function filter with list
comprehension. Replace basestring with six.string_types.
* tests/runtestdefaultsupprspy3.py.in: New shell script to run
test runtestdefaultsupprs with Python 3.
* tests/runtestdefaultsupprs.py.in: Repalce a few tabs with
proper number of spaces which is detected by Python 3
interpreter.
* tests/runtestfedabipkgdiffpy3.py.in: New shell script to run
test runtestfedabipkgdiff with Python 3.
* tests/runtestfedabipkgdiff.py.in: Use python from env in
shebang instead of a fixed path to a Python interpreter.
* tools/fedabipkgdiff: Globally replace print statement with a
function call to print which is available by importing
print_function from __future__ module. Use six.print_ to output
string to stderr instead. Convert function call to map to
for-loop. (cmp_nvr): Change argument to handle a Koji build
mapping instead of only the nvr. (Brew.listBuilds): use the new
cmp_nvr to sort builds.
Signed-off-by: Chenxiong Qi <cqi@redhat.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
.gitignore | 3 +-
configure.ac | 84 ++++++++++++++++++++++++++---
tests/Makefile.am | 21 ++++++++
tests/mockfedabipkgdiff.in | 21 ++++----
tests/runtestdefaultsupprs.py.in | 24 ++++-----
tests/runtestdefaultsupprspy3.py.in | 2 +
tests/runtestfedabipkgdiff.py.in | 2 +-
tests/runtestfedabipkgdiffpy3.py.in | 22 ++++++++
tools/fedabipkgdiff | 63 +++++++++++++---------
9 files changed, 184 insertions(+), 58 deletions(-)
create mode 100644 tests/runtestdefaultsupprspy3.py.in
create mode 100644 tests/runtestfedabipkgdiffpy3.py.in
diff --git a/.gitignore b/.gitignore
index a60cadb..ed4f43c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -22,4 +22,5 @@ Makefile.in
.tags
build/
TAGS
-fedabipkgdiffc
\ No newline at end of file
+fedabipkgdiffc
+tools/__pycache__/
\ No newline at end of file
diff --git a/configure.ac b/configure.ac
index 4738faa..df495e4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -97,6 +97,12 @@ AC_ARG_ENABLE([fedabipkgdiff],
ENABLE_FEDABIPKGDIFF=$enableval,
ENABLE_FEDABIPKGDIFF=auto)
+AC_ARG_ENABLE([python3],
+ AS_HELP_STRING([--enable-python3=yes|no|auto],
+ [enable running abigail tools with python3 (default is auto)]),
+ ENABLE_PYTHON3=$enableval,
+ ENABLE_PYTHON3=auto)
+
dnl *************************************************
dnl check for dependencies
dnl *************************************************
@@ -333,29 +339,84 @@ if test x$CHECK_DEPS_FOR_FEDABIPKGDIFF = xyes; then
fi
fi
- # The minimal python version we want to support is 2.6.6 because EL6
+ # The minimal python 2 version we want to support is 2.6.6 because EL6
# distributions have that version installed.
- MINIMAL_PYTHON_VERSION="2.6.6"
+ MINIMAL_PYTHON2_VERSION="2.6.6"
AC_PATH_PROG(PYTHON, python, no)
- AX_PROG_PYTHON_VERSION($MINIMAL_PYTHON_VERSION,
+ AX_PROG_PYTHON_VERSION($MINIMAL_PYTHON2_VERSION,
[MINIMAL_PYTHON_VERSION_FOUND=yes],
[MINIMAL_PYTHON_VERSION_FOUND=no])
+ # The minimal python 3 version we want to support is 3.5, which is
+ # available in Fedora releases and in EL7.
+ if test x$ENABLE_PYTHON3 != xno; then
+ AC_CHECK_PROGS(PYTHON3_INTERPRETER, [python3 python3.5 python3.6 python3.7], no)
+ else
+ PYTHON3_INTERPRETER=no
+ fi
+
+ if test x$ENABLE_PYTHON3 = xauto; then
+ if test x$PYTHON3_INTERPRETER != xno; then
+ ENABLE_PYTHON3=yes
+ else
+ # When enabling python3 is auto, tests only run if the
+ # python3 interpreter was found on the system. Otherwise,
+ # just ignore it.
+ ENABLE_PYTHON3=no
+ AC_MSG_NOTICE([Python 3 was not found. Skip running tests with Python 3.])
+ fi
+ fi
+
+ if test x$ENABLE_PYTHON3 = xyes; then
+ if test x$PYTHON3_INTERPRETER != xno; then
+ # We were asked to enable python3 implicitely (auto and
+ # python3 was found) or explicitly. So enable running tests
+ # using python3 then.
+ RUN_TESTS_WITH_PY3=yes
+ else
+ AC_MSG_ERROR([Python 3 was not found])
+ fi
+ fi
+
+ if test x$PYTHON3_INTERPRETER = xyes; then
+ MINIMAL_PYTHON_VERSION_FOUND=yes
+ fi
+
if test x$MINIMAL_PYTHON_VERSION_FOUND = xno; then
MISSING_FEDABIPKGDIFF_DEP=yes
if test x$MISSING_FEDABIPKGDIFF_DEP_FATAL = xyes; then
- AC_MSG_ERROR([could not find a python program of version at least $MINIMAL_PYTHON_VERSION])
+ AC_MSG_ERROR([could not find a python program of version at least $MINIMAL_PYTHON2_VERSION])
+ fi
+ else
+ if test x$PYTHON3_INTERPRETER != xno; then
+ # We were instructed to use python3 and it's present on the
+ # system. Let's update the PYTHON variable that points to the
+ # actual python interpreter we are going to be using
+ PYTHON=$PYTHON3_INTERPRETER
fi
fi
+ ###################################################################
+ # Now we are going to check the presence of the required python
+ # modules using either python2 or python3 as required until now.
+ ###################################################################
+
+ # Grrr, the urlparse python2 module got renamed in python3 as
+ # urllib.parse. Oh well.
+ if test x$PYTHON = xpython3; then
+ URLPARSE_MODULE=urllib.parse
+ else
+ URLPARSE_MODULE=urlparse
+ fi
+
REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF="\
- argparse logging os re subprocess sys urlparse \
- xdg koji mock rpm imp tempfile mimetypes shutil"
+ argparse logging os re subprocess sys $URLPARSE_MODULE \
+ xdg koji mock rpm imp tempfile mimetypes shutil six"
if test x$ENABLE_FEDABIPKGDIFF != xno; then
AX_CHECK_PYTHON_MODULES([$REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF],
- [python2],
+ [$PYTHON],
[FOUND_ALL_PYTHON_MODULES=yes],
[FOUND_ALL_PYTHON_MODULES=no])
@@ -398,6 +459,9 @@ koji.read_config('koji')"
fi
AM_CONDITIONAL(ENABLE_FEDABIPKGDIFF, test x$ENABLE_FEDABIPKGDIFF = xyes)
+AM_CONDITIONAL(ENABLE_RUNNING_TESTS_WITH_PY3, test x$RUN_TESTS_WITH_PY3 = xyes)
+AM_CONDITIONAL(ENABLE_PYTHON3_INTERPRETER, test x$PYTHON3_INTERPRETER != xno)
+
dnl Check for dependency: libzip
LIBZIP_VERSION=0.10.1
@@ -556,8 +620,12 @@ AC_CONFIG_FILES([tests/mockfedabipkgdiff],
[chmod +x tests/mockfedabipkgdiff])
AC_CONFIG_FILES([tests/runtestfedabipkgdiff.py],
[chmod +x tests/runtestfedabipkgdiff.py])
+AC_CONFIG_FILES([tests/runtestfedabipkgdiffpy3.py],
+ [chmod +x tests/runtestfedabipkgdiffpy3.py])
AC_CONFIG_FILES([tests/runtestdefaultsupprs.py],
[chmod +x tests/runtestdefaultsupprs.py])
+AC_CONFIG_FILES([tests/runtestdefaultsupprspy3.py],
+ [chmod +x tests/runtestdefaultsupprspy3.py])
AC_OUTPUT
@@ -573,6 +641,7 @@ AC_MSG_NOTICE([
C Compiler : ${CC}
C++ Compiler : ${CXX}
GCC visibility attribute supported : ${SUPPORTS_GCC_VISIBILITY_ATTRIBUTE}
+ Python : ${PYTHON}
OPTIONAL FEATURES:
Enable zip archives : ${ENABLE_ZIP_ARCHIVE}
@@ -583,6 +652,7 @@ AC_MSG_NOTICE([
Enable GNU tar archive support in abipkgdiff : ${ENABLE_TAR}
Enable bash completion : ${ENABLE_BASH_COMPLETION}
Enable fedabipkgdiff : ${ENABLE_FEDABIPKGDIFF}
+ Enable python 3 : ${ENABLE_PYTHON3}
Enable running tests under Valgrind : ${enable_valgrind}
Generate html apidoc : ${ENABLE_APIDOC}
Generate html manual : ${ENABLE_MANUAL}
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0a7f4b9..30bf46b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -17,9 +17,13 @@ AM_CXXFLAGS += "-std=gnu++11"
endif
FEDABIPKGDIFF_TEST =
+if ENABLE_RUNNING_TESTS_WITH_PY3
+FEDABIPKGDIFF_TEST += runtestfedabipkgdiffpy3.py
+else
if ENABLE_FEDABIPKGDIFF
FEDABIPKGDIFF_TEST += runtestfedabipkgdiff.py
endif
+endif
TESTS= \
$(FEDABIPKGDIFF_TEST) \
@@ -43,6 +47,9 @@ runtestabidiffexit \
runtestdefaultsupprs.py \
$(CXX11_TESTS)
+if ENABLE_RUNNING_TESTS_WITH_PY3
+TESTS += runtestdefaultsupprspy3.py
+endif
EXTRA_DIST = \
runtestcanonicalizetypes.sh.in \
@@ -50,6 +57,12 @@ runtestfedabipkgdiff.py.in \
mockfedabipkgdiff.in \
test-valgrind-suppressions.supp
+if ENABLE_RUNNING_TESTS_WITH_PY3
+EXTRA_DIST += \
+runtestfedabipkgdiffpy3.py.in \
+runtestdefaultsupprspy3.py.in
+endif
+
CLEANFILES = \
runtestcanonicalizetypes.output.txt \
runtestcanonicalizetypes.output.final.txt
@@ -139,6 +152,14 @@ runtestfedabipkgdiff.py$(EXEEXT):
runtestdefaultsupprs_py_SOURCES =
runtestdefaultsupprs.py$(EXEEXT):
+if ENABLE_RUNNING_TESTS_WITH_PY3
+runtestfedabipkgdiffpy3_py_SOURCES =
+runtestfedabipkgdiffpy3.py$(EXEEXT):
+
+runtestdefaultsupprspy3_py_SOURCES =
+runtestdefaultsupprspy3.py$(EXEEXT):
+endif
+
AM_CPPFLAGS=-I${abs_top_srcdir}/include \
-I${abs_top_builddir}/include -I${abs_top_srcdir}/tools -fPIC
diff --git a/tests/mockfedabipkgdiff.in b/tests/mockfedabipkgdiff.in
index aac99d1..47c8cc8 100644
--- a/tests/mockfedabipkgdiff.in
+++ b/tests/mockfedabipkgdiff.in
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
# -*- coding: utf-8 -*-
# -*- Mode: Python
#
@@ -55,13 +55,14 @@ variables.
import os
import tempfile
import imp
+import six
try:
from mock import patch
except ImportError:
import sys
- print >>sys.stderr, \
- 'mock is required to run tests. Please install before running tests.'
+ six.print_('mock is required to run tests. Please install before running'
+ ' tests.', file=sys.stderr)
sys.exit(1)
ABIPKGDIFF = '@abs_top_builddir@/tools/abipkgdiff'
@@ -307,12 +308,12 @@ class MockClientSession(object):
:return: the found package
:rtype: dict
"""
- assert isinstance(name, basestring)
+ assert isinstance(name, six.string_types)
def selector(package):
return package['name'] == name
- return filter(selector, packages)[0]
+ return [p for p in packages if selector(p)][0]
def getBuild(self, build_id):
"""Mock kojihub.getBuild
@@ -326,7 +327,7 @@ class MockClientSession(object):
def selector(build):
return build['build_id'] == build_id
- return filter(selector, builds)[0]
+ return [b for b in builds if selector(b)][0]
def listBuilds(self, packageID, state=None):
"""Mock kojihub.listBuilds
@@ -362,7 +363,7 @@ class MockClientSession(object):
rpm['release'] == rpminfo['release'] and \
rpm['arch'] == rpminfo['arch']
- return filter(selector, rpms)[0]
+ return [rpm for rpm in rpms if selector(rpm)][0]
def listRPMs(self, buildID, arches=None):
"""Mock kojihub.listRPMs
@@ -376,9 +377,9 @@ class MockClientSession(object):
"""
assert isinstance(buildID, int)
if arches is not None:
- assert isinstance(arches, (tuple, list, basestring))
+ assert isinstance(arches, (tuple, list, six.string_types))
- if arches is not None and isinstance(arches, basestring):
+ if arches is not None and isinstance(arches, six.string_types):
arches = [arches]
def selector(rpm):
@@ -387,7 +388,7 @@ class MockClientSession(object):
selected = selected and rpm['arch'] in arches
return selected
- return filter(selector, rpms)
+ return [rpm for rpm in rpms if selector(rpm)]
@patch('koji.ClientSession', new=MockClientSession)
diff --git a/tests/runtestdefaultsupprs.py.in b/tests/runtestdefaultsupprs.py.in
index 41b0b28..10b71d1 100644
--- a/tests/runtestdefaultsupprs.py.in
+++ b/tests/runtestdefaultsupprs.py.in
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
'''Runs tests for the default suppression specifications of libabigail.
@@ -125,19 +125,18 @@ def run_abidiff_tests():
if suppressions:
env_vars[env_name] = suppressions
- with open(output_path, 'w') as out_file:
- subprocess.call(cmd, env=env_vars, stdout=out_file)
+ with open(output_path, 'w') as out_file:
+ subprocess.call(cmd, env=env_vars, stdout=out_file)
- diffcmd = ['diff', '-u', reference_report_path,
- output_path]
+ diffcmd = ['diff', '-u', reference_report_path, output_path]
- return_code = subprocess.call(diffcmd)
+ return_code = subprocess.call(diffcmd)
if return_code:
result = return_code
sys.stderr.write("failed abidiff test "
"for env var '" + e + "'\n");
- del env_vars[env_name];
+ del env_vars[env_name];
try:
os.remove(default_suppression)
@@ -202,19 +201,18 @@ def run_abipkgdiff_tests():
if suppressions:
env_vars[env_name] = suppressions
- with open(output_path, 'w') as out_file:
- subprocess.call(cmd, env=env_vars, stdout=out_file)
+ with open(output_path, 'w') as out_file:
+ subprocess.call(cmd, env=env_vars, stdout=out_file)
- diffcmd = ['diff', '-u', reference_report_path,
- output_path]
+ diffcmd = ['diff', '-u', reference_report_path, output_path]
- return_code = subprocess.call(diffcmd)
+ return_code = subprocess.call(diffcmd)
if return_code:
result = return_code
sys.stderr.write("failed abipkgdiff test "
"for env var '" + e + "'\n");
- del env_vars[env_name];
+ del env_vars[env_name];
try:
os.remove(default_suppression)
diff --git a/tests/runtestdefaultsupprspy3.py.in b/tests/runtestdefaultsupprspy3.py.in
new file mode 100644
index 0000000..4985206
--- /dev/null
+++ b/tests/runtestdefaultsupprspy3.py.in
@@ -0,0 +1,2 @@
+#!/bin/bash
+@PYTHON3_INTERPRETER@ "@abs_top_builddir@/tests/runtestdefaultsupprs.py"
diff --git a/tests/runtestfedabipkgdiff.py.in b/tests/runtestfedabipkgdiff.py.in
index 78898f3..04429b6 100755
--- a/tests/runtestfedabipkgdiff.py.in
+++ b/tests/runtestfedabipkgdiff.py.in
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
# -*- coding: utf-8 -*-
# -*- Mode: Python
#
diff --git a/tests/runtestfedabipkgdiffpy3.py.in b/tests/runtestfedabipkgdiffpy3.py.in
new file mode 100644
index 0000000..209037c
--- /dev/null
+++ b/tests/runtestfedabipkgdiffpy3.py.in
@@ -0,0 +1,22 @@
+#!/bin/bash -e
+
+# Either tests runner script or the tools/fedabipkgdiff has shebang
+# `/usr/bin/env python`, as a result, to run tests in Python 3, we have to
+# change PATH in order to make sure python can be found before the current
+# $PATH.
+
+PY3_TEMP=$(mktemp -d --tmpdir libabigail-py3-temp-XXXXXXXX)
+
+ln -s $(which @PYTHON3_INTERPRETER@) $PY3_TEMP/python
+
+export PATH=$PY3_TEMP:$PATH
+
+function clean_env
+{
+ unlink $PY3_TEMP/python
+ rmdir $PY3_TEMP
+}
+
+trap "clean_env" EXIT
+
+@PYTHON3_INTERPRETER@ "@abs_top_builddir@/tests/runtestfedabipkgdiff.py"
diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
index da303ad..2191613 100755
--- a/tools/fedabipkgdiff
+++ b/tools/fedabipkgdiff
@@ -22,13 +22,17 @@
#
# Author: Chenxiong Qi
+from __future__ import print_function
+
import argparse
+import functools
import glob
import logging
import mimetypes
import os
import re
import shutil
+import six
import subprocess
import sys
@@ -226,8 +230,8 @@ def cmp_nvr(left, right):
equal to, or larger than the right individually.
:rtype: int
"""
- left_nvr = koji.parse_NVR(left)
- right_nvr = koji.parse_NVR(right)
+ left_nvr = koji.parse_NVR(left['nvr'])
+ right_nvr = koji.parse_NVR(right['nvr'])
return rpm.labelCompare(
(left_nvr['epoch'], left_nvr['version'], left_nvr['release']),
(right_nvr['epoch'], right_nvr['version'], right_nvr['release']))
@@ -461,7 +465,8 @@ class RPMCollection(object):
self.ancillary_rpms = {}
if rpms:
- map(self.add, rpms)
+ for rpm in rpms:
+ self.add(rpm)
@classmethod
def gather_from_dir(cls, rpm_file, all_rpms=None):
@@ -517,8 +522,7 @@ class RPMCollection(object):
def rpms_iter(self, arches=None, default_behavior=True):
"""Iterator of RPMs to go through RPMs with specific arches"""
- arches = self.rpms.keys()
- arches.sort()
+ arches = sorted(self.rpms.keys())
for arch in arches:
for _rpm in self.rpms[arch]:
@@ -561,7 +565,7 @@ class RPMCollection(object):
if r.is_debuginfo:
result.append(r)
return result
-
+
def generate_comparison_halves(rpm_col1, rpm_col2):
"""Iterate RPM collection and peer's to generate comparison halves"""
@@ -739,7 +743,7 @@ class Brew(object):
raise TypeError(
'{0} is not a callable object.'.format(str(selector)))
- if order_by is not None and not isinstance(order_by, basestring):
+ if order_by is not None and not isinstance(order_by, six.string_types):
raise TypeError('order_by {0} is invalid.'.format(order_by))
builds = self.session.listBuilds(packageID=packageID, state=state)
@@ -748,11 +752,16 @@ class Brew(object):
if order_by is not None:
# FIXME: is it possible to sort builds by using opts parameter of
# listBuilds
- cmp_func = cmp_nvr if order_by == 'nvr' else None
- builds = sorted(builds,
- key=lambda item: item[order_by],
- cmp=cmp_func,
- reverse=reverse)
+ if order_by == 'nvr':
+ if six.PY2:
+ builds = sorted(builds, cmp=cmp_nvr, reverse=reverse)
+ else:
+ builds = sorted(builds,
+ key=functools.cmp_to_key(cmp_nvr),
+ reverse=reverse)
+ else:
+ builds = sorted(
+ builds, key=lambda b: b[order_by], reverse=reverse)
if topone:
builds = builds[0:1]
@@ -974,7 +983,7 @@ def download_rpm(url):
url, os.path.join(get_download_dir(),
os.path.basename(url)))
if global_config.dry_run:
- print 'DRY-RUN:', cmd
+ print('DRY-RUN: {0}'.format(cmd))
return
return_code = subprocess.call(cmd, shell=True)
@@ -997,7 +1006,8 @@ def download_rpms(rpms):
logger.debug('Download %s', rpm.download_url)
download_rpm(rpm.download_url)
- map(_download, rpms)
+ for rpm in rpms:
+ _download(rpm)
@log_call
@@ -1019,7 +1029,7 @@ def build_path_to_abipkgdiff():
def format_debug_info_pkg_options(option, debuginfo_list):
"""Given a list of debug info package descriptors return an option
string that looks like:
-
+
option dbg.rpm1 option dbgrpm2 ...
:param: list debuginfo_list a list of instances of the RPM class
@@ -1121,20 +1131,21 @@ def abipkgdiff(cmp_half1, cmp_half2):
cmp_half1.subject.downloaded_file,
cmp_half2.subject.downloaded_file,
]
- cmd = filter(lambda s: s != '', cmd)
+ cmd = [s for s in cmd if s != '']
if global_config.dry_run:
- print 'DRY-RUN:', ' '.join(cmd)
+ print('DRY-RUN: {0}'.format(' '.join(cmd)))
return
logger.debug('Run: %s', ' '.join(cmd))
- print 'Comparing the ABI of binaries between {0} and {1}:'.format(
- cmp_half1.subject.filename, cmp_half2.subject.filename)
- print
+ print('Comparing the ABI of binaries between {0} and {1}:'.format(
+ cmp_half1.subject.filename, cmp_half2.subject.filename))
+ print()
proc = subprocess.Popen(' '.join(cmd), shell=True,
- stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+ stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+ universal_newlines=True)
# So we could have done: stdout, stderr = proc.communicate()
# But then the documentatin of proc.communicate says:
#
@@ -1160,9 +1171,9 @@ def abipkgdiff(cmp_half1, cmp_half2):
has_abi_change = proc.returncode & ABIDIFF_ABI_CHANGE
if is_internal_error:
- print >>sys.stderr, stderr
+ six.print_(stderr, file=sys.stderr)
elif is_ok or has_abi_change:
- print stdout
+ print(stdout)
return proc.returncode
@@ -1555,7 +1566,7 @@ def main():
if both_nvr or both_nvra:
return diff_two_nvras_from_koji()
- print >>sys.stderr, 'Unknown arguments. Please refer to --help.'
+ six.print_('Unknown arguments. Please refer to --help.', file=sys.stderr)
return 1
@@ -1568,7 +1579,7 @@ if __name__ == '__main__':
if global_config.debug:
logger.debug('Terminate by user')
else:
- print >>sys.stderr, 'Terminate by user'
+ six.print_('Terminate by user', file=sys.stderr)
if global_config.show_traceback:
raise
else:
@@ -1579,7 +1590,7 @@ if __name__ == '__main__':
if global_config.debug:
logger.debug(str(e))
else:
- print >>sys.stderr, str(e)
+ six.print_(str(e), file=sys.stderr)
if global_config.show_traceback:
raise
else:
--
2.17.0.rc1
[-- Attachment #3: Type: text/plain, Size: 14 bytes --]
--
Dodji
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Bug 22722 - Make fedabidiff and its tests support both python 3 and 2
@ 2018-01-01 0:00 cqi
2018-01-01 0:00 ` Dodji Seketeli
0 siblings, 1 reply; 4+ messages in thread
From: cqi @ 2018-01-01 0:00 UTC (permalink / raw)
To: libabigail; +Cc: Chenxiong Qi
From: Chenxiong Qi <cqi@redhat.com>
This patch makes fedabipkgdiff Python 3 compatible. All tests written in
Python are updated and compatible with Python 3 as well. Tests run with
Python 3 by default. It could be disabled by a new option
--disable-py3-tests if it is unnecessary.
* configure.ac: Add new option --disable-py3-tests. Add new
test runner file tests/runtestdefaultsupprs-py3 and
tests/runtestfedabipkgdiff-py3. Add required six Python module.
* tests/Makefile.am: Add new test files
tests/runtestdefaultsupprs-py3 and
tests/runtestfedabipkgdiff-py3 according to the new option
--disable-py3-tests.
* tests/mockfedabipkgdiff.in: Convert print statement to
six.print_.
* tests/runtestdefaultsupprs-py3.in: New shell script to run
test runtestdefaultsupprs with Python 3.
* tests/runtestdefaultsupprs.py.in: Remove trailing
semicolons. Repalce tabs with proper number of spaces.
* tests/runtestfedabipkgdiff-py3.in: New shell script to run
test runtestfedabipkgdiff with Python 3.
* tests/runtestfedabipkgdiff.py.in: Use python from env in
shebang instead of a fixed path to a Python interpreter.
* tests/update-test-output.py: Likewise. And convert print
statement to the print function call which is available after
importing print_function from __future__ module.
* tools/fedabipkgdiff: Globally replace print statement with a
function call to print which is available by importing
print_function from __future__ module. Use six.print_ to output
string to stderr instead. Convert function call to map to
for-loop.
Signed-off-by: Chenxiong Qi <cqi@redhat.com>
---
configure.ac | 20 ++++++++++++-
tests/Makefile.am | 14 +++++++++
tests/mockfedabipkgdiff.in | 19 ++++++------
tests/runtestdefaultsupprs-py3.in | 2 ++
tests/runtestdefaultsupprs.py.in | 62 ++++++++++++++++++++-------------------
tests/runtestfedabipkgdiff-py3.in | 2 ++
tests/runtestfedabipkgdiff.py.in | 2 +-
tests/update-test-output.py | 8 +++--
tools/fedabipkgdiff | 33 ++++++++++++---------
9 files changed, 104 insertions(+), 58 deletions(-)
create mode 100644 tests/runtestdefaultsupprs-py3.in
create mode 100644 tests/runtestfedabipkgdiff-py3.in
diff --git a/configure.ac b/configure.ac
index 4963ee5..9bc92f2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -97,6 +97,12 @@ AC_ARG_ENABLE([fedabipkgdiff],
ENABLE_FEDABIPKGDIFF=$enableval,
ENABLE_FEDABIPKGDIFF=auto)
+AC_ARG_ENABLE([py3-tests],
+ AS_HELP_STRING([--disable-py3-tests=yes|no|auto],
+ [disable running tests with Python 3]),
+ DISABLE_PY3_TESTS=$enableval,
+ DISABLE_PY3_TESTS=auto)
+
dnl *************************************************
dnl check for dependencies
dnl *************************************************
@@ -351,7 +357,7 @@ if test x$CHECK_DEPS_FOR_FEDABIPKGDIFF = xyes; then
REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF="\
argparse logging os re subprocess sys urlparse \
- xdg koji mock rpm imp tempfile mimetypes shutil"
+ xdg koji mock rpm imp tempfile mimetypes shutil six"
if test x$ENABLE_FEDABIPKGDIFF != xno; then
AX_CHECK_PYTHON_MODULES([$REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF],
@@ -399,6 +405,14 @@ fi
AM_CONDITIONAL(ENABLE_FEDABIPKGDIFF, test x$ENABLE_FEDABIPKGDIFF = xyes)
+
+AC_MSG_NOTICE(Run tests with Python 3... $DISABLE_PY3_TESTS)
+if test x$DISABLE_PY3_TESTS = xauto -o x$DISABLE_PY3_TESTS = xyes; then
+ ENABLE_PY3_TESTS=yes
+fi
+
+AM_CONDITIONAL(ENABLE_PY3_TESTS, test x$ENABLE_PY3_TESTS = xyes)
+
dnl Check for dependency: libzip
LIBZIP_VERSION=0.10.1
@@ -556,8 +570,12 @@ AC_CONFIG_FILES([tests/mockfedabipkgdiff],
[chmod +x tests/mockfedabipkgdiff])
AC_CONFIG_FILES([tests/runtestfedabipkgdiff.py],
[chmod +x tests/runtestfedabipkgdiff.py])
+AC_CONFIG_FILES([tests/runtestfedabipkgdiff-py3],
+ [chmod +x tests/runtestfedabipkgdiff-py3])
AC_CONFIG_FILES([tests/runtestdefaultsupprs.py],
[chmod +x tests/runtestdefaultsupprs.py])
+AC_CONFIG_FILES([tests/runtestdefaultsupprs-py3],
+ [chmod +x tests/runtestdefaultsupprs-py3])
AC_OUTPUT
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0a7f4b9..9b31e98 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -20,6 +20,9 @@ FEDABIPKGDIFF_TEST =
if ENABLE_FEDABIPKGDIFF
FEDABIPKGDIFF_TEST += runtestfedabipkgdiff.py
endif
+if ENABLE_PY3_TESTS
+FEDABIPKGDIFF_TEST += runtestfedabipkgdiff-py3
+endif
TESTS= \
$(FEDABIPKGDIFF_TEST) \
@@ -43,10 +46,15 @@ runtestabidiffexit \
runtestdefaultsupprs.py \
$(CXX11_TESTS)
+if ENABLE_PY3_TESTS
+TESTS += runtestdefaultsupprs-py3
+endif
+
EXTRA_DIST = \
runtestcanonicalizetypes.sh.in \
runtestfedabipkgdiff.py.in \
+runtestfedabipkgdiff-py3.in \
mockfedabipkgdiff.in \
test-valgrind-suppressions.supp
@@ -136,9 +144,15 @@ runtestcanonicalizetypes.sh$(EXEEXT):
runtestfedabipkgdiff_py_SOURCES =
runtestfedabipkgdiff.py$(EXEEXT):
+runtestfedabipkgdiff_py3_SOURCES =
+runtestfedabipkgdiff-py3$(EXEEXT):
+
runtestdefaultsupprs_py_SOURCES =
runtestdefaultsupprs.py$(EXEEXT):
+runtestdefaultsupprs_py3_SOURCES =
+runtestdefaultsupprs-py3$(EXEEXT):
+
AM_CPPFLAGS=-I${abs_top_srcdir}/include \
-I${abs_top_builddir}/include -I${abs_top_srcdir}/tools -fPIC
diff --git a/tests/mockfedabipkgdiff.in b/tests/mockfedabipkgdiff.in
index aac99d1..1168b92 100644
--- a/tests/mockfedabipkgdiff.in
+++ b/tests/mockfedabipkgdiff.in
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
# -*- coding: utf-8 -*-
# -*- Mode: Python
#
@@ -55,13 +55,14 @@ variables.
import os
import tempfile
import imp
+import six
try:
from mock import patch
except ImportError:
import sys
- print >>sys.stderr, \
- 'mock is required to run tests. Please install before running tests.'
+ six.print_('mock is required to run tests. Please install before running '
+ 'tests.', file=sys.stderr)
sys.exit(1)
ABIPKGDIFF = '@abs_top_builddir@/tools/abipkgdiff'
@@ -246,29 +247,29 @@ rpms = [
},
# RPMs of build vte291-0.39.1-1.fc22
{'build_id': 600011,
- 'name': 'vte291', 'version': '0.39.1', 'release': '1.fc22',
+ 'name': 'vte291', 'version': '0.39.1', 'release': '1.fc22',
'arch': 'x86_64', 'nvr': 'vte291-0.39.1-1.fc22',
},
{'build_id': 600011,
- 'name': 'vte291-devel', 'version': '0.39.1', 'release': '1.fc22',
+ 'name': 'vte291-devel', 'version': '0.39.1', 'release': '1.fc22',
'arch': 'x86_64', 'nvr': 'vte291-0.39.1-1.fc22',
},
{'build_id': 600011,
- 'name': 'vte291-debuginfo', 'version': '0.39.1', 'release': '1.fc22',
+ 'name': 'vte291-debuginfo', 'version': '0.39.1', 'release': '1.fc22',
'arch': 'x86_64', 'nvr': 'vte291-0.39.1-1.fc22',
},
# RPMs of build vte291-0.39.90-1.fc22
{'build_id': 612610,
- 'name': 'vte291', 'version': '0.39.90', 'release': '1.fc22',
+ 'name': 'vte291', 'version': '0.39.90', 'release': '1.fc22',
'arch': 'x86_64', 'nvr': 'vte291-0.39.90-1.fc22',
},
{'build_id': 612610,
- 'name': 'vte291-devel', 'version': '0.39.90', 'release': '1.fc22',
+ 'name': 'vte291-devel', 'version': '0.39.90', 'release': '1.fc22',
'arch': 'x86_64', 'nvr': 'vte291-0.39.90-1.fc22',
},
{'build_id': 612610,
- 'name': 'vte291-debuginfo', 'version': '0.39.90', 'release': '1.fc22',
+ 'name': 'vte291-debuginfo', 'version': '0.39.90', 'release': '1.fc22',
'arch': 'x86_64', 'nvr': 'vte291-0.39.90-1.fc22',
},
]
diff --git a/tests/runtestdefaultsupprs-py3.in b/tests/runtestdefaultsupprs-py3.in
new file mode 100644
index 0000000..7b856d0
--- /dev/null
+++ b/tests/runtestdefaultsupprs-py3.in
@@ -0,0 +1,2 @@
+#!/bin/bash
+python3 "@abs_top_builddir@/tests/runtestdefaultsupprs.py"
\ No newline at end of file
diff --git a/tests/runtestdefaultsupprs.py.in b/tests/runtestdefaultsupprs.py.in
index 41b0b28..9feafe7 100644
--- a/tests/runtestdefaultsupprs.py.in
+++ b/tests/runtestdefaultsupprs.py.in
@@ -67,7 +67,8 @@ def ensure_output_dir_created():
pass
if not os.path.exists(output_dir):
- sys.exit(1);
+ sys.exit(1)
+
def run_abidiff_tests():
"""Run the abidiff default suppression tests.
@@ -90,9 +91,9 @@ def run_abidiff_tests():
default_suppression = output_dir + "/default.abignore"
with open(default_suppression, 'w') as out:
- out.write('\n');
+ out.write('\n')
- result = 0;
+ result = 0
for test_spec in abidiff_test_specs:
binary1 = test_spec[0]
binary2 = test_spec[1]
@@ -111,40 +112,40 @@ def run_abidiff_tests():
# The environment variables that say where to find the default
# suppression specifications loaded by libabigail.
envs = {
- 'LIBABIGAIL_DEFAULT_SYSTEM_SUPPRESSION_FILE' : default_suppression,
- 'LIBABIGAIL_DEFAULT_USER_SUPPRESSION_FILE' : default_suppression
+ 'LIBABIGAIL_DEFAULT_SYSTEM_SUPPRESSION_FILE': default_suppression,
+ 'LIBABIGAIL_DEFAULT_USER_SUPPRESSION_FILE': default_suppression
}
# Initialize the environment variables above to their default
# value.
for name, value in envs.items():
- os.environ[name] = value;
+ os.environ[name] = value
for env_name in envs:
env_vars = os.environ
if suppressions:
env_vars[env_name] = suppressions
- with open(output_path, 'w') as out_file:
- subprocess.call(cmd, env=env_vars, stdout=out_file)
+ with open(output_path, 'w') as out_file:
+ subprocess.call(cmd, env=env_vars, stdout=out_file)
- diffcmd = ['diff', '-u', reference_report_path,
- output_path]
+ diffcmd = ['diff', '-u', reference_report_path, output_path]
- return_code = subprocess.call(diffcmd)
+ return_code = subprocess.call(diffcmd)
if return_code:
result = return_code
sys.stderr.write("failed abidiff test "
- "for env var '" + e + "'\n");
+ "for env var '" + e + "'\n")
- del env_vars[env_name];
+ del env_vars[env_name]
try:
os.remove(default_suppression)
except:
pass
- return result;
+ return result
+
def run_abipkgdiff_tests():
"""Run the abipkgdiff default suppression tests.
@@ -167,9 +168,9 @@ def run_abipkgdiff_tests():
default_suppression = output_dir + "/default.abignore"
with open(default_suppression, 'w') as out:
- out.write('\n');
+ out.write('\n')
- result = 0;
+ result = 0
for test_spec in abipkgdiff_test_specs:
pkg1 = test_spec[0]
pkg2 = test_spec[1]
@@ -188,40 +189,40 @@ def run_abipkgdiff_tests():
# The environment variables that say where to find the default
# suppression specifications loaded by libabigail.
envs = {
- 'LIBABIGAIL_DEFAULT_SYSTEM_SUPPRESSION_FILE' : default_suppression,
- 'LIBABIGAIL_DEFAULT_USER_SUPPRESSION_FILE' : default_suppression
+ 'LIBABIGAIL_DEFAULT_SYSTEM_SUPPRESSION_FILE': default_suppression,
+ 'LIBABIGAIL_DEFAULT_USER_SUPPRESSION_FILE': default_suppression
}
# Initialize the environment variables above to their default
# value.
for name, value in envs.items():
- os.environ[name] = value;
+ os.environ[name] = value
for env_name in envs:
env_vars = os.environ
if suppressions:
env_vars[env_name] = suppressions
- with open(output_path, 'w') as out_file:
- subprocess.call(cmd, env=env_vars, stdout=out_file)
+ with open(output_path, 'w') as out_file:
+ subprocess.call(cmd, env=env_vars, stdout=out_file)
- diffcmd = ['diff', '-u', reference_report_path,
- output_path]
+ diffcmd = ['diff', '-u', reference_report_path, output_path]
- return_code = subprocess.call(diffcmd)
+ return_code = subprocess.call(diffcmd)
if return_code:
result = return_code
sys.stderr.write("failed abipkgdiff test "
- "for env var '" + e + "'\n");
+ "for env var '" + e + "'\n")
- del env_vars[env_name];
+ del env_vars[env_name]
try:
os.remove(default_suppression)
except:
pass
- return result;
+ return result
+
def main():
"""The main entry point of this program.
@@ -235,12 +236,13 @@ def main():
"""
ensure_output_dir_created()
- result = 0;
+ result = 0
result = run_abidiff_tests()
if result:
- return result;
+ return result
result = run_abipkgdiff_tests()
- return result;
+ return result
+
if __name__ == '__main__':
exit_code = main()
diff --git a/tests/runtestfedabipkgdiff-py3.in b/tests/runtestfedabipkgdiff-py3.in
new file mode 100644
index 0000000..b4cbe78
--- /dev/null
+++ b/tests/runtestfedabipkgdiff-py3.in
@@ -0,0 +1,2 @@
+#!/bin/bash
+python3 "@abs_top_builddir@/tests/runtestfedabipkgdiff.py"
\ No newline at end of file
diff --git a/tests/runtestfedabipkgdiff.py.in b/tests/runtestfedabipkgdiff.py.in
index 78898f3..04429b6 100755
--- a/tests/runtestfedabipkgdiff.py.in
+++ b/tests/runtestfedabipkgdiff.py.in
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
# -*- coding: utf-8 -*-
# -*- Mode: Python
#
diff --git a/tests/update-test-output.py b/tests/update-test-output.py
index 4017dd0..84afd4e 100755
--- a/tests/update-test-output.py
+++ b/tests/update-test-output.py
@@ -1,14 +1,16 @@
-#!/bin/python
+#!/usr/bin/env python
# This program generates the copy commands you should use to update
# the reference data for tests <build-dir>/tests/runtest* that emit an
# output that is compared against a reference output.
-#
+#
# It takes in argument the diff result emitted by
# <build-dir>/tests/runtest*, and emits on standard output a series of
# 'cp <src> <dest>' commands to execute to update reference data of
# the test.
+from __future__ import print_function
+
import fileinput
import re
import sys
@@ -31,7 +33,7 @@ def main():
try:
opts, args = getopt.getopt(sys.argv[1:], "hi", ["help"])
except getopt.GetoptError as err:
- print str(err)
+ print(str(err))
display_usage()
for opt, arg in opts:
diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
index da303ad..5500104 100755
--- a/tools/fedabipkgdiff
+++ b/tools/fedabipkgdiff
@@ -22,6 +22,8 @@
#
# Author: Chenxiong Qi
+from __future__ import print_function
+
import argparse
import glob
import logging
@@ -39,6 +41,7 @@ import xdg.BaseDirectory
import rpm
import koji
+import six
# @file
#
@@ -461,7 +464,8 @@ class RPMCollection(object):
self.ancillary_rpms = {}
if rpms:
- map(self.add, rpms)
+ for rpm in rpms:
+ self.add(rpm)
@classmethod
def gather_from_dir(cls, rpm_file, all_rpms=None):
@@ -561,7 +565,7 @@ class RPMCollection(object):
if r.is_debuginfo:
result.append(r)
return result
-
+
def generate_comparison_halves(rpm_col1, rpm_col2):
"""Iterate RPM collection and peer's to generate comparison halves"""
@@ -974,7 +978,7 @@ def download_rpm(url):
url, os.path.join(get_download_dir(),
os.path.basename(url)))
if global_config.dry_run:
- print 'DRY-RUN:', cmd
+ print('DRY-RUN:', cmd)
return
return_code = subprocess.call(cmd, shell=True)
@@ -997,7 +1001,8 @@ def download_rpms(rpms):
logger.debug('Download %s', rpm.download_url)
download_rpm(rpm.download_url)
- map(_download, rpms)
+ for rpm in rpms:
+ _download(rpm)
@log_call
@@ -1019,7 +1024,7 @@ def build_path_to_abipkgdiff():
def format_debug_info_pkg_options(option, debuginfo_list):
"""Given a list of debug info package descriptors return an option
string that looks like:
-
+
option dbg.rpm1 option dbgrpm2 ...
:param: list debuginfo_list a list of instances of the RPM class
@@ -1124,14 +1129,14 @@ def abipkgdiff(cmp_half1, cmp_half2):
cmd = filter(lambda s: s != '', cmd)
if global_config.dry_run:
- print 'DRY-RUN:', ' '.join(cmd)
+ print('DRY-RUN:', ' '.join(cmd))
return
logger.debug('Run: %s', ' '.join(cmd))
- print 'Comparing the ABI of binaries between {0} and {1}:'.format(
- cmp_half1.subject.filename, cmp_half2.subject.filename)
- print
+ print('Comparing the ABI of binaries between {0} and {1}:'.format(
+ cmp_half1.subject.filename, cmp_half2.subject.filename))
+ print()
proc = subprocess.Popen(' '.join(cmd), shell=True,
stdout=subprocess.PIPE, stderr=subprocess.PIPE)
@@ -1160,9 +1165,9 @@ def abipkgdiff(cmp_half1, cmp_half2):
has_abi_change = proc.returncode & ABIDIFF_ABI_CHANGE
if is_internal_error:
- print >>sys.stderr, stderr
+ six.print_(stderr, file=sys.stderr)
elif is_ok or has_abi_change:
- print stdout
+ print(stdout)
return proc.returncode
@@ -1555,7 +1560,7 @@ def main():
if both_nvr or both_nvra:
return diff_two_nvras_from_koji()
- print >>sys.stderr, 'Unknown arguments. Please refer to --help.'
+ six.print_('Unknown arguments. Please refer to --help.', file=sys.stderr)
return 1
@@ -1568,7 +1573,7 @@ if __name__ == '__main__':
if global_config.debug:
logger.debug('Terminate by user')
else:
- print >>sys.stderr, 'Terminate by user'
+ six.print_('Terminate by user', file=sys.stderr)
if global_config.show_traceback:
raise
else:
@@ -1579,7 +1584,7 @@ if __name__ == '__main__':
if global_config.debug:
logger.debug(str(e))
else:
- print >>sys.stderr, str(e)
+ six.print_(str(e), file=sys.stderr)
if global_config.show_traceback:
raise
else:
--
2.14.3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Bug 22722 - Make fedabidiff and its tests support both python 3 and 2
2018-01-01 0:00 ` Dodji Seketeli
@ 2018-01-01 0:00 ` Chenxiong Qi
2018-01-01 0:00 ` Dodji Seketeli
0 siblings, 1 reply; 4+ messages in thread
From: Chenxiong Qi @ 2018-01-01 0:00 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail
[-- Attachment #1: Type: text/plain, Size: 5205 bytes --]
Hi Dodji,
My patch is updated. Please take another look. This time it also
contains a few replacement from tab to spaces which are detected
during running in Python 3.
On Wed, Jan 31, 2018 at 7:37 PM, Dodji Seketeli <dodji@seketeli.org> wrote:
> Hello Chenxiong,
>
> Thank you very much for putting this patch together, it's really
> appreciated.
>
> I have a few comments about the patch though, please find them below.
>
> [...]
>
>> diff --git a/configure.ac b/configure.ac
>> index 4963ee5..9bc92f2 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -97,6 +97,12 @@ AC_ARG_ENABLE([fedabipkgdiff],
>> ENABLE_FEDABIPKGDIFF=$enableval,
>> ENABLE_FEDABIPKGDIFF=auto)
>>
>> +AC_ARG_ENABLE([py3-tests],
>> + AS_HELP_STRING([--disable-py3-tests=yes|no|auto],
>> + [disable running tests with Python 3]),
>
> What AC_ARG_ENABLE does here is to define both --enable-py3-test *and*
> --disable-py3-test options. So, I'd prefer that in the help string we
> refer to the positive option, which is --enable-py3-test. That
> positive option is of course "applied" by default in this case.
>
>
>> + DISABLE_PY3_TESTS=$enableval,
>> + DISABLE_PY3_TESTS=auto)
>
> So here, I'd prefer that we use a "positive" variable name, which
> would be ENABLE_PY3_TESTS, rather than the DISABLE_PY3_TESTS that you
> are using. This would be just like what is done above for the other
> options.
>
>> +
>> dnl *************************************************
>> dnl check for dependencies
>> dnl *************************************************
>> @@ -351,7 +357,7 @@ if test x$CHECK_DEPS_FOR_FEDABIPKGDIFF = xyes; then
>>
>> REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF="\
>> argparse logging os re subprocess sys urlparse \
>> - xdg koji mock rpm imp tempfile mimetypes shutil"
>> + xdg koji mock rpm imp tempfile mimetypes shutil six"
>
> Just for my own education, is the python six package available on el6
> systems?
>
>>
>> if test x$ENABLE_FEDABIPKGDIFF != xno; then
>> AX_CHECK_PYTHON_MODULES([$REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF],
>> @@ -399,6 +405,14 @@ fi
>>
>> AM_CONDITIONAL(ENABLE_FEDABIPKGDIFF, test x$ENABLE_FEDABIPKGDIFF = xyes)
>>
>> +
>> +AC_MSG_NOTICE(Run tests with Python 3... $DISABLE_PY3_TESTS)
>> +if test x$DISABLE_PY3_TESTS = xauto -o x$DISABLE_PY3_TESTS = xyes; then
>> + ENABLE_PY3_TESTS=yes
>
> Hmmh. I disagree with the logic here.
>
> I think the logic should be that if we are in 'auto' mode and if
> python3 is available, then we should enable the python3 tests. If we
> are in auto mode and if python3 is not available, then we should *NOT*
> enable the python3 tests. We should thus assume python2 mode.
>
> If on the other hand the user chose to explicitly disable or enable
> the python3 mode (i.e, not choosing 'auto') then we should disable or
> enable python3 as per the user request.
>
> [...]
>
>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> index 0a7f4b9..9b31e98 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -20,6 +20,9 @@ FEDABIPKGDIFF_TEST =
>> if ENABLE_FEDABIPKGDIFF
>> FEDABIPKGDIFF_TEST += runtestfedabipkgdiff.py
>> endif
>> +if ENABLE_PY3_TESTS
>> +FEDABIPKGDIFF_TEST += runtestfedabipkgdiff-py3
>> +endif
>
> I think this should be included in the body of the "if
> ENABLE_FEDABIPKGDIFF" clause above.
>
>>
>> TESTS= \
>> $(FEDABIPKGDIFF_TEST) \
>> @@ -43,10 +46,15 @@ runtestabidiffexit \
>> runtestdefaultsupprs.py \
>> $(CXX11_TESTS)
>>
>> +if ENABLE_PY3_TESTS
>> +TESTS += runtestdefaultsupprs-py3
>> +endif
>> +
>
> I think this is unnecessary if my previous comment is right.
>
> [...]
>
>> diff --git a/tests/mockfedabipkgdiff.in b/tests/mockfedabipkgdiff.in
>> index aac99d1..1168b92 100644
>> --- a/tests/mockfedabipkgdiff.in
>> +++ b/tests/mockfedabipkgdiff.in
>
> [...]
>
>> {'build_id': 600011,
>> - 'name': 'vte291', 'version': '0.39.1', 'release': '1.fc22',
>> + 'name': 'vte291', 'version': '0.39.1', 'release': '1.fc22',
>
> I believe these are (white space) changes that are unrelated to the purpose of
> the patch, which is to move to python3. I am correct?
>
> If so, please file a separate patch that contains these white space
> changes. There are plenty of hunks following this one, which similar
> white space changes, please include them in that white-space/style
> patch.
>
> [...]
>
>> diff --git a/tests/runtestdefaultsupprs.py.in b/tests/runtestdefaultsupprs.py.in
>> index 41b0b28..9feafe7 100644
>> --- a/tests/runtestdefaultsupprs.py.in
>> +++ b/tests/runtestdefaultsupprs.py.in
>> @@ -67,7 +67,8 @@ def ensure_output_dir_created():
>> pass
>>
>> if not os.path.exists(output_dir):
>> - sys.exit(1);
>> + sys.exit(1)
>> +
>
> Just like I said above concerning the white space changes, I think
> this change would be better suited to a separate "style-only" patch.
> There are similar other changes after this one that should be included
> in that separate patch as well.
>
> [...]
>
> Thanks!
>
>
> --
> Dodji
--
Regards,
Chenxiong Qi
[-- Attachment #2: 0001-Bug-22722-Make-fedabipkgdiff-and-its-tests-support-b.patch --]
[-- Type: text/x-patch, Size: 19452 bytes --]
From d53b8dd2920c46af58131072a5296121a0b55978 Mon Sep 17 00:00:00 2001
From: Chenxiong Qi <cqi@redhat.com>
Date: Sun, 25 Mar 2018 15:34:59 +0800
Subject: [PATCH] Bug 22722 - Make fedabipkgdiff and its tests support both
python 3 and 2
This patch makes fedabipkgdiff Python 3 compatible. All tests written in
Python are updated and compatible with Python 3 as well. It defaults to
look for a Python 3 interpreter, if there is such one, run tests with
found interpreter, if there is no one, tests will run only with Python
2. But, this behavior can be disabled by --enable-py3-tests=no.
* configure.ac: Add new option --enable-py3-tests. Add new
test runner file tests/runtestdefaultsupprs-py3 and
tests/runtestfedabipkgdiff-py3. Add required six Python module.
* tests/Makefile.am: Add new test files
tests/runtestdefaultsupprs-py3 and
tests/runtestfedabipkgdiff-py3 accordingly.
* tests/mockfedabipkgdiff.in: Convert print statement to
six.print_. Replace call to function filter with list
comprehension. Replace basestring with six.string_types.
* tests/runtestdefaultsupprs-py3.in: New shell script to run
test runtestdefaultsupprs with Python 3.
* tests/runtestdefaultsupprs.py.in: Repalce a few tabs with
proper number of spaces which is detected by Python 3
interpreter.
* tests/runtestfedabipkgdiff-py3.in: New shell script to run
test runtestfedabipkgdiff with Python 3.
* tests/runtestfedabipkgdiff.py.in: Use python from env in
shebang instead of a fixed path to a Python interpreter.
* tools/fedabipkgdiff: Globally replace print statement with a
function call to print which is available by importing
print_function from __future__ module. Use six.print_ to output
string to stderr instead. Convert function call to map to
for-loop. (cmp_nvr): Change argument to handle a Koji build
mapping instead of only the nvr. (Brew.listBuilds): use the new
cmp_nvr to sort builds.
Signed-off-by: Chenxiong Qi <cqi@redhat.com>
---
.gitignore | 3 +-
configure.ac | 39 +++++++++++++++++++++++-
tests/Makefile.am | 20 +++++++++++++
tests/mockfedabipkgdiff.in | 21 ++++++-------
tests/runtestdefaultsupprs-py3.in | 2 ++
tests/runtestdefaultsupprs.py.in | 24 +++++++--------
tests/runtestfedabipkgdiff-py3.in | 22 ++++++++++++++
tests/runtestfedabipkgdiff.py.in | 2 +-
tools/fedabipkgdiff | 63 +++++++++++++++++++++++----------------
9 files changed, 144 insertions(+), 52 deletions(-)
create mode 100644 tests/runtestdefaultsupprs-py3.in
create mode 100644 tests/runtestfedabipkgdiff-py3.in
diff --git a/.gitignore b/.gitignore
index a60cadb..ed4f43c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -22,4 +22,5 @@ Makefile.in
.tags
build/
TAGS
-fedabipkgdiffc
\ No newline at end of file
+fedabipkgdiffc
+tools/__pycache__/
\ No newline at end of file
diff --git a/configure.ac b/configure.ac
index 4738faa..762c8e8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -97,6 +97,12 @@ AC_ARG_ENABLE([fedabipkgdiff],
ENABLE_FEDABIPKGDIFF=$enableval,
ENABLE_FEDABIPKGDIFF=auto)
+AC_ARG_ENABLE([py3-tests],
+ AS_HELP_STRING([--enable-py3-tests=yes|no|auto],
+ [enable to run tests with Python 3 (default is auto)]),
+ ENABLE_PY3_TESTS=$enableval,
+ ENABLE_PY3_TESTS=auto)
+
dnl *************************************************
dnl check for dependencies
dnl *************************************************
@@ -351,7 +357,7 @@ if test x$CHECK_DEPS_FOR_FEDABIPKGDIFF = xyes; then
REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF="\
argparse logging os re subprocess sys urlparse \
- xdg koji mock rpm imp tempfile mimetypes shutil"
+ xdg koji mock rpm imp tempfile mimetypes shutil six"
if test x$ENABLE_FEDABIPKGDIFF != xno; then
AX_CHECK_PYTHON_MODULES([$REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF],
@@ -399,6 +405,33 @@ fi
AM_CONDITIONAL(ENABLE_FEDABIPKGDIFF, test x$ENABLE_FEDABIPKGDIFF = xyes)
+
+# The minimal python 3 version we want to support is 3.5, which is
+# available in Fedora releases and the version that is available in EL7.
+AC_CHECK_PROGS(PYTHON3_INTERPRETER, [python3 python3.5 python3.6 python3.7], no)
+
+AC_MSG_NOTICE(Run tests with Python 3... $ENABLE_PY3_TESTS)
+if test x$ENABLE_PY3_TESTS = xauto; then
+ if test x$PYTHON3_INTERPRETER != xno; then
+ RUN_TESTS_WITH_PY3=yes
+ else
+ # When enabling py3 tests is auto, tests only run if python3
+ # interpreter is found from system. Otherwise, just ignore it.
+ AC_MSG_NOTICE([Python 3 is not found. Skip to run tests with Python 3.])
+ fi
+fi
+if test x$ENABLE_PY3_TESTS = xyes; then
+ if test x$PYTHON3_INTERPRETER != xno; then
+ RUN_TESTS_WITH_PY3=yes
+ else
+ AC_MSG_ERROR([Python 3 is not found.])
+ fi
+fi
+
+AM_CONDITIONAL(RUN_TESTS_WITH_PY3, test x$RUN_TESTS_WITH_PY3 = xyes)
+AM_CONDITIONAL(PYTHON3_INTERPRETER, test x$PYTHON3_INTERPRETER != xno)
+
+
dnl Check for dependency: libzip
LIBZIP_VERSION=0.10.1
@@ -556,8 +589,12 @@ AC_CONFIG_FILES([tests/mockfedabipkgdiff],
[chmod +x tests/mockfedabipkgdiff])
AC_CONFIG_FILES([tests/runtestfedabipkgdiff.py],
[chmod +x tests/runtestfedabipkgdiff.py])
+AC_CONFIG_FILES([tests/runtestfedabipkgdiff-py3],
+ [chmod +x tests/runtestfedabipkgdiff-py3])
AC_CONFIG_FILES([tests/runtestdefaultsupprs.py],
[chmod +x tests/runtestdefaultsupprs.py])
+AC_CONFIG_FILES([tests/runtestdefaultsupprs-py3],
+ [chmod +x tests/runtestdefaultsupprs-py3])
AC_OUTPUT
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0a7f4b9..e528aa4 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -19,6 +19,9 @@ endif
FEDABIPKGDIFF_TEST =
if ENABLE_FEDABIPKGDIFF
FEDABIPKGDIFF_TEST += runtestfedabipkgdiff.py
+if RUN_TESTS_WITH_PY3
+FEDABIPKGDIFF_TEST += runtestfedabipkgdiff-py3
+endif
endif
TESTS= \
@@ -43,6 +46,9 @@ runtestabidiffexit \
runtestdefaultsupprs.py \
$(CXX11_TESTS)
+if RUN_TESTS_WITH_PY3
+TESTS += runtestdefaultsupprs-py3
+endif
EXTRA_DIST = \
runtestcanonicalizetypes.sh.in \
@@ -50,6 +56,12 @@ runtestfedabipkgdiff.py.in \
mockfedabipkgdiff.in \
test-valgrind-suppressions.supp
+if RUN_TESTS_WITH_PY3
+EXTRA_DIST += \
+runtestfedabipkgdiff-py3.in \
+runtestdefaultsupprs-py3.in
+endif
+
CLEANFILES = \
runtestcanonicalizetypes.output.txt \
runtestcanonicalizetypes.output.final.txt
@@ -139,6 +151,14 @@ runtestfedabipkgdiff.py$(EXEEXT):
runtestdefaultsupprs_py_SOURCES =
runtestdefaultsupprs.py$(EXEEXT):
+if RUN_TESTS_WITH_PY3
+runtestfedabipkgdiff_py3_SOURCES =
+runtestfedabipkgdiff-py3$(EXEEXT):
+
+runtestdefaultsupprs_py3_SOURCES =
+runtestdefaultsupprs-py3$(EXEEXT):
+endif
+
AM_CPPFLAGS=-I${abs_top_srcdir}/include \
-I${abs_top_builddir}/include -I${abs_top_srcdir}/tools -fPIC
diff --git a/tests/mockfedabipkgdiff.in b/tests/mockfedabipkgdiff.in
index aac99d1..47c8cc8 100644
--- a/tests/mockfedabipkgdiff.in
+++ b/tests/mockfedabipkgdiff.in
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
# -*- coding: utf-8 -*-
# -*- Mode: Python
#
@@ -55,13 +55,14 @@ variables.
import os
import tempfile
import imp
+import six
try:
from mock import patch
except ImportError:
import sys
- print >>sys.stderr, \
- 'mock is required to run tests. Please install before running tests.'
+ six.print_('mock is required to run tests. Please install before running'
+ ' tests.', file=sys.stderr)
sys.exit(1)
ABIPKGDIFF = '@abs_top_builddir@/tools/abipkgdiff'
@@ -307,12 +308,12 @@ class MockClientSession(object):
:return: the found package
:rtype: dict
"""
- assert isinstance(name, basestring)
+ assert isinstance(name, six.string_types)
def selector(package):
return package['name'] == name
- return filter(selector, packages)[0]
+ return [p for p in packages if selector(p)][0]
def getBuild(self, build_id):
"""Mock kojihub.getBuild
@@ -326,7 +327,7 @@ class MockClientSession(object):
def selector(build):
return build['build_id'] == build_id
- return filter(selector, builds)[0]
+ return [b for b in builds if selector(b)][0]
def listBuilds(self, packageID, state=None):
"""Mock kojihub.listBuilds
@@ -362,7 +363,7 @@ class MockClientSession(object):
rpm['release'] == rpminfo['release'] and \
rpm['arch'] == rpminfo['arch']
- return filter(selector, rpms)[0]
+ return [rpm for rpm in rpms if selector(rpm)][0]
def listRPMs(self, buildID, arches=None):
"""Mock kojihub.listRPMs
@@ -376,9 +377,9 @@ class MockClientSession(object):
"""
assert isinstance(buildID, int)
if arches is not None:
- assert isinstance(arches, (tuple, list, basestring))
+ assert isinstance(arches, (tuple, list, six.string_types))
- if arches is not None and isinstance(arches, basestring):
+ if arches is not None and isinstance(arches, six.string_types):
arches = [arches]
def selector(rpm):
@@ -387,7 +388,7 @@ class MockClientSession(object):
selected = selected and rpm['arch'] in arches
return selected
- return filter(selector, rpms)
+ return [rpm for rpm in rpms if selector(rpm)]
@patch('koji.ClientSession', new=MockClientSession)
diff --git a/tests/runtestdefaultsupprs-py3.in b/tests/runtestdefaultsupprs-py3.in
new file mode 100644
index 0000000..4985206
--- /dev/null
+++ b/tests/runtestdefaultsupprs-py3.in
@@ -0,0 +1,2 @@
+#!/bin/bash
+@PYTHON3_INTERPRETER@ "@abs_top_builddir@/tests/runtestdefaultsupprs.py"
diff --git a/tests/runtestdefaultsupprs.py.in b/tests/runtestdefaultsupprs.py.in
index 41b0b28..10b71d1 100644
--- a/tests/runtestdefaultsupprs.py.in
+++ b/tests/runtestdefaultsupprs.py.in
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
'''Runs tests for the default suppression specifications of libabigail.
@@ -125,19 +125,18 @@ def run_abidiff_tests():
if suppressions:
env_vars[env_name] = suppressions
- with open(output_path, 'w') as out_file:
- subprocess.call(cmd, env=env_vars, stdout=out_file)
+ with open(output_path, 'w') as out_file:
+ subprocess.call(cmd, env=env_vars, stdout=out_file)
- diffcmd = ['diff', '-u', reference_report_path,
- output_path]
+ diffcmd = ['diff', '-u', reference_report_path, output_path]
- return_code = subprocess.call(diffcmd)
+ return_code = subprocess.call(diffcmd)
if return_code:
result = return_code
sys.stderr.write("failed abidiff test "
"for env var '" + e + "'\n");
- del env_vars[env_name];
+ del env_vars[env_name];
try:
os.remove(default_suppression)
@@ -202,19 +201,18 @@ def run_abipkgdiff_tests():
if suppressions:
env_vars[env_name] = suppressions
- with open(output_path, 'w') as out_file:
- subprocess.call(cmd, env=env_vars, stdout=out_file)
+ with open(output_path, 'w') as out_file:
+ subprocess.call(cmd, env=env_vars, stdout=out_file)
- diffcmd = ['diff', '-u', reference_report_path,
- output_path]
+ diffcmd = ['diff', '-u', reference_report_path, output_path]
- return_code = subprocess.call(diffcmd)
+ return_code = subprocess.call(diffcmd)
if return_code:
result = return_code
sys.stderr.write("failed abipkgdiff test "
"for env var '" + e + "'\n");
- del env_vars[env_name];
+ del env_vars[env_name];
try:
os.remove(default_suppression)
diff --git a/tests/runtestfedabipkgdiff-py3.in b/tests/runtestfedabipkgdiff-py3.in
new file mode 100644
index 0000000..209037c
--- /dev/null
+++ b/tests/runtestfedabipkgdiff-py3.in
@@ -0,0 +1,22 @@
+#!/bin/bash -e
+
+# Either tests runner script or the tools/fedabipkgdiff has shebang
+# `/usr/bin/env python`, as a result, to run tests in Python 3, we have to
+# change PATH in order to make sure python can be found before the current
+# $PATH.
+
+PY3_TEMP=$(mktemp -d --tmpdir libabigail-py3-temp-XXXXXXXX)
+
+ln -s $(which @PYTHON3_INTERPRETER@) $PY3_TEMP/python
+
+export PATH=$PY3_TEMP:$PATH
+
+function clean_env
+{
+ unlink $PY3_TEMP/python
+ rmdir $PY3_TEMP
+}
+
+trap "clean_env" EXIT
+
+@PYTHON3_INTERPRETER@ "@abs_top_builddir@/tests/runtestfedabipkgdiff.py"
diff --git a/tests/runtestfedabipkgdiff.py.in b/tests/runtestfedabipkgdiff.py.in
index 78898f3..04429b6 100755
--- a/tests/runtestfedabipkgdiff.py.in
+++ b/tests/runtestfedabipkgdiff.py.in
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
# -*- coding: utf-8 -*-
# -*- Mode: Python
#
diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
index da303ad..2191613 100755
--- a/tools/fedabipkgdiff
+++ b/tools/fedabipkgdiff
@@ -22,13 +22,17 @@
#
# Author: Chenxiong Qi
+from __future__ import print_function
+
import argparse
+import functools
import glob
import logging
import mimetypes
import os
import re
import shutil
+import six
import subprocess
import sys
@@ -226,8 +230,8 @@ def cmp_nvr(left, right):
equal to, or larger than the right individually.
:rtype: int
"""
- left_nvr = koji.parse_NVR(left)
- right_nvr = koji.parse_NVR(right)
+ left_nvr = koji.parse_NVR(left['nvr'])
+ right_nvr = koji.parse_NVR(right['nvr'])
return rpm.labelCompare(
(left_nvr['epoch'], left_nvr['version'], left_nvr['release']),
(right_nvr['epoch'], right_nvr['version'], right_nvr['release']))
@@ -461,7 +465,8 @@ class RPMCollection(object):
self.ancillary_rpms = {}
if rpms:
- map(self.add, rpms)
+ for rpm in rpms:
+ self.add(rpm)
@classmethod
def gather_from_dir(cls, rpm_file, all_rpms=None):
@@ -517,8 +522,7 @@ class RPMCollection(object):
def rpms_iter(self, arches=None, default_behavior=True):
"""Iterator of RPMs to go through RPMs with specific arches"""
- arches = self.rpms.keys()
- arches.sort()
+ arches = sorted(self.rpms.keys())
for arch in arches:
for _rpm in self.rpms[arch]:
@@ -561,7 +565,7 @@ class RPMCollection(object):
if r.is_debuginfo:
result.append(r)
return result
-
+
def generate_comparison_halves(rpm_col1, rpm_col2):
"""Iterate RPM collection and peer's to generate comparison halves"""
@@ -739,7 +743,7 @@ class Brew(object):
raise TypeError(
'{0} is not a callable object.'.format(str(selector)))
- if order_by is not None and not isinstance(order_by, basestring):
+ if order_by is not None and not isinstance(order_by, six.string_types):
raise TypeError('order_by {0} is invalid.'.format(order_by))
builds = self.session.listBuilds(packageID=packageID, state=state)
@@ -748,11 +752,16 @@ class Brew(object):
if order_by is not None:
# FIXME: is it possible to sort builds by using opts parameter of
# listBuilds
- cmp_func = cmp_nvr if order_by == 'nvr' else None
- builds = sorted(builds,
- key=lambda item: item[order_by],
- cmp=cmp_func,
- reverse=reverse)
+ if order_by == 'nvr':
+ if six.PY2:
+ builds = sorted(builds, cmp=cmp_nvr, reverse=reverse)
+ else:
+ builds = sorted(builds,
+ key=functools.cmp_to_key(cmp_nvr),
+ reverse=reverse)
+ else:
+ builds = sorted(
+ builds, key=lambda b: b[order_by], reverse=reverse)
if topone:
builds = builds[0:1]
@@ -974,7 +983,7 @@ def download_rpm(url):
url, os.path.join(get_download_dir(),
os.path.basename(url)))
if global_config.dry_run:
- print 'DRY-RUN:', cmd
+ print('DRY-RUN: {0}'.format(cmd))
return
return_code = subprocess.call(cmd, shell=True)
@@ -997,7 +1006,8 @@ def download_rpms(rpms):
logger.debug('Download %s', rpm.download_url)
download_rpm(rpm.download_url)
- map(_download, rpms)
+ for rpm in rpms:
+ _download(rpm)
@log_call
@@ -1019,7 +1029,7 @@ def build_path_to_abipkgdiff():
def format_debug_info_pkg_options(option, debuginfo_list):
"""Given a list of debug info package descriptors return an option
string that looks like:
-
+
option dbg.rpm1 option dbgrpm2 ...
:param: list debuginfo_list a list of instances of the RPM class
@@ -1121,20 +1131,21 @@ def abipkgdiff(cmp_half1, cmp_half2):
cmp_half1.subject.downloaded_file,
cmp_half2.subject.downloaded_file,
]
- cmd = filter(lambda s: s != '', cmd)
+ cmd = [s for s in cmd if s != '']
if global_config.dry_run:
- print 'DRY-RUN:', ' '.join(cmd)
+ print('DRY-RUN: {0}'.format(' '.join(cmd)))
return
logger.debug('Run: %s', ' '.join(cmd))
- print 'Comparing the ABI of binaries between {0} and {1}:'.format(
- cmp_half1.subject.filename, cmp_half2.subject.filename)
- print
+ print('Comparing the ABI of binaries between {0} and {1}:'.format(
+ cmp_half1.subject.filename, cmp_half2.subject.filename))
+ print()
proc = subprocess.Popen(' '.join(cmd), shell=True,
- stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+ stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+ universal_newlines=True)
# So we could have done: stdout, stderr = proc.communicate()
# But then the documentatin of proc.communicate says:
#
@@ -1160,9 +1171,9 @@ def abipkgdiff(cmp_half1, cmp_half2):
has_abi_change = proc.returncode & ABIDIFF_ABI_CHANGE
if is_internal_error:
- print >>sys.stderr, stderr
+ six.print_(stderr, file=sys.stderr)
elif is_ok or has_abi_change:
- print stdout
+ print(stdout)
return proc.returncode
@@ -1555,7 +1566,7 @@ def main():
if both_nvr or both_nvra:
return diff_two_nvras_from_koji()
- print >>sys.stderr, 'Unknown arguments. Please refer to --help.'
+ six.print_('Unknown arguments. Please refer to --help.', file=sys.stderr)
return 1
@@ -1568,7 +1579,7 @@ if __name__ == '__main__':
if global_config.debug:
logger.debug('Terminate by user')
else:
- print >>sys.stderr, 'Terminate by user'
+ six.print_('Terminate by user', file=sys.stderr)
if global_config.show_traceback:
raise
else:
@@ -1579,7 +1590,7 @@ if __name__ == '__main__':
if global_config.debug:
logger.debug(str(e))
else:
- print >>sys.stderr, str(e)
+ six.print_(str(e), file=sys.stderr)
if global_config.show_traceback:
raise
else:
--
2.14.3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Bug 22722 - Make fedabidiff and its tests support both python 3 and 2
2018-01-01 0:00 [PATCH] Bug 22722 - Make fedabidiff and its tests support both python 3 and 2 cqi
@ 2018-01-01 0:00 ` Dodji Seketeli
2018-01-01 0:00 ` Chenxiong Qi
0 siblings, 1 reply; 4+ messages in thread
From: Dodji Seketeli @ 2018-01-01 0:00 UTC (permalink / raw)
To: cqi; +Cc: libabigail
Hello Chenxiong,
Thank you very much for putting this patch together, it's really
appreciated.
I have a few comments about the patch though, please find them below.
[...]
> diff --git a/configure.ac b/configure.ac
> index 4963ee5..9bc92f2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -97,6 +97,12 @@ AC_ARG_ENABLE([fedabipkgdiff],
> ENABLE_FEDABIPKGDIFF=$enableval,
> ENABLE_FEDABIPKGDIFF=auto)
>
> +AC_ARG_ENABLE([py3-tests],
> + AS_HELP_STRING([--disable-py3-tests=yes|no|auto],
> + [disable running tests with Python 3]),
What AC_ARG_ENABLE does here is to define both --enable-py3-test *and*
--disable-py3-test options. So, I'd prefer that in the help string we
refer to the positive option, which is --enable-py3-test. That
positive option is of course "applied" by default in this case.
> + DISABLE_PY3_TESTS=$enableval,
> + DISABLE_PY3_TESTS=auto)
So here, I'd prefer that we use a "positive" variable name, which
would be ENABLE_PY3_TESTS, rather than the DISABLE_PY3_TESTS that you
are using. This would be just like what is done above for the other
options.
> +
> dnl *************************************************
> dnl check for dependencies
> dnl *************************************************
> @@ -351,7 +357,7 @@ if test x$CHECK_DEPS_FOR_FEDABIPKGDIFF = xyes; then
>
> REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF="\
> argparse logging os re subprocess sys urlparse \
> - xdg koji mock rpm imp tempfile mimetypes shutil"
> + xdg koji mock rpm imp tempfile mimetypes shutil six"
Just for my own education, is the python six package available on el6
systems?
>
> if test x$ENABLE_FEDABIPKGDIFF != xno; then
> AX_CHECK_PYTHON_MODULES([$REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF],
> @@ -399,6 +405,14 @@ fi
>
> AM_CONDITIONAL(ENABLE_FEDABIPKGDIFF, test x$ENABLE_FEDABIPKGDIFF = xyes)
>
> +
> +AC_MSG_NOTICE(Run tests with Python 3... $DISABLE_PY3_TESTS)
> +if test x$DISABLE_PY3_TESTS = xauto -o x$DISABLE_PY3_TESTS = xyes; then
> + ENABLE_PY3_TESTS=yes
Hmmh. I disagree with the logic here.
I think the logic should be that if we are in 'auto' mode and if
python3 is available, then we should enable the python3 tests. If we
are in auto mode and if python3 is not available, then we should *NOT*
enable the python3 tests. We should thus assume python2 mode.
If on the other hand the user chose to explicitly disable or enable
the python3 mode (i.e, not choosing 'auto') then we should disable or
enable python3 as per the user request.
[...]
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 0a7f4b9..9b31e98 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -20,6 +20,9 @@ FEDABIPKGDIFF_TEST =
> if ENABLE_FEDABIPKGDIFF
> FEDABIPKGDIFF_TEST += runtestfedabipkgdiff.py
> endif
> +if ENABLE_PY3_TESTS
> +FEDABIPKGDIFF_TEST += runtestfedabipkgdiff-py3
> +endif
I think this should be included in the body of the "if
ENABLE_FEDABIPKGDIFF" clause above.
>
> TESTS= \
> $(FEDABIPKGDIFF_TEST) \
> @@ -43,10 +46,15 @@ runtestabidiffexit \
> runtestdefaultsupprs.py \
> $(CXX11_TESTS)
>
> +if ENABLE_PY3_TESTS
> +TESTS += runtestdefaultsupprs-py3
> +endif
> +
I think this is unnecessary if my previous comment is right.
[...]
> diff --git a/tests/mockfedabipkgdiff.in b/tests/mockfedabipkgdiff.in
> index aac99d1..1168b92 100644
> --- a/tests/mockfedabipkgdiff.in
> +++ b/tests/mockfedabipkgdiff.in
[...]
> {'build_id': 600011,
> - 'name': 'vte291', 'version': '0.39.1', 'release': '1.fc22',
> + 'name': 'vte291', 'version': '0.39.1', 'release': '1.fc22',
I believe these are (white space) changes that are unrelated to the purpose of
the patch, which is to move to python3. I am correct?
If so, please file a separate patch that contains these white space
changes. There are plenty of hunks following this one, which similar
white space changes, please include them in that white-space/style
patch.
[...]
> diff --git a/tests/runtestdefaultsupprs.py.in b/tests/runtestdefaultsupprs.py.in
> index 41b0b28..9feafe7 100644
> --- a/tests/runtestdefaultsupprs.py.in
> +++ b/tests/runtestdefaultsupprs.py.in
> @@ -67,7 +67,8 @@ def ensure_output_dir_created():
> pass
>
> if not os.path.exists(output_dir):
> - sys.exit(1);
> + sys.exit(1)
> +
Just like I said above concerning the white space changes, I think
this change would be better suited to a separate "style-only" patch.
There are similar other changes after this one that should be included
in that separate patch as well.
[...]
Thanks!
--
Dodji
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-29 12:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-01 0:00 [PATCH] Bug 22722 - Make fedabidiff and its tests support both python 3 and 2 cqi
2018-01-01 0:00 ` Dodji Seketeli
2018-01-01 0:00 ` Chenxiong Qi
2018-01-01 0:00 ` Dodji Seketeli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).