Re: [RFC PATCH 5/8] tests_pytest: Implement fetch_asset() method for downloading assets

[Thomas Huth <thuth@redhat.com>]

On 12/07/2024 11.09, Daniel P. Berrangé wrote:
> On Thu, Jul 11, 2024 at 01:55:43PM +0200, Thomas Huth wrote:
>> In the pytests, we cannot use the fetch_asset() function from Avocado
>> anymore, so we have to provide our own implementation now instead.
>> Thus add such a function based on the _download_with_cache() function
>> from tests/vm/basevm.py for this purpose.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/pytest/qemu_pytest/__init__.py | 40 ++++++++++++++++++++--------
>>   1 file changed, 29 insertions(+), 11 deletions(-)
>>
>> diff --git a/tests/pytest/qemu_pytest/__init__.py b/tests/pytest/qemu_pytest/__init__.py
>> index e3ed32e3de..73d80b3828 100644
>> --- a/tests/pytest/qemu_pytest/__init__.py
>> +++ b/tests/pytest/qemu_pytest/__init__.py
>> @@ -11,6 +11,7 @@
>>   # This work is licensed under the terms of the GNU GPL, version 2 or
>>   # later.  See the COPYING file in the top-level directory.
>>   
>> +import hashlib
>>   import logging
>>   import os
>>   import shutil
>> @@ -201,17 +202,34 @@ def setUp(self, bin_prefix):
>>           self.assertIsNotNone(SOURCE_DIR,'PYTEST_SOURCE_ROOT must be set')
>>           self.assertIsNotNone(self.qemu_bin, 'PYTEST_QEMU_BINARY must be set')
>>   
>> -    def fetch_asset(self, name,
>> -                    asset_hash, algorithm=None,
>> -                    locations=None, expire=None,
>> -                    find_only=False, cancel_on_missing=True):
>> -        return super().fetch_asset(name,
>> -                        asset_hash=asset_hash,
>> -                        algorithm=algorithm,
>> -                        locations=locations,
>> -                        expire=expire,
>> -                        find_only=find_only,
>> -                        cancel_on_missing=cancel_on_missing)
>> +    def check_hash(self, file_name, expected_hash):
>> +        if not expected_hash:
>> +            return True
>> +        if len(expected_hash) == 32:
>> +            sum_prog = 'md5sum'
>> +        elif len(expected_hash) == 40:
>> +            sum_prog = 'sha1sum'
>> +        elif len(expected_hash) == 64:
>> +            sum_prog = 'sha256sum'
>> +        elif len(expected_hash) == 128:
>> +            sum_prog = 'sha512sum'
>> +        else:
>> +            raise Exception("unknown hash type")
> 
> Why shouldn't we just standardize on sha256 as we convert each test
> to pytest ? sha512 is overkill, and md5/sha1 shouldn't really be used
> anymore.

I mainly added that for minimizing the changes that I need to do on the 
existing tests. Updating all the hashsums there is certainly some additional 
work... If you want to help, feel free to send patches for the existing 
avocado tests to update the md5 and sha1 sums there!

>> +        checksum = subprocess.check_output([sum_prog, file_name]).split()[0]
>> +        return expected_hash == checksum.decode("utf-8")
>> +
>> +    def fetch_asset(self, url, asset_hash):
>> +        cache_dir = os.path.expanduser("~/.cache/qemu/download")
>> +        if not os.path.exists(cache_dir):
>> +            os.makedirs(cache_dir)
>> +        fname = os.path.join(cache_dir,
>> +                             hashlib.sha1(url.encode("utf-8")).hexdigest())
>> +        if os.path.exists(fname) and self.check_hash(fname, asset_hash):
>> +            return fname
>> +        logging.debug("Downloading %s to %s...", url, fname)
>> +        subprocess.check_call(["wget", "-c", url, "-O", fname + ".download"])
>> +        os.rename(fname + ".download", fname)
>> +        return fname
> 
> To avoid a dep on an external command that may not be installed,
> I think we could replace wget with native python code:
> 
>   import urllib
...

Yes, I came across urllib after I sent out the patches, too, sounds like the 
right way to go. We should also update tests/vm/basevm.py accordingly!

  Thomas