Re: [PATCH v4 0/6] Python refactoring

[Claudius Heine <claudius.heine.ext@siemens.com>]

On Tue, 2019-03-12 at 17:03 +0100, Jan Kiszka wrote:
> On 12.03.19 16:34, Claudius Heine wrote:
> > On 12/03/2019 16.26, Jan Kiszka wrote:
> > > On 12.03.19 15:56, [ext] Claudius Heine wrote:
> > > > This should probaly also be part of the isar release. It should
> > > > fix
> > > > those 'ResourceWarning: unclosed file <_io.BufferedReader
> > > > name=32>'
> > > > bitbake warnings.
> > > 
> > > Which of the commits should fix that, and why? Remains non-
> > > obvious to me. 
> > 
> > The first one 'Remove all uses of subprocess.call(shell=True)'.
> > 
> > The usage of 'Popen' requires that the process handler should be
> > closed, which 
> > was not done in isar-bootstrap-helper.bbclass. The right solution
> > is either not 
> > use Popen (which this patch changes) or use Popen with a context
> > manager (which 
> > I have done in the template.bbclass).
> > 
> 
> Ah, ok. Should be in the commit log then. These warnings were pretty
> annoying.

I don't think that Harald was aware that his patch fixed those when he
wrote his patch. He just fixed general mispractices in the python code
that he discovered by himself or by using pylint etc.

This is also just my expectation of what this change probably fixes. I
have not investigated that if that is definitely the case. So that was
more or less just a side-note from me. If someone wants to determine if
my theory is correct, then be welcomed to it.

Claudius

> 
> Jan
> 
> > > But I don't disagree that fixes, even minor ones, should be part
> > > of the 
> > > release. There are way more fixes pending, let's give Maxim some
> > > time.
> > 
> > I just wanted to give the additional information here, because that
> > could have 
> > been missed.
> > 
> > Claudius
> > 
> > > Jan
> > > 
> > > > On Mon, 2019-03-04 at 14:07 +0100, [ext] 
> > > > claudius.heine.ext@siemens.com
> > > > wrote:
> > > > > From: Claudius Heine <ch@denx.de>
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > I just took Haralds patchset + Maxims rebase and fixed it up.
> > > > > 
> > > > > There was just a tiny change necessary, I added that in an
> > > > > additional
> > > > > patch.
> > > > > It builds on my machine (qemuamd64) and I am currently
> > > > > testing it on
> > > > > isar-ci.
> > > > > 
> > > > > If that is successful and review is ok, my patch can be
> > > > > merged into
> > > > > the
> > > > > 'Remove all uses of subprocess.call(shell=True)' or left
> > > > > separately.
> > > > > 
> > > > > regards,
> > > > > Claudius
> > > > > 
> > > > > Claudius Heine (1):
> > > > >    isar-bootstrap-helper: get_deb_host_arch: fix decoding
> > > > > 
> > > > > Harald Seiler (5):
> > > > >    Remove all uses of subprocess.call(shell=True)
> > > > >    Use modern python formatting
> > > > >    image: Remove recursion in get_image_name
> > > > >    wic: Refactor fakeroot script
> > > > >    Fix python style
> > > > > 
> > > > >   .../example-module/example-module.bb          |  8 ++-
> > > > >   meta/classes/base.bbclass                     | 70
> > > > > ++++++++++++-----
> > > > > -- 
> > > > >   meta/classes/image.bbclass                    | 22 ++++--
> > > > >   meta/classes/isar-bootstrap-helper.bbclass    |  8 +--
> > > > >   meta/classes/isar-events.bbclass              | 14 ++--
> > > > >   meta/classes/patch.bbclass                    | 13 ++--
> > > > >   meta/classes/wic-img.bbclass                  | 21 ++++--
> > > > >   .../isar-bootstrap/isar-bootstrap-host.bb     |  9 ++-
> > > > >   .../isar-bootstrap/isar-bootstrap-target.bb   |  5 +-
> > > > >   .../isar-bootstrap/isar-bootstrap.inc         | 12 ++--
> > > > >   scripts/wic_fakeroot                          |  9 +--
> > > > >   11 files changed, 120 insertions(+), 71 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.20.1
> > > > >