Re: a new tool fedabipkgdiff

[Dodji Seketeli <dodji@redhat.com>]

Hello Chenxiong,

[...]

> This time, the patch is updated
> 
> - more documentation inside fedabipkgdiff and runtestfedabipkgdiff.py.in
> - using curl instead of wget to download packages from Koji
> - a new manual (with Dodji's help, man page can be generated from this manual by sphnix)
> - rebased, on latest commit fa5a5acbbcb1e

Thank you for all these efforts!

So I have reviewed this updated patch, made some changes and committed
the result to the master branch of the git repository.  Woohoo :-)

Below are some comments about the parts of your patch where I made
some changes.  If you think we should amend them, please do not
hesitate to say it.

[...]

> diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff

[...]

> +import xdg.BaseDirectory

You are importing a new python module (xdg) that needs to be installed
otherwise the execution of the fedabipkgdiff program just halts here.
We thus need to ensure that the configure scripts detects when the
python module is not installed, and gives a hint to the user saying
that she needs to install it.  So I added this hunk:

    --- a/configure.ac
    +++ b/configure.ac
    @@ -288,6 +288,7 @@ if test x$CHECK_DEPS_FOR_FEDABIPKGDIFF = xyes; then
       AX_PYTHON_MODULE(itertools, $FATAL, python2)
       AX_PYTHON_MODULE(shutil, $FATAL, python2)
       AX_PYTHON_MODULE(unittest, $FATAL, python2)
    +  AX_PYTHON_MODULE(xdg, $FATAL, python2)
       AX_PYTHON_MODULE(koji, $FATAL, python2)
       AX_PYTHON_MODULE(mock, $FATAL, python2)
       ENABLE_FEDABIPKGDIFF=yes

[...]

> diff --git a/doc/manuals/Makefile.am b/doc/manuals/Makefile.am
> index f23d1ab..5128bc0 100644
> --- a/doc/manuals/Makefile.am
> +++ b/doc/manuals/Makefile.am
> @@ -10,7 +10,8 @@ conf.py \
>  index.rst \
>  libabigail-concepts.rst \
>  libabigail-overview.rst \
> -libabigail-tools.rst
> +libabigail-tools.rst \
> +fedabipkgdiff

Here there is no file named 'fedabipkgdiff' to be added to the set of
files that are doing to be distributed in the tarball that is
constructed by the 'make dist' command.

What you want is to add the file fedabipkgdiff.rst.  So I have added
this the hunk below that fixes that.

    diff --git a/doc/manuals/Makefile.am b/doc/manuals/Makefile.am
    index 5128bc0..573fb3b 100644
    --- a/doc/manuals/Makefile.am
    +++ b/doc/manuals/Makefile.am
    @@ -11,7 +11,7 @@ index.rst \
     libabigail-concepts.rst \
     libabigail-overview.rst \
     libabigail-tools.rst \
    -fedabipkgdiff
    +fedabipkgdiff.rst

Note that you could have detected the issue by typing the command:

    make distcheck

which checks that constructing the tarball works correctly.  That
'distcheck' target goes even further and tries to *compile* the
tarball that was successfully built and runs all the tests.  When
"make distcheck" works, it means the software is ready to be released
:-) We should always run make distcheck before submitting a patch, I
believe.

When looking at the tools/Makefile.am file I noticed that I forgot to
add the fedabipkgdiff to the set of files that needs to be
*distributed*.  It was just meant to be installed in the /usr/bin/
directory.  I have thus added the hunk below to fix that:

    diff --git a/tools/Makefile.am b/tools/Makefile.am
    index 0d96215..3e53eb1 100644
    --- a/tools/Makefile.am
    +++ b/tools/Makefile.am
    @@ -7,7 +7,7 @@ else
     endif

     if ENABLE_FEDABIPKGDIFF
    -  bin_SCRIPTS = fedabipkgdiff
    +  dist_bin_SCRIPTS = fedabipkgdiff
     else
       noinst_SCRIPTS = fedabipkgdiff
     endif

[...]

> diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff


> +HOME_DIR = os.path.join(xdg.BaseDirectory.xdg_data_home,
> +                        os.path.splitext(os.path.basename(__file__))[0])

As I said in my previous review, I prefer this to be XDG_CACHE_HOME,
rather than XDG_DATA_HOME.  The reason being that I consider the
downloaded packages as being cached data, rather than application
data.  So I changed this to XDG_CACHE_HOME.

The resulting hunk is:

    diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
    index e7c4785..f2b20be 100755
    --- a/tools/fedabipkgdiff
    +++ b/tools/fedabipkgdiff
    @@ -60,7 +60,7 @@ DEFAULT_KOJI_TOPDIR = 'https://kojipkgs.fedoraproject.org'
     # The working directory where to hold all data including downloaded RPM
     # packages Currently, it's not configurable and hardcode here. In the future
     # version of fedabipkgdiff, I'll make it configurable by users.
    -HOME_DIR = os.path.join(xdg.BaseDirectory.xdg_data_home,
    +HOME_DIR = os.path.join(xdg.BaseDirectory.xdg_cache_home,
			     os.path.splitext(os.path.basename(__file__))[0])

     # Used to construct abipkgdiff command line argument, package and
       associated

[...]

> +    def listBuilds(self, packageID, state=None, topone=None,
> +                   selector=None, order_by=None, reverse=None):

[...]

> +        builds = self.session.listBuilds(packageId=packageID, state=state)

Here, packageId should be packageID instead.  And this is causing a
runtime error.  So I fixed it.  The resulting hunk is:

@@ -417,7 +417,7 @@ class Brew(object):
         if order_by is not None and not isinstance(order_by, basestring):
             raise TypeError('order_by {0} is invalid.'.format(order_by))
 
-        builds = self.session.listBuilds(packageId=packageID, state=state)
+        builds = self.session.listBuilds(packageID=packageID, state=state)
         if selector is not None:
             builds = [build for build in builds if selector(build)]
         if order_by is not None:

[...]

> +def download_rpm(url):

[...]

> +    cmd = 'curl --silent -C - -O {} > {}'.format(
> +        url, os.path.join(get_download_dir(),
> +                          os.path.basename(url)))

The resulting rpm file named

    os.path.join(get_download_dir(), os.path.basename(url))

are empty here.  So the comparison performed by abipkgdiff is failing
on my system.  This is because when you use the -O option, curl
doesn't emit anything on standard output, at least on the curl 7.29.0
version that I am using on my system.

Also, I think the -C option is not critically useful here.

Furthermore, I noticed that this program tries to download packages
even when  the --dry-run option is used.  I think it's useful to avoid
downloading packages when --dry-run is turned on.

So I changed this using this hunk:

    @@ -621,9 +621,13 @@ def download_rpm(url):
	 :return: True if a RPM is downloaded successfully, False otherwise.
	 :rtype: bool
	 """
    -    cmd = 'curl --silent -C - -O {} > {}'.format(
    +    cmd = 'curl --silent {} -o {}'.format(
	     url, os.path.join(get_download_dir(),
			       os.path.basename(url)))
    +    if global_config.dry_run:
    +        print 'DRY-RUN:', cmd
    +        return
    +
	 return_code = subprocess.call(cmd, shell=True)
	 if return_code > 0:
	     logger.error('curl fails with returned code: %d.', return_code)

[...]

> +def abipkgdiff(pkg_info1, pkg_info2):

[...]

> +    cmd = 'abipkgdiff --d1 {0} --d2 {1} {2} {3}'.format(
> +        pkg_info1.debuginfo_package.downloaded_file,
> +        pkg_info2.debuginfo_package.downloaded_file,
> +        pkg_info1.package.downloaded_file,
> +        pkg_info2.package.downloaded_file)

So here fedabipkgdiff is comparing the ABI of *all* binaries in the
packages, including executable binaries.  By default I think it makes
more sense to only compare the ABI of shared libraries, not executable
binaries.  And it's faster too.  So I changed this by doing:

    @@ -672,7 +672,7 @@ def abipkgdiff(pkg_info1, pkg_info2):
	 :return: return code of underlying abipkgdiff execution.
	 :rtype: int
	 """
    -    cmd = 'abipkgdiff --d1 {0} --d2 {1} {2} {3}'.format(
    +    cmd = 'abipkgdiff --dso-only --d1 {0} --d2 {1} {2} {3}'.format(
	     pkg_info1.debuginfo_package.downloaded_file,
	     pkg_info2.debuginfo_package.downloaded_file,
	     pkg_info1.package.downloaded_file,

I made wording changes in the build_commandline_args_parser function.
The resulting hunk is:

    @@ -924,7 +928,7 @@ def diff_two_nvras_from_koji():

     def build_commandline_args_parser():
	 parser = argparse.ArgumentParser(
    -        description='Run abipkgdiff against RPM packages from koji')
    +        description='Compare ABI of shared libraries in RPM packages from the Koji build system')

	 parser.add_argument(
	     'NVR',
    @@ -936,20 +940,20 @@ def build_commandline_args_parser():
	     required=False,
	     dest='dry_run',
	     action='store_true',
    -        help='Don\'t actually run abipkgdiff. The commands that should be '
    +        help='Don\'t actually do the work. The commands that should be '
		  'run will be sent to stdout.')
	 parser.add_argument(
	     '--from',
	     required=False,
	     metavar='DISTRO',
	     dest='from_distro',
    -        help='baseline Fedora distro, for example, fc23')
    +        help='baseline Fedora distribution name, for example, fc23')
	 parser.add_argument(
	     '--to',
	     required=False,
	     metavar='DISTRO',
	     dest='to_distro',
    -        help='which Fedora distro to compare, for example, fc24')
    +        help='Fedora distribution name to compare against the baseline, for example, fc24')
	 parser.add_argument(
	     '-a',
	     '--all-subpackages',

After I started to use the global_config.dry_run member variable in
download_rpm, I had to update the unit tests that exercise that member
function in tests/runtestfedabipkgdiff.py.in.

To do so, I had to move the test class DownloadRPMTest to *after* the
class MockGlobalConfig is defined, so that member functions of
DownloadRPMTest can use the MockGlobalConfig, which is now needed by
the member functions of DownloadRPMTest that exercise the
download_rpm function.  Of course, I had to add a dry_run data member
to MockGlobalConfig and update the comment in there accordingly.

The resulting hunk is:

    diff --git a/tests/runtestfedabipkgdiff.py.in b/tests/runtestfedabipkgdiff.py.in
    index 84a4320..483cfdb 100755
    --- a/tests/runtestfedabipkgdiff.py.in
    +++ b/tests/runtestfedabipkgdiff.py.in
    @@ -410,64 +410,6 @@ class RunAbipkgdiffTest(unittest.TestCase):
						       self.pkg2_infos)
	     self.assertTrue(result > 0)

    -
    -class DownloadRPMTest(unittest.TestCase):
    -    """Test case for download_rpm
    -
    -    Download a remote file, which is a local file simulating a remote file with
    -    scheme file://, for example file:///tmp/a.txt, to download directory.
    -    """
    -
    -    def setUp(self):
    -        # Create a remote file for testing download of this file
    -        self.fd, self.remote_filename = tempfile.mkstemp(
    -            prefix=temp_file_or_dir_prefix)
    -        # Whatever the content is, this case does not care about. Close it
    -        # immediately.
    -        os.close(self.fd)
    -
    -        # Used as a fake download directory to mock get_download_dir method
    -        self.download_dir = tempfile.mkdtemp(prefix=temp_file_or_dir_prefix)
    -
    -    def tearDown(self):
    -        os.remove(self.remote_filename)
    -        shutil.rmtree(self.download_dir)
    -
    -    def make_remote_file_url(self):
    -        """Make URL of remote file that is used for downloading this file"""
    -        return 'file://{}'.format(self.remote_filename)
    -
    -    def make_nonexistent_remote_file_url(self):
    -        """Return URL to a non-existent remote file"""
    -        return os.path.join(self.make_remote_file_url(), 'nonexistent-file')
    -
    -    @patch('__main__.fedabipkgdiff_mod.get_download_dir')
    -    def test_succeed_to_download_a_rpm(self, mock_get_download_dir):
    -        """Enusre True is returned if curl succeeds to download remote file
    -
    -        Download remote file to a fake download directory. Ensure everything is
    -        okay, and return value from download_rpm should be truth.
    -        """
    -        mock_get_download_dir.return_value = self.download_dir
    -
    -        url = self.make_remote_file_url()
    -        ret = fedabipkgdiff_mod.download_rpm(url)
    -        self.assertTrue(ret)
    -
    -    @patch('__main__.fedabipkgdiff_mod.get_download_dir')
    -    def test_failed_to_download_a_rpm(self, mock_get_download_dir):
    -        """Ensure False is returned if curl fails to download remote file
    -
    -        Download remote file to a fake download directory. But, making
    -        something wrong to cause download_rpm returns false.
    -        """
    -        mock_get_download_dir.return_value = self.download_dir
    -
    -        url = self.make_nonexistent_remote_file_url()
    -        ret = fedabipkgdiff_mod.download_rpm(url)
    -        self.assertFalse(ret)
    -
    -
     class MockGlobalConfig(object):
	 """Used to mock global_config

    @@ -475,11 +417,15 @@ class MockGlobalConfig(object):
	 helpful for tests to contain all potential parsed (simulated)
	 options.

    -    Currently, only koji_server is required for running tests. If any new test
    -    cases need others, please add them add as class attribute directly.
    +    Currently, only koji_server and dry_run are required for running
    +    tests. If any new test cases need others, please add them add as
    +    class attribute directly.
    +
	 """
	 koji_server = fedabipkgdiff_mod.DEFAULT_KOJI_SERVER

    +    dry_run = False
    +

     def mock_get_session():
	 """Used to mock get_session method to get mocked KojiSession instance"""
    @@ -608,6 +554,64 @@ class GetPackageLatestBuildTest(unittest.TestCase):
			       session.get_package_latest_build, 'httpd', 'xxxx')


    +class DownloadRPMTest(unittest.TestCase):
    +    """Test case for download_rpm
    +
    +    Download a remote file, which is a local file simulating a remote file with
    +    scheme file://, for example file:///tmp/a.txt, to download directory.
    +    """
    +
    +    def setUp(self):
    +        # Create a remote file for testing download of this file
    +        self.fd, self.remote_filename = tempfile.mkstemp(
    +            prefix=temp_file_or_dir_prefix)
    +        # Whatever the content is, this case does not care about. Close it
    +        # immediately.
    +        os.close(self.fd)
    +
    +        # Used as a fake download directory to mock get_download_dir method
    +        self.download_dir = tempfile.mkdtemp(prefix=temp_file_or_dir_prefix)
    +
    +    def tearDown(self):
    +        os.remove(self.remote_filename)
    +        shutil.rmtree(self.download_dir)
    +
    +    def make_remote_file_url(self):
    +        """Make URL of remote file that is used for downloading this file"""
    +        return 'file://{}'.format(self.remote_filename)
    +
    +    def make_nonexistent_remote_file_url(self):
    +        """Return URL to a non-existent remote file"""
    +        return os.path.join(self.make_remote_file_url(), 'nonexistent-file')
    +
    +    @patch('fedabidiff.global_config', new=MockGlobalConfig)
    +    @patch('__main__.fedabipkgdiff_mod.get_download_dir')
    +    def test_succeed_to_download_a_rpm(self, mock_get_download_dir):
    +        """Enusre True is returned if curl succeeds to download remote file
    +
    +        Download remote file to a fake download directory. Ensure everything is
    +        okay, and return value from download_rpm should be truth.
    +        """
    +        mock_get_download_dir.return_value = self.download_dir
    +
    +        url = self.make_remote_file_url()
    +        ret = fedabipkgdiff_mod.download_rpm(url)
    +        self.assertTrue(ret)
    +
    +    @patch('fedabidiff.global_config', new=MockGlobalConfig)
    +    @patch('__main__.fedabipkgdiff_mod.get_download_dir')
    +    def test_failed_to_download_a_rpm(self, mock_get_download_dir):
    +        """Ensure False is returned if curl fails to download remote file
    +
    +        Download remote file to a fake download directory. But, making
    +        something wrong to cause download_rpm returns false.
    +        """
    +        mock_get_download_dir.return_value = self.download_dir
    +
    +        url = self.make_nonexistent_remote_file_url()
    +        ret = fedabipkgdiff_mod.download_rpm(url)
    +        self.assertFalse(ret)
    +
     class BrewListRPMsTest(unittest.TestCase):
	 """Test case for Brew.listRPMs"""

So I applied the resulting patch to
https://sourceware.org/git/gitweb.cgi?p=libabigail.git;a=commit;h=57dcfb18f5599a5ec7d9faa8ad2ced33556ac4bd.

Thanks for your hard work!

Cheers,


-- 
		Dodji