RE: [RFC PATCH v1 00/15] merge DTS core files to DPDK

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> >
> > 07/04/2022 07:04, Jerin Jacob:
> > > On Wed, Apr 6, 2022 at 8:26 PM Juraj Linkeš
> > > <juraj.linkes@pantheon.tech>
> > wrote:
> > > >
> 
> First of all, thanks for the feedback. We've already done a bit of a pre-review
> and this is where we decided we want the feedback from the rest of the
> community. We understand it's going to take time which is why we wanted to
> get feedback sooner than later.
> 
> > > > These are the basic libraries that other libraries depend on.
> > > > There's also the basic framework functionality related to test execution.
> > > >
> > > > Juraj Linkeš (15):
> > > >   dts: merge DTS dep/tclclient.tgz to DPDK
> > > >   dts: merge DTS dep/tgen.tgz to DPDK
> > > >   dts: merge DTS dts to DPDK
> > > >   dts: merge DTS framework/__init__.py to DPDK
> > > >   dts: merge DTS framework/asan_test.py to DPDK
> > > >   dts: merge DTS framework/checkCase.py to DPDK
> > > >   dts: merge DTS framework/dts.py to DPDK
> > > >   dts: merge DTS framework/exception.py to DPDK
> > > >   dts: merge DTS framework/logger.py to DPDK
> > > >   dts: merge DTS framework/packet.py to DPDK
> > > >   dts: merge DTS framework/project_dpdk.py to DPDK
> > > >   dts: merge DTS framework/serializer.py to DPDK
> > > >   dts: merge DTS framework/utils.py to DPDK
> > > >   dts: merge DTS main.py to DPDK
> > > >   dts: merge DTS version.py to DPDK
> > >
> > > merge->import
> > >
> 
> Ack. I'll keep this in mind when composing commit messages in the future.
> 
> > > >
> > > >  dts/dep/tclclient.tgz         |  Bin 0 -> 199327 bytes
> > > >  dts/dep/tgen.tgz              |  Bin 0 -> 134392 bytes
> > >
> > > Some top level comments:
> > > - I think, we should not check in binary files.
> >
> > +1
> >
> 
> Thanks, we'll make sure to make this a requirement - it makes sense to us as
> well.
Should we move these to the external repo where the GPL file would be stored? I do not expect these files to change frequently. But the external repo also needs to be tagged (along with tagging the DPDK/DTS).

> 
> > > - git commit comment should much more than "dts: merge DTS xxxx to
> > > DPDK" where the commit log should have details on check in.
> >
> > +1
> 
> Right, this is part of the broader question of how exactly are we going to
> move the whole thing. More below.
> 
> >
> > > -Add the documentation from the first patch and update the
> > > documentation per patch based on the content.
> >
> > +1
> >
> > More comments:
> >
> > - Please don't send so many patches, it looks like spam.
> > - Please let's start small with the very minimal code to run a dummy test.
> > - Split by file does not make sense
> 
> This is very useful. We started with splitting up DTS into logical chunks, but it's
> not quite what you have in mind. We'll move things around and send just the
> first part (containing just a dummy test (is a smoke test ok?) with the
> necessary code). Now that we have a better understanding of the first step
> we'll produce a more complete patch set, with proper commit messages and
> files in each commit.
I think bringing in this big chunk of code is a challenge. We need to discuss more on how to get this into DPDK. From that perspective:

1) the current set of patches are just the framework and documentation, no test suites yet. I am not sure if there are further parts that can be stripped out.

2) agree, the split by files is too much. We have tried to group the files into some logical components. So, would it be fine if we remove the commit by file, but still keep the logical component split? i.e. we will squash the files in a series into a single commit and then combine the logical splits as commits in a single series.

3) In my private discussions with David Marchand, he expressed interest in getting the git log history. The current review process will not help in this regard. Is this a must? If yes, are there any known methods to do this?

> 
> >
> > The process is going to be very long.
> > The techboard said in the past that we must have a very careful review
> > of an import piece by piece. So please be patient.
Agree, we need to give time for review.
I would prefer to set some timeline so that we are working accordingly. Currently, we are working with the goal of merging this in 22.07 (assuming the reviews are coming through quick enough). But, we can align on a different timeline.

> > Thank you
> >
> >