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

[Wainer dos Santos Moschetta]

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