diff mbox series

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

Message ID 1580142994-1836-6-git-send-email-liam.merwick@oracle.com (mailing list archive)
State New, archived
Headers show
Series tests/boot_linux_console: add extra boot acceptance tests | expand

Commit Message

Liam Merwick Jan. 27, 2020, 4:36 p.m. UTC
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(+)

Comments

Stefano Garzarella Jan. 30, 2020, 12:05 p.m. UTC | #1
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
> +        :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)

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
>
Liam Merwick Jan. 30, 2020, 3:34 p.m. UTC | #2
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
>> +        :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 '/'.

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
>>
>
Wainer dos Santos Moschetta Jan. 30, 2020, 7:19 p.m. UTC | #3
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?

>>>
>>> +        :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'

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
>>>
>>
>
Philippe Mathieu-Daudé Jan. 30, 2020, 11:59 p.m. UTC | #4
On 1/30/20 8:19 PM, 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?
> 
>>>>
>>>> +        :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'

With Wainer suggestion addressed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> 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
>>>>
>>>
>>
>
Liam Merwick Jan. 31, 2020, 3:02 p.m. UTC | #5
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
>>>>
>>>
>>
>
Liam Merwick Feb. 4, 2020, 1:31 p.m. UTC | #6
On 31/01/2020 15:02, Liam Merwick wrote:

[... deleted ...]

>>
>>>>>
>>>>> +        :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.

Using the exact same code didn't work with extract_from_deb() because 
the callers set 'path' to an absolute path (so os.path.join() drops the 
self.workdir part).  I'll include a patch with the following change and 
it can be dropped if people think using os.path.relpath() is too much of 
a hack.

--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -49,7 +49,12 @@ class BootLinuxConsole(Test):
          process.run("ar x %s %s" % (deb, file_path))
          archive.extract(file_path, self.workdir)
          os.chdir(cwd)
-        return self.workdir + path
+        # Return complete path to extracted file.  We need to use
+        # os.path.relpath() because callers specify 'path' with a leading
+        # slash which causes os.path.join() to interpret it as an absolute
+        # path and to drop self.workdir part.
+        return os.path.normpath(os.path.join(self.workdir,
+                                             os.path.relpath(path, '/')))

      def extract_from_rpm(self, rpm, path):
          """

Regards,
Liam
Philippe Mathieu-Daudé Feb. 4, 2020, 2:22 p.m. UTC | #7
On Tue, Feb 4, 2020 at 2:34 PM Liam Merwick <liam.merwick@oracle.com> wrote:
> On 31/01/2020 15:02, Liam Merwick wrote:
>
> [... deleted ...]
>
> >>
> >>>>>
> >>>>> +        :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.
>
> Using the exact same code didn't work with extract_from_deb() because
> the callers set 'path' to an absolute path (so os.path.join() drops the
> self.workdir part).  I'll include a patch with the following change and
> it can be dropped if people think using os.path.relpath() is too much of
> a hack.
>
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -49,7 +49,12 @@ class BootLinuxConsole(Test):
>           process.run("ar x %s %s" % (deb, file_path))
>           archive.extract(file_path, self.workdir)
>           os.chdir(cwd)
> -        return self.workdir + path
> +        # Return complete path to extracted file.  We need to use
> +        # os.path.relpath() because callers specify 'path' with a leading
> +        # slash which causes os.path.join() to interpret it as an absolute
> +        # path and to drop self.workdir part.
> +        return os.path.normpath(os.path.join(self.workdir,
> +                                             os.path.relpath(path, '/')))

LGTM, so the next one modifying this code won't make a mistake.

>
>       def extract_from_rpm(self, rpm, path):
>           """
>
> Regards,
> Liam
>
diff mbox series

Patch

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
+        :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
+
     def do_test_x86_64_machine(self):
         """
         :avocado: tags=arch:x86_64