Re: [dpdk-dev] [PATCH v9 0/7] export PMD infos

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hi Neil,

I don't really understand why you don't accept I contribute to this
patchset. More details below.

2016-07-04 08:34, Neil Horman:
> On Mon, Jul 04, 2016 at 03:13:58AM +0200, Thomas Monjalon wrote:
> > This is a respin of the series from Neil.
> > It was planned to be integrated in 16.07-rc1 but the discovered issues
> > make a new revision needed.
> > There are still few things which could be improved but it is not mandatory
> > to fix them for an integration in 16.07-rc2:
> >   - fix make clean after pmdinfogen
> Clean works just fine, whats wrong with it?

"make clean" do not remove the .pmd.* files.
But it is not a blocking issue.

> >   - build installable pmdinfogen for target
> Again, this seems to be working just fine for me, what do you see as wrong with
> it?

If we install the SDK as a binary package for another arch, we cannot use
pmdinfogen as it is not cross-compiled (on purpose).
It was just a thought for possible future enhancement and is not blocking.

> >   - convert pmdinfo.py to Python 3
> This was discussed weeks ago earlier in this thread, since there is no
> requirement for python3 we decided not to use it.  Why renege on that now?

Yes we can take the script as is. It is just a note for future enhancement.
Not blocking.

> >   - document dependency pyelftools
> In what capacity beyond simply noting the fact that it imports that package?

I don't know where we could document this dependency.
Just a note, not blocking.

> > Changes done in this v9:
> >   - fix build dependency of drivers on pmdinfogen
> In what way is it broken?  The drivers directory is dependent on building
> pmdinfogen, what more do you want?

The drivers directory Makefile is a rte.subdir.mk. It does not handle
DEPDIRS. I've fix it in mk/rte.sdkbuild.mk
	drivers: | buildtools

> >   - fix build of mlx4, mlx5, aesni
> They weren't broken as of V8, not sure what you're getting at here

Yes they were because of typos.
I guess you were not compiling them.

> >   - fix new drivers bnxt, thunderx, kasumi
> >   - fix MAINTAINERS file
> >   - fix coding style in pmdinfogen
> >   - add compiler checks for pmdinfogen
> >   - remove useless functions in pmdinfogen
> >   - fail build if pmdinfogen fails (set -e)
> >   - fix verbose pmdinfogen run
> >   - build pmdinfogen in buildtools directory (was app)
> Are you kidding me!?  We just went 10 rounds over where to build this stupid
> application, and after I finally gave in to your demands, you change it one
> last time and call it a fix?  Thats a real slap in the face, thanks for that.

We were discussing reusing hostapp.mk.
And I was OK to use app/ as build directory.
After checking sdkinstall.mk, I changed my mind to build in buildtools/.
I do not call it a fix, just a change. Where do you see it is a fix? No slap.

> >   - install pmdinfogen in sdk package (was runtime)
> >   - fix CamelCase in pmdinfo.py
> >   - prefix executables with dpdk-
> Again, seriously?  Are you just trying to assert some sort of not invented here
> mentality?  We had this conversation over and over again weeks ago, but you
> couldn't possibly chime in then?  And you have to slip it in now, because this
> is the way you want it, as though I did it wrong previously?

You did not do it wrong.
After more thoughts, I really think we must be careful to have a consistent
namespace with tools we export publically.
I'm thinking to re-use the dpdk- prefix for other tools.
It's open to discussion as everything.

> >   - rename PMD_REGISTER_DRIVER -> RTE_REGISTER_DRIVER
> Annnd...one more, of course.  There simply no way you could leave this alone,
> and modify subsequent pending patches to match the existing macro format, is
> there?

I'm just trying to have a consistent namespace: RTE_ in code,
dpdk- for executables.

> >   - separate commit for hostapp.mk refresh
> >   - remove useless hostlib.mk
> What does this have to do with 'fixes' for this series?

It is not a fix, just a patch that I have added in the series.

> >   - spread doc in appropriate patches
> > 
> Again, why?  Whats wrong with adding documentation in its own patch?  To modify
> my work to fit your sensibilities and call it a fix is really quite an insult.

It is neither a fix nor an insult.
Having the doc and the code in the same patch helps to understand the patch.

> Honestly, at this point, I'm done.  Do what you want to it, I don't plan on
> making any more changes.

Please do not take it personnaly. I'm just trying to work on this
interesting feature.
Ideally you would have say "thank you for the work".
At least it would be interesting to have your review.