Re: [dpdk-dev] [PATCH v1 0/4] app: make python apps python2/3 compliant

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Dec 09, 2016 at 05:00:19PM +0000, Mcnamara, John wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Friday, December 9, 2016 3:29 PM
> > To: Mcnamara, John <john.mcnamara@intel.com>
> > Cc: dev@dpdk.org; mkletzan@redhat.com
> > Subject: Re: [dpdk-dev] [PATCH v1 0/4] app: make python apps python2/3
> > compliant
> > 
> > On Thu, Dec 08, 2016 at 03:51:01PM +0000, John McNamara wrote:
> > > These patches refactor the DPDK Python applications to make them
> > > Python 2/3 compatible.
> > >
> > > In order to do this the patchset starts by making the apps PEP8
> > > compliant in accordance with the DPDK Coding guidelines:
> > >
> > >
> > > http://dpdk.org/doc/guides/contributing/coding_style.html#python-code
> > >
> > > Implementing PEP8 and Python 2/3 compliance means that we can check
> > > all future Python patches for consistency. Python 2/3 support also
> > > makes downstream packaging easier as more distros move to Python 3 as
> > the system python.
> > >
> > > See the previous discussion about Python2/3 compatibilty here:
> > >
> > >     http://dpdk.org/ml/archives/dev/2016-December/051683.html
> > >
> > > I've tested that the apps compile with python 2 and 3 and I've tested
> > > some of the apps for consistent output but it needs additional testing.
> > >
> > > John McNamara (4):
> > >   app: make python apps pep8 compliant
> > >   app: make python apps python2/3 compliant
> > >   app: give python apps a consistent shebang line
> > >   doc: add required python versions to coding guidelines
> > >
> > >  app/cmdline_test/cmdline_test.py                   |  87 ++-
> > >  app/cmdline_test/cmdline_test_data.py              | 403 +++++-----
> > >  app/test/autotest.py                               |  46 +-
> > >  app/test/autotest_data.py                          | 831 +++++++++++---
> > -------
> > >  app/test/autotest_runner.py                        | 740 +++++++++-----
> > ----
> > >  app/test/autotest_test_funcs.py                    | 481 ++++++------
> > >  doc/guides/conf.py                                 |  11 +-
> > >  doc/guides/contributing/coding_style.rst           |   3 +-
> > >  examples/ip_pipeline/config/diagram-generator.py   |  13 +-
> > >  .../ip_pipeline/config/pipeline-to-core-mapping.py |  11 +-
> > >  tools/cpu_layout.py                                |  79 +-
> > >  tools/dpdk-devbind.py                              |  26 +-
> > >  tools/dpdk-pmdinfo.py                              |  73 +-
> > >  13 files changed, 1410 insertions(+), 1394 deletions(-)
> > >
> > > --
> > > 2.7.4
> > >
> > I think the changelog is deceptive.  It claims to make all the utilities
> > python2 and 3 compliant.  But compliance with python3 is more than just
> > stylistic formatting.  After this series several of these apps continue to
> > fail under python3.  dpdk-pmdinfo as an example:
> > 
> > [nhorman@hmsreliant dpdk]$ ./tools/dpdk-pmdinfo.py ./build/app/testacl
> > Traceback (most recent call last):
> >   File "./tools/dpdk-pmdinfo.py", line 607, in <module>
> >     main()
> >   File "./tools/dpdk-pmdinfo.py", line 596, in main
> >     readelf.process_dt_needed_entries()
> >   File "./tools/dpdk-pmdinfo.py", line 437, in process_dt_needed_entries
> >     rc = tag.needed.find("librte_pmd")
> > TypeError: a bytes-like object is required, not 'str'
> > 
> > 
> > I'm not saying its a bad patchset, but the changelog should reflect that
> > the change is purely stylistic, not functional.
> > 
> 
> Hi Neil,
> 
> Mea cupla. In my defense I did say in the cover letter that I'd tested that the apps compiled but that they needed extra testing. I did functionally test some of the apps that I was more familiar with, but not all of them. In particular the test apps need functional testing.
> 
> However, the changes need to be functional rather than just cosmetic so I'll look into fixing pmdinfo with Python 3, unless you'd prefer to do that ;-). Since pmdinfo is dealing with binary data it may be tricky. That is often one of the real challenges of porting Python 2 code to Python 3. Hopefully elftools is compatible. Anyway I'll look into it.
> 
> And just to be clear, I don't think this patchset should be merged until all of the apps have been functionally tested. I'll put something in the final patchset to indicate that the modified apps have been tested.
> 
I completely agree that the utilities should be functionally compatible with
python 3, but in regards to this patch set, all I'm really asking for is that
the changelog reflect that its just making stylistic changes to comply with pep8
(i.e. not fixing python 2/3 compatibility issues).  The latter can be handled in
subsequent patches piecemeal.

Neil

> John
> 
> 
> 
> 
> 
> 
>