[PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[Joshua Watt]

Replace usage of os.utils.getstatusoutput() with direct subprocess
calls. Pass a modified environment and working directory where necessary
to bypass the need to execute in a shell.

Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
---
 meta/classes/kernel-yocto.bbclass | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/meta/classes/kernel-yocto.bbclass b/meta/classes/kernel-yocto.bbclass
index 82d80741a95..3a7717282f3 100644
--- a/meta/classes/kernel-yocto.bbclass
+++ b/meta/classes/kernel-yocto.bbclass
@@ -319,7 +319,7 @@ do_kernel_configme() {
 addtask kernel_configme before do_configure after do_patch
 
 python do_kernel_configcheck() {
-    import re, string, sys
+    import re, string, sys, subprocess
 
     # if KMETA isn't set globally by a recipe using this routine, we need to
     # set the default to 'meta'. Otherwise, kconf_check is not passed a valid
@@ -328,13 +328,23 @@ python do_kernel_configcheck() {
     if not os.path.exists(kmeta):
         kmeta = "." + kmeta
 
-    pathprefix = "export PATH=%s:%s; " % (d.getVar('PATH'), "${S}/scripts/util/")
+    s = d.getVar('S')
 
-    cmd = d.expand("scc --configs -o ${S}/.kernel-meta")
-    ret, configs = oe.utils.getstatusoutput("%s%s" % (pathprefix, cmd))
+    env = os.environ.copy()
+    env['PATH'] = "%s:%s%s" % (d.getVar('PATH'), s, "/scripts/util/")
 
-    cmd = d.expand("cd ${S}; kconf_check --report -o ${S}/%s/cfg/ ${B}/.config ${S} %s" % (kmeta,configs))
-    ret, result = oe.utils.getstatusoutput("%s%s" % (pathprefix, cmd))
+    try:
+        configs = subprocess.check_output(['scc', '--configs', '-o', s + '/.kernel-meta'], env=env).decode('utf-8')
+    except subprocess.CalledProcessError:
+        # Is this really non-fatal?
+        pass
+
+    try:
+        subprocess.check_call(['kconf_check', '--report', '-o',
+                '%s/%s/cfg' % (s, kmeta), d.getVar('B') + '/.config', s, configs], cwd=s, env=env)
+    except subprocess.CalledProcessError:
+        # Is this really non-fatal?
+        pass
 
     config_check_visibility = int(d.getVar("KCONF_AUDIT_LEVEL") or 0)
     bsp_check_visibility = int(d.getVar("KCONF_BSP_AUDIT_LEVEL") or 0)
-- 
2.17.1

Re: [PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[Burton, Ross]

On 25 July 2018 at 19:35, Joshua Watt <jpewhacker@gmail.com> wrote:
> -    cmd = d.expand("cd ${S}; kconf_check --report -o ${S}/%s/cfg/ ${B}/.config ${S} %s" % (kmeta,configs))
> -    ret, result = oe.utils.getstatusoutput("%s%s" % (pathprefix, cmd))
> +    try:
> +        configs = subprocess.check_output(['scc', '--configs', '-o', s + '/.kernel-meta'], env=env).decode('utf-8')
> +    except subprocess.CalledProcessError:
> +        # Is this really non-fatal?
> +        pass
> +
> +    try:
> +        subprocess.check_call(['kconf_check', '--report', '-o',
> +                '%s/%s/cfg' % (s, kmeta), d.getVar('B') + '/.config', s, configs], cwd=s, env=env)
> +    except subprocess.CalledProcessError:
> +        # Is this really non-fatal?
> +        pass

CCing Bruce for his feedback.  Either one or both of these calls can
fail without it being fatal which should be documented, or it
shouldn't be catching the exceptions.

Ross

Re: [PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[Bruce Ashfield]

On Wed, Jul 25, 2018 at 3:50 PM, Burton, Ross <ross.burton@intel.com> wrote:
> On 25 July 2018 at 19:35, Joshua Watt <jpewhacker@gmail.com> wrote:
>> -    cmd = d.expand("cd ${S}; kconf_check --report -o ${S}/%s/cfg/ ${B}/.config ${S} %s" % (kmeta,configs))
>> -    ret, result = oe.utils.getstatusoutput("%s%s" % (pathprefix, cmd))
>> +    try:
>> +        configs = subprocess.check_output(['scc', '--configs', '-o', s + '/.kernel-meta'], env=env).decode('utf-8')
>> +    except subprocess.CalledProcessError:
>> +        # Is this really non-fatal?
>> +        pass
>> +
>> +    try:
>> +        subprocess.check_call(['kconf_check', '--report', '-o',
>> +                '%s/%s/cfg' % (s, kmeta), d.getVar('B') + '/.config', s, configs], cwd=s, env=env)
>> +    except subprocess.CalledProcessError:
>> +        # Is this really non-fatal?
>> +        pass
>
> CCing Bruce for his feedback.  Either one or both of these calls can
> fail without it being fatal which should be documented, or it
> shouldn't be catching the exceptions.

Yah.

Don't grab this patch at all. I have a set of changes that muck all
about in here, so I'll have to look and integrate things by hand.

My question about the change .. is why ? I really don't like cleanup
patches on principle, and want things pinned to a bug, a performance
issue or something else.

Cheers,

Bruce

>
> Ross
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"

Re: [PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[Burton, Ross]

On 25 July 2018 at 21:05, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
> My question about the change .. is why ? I really don't like cleanup
> patches on principle, and want things pinned to a bug, a performance
> issue or something else.

See a post by RP to the oe-core list a few days ago: a general cleanup
of Python code that spawns is well overdue.  In this case
oe.utils.getstatusoutpus is a wrapper that just calls a deprecated
function in subprocess, is best to pass arguments as an array instead
of string, best not to use a shell, etc.

Ross

Re: [PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[Bruce Ashfield]

On Wed, Jul 25, 2018 at 4:17 PM, Burton, Ross <ross.burton@intel.com> wrote:
> On 25 July 2018 at 21:05, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
>> My question about the change .. is why ? I really don't like cleanup
>> patches on principle, and want things pinned to a bug, a performance
>> issue or something else.
>
> See a post by RP to the oe-core list a few days ago: a general cleanup
> of Python code that spawns is well overdue.  In this case
> oe.utils.getstatusoutpus is a wrapper that just calls a deprecated
> function in subprocess, is best to pass arguments as an array instead
> of string, best not to use a shell, etc.

ah ok.

I still don't like it, but it is what it is.

My comment stands, I'll figure out how to incorporate this and send it
along with my next pull request.

Bruce

>
> Ross



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"

Re: [PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[Bruce Ashfield]

On Wed, Jul 25, 2018 at 4:20 PM, Bruce Ashfield
<bruce.ashfield@gmail.com> wrote:
> On Wed, Jul 25, 2018 at 4:17 PM, Burton, Ross <ross.burton@intel.com> wrote:
>> On 25 July 2018 at 21:05, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
>>> My question about the change .. is why ? I really don't like cleanup
>>> patches on principle, and want things pinned to a bug, a performance
>>> issue or something else.
>>
>> See a post by RP to the oe-core list a few days ago: a general cleanup
>> of Python code that spawns is well overdue.  In this case
>> oe.utils.getstatusoutpus is a wrapper that just calls a deprecated
>> function in subprocess, is best to pass arguments as an array instead
>> of string, best not to use a shell, etc.
>
> ah ok.
>
> I still don't like it, but it is what it is.

... and I found the original email, I see the part about things being
depreciated. Makes sense.

I'll queue it up and run some tests on some intentionally bad BSPs.

Bruce

>
> My comment stands, I'll figure out how to incorporate this and send it
> along with my next pull request.
>
> Bruce
>
>>
>> Ross
>
>
>
> --
> "Thou shalt not follow the NULL pointer, for chaos and madness await
> thee at its end"



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"

Re: [PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[Joshua Watt]

On Wed, 2018-07-25 at 16:32 -0400, Bruce Ashfield wrote:
> On Wed, Jul 25, 2018 at 4:20 PM, Bruce Ashfield
> <bruce.ashfield@gmail.com> wrote:
> > On Wed, Jul 25, 2018 at 4:17 PM, Burton, Ross <ross.burton@intel.co
> > m> wrote:
> > > On 25 July 2018 at 21:05, Bruce Ashfield <bruce.ashfield@gmail.co
> > > m> wrote:
> > > > My question about the change .. is why ? I really don't like
> > > > cleanup
> > > > patches on principle, and want things pinned to a bug, a
> > > > performance
> > > > issue or something else.
> > > 
> > > See a post by RP to the oe-core list a few days ago: a general
> > > cleanup
> > > of Python code that spawns is well overdue.  In this case
> > > oe.utils.getstatusoutpus is a wrapper that just calls a
> > > deprecated
> > > function in subprocess, is best to pass arguments as an array
> > > instead
> > > of string, best not to use a shell, etc.
> > 
> > ah ok.
> > 
> > I still don't like it, but it is what it is.
> 
> ... and I found the original email, I see the part about things being
> depreciated. Makes sense.
> 
> I'll queue it up and run some tests on some intentionally bad BSPs.

Works for me.

I wasn't sure about if the subprocess exceptions should be fatal or
not. Suppressing them maintained the old behavior, but it didn't quite
seem "correct" to me.

Thanks,
Joshua Watt


> 
> Bruce
> 
> > 
> > My comment stands, I'll figure out how to incorporate this and send
> > it
> > along with my next pull request.
> > 
> > Bruce
> > 
> > > 
> > > Ross
> > 
> > 
> > 
> > --
> > "Thou shalt not follow the NULL pointer, for chaos and madness
> > await
> > thee at its end"
> 
> 
> 

Re: [PATCH] classes/kernel-yocto: Cleanup getstatusoutput usage

[Richard Purdie]

On Wed, 2018-07-25 at 15:44 -0500, Joshua Watt wrote:
> On Wed, 2018-07-25 at 16:32 -0400, Bruce Ashfield wrote:
> > On Wed, Jul 25, 2018 at 4:20 PM, Bruce Ashfield
> > <bruce.ashfield@gmail.com> wrote:
> > > On Wed, Jul 25, 2018 at 4:17 PM, Burton, Ross <ross.burton@intel.
> > > co
> > > m> wrote:
> > > > On 25 July 2018 at 21:05, Bruce Ashfield <bruce.ashfield@gmail.
> > > > co
> > > > m> wrote:
> > > > > My question about the change .. is why ? I really don't like
> > > > > cleanup
> > > > > patches on principle, and want things pinned to a bug, a
> > > > > performance
> > > > > issue or something else.
> > > > 
> > > > See a post by RP to the oe-core list a few days ago: a general
> > > > cleanup
> > > > of Python code that spawns is well overdue.  In this case
> > > > oe.utils.getstatusoutpus is a wrapper that just calls a
> > > > deprecated
> > > > function in subprocess, is best to pass arguments as an array
> > > > instead
> > > > of string, best not to use a shell, etc.
> > > 
> > > ah ok.
> > > 
> > > I still don't like it, but it is what it is.
> > 
> > ... and I found the original email, I see the part about things
> > being
> > depreciated. Makes sense.
> > 
> > I'll queue it up and run some tests on some intentionally bad BSPs.
> 
> Works for me.
> 
> I wasn't sure about if the subprocess exceptions should be fatal or
> not. Suppressing them maintained the old behavior, but it didn't
> quite seem "correct" to me.

We should at least put a comment in about this as its not best practise
not to check return codes. I do understand it can make sense in some
cases. Part of the reason to clean these calls up is also to ensure we
illustrate best practise in OE-Core so that when people copy and paste
it to other layers, we don't end up with bad things spreading.

In general, using the "proper" calls encourages better error handling
so that when something does fail, the user gets decent error messages.
For that reason I'd usually recommend the stderr -> stdout mapping so
that errors are displayed if a command fails. I appreciate that its a
moot point here since you ignore failures but in general it is a good
practise to encourage. We've had a spate of problems recently where
errors have been hidden making problems much harder to understand and
debug.

Cheers,

Richard