Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Liam Merwick <liam.merwick@oracle.com>]

On 30/01/2020 19:19, Wainer dos Santos Moschetta wrote:
> 
> On 1/30/20 1:34 PM, Liam Merwick wrote:
>> On 30/01/2020 12:05, Stefano Garzarella wrote:
>>> On Mon, Jan 27, 2020 at 04:36:33PM +0000, Liam Merwick wrote:
>>>> Add a method to extract a specified file from an RPM to the test's
>>>> working directory and return the path to the extracted file.
>>>>
>>>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>>>> ---
>>>>   tests/acceptance/boot_linux_console.py | 14 ++++++++++++++
>>>>   1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/tests/acceptance/boot_linux_console.py 
>>>> b/tests/acceptance/boot_linux_console.py
>>>> index 43bc928b03a2..6af19ae3b14a 100644
>>>> --- a/tests/acceptance/boot_linux_console.py
>>>> +++ b/tests/acceptance/boot_linux_console.py
>>>> @@ -51,6 +51,20 @@ class BootLinuxConsole(Test):
>>>>           os.chdir(cwd)
>>>>           return self.workdir + path
>>>>   +    def extract_from_rpm(self, rpm, path):
>>>> +        """
>>>> +        Extracts a file from a rpm package into the test workdir
>>>> +
>>>> +        :param rpm: path to the rpm archive
>>>> +        :param path: path within the rpm archive of the file to be 
>>>> extracted
> 
> 
> Might not be obvious to users that `path` should start with '.', and if 
> he/she doesn't do that then extract_from_rpm() will silently fail to 
> extract the file. So could you document that?


Sure.

> 
>>>>
>>>> +        :returns: path of the extracted file
>>>> +        """
>>>> +        cwd = os.getcwd()
>>>> +        os.chdir(self.workdir)
>>>> +        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path), 
>>>> shell=True)
>>>> +        os.chdir(cwd)
>>>> +        return self.workdir + '/' + path
>>>                                    ^
>>>      Is the extra slash needed? (just because the extract_from_deb()
>>>      doesn't put it)
>>>
>>
>>
>> Yes, I needed to put it in there because the 'path' passed in for
>> processing by cpio is a relative patch unlike the deb arg so it
>> couldn't be just appended to 'self.workdir' which doesn't end in a '/'.
> 
> 
> It is a good practice use the `os.path` module methods when dealing with 
> filesystem paths. So that can be replaced with:
> 
>  >>> os.path.normpath(os.path.join('/path/to/workdir', './file/in/rpm'))
> '/path/to/workdir/file/in/rpm'
> 


Will do.  I'll add a patch to fix extract_from_deb() too.

Regards,
Liam


> Thanks,
> 
> Wainer
> 
> 
>>
>>
>> Regards,
>> Liam
>>
>>
>>> Anyway this patch LGTM:
>>>
>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>>
>>>> +
>>>>       def do_test_x86_64_machine(self):
>>>>           """
>>>>           :avocado: tags=arch:x86_64
>>>> -- 
>>>> 1.8.3.1
>>>>
>>>
>>
>