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

[Neil Horman]

On Mon, Jul 04, 2016 at 03:10:14PM +0200, Thomas Monjalon wrote:
> Hi Neil,
> 
> I don't really understand why you don't accept I contribute to this
> patchset. More details below.
> 
I don't object to your contribution to this patchset.  What I object to is you
making changes contrary to what I just spent I don't even know how many weeks
arguing with you and others about.  Given your role as maintainer, it is
tantamout to a 'my way or the highway' position.

> 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.
> 
They don't really need to be removed, as pmdinfogen will overwrite them
unilaterally when a rebuild is done.

> > >   - 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.
> 
Ah, you want cross-compile support, rather than just native target support?  I
can agree with that.

> > >   - 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.
Ok, but again, this was discussed weeks ago, the way its written currently
should work with python2 or python3 (dependent on the python environment setup).
converting any code to make it work better/more efficiently/etc with python3
seems like a step backwards

> 
> > >   - 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.
> 
Ok, so you dont have any standard here, you just have some vague desire
for...something.  No other python script bothers with documenting what it
imports, so I think you can understand when you list this as an enhancement for
my work, but don't worry about any of the other work, its both confusing and
frustrating.

> > > 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.
> 
No, I specifically compiled them in v8, as they were called out.

> > >   - 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.
> 
No, its decidedly a slap.  You and I argued about this back and forth very
publically over the course of a week, and I finally aquiesced to your demands.
If you've changed your mind, thats fine I suppose, but to do so after you've
gotten your way is fairly frustrating.  To then make the change and repost it
without any prior consultation sends a very clear signal that my opinion on the
subject doesn't really matter.  You've put me in a position where I now either
need to say whatever change you want to make is ok with me, or I need to argue
against the thing that I wanted in the first place.  Its very insulting.

> > >   - 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.
Except that we already had the discussion!  Panu, myself, and several others
went over this over a week ago, and I made the stated changes in response.  To
have you then post yet another version on my behalf sends a clear signal that
both our opinions and my work is irrelevant.  If you disagree with it, please
say so, but as the maintainer, posting a new version of someone elses work is
a fairly heavy handed way of saying that the default is the way you want it, and
we are all welcome to offer opinions on that.


> 
> > >   - 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.
> 
Ok, thats a fine opinion I suppose, but its both:

1) My work
2) the existing pattern

This patchset is my work.  If you want to add features, fix things, ask me to,
and we can have that discussion (I personally disagree with it, but as we've
seen previously, I'm willing to bend).  Point being, its my work, I don't
appreciate you making these sorts of changes on my behalf, as it immediately
implies I'm ok with them (they've still got my sign offs on them)

Its also orthogonal to the purpose of the patchset.  If you want to rewrite the
macro name, fine, submit a patchset that does so, please don't co-opt my work to
get your changes in.  I honestly have no opinion regarding this macro name, so
if you want to change it, its fine, but there was no need to do it as part of
this patchset. 

I'll also note that you changed the REGISTER_DRIVER macro, but left all the new
macros (REGISTER_CMDLINE_STRING and REGISTER_PCI_TBL) with the PMD_ prefix, so
you've created a bifurcation here.

> > >   - 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.
> 
Again, completely orthogonal to the purpose of 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".
Thomas, you took my work, changed it in ways I don't agree with, reverted things
I fought you on, and eventually aquiesced to, added changes that have no
association with the purpose of this series, and signed my name to it.

I apprecate the desire to help, but the way you've gone about this has
effectively said to me that you feel like I can't do it the way you want, so
you'll just do it for me.  Please understand how insulting that is.

> At least it would be interesting to have your review.
> 
Maybe, I need to take a breath over this.

Neil