Thanks, this looks fine. Can you resend the patch using 'git send-email'? I'm not sure if sending the patch as a quoted reply to me allows it to be picked from the mailing list.
Alex On Thu, 21 Jul 2022 at 02:19, Shruthi Ravichandran <[email protected]> wrote: > > Some opkg commands print an error during cleanup when the tmp_dir > does not exist and an attempt is made to delete it. The error messages > are harmless and the opkg commands eventually succeed. > When these commands are run and stderr is piped to stdout, the error > messages may clobber the stdout and cause unexpected results while > parsing the output of the command. Therefore, when parsing the output > of a command, do not pipe stderr to stdout. Instead, capture stderr > and stdout separately, and upon success, send stderr to bb.note(). > > Signed-off-by: Shruthi Ravichandran <[email protected]> > --- > meta/lib/oe/package_manager/ipk/__init__.py | 23 ++++++++++++--------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/meta/lib/oe/package_manager/ipk/__init__.py > b/meta/lib/oe/package_manager/ipk/__init__.py > index 6fd2f021b6..7cbea0fa80 100644 > --- a/meta/lib/oe/package_manager/ipk/__init__.py > +++ b/meta/lib/oe/package_manager/ipk/__init__.py > @@ -102,12 +102,14 @@ class OpkgDpkgPM(PackageManager): > This method extracts the common parts for Opkg and Dpkg > """ > > - try: > - output = subprocess.check_output(cmd, stderr=subprocess.STDOUT, > shell=True).decode("utf-8") > - except subprocess.CalledProcessError as e: > + proc = subprocess.run(cmd, capture_output=True, encoding="utf-8", > shell=True) > + if proc.returncode: > bb.fatal("Unable to list available packages. Command '%s' " > - "returned %d:\n%s" % (cmd, e.returncode, > e.output.decode("utf-8"))) > - return opkg_query(output) > + "returned %d:\n%s" % (cmd, proc.returncode, > proc.stderr)) > + elif proc.stderr: > + bb.note("Command '%s' returned stderr: %s" % (cmd, proc.stderr)) > + > + return opkg_query(proc.stdout) > > def extract(self, pkg, pkg_info): > """ > @@ -445,15 +447,16 @@ class OpkgPM(OpkgDpkgPM): > cmd = "%s %s --noaction install %s " % (self.opkg_cmd, > opkg_args, > ' '.join(pkgs)) > - try: > - output = subprocess.check_output(cmd, stderr=subprocess.STDOUT, > shell=True) > - except subprocess.CalledProcessError as e: > + proc = subprocess.run(cmd, capture_output=True, encoding="utf-8", > shell=True) > + if proc.returncode: > bb.fatal("Unable to dummy install packages. Command '%s' " > - "returned %d:\n%s" % (cmd, e.returncode, > e.output.decode("utf-8"))) > + "returned %d:\n%s" % (cmd, proc.returncode, > proc.stderr)) > + elif proc.stderr: > + bb.note("Command '%s' returned stderr: %s" % (cmd, proc.stderr)) > > bb.utils.remove(temp_rootfs, True) > > - return output > + return proc.stdout > > def backup_packaging_data(self): > # Save the opkglib for increment ipk image generation > -- > 2.20.1 > > > > -----Original Message----- > > From: Alexander Kanavin <[email protected]> > > Sent: Wednesday, July 20, 2022 1:37 AM > > To: Shruthi Ravichandran <[email protected]> > > Cc: [email protected] > > Subject: Re: [OE-core][PATCH v2] package_manager/ipk: do not pipe stderr to > > stdout > > > > Please use subprocess.run() (which allows separate capture of stderr), > > rather than switch the whole thing to popen. > > > > Alex > > > > On Tue, 19 Jul 2022 at 23:51, Shruthi Ravichandran > > <[email protected]> wrote: > > > > > > Some opkg commands print an error during cleanup when the tmp_dir > > > does not exist and an attempt is made to delete it. The error messages > > > are harmless and the opkg commands eventually succeed. > > > When these commands are run and stderr is piped to stdout, the error > > > messages may clobber the stdout and cause unexpected results while > > > parsing the output of the command. Therefore, when parsing the output > > > of a command, do not pipe stderr to stdout. Instead, capture stderr > > > and stdout separately, and upon success, send stderr to bb.note(). > > > > > > Signed-off-by: Shruthi Ravichandran <[email protected]> > > > --- > > > meta/lib/oe/package_manager/ipk/__init__.py | 30 ++++++++++++++------- > > > 1 file changed, 20 insertions(+), 10 deletions(-) > > > > > > diff --git a/meta/lib/oe/package_manager/ipk/__init__.py > > > b/meta/lib/oe/package_manager/ipk/__init__.py > > > index 6fd2f021b6..a768de5c30 100644 > > > --- a/meta/lib/oe/package_manager/ipk/__init__.py > > > +++ b/meta/lib/oe/package_manager/ipk/__init__.py > > > @@ -102,12 +102,17 @@ class OpkgDpkgPM(PackageManager): > > > This method extracts the common parts for Opkg and Dpkg > > > """ > > > > > > - try: > > > - output = subprocess.check_output(cmd, > > > stderr=subprocess.STDOUT, shell=True).decode("utf-8") > > > - except subprocess.CalledProcessError as e: > > > + proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, > > > stderr=subprocess.PIPE, shell=True) > > > + cmd_stdout, cmd_stderr = proc.communicate() > > > + cmd_stdout = cmd_stdout.decode("utf-8") > > > + cmd_stderr = cmd_stderr.decode("utf-8") > > > + if proc.returncode: > > > bb.fatal("Unable to list available packages. Command '%s' " > > > - "returned %d:\n%s" % (cmd, e.returncode, > > > e.output.decode("utf-8"))) > > > - return opkg_query(output) > > > + "returned %d:\n%s" % (cmd, proc.returncode, > > > cmd_stderr)) > > > + elif cmd_stderr: > > > + bb.note("Command '%s' returned stderr: %s" % (cmd, > > > cmd_stderr)) > > > + > > > + return opkg_query(cmd_stdout) > > > > > > def extract(self, pkg, pkg_info): > > > """ > > > @@ -445,15 +450,20 @@ class OpkgPM(OpkgDpkgPM): > > > cmd = "%s %s --noaction install %s " % (self.opkg_cmd, > > > opkg_args, > > > ' '.join(pkgs)) > > > - try: > > > - output = subprocess.check_output(cmd, > > > stderr=subprocess.STDOUT, shell=True) > > > - except subprocess.CalledProcessError as e: > > > + > > > + proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, > > > stderr=subprocess.PIPE, shell=True) > > > + cmd_stdout, cmd_stderr = proc.communicate() > > > + cmd_stdout = cmd_stdout.decode("utf-8") > > > + cmd_stderr = cmd_stderr.decode("utf-8") > > > + if proc.returncode: > > > bb.fatal("Unable to dummy install packages. Command '%s' " > > > - "returned %d:\n%s" % (cmd, e.returncode, > > > e.output.decode("utf-8"))) > > > + "returned %d:\n%s" % (cmd, proc.returncode, > > > cmd_stderr)) > > > + elif cmd_stderr: > > > + bb.note("Command '%s' returned stderr: %s" % (cmd, > > > cmd_stderr)) > > > > > > bb.utils.remove(temp_rootfs, True) > > > > > > - return output > > > + return cmd_stdout > > > > > > def backup_packaging_data(self): > > > # Save the opkglib for increment ipk image generation > > > -- > > > 2.20.1 > > > > > > > > > > -----Original Message----- > > > > From: Alexander Kanavin <[email protected]> > > > > Sent: Friday, July 1, 2022 5:26 AM > > > > To: Shruthi Ravichandran <[email protected]> > > > > Cc: [email protected] > > > > Subject: Re: [OE-core][PATCH] package_manager/ipk: do not pipe stderr > > > > to stdout > > > > > > > > Yes, certainly. > > > > > > > > Alex > > > > > > > > On Fri, 1 Jul 2022 at 00:30, Shruthi Ravichandran > > > > <[email protected]> wrote: > > > > > > > > > > The change currently does discard everything in stderr. I can update > > > > > it to capture stderr and push it to bb.note on command success and > > > > > bb.fatal on command failure. In fact, I can make those changes to the > > > > > several other instances in this file where stderr is piped to stdout > > > > > too. Would that be acceptable? > > > > > > > > > > Shruthi > > > > > > > > > > > -----Original Message----- > > > > > > From: Alexander Kanavin <[email protected]> > > > > > > Sent: Thursday, June 30, 2022 1:47 AM > > > > > > To: Shruthi Ravichandran <[email protected]> > > > > > > Cc: [email protected] > > > > > > Subject: Re: [OE-core][PATCH] package_manager/ipk: do not pipe > > > > > > stderr to stdout > > > > > > > > > > > > Thanks for the information - perhaps this should be added to the > > > > > > commit message? > > > > > > > > > > > > Does this change discard things that appear on stderr completely, or > > > > > > does it still go somewhere where it can be seen later? > > > > > > > > > > > > Alex > > > > > > > > > > > > On Wed, 29 Jun 2022 at 21:22, Shruthi Ravichandran > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > Hi Alex, > > > > > > > > > > > > > > I've found that some OE commands print an error during cleanup > > > > > > > when > > > > > > > the tmp_dir does not exist and an attempt is made to delete it. > > > > > > > I've > > > > > > > submitted a patch to opkg to fix that. > > > > > > > Link: > > > > > > > > > > > > https://urldefense.com/v3/__https://git.yoctoproject.org/opkg/commit/?id=8dfdda86afd407a66e3dc00a077bdcc8b53d54ea__;!!FbZ0ZwI > > > > > > 3Qg!omDrJfbrjlLbY2OMgsAgQrIcnap222jrjZJAhJX_BWhVJfMi09419QAHH1bVf1VafwMRcVuWBaQ5k8y6gk-W-iLjh48$ > > > > > > . > > > > > > > That was the one instance that was causing an issue in our builds. > > > > > > > There may be other instances that I don't know of. Given that, I > > > > > > > think > > > > > > > the package_manager code should be resilient against any such > > > > > > > miscellaneous stderr messages, which do not result in the command > > > > > > > itself failing. > > > > > > > > > > > > > > Hope that helps, > > > > > > > Shruthi > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Alexander Kanavin <[email protected]> > > > > > > > > Sent: Tuesday, June 28, 2022 1:33 PM > > > > > > > > To: Shruthi Ravichandran <[email protected]> > > > > > > > > Cc: [email protected] > > > > > > > > Subject: Re: [OE-core][PATCH] package_manager/ipk: do not pipe > > > > > > > > stderr to stdout > > > > > > > > > > > > > > > > This needs additional justification. What are the error > > > > > > > > messages, why are they harmless and why the solution is to > > > > > > > > suppress them > > > > instead > > > > > > of > > > > > > > > addressing the reasons they appear? > > > > > > > > > > > > > > > > Alex > > > > > > > > > > > > > > > > On Tue 28. Jun 2022 at 23.13, Shruthi Ravichandran > > > > > > > > <[email protected] > > > > > > > > <mailto:[email protected]> > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > When parsing the output of a command, do not pipe stderr > > > > > > > > to stdout. > > > > > > > > Opkg sometimes prints harmless error messages even when > > > > > > > > the opkg > > > > > > > > command succeeds. When stderr is piped to stdout, these > > > > > > > > error > > > > > > > > messages may clobber the stdout and cause unexpected > > > > > > > > results while > > > > > > > > parsing the output. > > > > > > > > > > > > > > > > Signed-off-by: Shruthi Ravichandran > > > > > > > > <[email protected] > > > > > > > > <mailto:[email protected]> > > > > > > > > > --- > > > > > > > > meta/lib/oe/package_manager/ipk/__init__.py | 2 +- > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/meta/lib/oe/package_manager/ipk/__init__.py > > > > > > > > b/meta/lib/oe/package_manager/ipk/__init__.py > > > > > > > > index 4cd3963111..d7f3f31853 100644 > > > > > > > > --- a/meta/lib/oe/package_manager/ipk/__init__.py > > > > > > > > +++ b/meta/lib/oe/package_manager/ipk/__init__.py > > > > > > > > @@ -103,7 +103,7 @@ class OpkgDpkgPM(PackageManager): > > > > > > > > """ > > > > > > > > > > > > > > > > try: > > > > > > > > - output = subprocess.check_output(cmd, > > > > > > > > stderr=subprocess.STDOUT, shell=True).decode("utf-8") > > > > > > > > + output = subprocess.check_output(cmd, > > > > > > > > shell=True).decode("utf-8") > > > > > > > > except subprocess.CalledProcessError as e: > > > > > > > > bb.fatal("Unable to list available packages. > > > > > > > > Command '%s' " > > > > > > > > "returned %d:\n%s" % (cmd, > > > > > > > > e.returncode, e.output.decode("utf-8"))) > > > > > > > > -- > > > > > > > > 2.20.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > INTERNAL - NI CONFIDENTIAL > > > > > > > > > > INTERNAL - NI CONFIDENTIAL > > > > > > INTERNAL - NI CONFIDENTIAL > > INTERNAL - NI CONFIDENTIAL
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#168417): https://lists.openembedded.org/g/openembedded-core/message/168417 Mute This Topic: https://lists.openembedded.org/mt/92517610/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
