Drop whipper caching (#336)

Whipper's caching implementation causes a few issues (#196, #230, [#321 (comment)](https://github.com/whipper-team/whipper/pull/321#issuecomment-437588821)) and complicates the code: it's better to drop this feature.

The rip resume feature doesn't work anymore: if possible it will be restored in the future.

* Remove caching item from TODO
* Delete unneeded files related to caching
* Update 'common/directory.py' & 'test/test_common_directory.py' (caching removal)
* Update 'common/accurip.py' & 'test/test_common_accurip.py' (caching removal)
* Update 'common/program.py' (caching removal)
* Update 'command/cd.py' (caching removal)

This fixes #335, fixes #196 and fixes #230.

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>

TODO

@@ -44,8 +44,6 @@ MEDIUM
 
 - retry cdrdao a few times when it had to load the tray
 
-- getting cache results should depend on same drive/offset
-
 - do some character mangling so trail of dead is not in a hidden dir
 
 HARD

whipper/command/cd.py

@@ -99,7 +99,6 @@ class _CD(BaseCommand):
         self.ittoc = self.program.getFastToc(self.runner, self.device)
 
         # already show us some info based on this
-        self.program.getRipResult(self.ittoc.getCDDBDiscId())
         print("CDDB disc id: %s" % self.ittoc.getCDDBDiscId())
         self.mbdiscid = self.ittoc.getMusicBrainzDiscId()
         print("MusicBrainz disc id %s" % self.mbdiscid)
@@ -499,8 +498,6 @@ Log files will log the path to tracks relative to this directory.
                 self.itable.setFile(number, 1, trackResult.filename,
                                     self.itable.getTrackLength(number), number)
 
-            self.program.saveRipResult()
-
         # check for hidden track one audio
         htoa = self.program.getHTOA()
         if htoa:
@@ -541,8 +538,6 @@ Log files will log the path to tracks relative to this directory.
 
         accurip.print_report(self.program.result)
 
-        self.program.saveRipResult()
-
         self.program.writeLog(discName, self.logger)
 
 

whipper/common/accurip.py

@@ -21,12 +21,9 @@
 
 import struct
 import whipper
-from os import makedirs
-from os.path import dirname, exists, join
 from urllib.error import URLError, HTTPError
 from urllib.request import urlopen, Request
 
-from whipper.common import directory
 from whipper.program.arc import accuraterip_checksum
 
 import logging
@@ -34,7 +31,6 @@ logger = logging.getLogger(__name__)
 
 
 ACCURATERIP_URL = "http://www.accuraterip.com/accuraterip/"
-_CACHE_DIR = join(directory.cache_path(), 'accurip')
 
 
 class EntryNotFound(Exception):
@@ -142,34 +138,13 @@ def _download_entry(path):
         logger.error('error retrieving AccurateRip entry: %s', e)
 
 
-def _save_entry(raw_entry, path):
-    logger.debug('saving AccurateRip entry to %s', path)
-    try:
-        makedirs(dirname(path), exist_ok=True)
-    except OSError as e:
-        logger.error('could not save entry to %s: %s', path, e)
-        return
-    with open(path, 'wb') as f:
-        f.write(raw_entry)
-
-
 def get_db_entry(path):
     """
-    Retrieve cached AccurateRip disc entry as array of _AccurateRipResponses.
+    Download entry from accuraterip.com.
-
-    Downloads entry from accuraterip.com on cache fault.
 
     ``path`` is in the format of the output of ``table.accuraterip_path()``.
     """
-    cached_path = join(_CACHE_DIR, path)
+    raw_entry = _download_entry(path)
-    if exists(cached_path):
-        logger.debug('found accuraterip entry at %s', cached_path)
-        with open(cached_path, 'rb') as f:
-            raw_entry = f.read()
-    else:
-        raw_entry = _download_entry(path)
-        if raw_entry:
-            _save_entry(raw_entry, cached_path)
     if not raw_entry:
         logger.warning('entry not found in AccurateRip database')
         raise EntryNotFound

whipper/common/cache.py

@@ -1,218 +0,0 @@
-# -*- Mode: Python; test-case-name: whipper.test.test_common_cache -*-
-# vi:si:et:sw=4:sts=4:ts=4
-
-# Copyright (C) 2009 Thomas Vander Stichele
-
-# This file is part of whipper.
-#
-# whipper is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# whipper is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with whipper.  If not, see <http://www.gnu.org/licenses/>.
-
-import os
-import os.path
-import glob
-import tempfile
-import shutil
-
-from whipper.result import result
-from whipper.common import directory
-
-import logging
-logger = logging.getLogger(__name__)
-
-
-class Persister:
-    """
-    I wrap an optional pickle to persist an object to disk.
-
-    Instantiate me with a path to automatically unpickle the object.
-    Call persist to store the object to disk; it will get stored if it
-    changed from the on-disk object.
-
-    :ivar object: the persistent object
-    """
-
-    def __init__(self, path=None, default=None):
-        """
-        If path is not given, the object will not be persisted.
-
-        This allows code to transparently deal with both persisted and
-        non-persisted objects, since the persist method will just end up
-        doing nothing.
-        """
-        self._path = path
-        self.object = None
-
-        self._unpickle(default)
-
-    def persist(self, obj=None):
-        """
-        Persist the given obj if we have a persist. path and the obj changed.
-
-        If object is not given, re-persist our object, always.
-        If object is given, only persist if it was changed.
-        """
-        # don't pickle if it's already ok
-        if obj and obj == self.object:
-            return
-
-        # store the object on ourselves if not None
-        if obj is not None:
-            self.object = obj
-
-        # don't pickle if there is no path
-        if not self._path:
-            return
-
-        # default to pickling our object again
-        if obj is None:
-            obj = self.object
-
-        # pickle
-        self.object = obj
-        (fd, path) = tempfile.mkstemp(suffix='.whipper.pickle')
-        handle = os.fdopen(fd, 'wb')
-        import pickle
-        pickle.dump(obj, handle, 2)
-        handle.close()
-        # do an atomic move
-        shutil.move(path, self._path)
-        logger.debug('saved persisted object to %r', self._path)
-
-    def _unpickle(self, default=None):
-        self.object = default
-
-        if not self._path:
-            return
-
-        if not os.path.exists(self._path):
-            return
-
-        with open(self._path, 'rb') as handle:
-            import pickle
-            try:
-                self.object = pickle.load(handle)
-                logger.debug('loaded persisted object from %r', self._path)
-            # FIXME: catching too general exception (Exception)
-            except Exception as e:
-                # can fail for various reasons; in that case, pretend we didn't
-                # load it
-                logger.debug(e)
-
-    def delete(self):
-        self.object = None
-        os.unlink(self._path)
-
-
-class PersistedCache:
-    """Wrap a directory of persisted objects."""
-
-    path = None
-
-    def __init__(self, path):
-        self.path = path
-        os.makedirs(self.path, exist_ok=True)
-
-    def _getPath(self, key):
-        return os.path.join(self.path, '%s.pickle' % key)
-
-    def get(self, key):
-        """Return the persister for the given key."""
-        persister = Persister(self._getPath(key))
-        if persister.object:
-            if hasattr(persister.object, 'instanceVersion'):
-                o = persister.object
-                if o.instanceVersion < o.__class__.classVersion:
-                    logger.debug('key %r persisted object version %d '
-                                 'is outdated', key, o.instanceVersion)
-                    persister.object = None
-        # FIXME: don't delete old objects atm
-        #             persister.delete()
-
-        return persister
-
-
-class ResultCache:
-
-    def __init__(self, path=None):
-        self._path = path or directory.cache_path('result')
-        self._pcache = PersistedCache(self._path)
-
-    def getRipResult(self, cddbdiscid, create=True):
-        """
-        Get the persistable RipResult either from our cache or ret. a new one.
-
-        The cached RipResult may come from an aborted rip.
-
-        :rtype: :any:`Persistable` for :any:`result.RipResult`
-        """
-        presult = self._pcache.get(cddbdiscid)
-
-        if not presult.object:
-            logger.debug('result for cddbdiscid %r not in cache', cddbdiscid)
-            if not create:
-                logger.debug('returning None')
-                return None
-
-            logger.debug('creating result')
-            presult.object = result.RipResult()
-            presult.persist(presult.object)
-        else:
-            logger.debug('result for cddbdiscid %r found in cache, reusing',
-                         cddbdiscid)
-
-        return presult
-
-    def getIds(self):
-        paths = glob.glob(os.path.join(self._path, '*.pickle'))
-
-        return [os.path.splitext(os.path.basename(path))[0] for path in paths]
-
-
-class TableCache:
-    """
-    Read and write entries to and from the cache of tables.
-
-    If no path is specified, the cache will write to the current cache
-    directory and read from all possible cache directories (to allow for
-    pre-0.2.1 cddbdiscid-keyed entries).
-    """
-
-    def __init__(self, path=None):
-        if not path:
-            self._path = directory.cache_path('table')
-        else:
-            self._path = path
-
-        self._pcache = PersistedCache(self._path)
-
-    def get(self, cddbdiscid, mbdiscid):
-        # Before 0.2.1, we only saved by cddbdiscid, and had collisions
-        # mbdiscid collisions are a lot less likely
-        ptable = self._pcache.get('mbdiscid.' + mbdiscid)
-
-        if not ptable.object:
-            ptable = self._pcache.get(cddbdiscid)
-            if ptable.object:
-                if ptable.object.getMusicBrainzDiscId() != mbdiscid:
-                    logger.debug('cached table is for different mb id %r',
-                                 ptable.object.getMusicBrainzDiscId())
-                ptable.object = None
-            else:
-                logger.debug('no valid cached table found for %r', cddbdiscid)
-
-        if not ptable.object:
-            # get an empty persistable from the writable location
-            ptable = self._pcache.get('mbdiscid.' + mbdiscid)
-
-        return ptable

whipper/common/directory.py

@@ -29,15 +29,6 @@ def config_path():
     return join(path, 'whipper.conf')
 
 
-def cache_path(name=None):
-    path = join(getenv('XDG_CACHE_HOME') or join(expanduser('~'), '.cache'),
-                'whipper')
-    if name:
-        path = join(path, name)
-    makedirs(path, exist_ok=True)
-    return path
-
-
 def data_path(name=None):
     path = join(getenv('XDG_DATA_HOME') or
                 join(expanduser('~'), '.local/share'),

whipper/common/program.py

@@ -27,7 +27,7 @@ import shutil
 import time
 
 from tempfile import NamedTemporaryFile
-from whipper.common import accurip, cache, checksum, common, mbngs, path
+from whipper.common import accurip, checksum, common, mbngs, path
 from whipper.program import cdrdao, cdparanoia
 from whipper.image import image
 from whipper.extern import freedb
@@ -64,7 +64,6 @@ class Program:
         :param record: whether to record results of API calls for playback
         """
         self._record = record
-        self._cache = cache.ResultCache()
         self._config = config
 
         d = {}
@@ -113,37 +112,19 @@ class Program:
     def getTable(self, runner, cddbdiscid, mbdiscid, device, offset,
                  toc_path):
         """
-        Retrieve the Table either from the cache or the drive.
+        Retrieve the Table from the drive.
 
         :rtype: table.Table
         """
-        tcache = cache.TableCache()
-        ptable = tcache.get(cddbdiscid, mbdiscid)
         itable = None
         tdict = {}
 
-        # Ignore old cache, since we do not know what offset it used.
+        t = cdrdao.ReadTOCTask(device, toc_path=toc_path)
-        if isinstance(ptable.object, dict):
+        t.description = "Reading table"
-            tdict = ptable.object
+        runner.run(t)
-
+        itable = t.toc.table
-            if offset in tdict:
+        tdict[offset] = itable
-                itable = tdict[offset]
+        logger.debug('getTable: read table %r', itable)
-
-        if not itable:
-            logger.debug('getTable: cddbdiscid %s, mbdiscid %s not in cache '
-                         'for offset %s, reading table', cddbdiscid, mbdiscid,
-                         offset)
-            t = cdrdao.ReadTOCTask(device, toc_path=toc_path)
-            t.description = "Reading table"
-            runner.run(t)
-            itable = t.toc.table
-            tdict[offset] = itable
-            ptable.persist(tdict)
-            logger.debug('getTable: read table %r', itable)
-        else:
-            logger.debug('getTable: cddbdiscid %s, mbdiscid %s in cache '
-                         'for offset %s', cddbdiscid, mbdiscid, offset)
-            logger.debug('getTable: loaded table %r', itable)
 
         assert itable.hasTOC()
 
@@ -153,24 +134,6 @@ class Program:
                      itable.getMusicBrainzDiscId())
         return itable
 
-    def getRipResult(self, cddbdiscid):
-        """
-        Get the persistable RipResult either from our cache or ret. a new one.
-
-        The cached RipResult may come from an aborted rip.
-
-        :rtype: result.RipResult
-        """
-        assert self.result is None
-
-        self._presult = self._cache.getRipResult(cddbdiscid)
-        self.result = self._presult.object
-
-        return self.result
-
-    def saveRipResult(self):
-        self._presult.persist()
-
     @staticmethod
     def addDisambiguation(template_part, metadata):
         """Add disambiguation to template path part string."""

whipper/test/test_common_accurip.py

@@ -3,13 +3,9 @@
 
 import sys
 from io import StringIO
-from os import chmod, makedirs
+from os.path import dirname, join
-from os.path import dirname, exists, join
-from shutil import copy, rmtree
-from tempfile import mkdtemp
 from unittest import TestCase
 
-from whipper.common import accurip
 from whipper.common.accurip import (
     calculate_checksums, get_db_entry, print_report, verify_result,
     _split_responses, EntryNotFound
@@ -25,41 +21,10 @@ class TestAccurateRipResponse(TestCase):
             cls.entry = _split_responses(f.read())
         cls.other_path = '4/8/2/dBAR-011-0010e284-009228a3-9809ff0b.bin'
 
-    def setUp(self):
-        self.cache_dir = mkdtemp(suffix='whipper_accurip_cache_test')
-        accurip._CACHE_DIR = self.cache_dir
-
-        def cleanup(cachedir):
-            chmod(cachedir, 0o755)
-            rmtree(cachedir)
-        self.addCleanup(cleanup, self.cache_dir)
-
-    def test_uses_cache_dir(self):
-        # copy normal entry into other entry's place
-        makedirs(dirname(join(self.cache_dir, self.other_path)))
-        copy(
-            join(dirname(__file__), self.path[6:]),
-            join(self.cache_dir, self.other_path)
-        )
-        # ask cache for other entry and assert cached entry equals normal entry
-        self.assertEqual(self.entry, get_db_entry(self.other_path))
-
     def test_raises_entrynotfound_for_no_entry(self):
         with self.assertRaises(EntryNotFound):
             get_db_entry('definitely_a_404')
 
-    def test_can_return_entry_without_saving(self):
-        chmod(self.cache_dir, 0)
-        self.assertEqual(get_db_entry(self.path), self.entry)
-        chmod(self.cache_dir, 0o755)
-        self.assertFalse(exists(join(self.cache_dir, self.path)))
-
-    def test_retrieves_and_saves_accuraterip_entry(self):
-        # for path, entry in zip(self.paths[0], self.entries):
-        self.assertFalse(exists(join(self.cache_dir, self.path)))
-        self.assertEqual(get_db_entry(self.path), self.entry)
-        self.assertTrue(exists(join(self.cache_dir, self.path)))
-
     def test_AccurateRipResponse_parses_correctly(self):
         responses = get_db_entry(self.path)
         self.assertEqual(len(responses), 2)

whipper/test/test_common_cache.py

@@ -1,23 +0,0 @@
-# -*- Mode: Python; test-case-name: whipper.test.test_common_cache -*-
-# vi:si:et:sw=4:sts=4:ts=4
-
-import os
-
-from whipper.common import cache
-
-from whipper.test import common as tcommon
-
-
-class ResultCacheTestCase(tcommon.TestCase):
-
-    def setUp(self):
-        self.cache = cache.ResultCache(
-            os.path.join(os.path.dirname(__file__), 'cache', 'result'))
-
-    def testGetResult(self):
-        result = self.cache.getRipResult('fe105a11')
-        self.assertEqual(result.object.title, "The Writing's on the Wall")
-
-    def testGetIds(self):
-        ids = self.cache.getIds()
-        self.assertEqual(ids, ['fe105a11'])

whipper/test/test_common_directory.py

@@ -13,6 +13,3 @@ class DirectoryTestCase(common.TestCase):
     def testAll(self):
         path = directory.config_path()
         self.assertTrue(path.startswith(DirectoryTestCase.HOME_PARENT))
-
-        path = directory.cache_path()
-        self.assertTrue(path.startswith(DirectoryTestCase.HOME_PARENT))