[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Bug 22722 - Make fedabidiff and its tests support both python 3 and 2
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