Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method
On Tue, Feb 4, 2020 at 2:34 PM Liam Merwick 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
>
Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method
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
Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method
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 +, 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
---
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
+
def do_test_x86_64_machine(self):
"""
:avocado: tags=arch:x86_64
--
1.8.3.1
Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method
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 +, 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
---
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é
Thanks,
Wainer
Regards,
Liam
Anyway this patch LGTM:
Reviewed-by: Stefano Garzarella
+
def do_test_x86_64_machine(self):
"""
:avocado: tags=arch:x86_64
--
1.8.3.1
Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method
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 +, 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
---
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
+
def do_test_x86_64_machine(self):
"""
:avocado: tags=arch:x86_64
--
1.8.3.1
Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method
On 30/01/2020 12:05, Stefano Garzarella wrote:
On Mon, Jan 27, 2020 at 04:36:33PM +, 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
---
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
+
def do_test_x86_64_machine(self):
"""
:avocado: tags=arch:x86_64
--
1.8.3.1
Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method
On Mon, Jan 27, 2020 at 04:36:33PM +, 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
> ---
> 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
> +
> def do_test_x86_64_machine(self):
> """
> :avocado: tags=arch:x86_64
> --
> 1.8.3.1
>
