[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Bug 20087 - Clean cache before or after ABI comparison
Hi Dodji,
Thanks for your comment. Patch is updated. Please review.
On Thu, Mar 9, 2017 at 7:05 PM, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello,
>
> cqi@redhat.com a écrit:
>
> > +def clean_cache(func):
> > + """Clean cache according to the specified options from command line
> > +
> > + This is a decorator. Use it just as any other Python decorator.
> > +
> > + Currently, cache contains downloaded RPM packages from Koji. This decorator
> > + is able to clean cache before or after ABI comparison, or both.
> > +
> > + :param func: the method to compare ABI against select RPM packages.
> > + :type func: callable object.
> > + """
> > + @log_call
> > + def delete_caches():
> > + download_dir = get_download_dir()
> > + if global_config.dry_run:
> > + print 'DRY-RUN: Delete cached downloaded RPM packages at {0}'.format(download_dir)
> > + else:
> > + logger.debug('Delete cached downloaded RPM packages at {0}'.format(download_dir))
> > + shutil.rmtree(download_dir)
> > +
> > + def wrapper(*args, **kwargs):
> > + if global_config.clean_cache_before:
> > + delete_caches()
> > + result = func(*args, **kwargs)
> > + if global_config.clean_cache_after:
> > + delete_caches()
> > + return result
> > +
> > + return wrapper
> > +
> > +
> > class RPM(object):
> > """Wrapper of RPM representing a RPM got from Koji
> >
> > @@ -1034,6 +1074,7 @@ def run_abipkgdiff(rpm_col1, rpm_col2):
> > return max(return_codes)
> >
> >
> > +@clean_cache
> > @log_call
> > def diff_local_rpm_with_latest_rpm_from_koji():
> > """Diff against local rpm and remove latest rpm
> > @@ -1067,6 +1108,7 @@ def diff_local_rpm_with_latest_rpm_from_koji():
> > return run_abipkgdiff(rpm_col1, rpm_col2)
>
> I understand this whole decorator business and I think it can be really
> handy. For instance, I agree with the use of the @log_call decorator as
> it adds logging to the function which can be seen as a mere plumbing
> functionality.
>
> But for cleaning the cache, I don't like the use of a decorator here.
>
> Rather, I'd prefer that in both diff_local_rpm_with_latest_rpm_from_koji
> and diff_latest_rpms_based_on_distros we *repeat* the code snippet:
>
> if global_config.clean_cache_before:
> delete_caches()
>
> and
>
> if global_config.clean_cache_after:
> delete_caches()
>
> I think that it helps to understand the code we read better, without
> having assumptions about decorators in our heads; one such assumptions
> to have would be for instance a reply to the questions "what happens
> when that function has several decorators? How are the those decorators
> ordered; How do they interact?". I'd rather not force all readers of
> the code to have to know about these things to understand how handling
> the cache interacts with comparing the ABI of packages.
>
>
> And yes, some would say "but grrrr! Repeating code like that is bad!".
> And I'll reply that in this particular case, we are gaining an increased
> maintainability through an increased clarity. And my bias is towards
> clarity, unless something more important is at play.
>
> Cheers,
>
>
>
> --
> Dodji
From c629c15570e569c7fcf305a423524cc2803f867e Mon Sep 17 00:00:00 2001
From: Chenxiong Qi <cqi@redhat.com>
Date: Sun, 5 Mar 2017 13:54:16 +0800
Subject: [PATCH] Bug 20087 - Clean cache before or after ABI comparison
Cache data, currently containing downloaded RPM packages from Koji, is
stored in XDG_CACHE_HOME. This patch allows user to delete cache before
or after the ABI comparison, or both.
* configure.ac: Require shutil module.
* doc/manuals/fedabipkgdiff.rst: Add document for new option
clean-cache, clean-cache-before, and clean-cache-after.
* tools/fedabipkgdiff (build_commandline_args_parser): Add new
option --clean-cache, --clean-cache-before and
--clean-cache-after.
(diff_local_rpm_with_latest_rpm_from_koji): Delete download
cache directory before or after downloading RPMs.
(diff_latest_rpms_based_on_distros): Likewise.
(diff_two_nvras_from_koji): Likewise.
(diff_from_two_rpm_files): Likewise.
* bash-completion/fedabipkgdiff: Add new options.
* tests/mockfedabipkgdiff.in (get_download_dir): Rewrite to
behave just like the original get_download_dir.
(mock_get_download_dir): Removed.
(DOWNLOAD_CACHE_DIR): New global variable pointing directory
holding packages during tests.
(run_fedabipkgdiff): Mock original get_download_dir with the
rewrite get_download_dir.
* tests/runtestfedabipkgdiff.py.in (run_fedabipkgdiff_tests):
Add --clean-cache to run tests to ensure no regression.
Signed-off-by: Chenxiong Qi <cqi@redhat.com>
---
bash-completion/fedabipkgdiff | 5 ++-
configure.ac | 2 +-
doc/manuals/fedabipkgdiff.rst | 14 ++++++++
tests/mockfedabipkgdiff.in | 28 +++++++--------
tests/runtestfedabipkgdiff.py.in | 4 ++-
tools/fedabipkgdiff | 76 +++++++++++++++++++++++++++++++++++++---
6 files changed, 106 insertions(+), 23 deletions(-)
diff --git a/bash-completion/fedabipkgdiff b/bash-completion/fedabipkgdiff
index c01a2ae..789d990 100644
--- a/bash-completion/fedabipkgdiff
+++ b/bash-completion/fedabipkgdiff
@@ -16,7 +16,10 @@ _fedabipkgdiff_module()
--dso-only
--no-default-suppression
--no-devel-pkg
- --abipkgdiff"
+ --abipkgdiff
+ --clean-cache
+ --clean-cache-before
+ --clean-cache-after"
COMPREPLY=( $(compgen -W "${OPTS[*]}" -- $cur) )
return 0
;;
diff --git a/configure.ac b/configure.ac
index 882c652..bf3ff20 100644
--- a/configure.ac
+++ b/configure.ac
@@ -338,7 +338,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"
+ xdg koji mock rpm imp tempfile mimetypes shutil"
if test x$ENABLE_FEDABIPKGDIFF != xno; then
AX_CHECK_PYTHON_MODULES([$REQUIRED_PYTHON_MODULES_FOR_FEDABIPKGDIFF],
diff --git a/doc/manuals/fedabipkgdiff.rst b/doc/manuals/fedabipkgdiff.rst
index 7656410..c585266 100644
--- a/doc/manuals/fedabipkgdiff.rst
+++ b/doc/manuals/fedabipkgdiff.rst
@@ -142,6 +142,20 @@ Options
Specify an alternative abipkgdiff instead of the one installed in system.
+ * ``--clean-cache-before``
+
+ Clean cache before ABI comparison.
+
+ * ``--clean-cache-after``
+
+ Clean cache after ABI comparison.
+
+ * ``--clean-cache``
+
+ If you want to clean cache both before and after ABI comparison,
+ ``--clean-cache`` is the convenient way for you to save typing of two
+ options at same time.
+
.. _build:
Note that a build is a specific version and release of an RPM package.
diff --git a/tests/mockfedabipkgdiff.in b/tests/mockfedabipkgdiff.in
index 48a1866..aac99d1 100644
--- a/tests/mockfedabipkgdiff.in
+++ b/tests/mockfedabipkgdiff.in
@@ -68,24 +68,18 @@ ABIPKGDIFF = '@abs_top_builddir@/tools/abipkgdiff'
FEDABIPKGDIFF = '@abs_top_srcdir@/tools/fedabipkgdiff'
INPUT_DIR = '@abs_top_srcdir@/tests/data/test-fedabipkgdiff'
OUTPUT_DIR = '@abs_top_builddir@/tests/output/test-fedabipkgdiff'
-DOWNLOAD_CACHE_DIR_PREFIX=os.path.join(OUTPUT_DIR, 'download-cache')
TEST_TOPDIR = 'file://{0}'.format(INPUT_DIR)
-def get_download_dir():
-
- if not os.path.exists(DOWNLOAD_CACHE_DIR_PREFIX):
- try:
- os.makedirs(DOWNLOAD_CACHE_DIR_PREFIX)
- except:
- pass
- if not os.path.exists(DOWNLOAD_CACHE_DIR_PREFIX):
- sys.exit(1);
-
- return tempfile.mkdtemp(dir=DOWNLOAD_CACHE_DIR_PREFIX)
+DOWNLOAD_CACHE_DIR_PREFIX=os.path.join(OUTPUT_DIR, 'download-cache')
+if not os.path.exists(DOWNLOAD_CACHE_DIR_PREFIX):
+ os.makedirs(DOWNLOAD_CACHE_DIR_PREFIX)
+DOWNLOAD_CACHE_DIR=tempfile.mkdtemp(dir=DOWNLOAD_CACHE_DIR_PREFIX)
-DOWNLOAD_CACHE_DIR = get_download_dir()
-def mock_get_download_dir():
+def get_download_dir():
+ """Mock get_download_dir"""
+ if not os.path.exists(DOWNLOAD_CACHE_DIR):
+ os.makedirs(DOWNLOAD_CACHE_DIR)
return DOWNLOAD_CACHE_DIR
@@ -399,12 +393,14 @@ class MockClientSession(object):
@patch('koji.ClientSession', new=MockClientSession)
@patch('fedabipkgdiff.DEFAULT_KOJI_TOPURL', new=TEST_TOPDIR)
@patch('fedabipkgdiff.DEFAULT_ABIPKGDIFF', new=ABIPKGDIFF)
-@patch('fedabipkgdiff.get_download_dir', side_effect=mock_get_download_dir)
-def run_fedabipkgdiff(get_download_dir):
+@patch('fedabipkgdiff.get_download_dir', new=get_download_dir)
+def run_fedabipkgdiff():
return fedabipkgdiff_mod.main()
+
def do_main():
run_fedabipkgdiff()
+
if __name__ == '__main__':
do_main()
diff --git a/tests/runtestfedabipkgdiff.py.in b/tests/runtestfedabipkgdiff.py.in
index 5b10a71..0ac054a 100755
--- a/tests/runtestfedabipkgdiff.py.in
+++ b/tests/runtestfedabipkgdiff.py.in
@@ -129,7 +129,8 @@ def run_fedabipkgdiff_tests():
output_path = os.path.join(TEST_BUILD_DIR, output_path)
cmd = [FEDABIPKGDIFF,
'--no-default-suppression',
- '--show-identical-binaries'] + args
+ '--show-identical-binaries',
+ '--clean-cache'] + args
with open(output_path, 'w') as out_file:
subprocess.call(cmd, stdout=out_file)
@@ -163,6 +164,7 @@ def main():
result = run_fedabipkgdiff_tests()
return not result
+
if __name__ == '__main__':
exit_code = main()
sys.exit(exit_code)
diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
index f9504f4..8217070 100755
--- a/tools/fedabipkgdiff
+++ b/tools/fedabipkgdiff
@@ -28,6 +28,7 @@ import logging
import mimetypes
import os
import re
+import shutil
import subprocess
import sys
@@ -151,6 +152,14 @@ class CannotFindLatestBuildError(Exception):
"""Cannot find latest build from a package"""
+class SetCleanCacheAction(argparse._StoreTrueAction):
+ """Custom Action making clean-cache as bundle of clean-cache-before and clean-cache-after"""
+
+ def __call__(self, parser, namespace, values, option_string=None):
+ setattr(namespace, 'clean_cache_before', self.const)
+ setattr(namespace, 'clean_cache_after', self.const)
+
+
def is_distro_valid(distro):
"""Adjust if a distro is valid
@@ -223,6 +232,16 @@ def log_call(func):
return proxy
+def delete_download_cache():
+ """Delete download cache directory"""
+ download_dir = get_download_dir()
+ if global_config.dry_run:
+ print('DRY-RUN: Delete cached downloaded RPM packages at {0}'.format(download_dir))
+ else:
+ logger.debug('Delete cached downloaded RPM packages at {0}'.format(download_dir))
+ shutil.rmtree(download_dir)
+
+
class RPM(object):
"""Wrapper of RPM representing a RPM got from Koji
@@ -1063,8 +1082,16 @@ def diff_local_rpm_with_latest_rpm_from_koji():
arches=local_rpm.arch)
rpm_col2 = RPMCollection.gather_from_dir(local_rpm_file)
+ if global_config.clean_cache_before:
+ delete_download_cache()
+
download_rpms(rpm_col1.rpms_iter())
- return run_abipkgdiff(rpm_col1, rpm_col2)
+ result = run_abipkgdiff(rpm_col1, rpm_col2)
+
+ if global_config.clean_cache_after:
+ delete_download_cache()
+
+ return result
@log_call
@@ -1093,9 +1120,16 @@ def diff_latest_rpms_based_on_distros():
rpm_col2 = session.get_latest_built_rpms(package_name,
distro=global_config.to_distro)
+ if global_config.clean_cache_before:
+ delete_download_cache()
+
download_rpms(chain(rpm_col1.rpms_iter(), rpm_col2.rpms_iter()))
+ result = run_abipkgdiff(rpm_col1, rpm_col2)
- return run_abipkgdiff(rpm_col1, rpm_col2)
+ if global_config.clean_cache_after:
+ delete_download_cache()
+
+ return result
@log_call
@@ -1141,9 +1175,16 @@ def diff_two_nvras_from_koji():
build_id, params2[0], arches=params2[3],
select_subpackages=global_config.check_all_subpackages)
+ if global_config.clean_cache_before:
+ delete_download_cache()
+
download_rpms(chain(rpm_col1.rpms_iter(), rpm_col2.rpms_iter()))
+ result = run_abipkgdiff(rpm_col1, rpm_col2)
+
+ if global_config.clean_cache_after:
+ delete_download_cache()
- return run_abipkgdiff(rpm_col1, rpm_col2)
+ return result
@log_call
@@ -1151,8 +1192,13 @@ def diff_from_two_rpm_files(from_rpm_file, to_rpm_file):
"""Diff two RPM files"""
rpm_col1 = RPMCollection.gather_from_dir(from_rpm_file)
rpm_col2 = RPMCollection.gather_from_dir(to_rpm_file)
+ if global_config.clean_cache_before:
+ delete_download_cache()
download_rpms(chain(rpm_col1.rpms_iter(), rpm_col2.rpms_iter()))
- return run_abipkgdiff(rpm_col1, rpm_col2)
+ result = run_abipkgdiff(rpm_col1, rpm_col2)
+ if global_config.clean_cache_after:
+ delete_download_cache()
+ return result
def build_commandline_args_parser():
@@ -1259,6 +1305,28 @@ def build_commandline_args_parser():
action='store_true',
dest='error_on_warning',
help='Raise error instead of warning')
+ parser.add_argument(
+ '--clean-cache',
+ required=False,
+ action=SetCleanCacheAction,
+ dest='clean_cache',
+ default=None,
+ help='A convenient way to clean cache without specifying '
+ '--clean-cache-before and --clean-cache-after at same time')
+ parser.add_argument(
+ '--clean-cache-before',
+ required=False,
+ action='store_true',
+ dest='clean_cache_before',
+ default=None,
+ help='Clean cache before ABI comparison')
+ parser.add_argument(
+ '--clean-cache-after',
+ required=False,
+ action='store_true',
+ dest='clean_cache_after',
+ default=None,
+ help='Clean cache after ABI comparison')
return parser
--
2.7.4