| Message ID | 1580142994-1836-6-git-send-email-liam.merwick@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/boot_linux_console: add extra boot acceptance tests | expand |
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 >
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 >> >
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 >>> >> >
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 >>>> >>> >> >
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 >>>> >>> >> >
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
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 --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
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(+)