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

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

2016-07-04 12:41, 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.

Thank you for explaining your feeling so clearly.
I'm sorry for any inconvenience or misunderstanding.
I now understand that you prefer I make some patches separately after
integrating your version.
So I can send a v10 based on your v8 which only fixes compilation.
Or you can do it yourself if you prefer.
Then I'll send another series for the other changes.
Deal?

> > 2016-07-04 08:34, Neil Horman:
> > > On Mon, Jul 04, 2016 at 03:13:58AM +0200, Thomas Monjalon wrote:
> > > > 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.

I didn't want to focus on these typos but as you insist, please check v8:
	http://dpdk.org/ml/archives/dev/2016-June/041994.html
Example:
	-PMD_REGISTER_DRIVER(rte_mlx4_driver)
	+PMD_REGISTER_DRIVER(rte_mlx4_driveri, mlx4)
	+DRIVER_REGISTER_PCI_TABLE(mlx4, mlx4_pci_id_map);
Which suggests that you are using vi: rte_mlx4_driveri
And as far as I remember your rework of PMD_REGISTER_DRIVER requires
to end the line with a semi-colon.

> > > >   - 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.

I was in favor of having a generic hostapp.mk. It's still the case.
I just changed my mind on building in build/buildtools.
I thought it was not a big deal and I'm sorry.

> > > >   - 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.

No you said you would prefix pmdinfogen but you did not.
As you said me you would add an entry in MAINTAINERS but you did not.
That's why I did it, thinking you agreed.

> 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

The existing PMD_REGISTER_DRIVER was already your work.
I think it would be better to use RTE_ prefix for consistency but I
will send a separate patch for it.

> 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)

By rejecting the patches, it clearly shows you are not OK with 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. 

OK

> 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.
[...]
> > 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.

Precision, I co-signed the patches.

> 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.

When I've seen your laconic answer to Remy pointing a required change:
	http://dpdk.org/ml/archives/dev/2016-June/042965.html
I've understood you didn't want to fix more things.
While at fixing those compilation errors, I've merged other changes, yes.

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

No need to review this v9.
We just need to agree on who makes the v10 based on v8 to fix compilation.