From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83861 invoked by alias); 31 Jan 2018 11:37:31 -0000 Mailing-List: contact libabigail-help@sourceware.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Id: List-Subscribe: Sender: libabigail-owner@sourceware.org Received: (qmail 83834 invoked by uid 89); 31 Jan 2018 11:37:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.99.2 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_NUMSUBJECT,SPF_PASS autolearn=ham version=3.3.2 spammy=suited X-Spam-Status: No, score=-24.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_NUMSUBJECT,SPF_PASS autolearn=ham version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sourceware.org X-Spam-Level: X-HELO: ms.seketeli.net Received: from seketeli.net (HELO ms.seketeli.net) (94.23.218.202) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 31 Jan 2018 11:37:23 +0000 Received: from localhost (torimasen.com [88.120.68.215]) by ms.seketeli.net (Postfix) with ESMTPSA id EF1BD236005C; Wed, 31 Jan 2018 12:37:20 +0100 (CET) Received: by localhost (Postfix, from userid 1000) id 85F9D5800FE; Wed, 31 Jan 2018 12:37:18 +0100 (CET) From: Dodji Seketeli To: cqi@redhat.com Cc: libabigail@sourceware.org Subject: Re: [PATCH] Bug 22722 - Make fedabidiff and its tests support both python 3 and 2 Organization: Me, myself and I References: <20180130135529.13076-1-cqi@redhat.com> X-Operating-System: Fedora 28 X-URL: http://www.seketeli.net/~dodji Date: Mon, 01 Jan 2018 00:00:00 -0000 In-Reply-To: <20180130135529.13076-1-cqi@redhat.com> (cqi@redhat.com's message of "Tue, 30 Jan 2018 21:55:29 +0800") Message-ID: <87k1vytg29.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-q1/txt/msg00011.txt.bz2 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