Re: [PATCH v4 14/35] tests/functional: add a module for handling asset download & caching

[Daniel P . Berrangé]

On Fri, Aug 23, 2024 at 08:24:45AM +0200, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On 21/8/24 10:27, Thomas Huth wrote:
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > The 'Asset' class is a simple module that declares a downloadable
> > asset that can be cached locally. Downloads are stored in the user's
> > home dir at ~/.cache/qemu/download, using a sha256 sum of the URL.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > [thuth: Drop sha1 support, use hash on file content for naming instead of 
> > URL,
> >          add the possibility to specify the cache dir via environment 
> > variable]
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   tests/functional/qemu_test/__init__.py |  1 +
> >   tests/functional/qemu_test/asset.py    | 97 ++++++++++++++++++++++++++
> >   2 files changed, 98 insertions(+)
> >   create mode 100644 tests/functional/qemu_test/asset.py
> 
> 
> > +    def fetch(self):
> > +        if not self.cache_dir.exists():
> > +            self.cache_dir.mkdir(parents=True, exist_ok=True)
> > +
> > +        if self.valid():
> > +            self.log.debug("Using cached asset %s for %s",
> > +                           self.cache_file, self.url)
> > +            return str(self.cache_file)
> > +
> > +        self.log.info("Downloading %s to %s...", self.url, self.cache_file)
> > +        tmp_cache_file = self.cache_file.with_suffix(".download")
> > +
> > +        try:
> > +            resp = urllib.request.urlopen(self.url)
> > +        except Exception as e:
> > +            self.log.error("Unable to download %s: %s", self.url, e)
> > +            raise
> > +
> > +        try:
> > +            with tmp_cache_file.open("wb+") as dst:
> > +                copyfileobj(resp, dst)
> > +        except:
> > +            tmp_cache_file.unlink()
> > +            raise
> > +        try:
> > +            # Set these just for informational purposes
> > +            os.setxattr(str(tmp_cache_file), "user.qemu-asset-url",
> > +                        self.url.encode('utf8'))
> > +            os.setxattr(str(tmp_cache_file), "user.qemu-asset-hash",
> > +                        self.hash.encode('utf8'))
> > +        except Exception as e:
> > +            self.log.info("Unable to set xattr on %s: %s", tmp_cache_file, 
> > e)
> 
> This line is annoying on macOS as it is logged for each file downloaded.
> Is it really useful? Can we demote to DEBUG level or log it just once,
> given all tmp_cache_files will always be on the same cache_dir thus
> filesystem?

Yeah, DEBUG would be fine.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|