classes/kernel-yocto: Cleanup getstatusoutput usage
PATCH 1/1 classes/kernel-yocto: Cleanup getstatusoutput usage
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.
--- meta/classes/kernel-yocto.bbclass | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
meta/classes/kernel-yocto.bbclass +16 -6
@@ -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)
Signature · 1 line
2.17.1
Quoted 4 lines · Joshua Watt
meta/classes/kernel-yocto.bbclass +2
-2- 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')
meta/classes/kernel-yocto.bbclass python do_kernel_configcheck() { · line 347
Quoted 10 lines
meta/classes/kernel-yocto.bbclass +10
-0+ 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
Quoted 16 lines · Ross
Quoted 4 lines · Joshua Watt
meta/classes/kernel-yocto.bbclass +2
-2- 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')meta/classes/kernel-yocto.bbclass python do_kernel_configcheck() { · line 347
Quoted 10 lines
meta/classes/kernel-yocto.bbclass +10
-0+ 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
Quoted 3 lines
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
Quoted 7 lines
Ross
--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core
Signature · 2 lines
"Thou shalt not follow the NULL pointer, for chaos and madness await thee at its end"
Quoted 3 lines · Bruce Ashfield
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
Quoted 10 lines · Ross
Quoted 3 lines · Bruce Ashfield
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
Quoted 2 lines
Ross
Signature · 2 lines
"Thou shalt not follow the NULL pointer, for chaos and madness await thee at its end"
Quoted 12 lines · Bruce Ashfield
Quoted 10 lines · Ross
Quoted 3 lines · Bruce Ashfield
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.
Quoted 3 lines
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
Quoted 14 lines
My comment stands, I'll figure out how to incorporate this and send it along with my next pull request.
Bruce
Quoted 2 lines
Ross
--
"Thou shalt not follow the NULL pointer, for chaos and madness await thee at its end"
Signature · 2 lines
"Thou shalt not follow the NULL pointer, for chaos and madness await thee at its end"
Quoted 21 lines · Bruce Ashfield
Quoted 19 lines · Bruce Ashfield
Quoted 16 lines · Ross
Quoted 5 lines · Bruce Ashfield
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.
Quoted 8 lines
Quoted 3 lines
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
Quoted 22 lines
Bruce
Quoted 16 lines
My comment stands, I'll figure out how to incorporate this and send it
along with my next pull request.Bruce
Quoted 2 lines
Ross
--
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"
Quoted 32 lines · Joshua Watt
Quoted 31 lines · Bruce Ashfield
Quoted 21 lines · Bruce Ashfield
Quoted 9 lines · Ross
Quoted 5 lines · Bruce Ashfield
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.Quoted 8 lines
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.Quoted 3 lines
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.
Quoted 7 lines
Quoted 1 line
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