Re: [PATCH v4 4/9] dts: add ssh pexpect library

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Sep 23, 2022 at 07:22:26AM +0000, Juraj Linkeš wrote:
> 
> 
> > -----Original Message-----
> > From: Stanislaw Kardach <kda@semihalf.com>
> > Sent: Thursday, September 22, 2022 4:32 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: thomas@monjalon.net; david.marchand@redhat.com;
> > ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com;
> > ohilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org
> > Subject: Re: [PATCH v4 4/9] dts: add ssh pexpect library
> > 
> > On Thu, Sep 22, 2022 at 09:41:40AM +0000, Juraj Linkeš wrote:
> > > Hi Stanislaw,
> > First a preface. I understand that the DTS rework is sponsored by someone and
> > there may be people waiting with their labs for this job to be done.
> > Everything that I'll write below is from a point of view of a developer who'd like
> > to utilize DTS as a test suite for DPDK when adding support for new PMDs or
> > architectures/boards. This might be in conflict with time-to-market metric at this
> > point in time but I'm more focused on the state of DPDK+DTS in the long run.
> > So feel free to disregard my comments if there are higher priorities.
> > >
> > > Neither of the current DTS maintainer nor me are the author of the code, so
> > we can only speculate as to why certain parts were implemented the way they
> > are.
> > >
> > > We've thought a lot about replacing pexpect with something else, such as
> > Fabric. Fabric is supposedly faster, which is the biggest draw and instead of
> > fixing/reworking pexpect code, it makes sense to switch our focus on Fabric. For
> > this PoC version though, we'd like to stay with this pexpect code and work on
> > other appoaches in the next release cycle. The code is well tested so there's not
> > much point in poking in it if it's to be replaced.
> > I have a nasty experience of code staying without re-factoring for long "because
> > it works". When it comes to DTS my experience is that it works only if used
> > exactly on the setups it was meant for. Adapting it to a new setup, new PMD or
> > "even" running in the cloud shows that parts of it are held together with a string.
> > I'm not blaming DTS devs here. Such approach is often needed for various
> > reasons (usually time-to-market) and it's hard to be forward-compatible.
> > 
> > That said I would suggest to use this opportunity to refactor DTS while it's still
> > not merged. Otherwise we'll be left with code that we're uncertain why it works.
> > That's not a quality-first approach and it'll bite us in the backside in the future.
> > 
> > Let's do things right, not fast.
> 
> Absolutely, but effective time use is also something to consider. Our current plan doesn't won't really have to contend with problems in the future, as we want to add the Farbic implementation in the next release cycle. I'm also working on refactoring the code a bit - I'm adding an abstraction that would allow us to easily replace the pexpect implementation with Fabric (with no impact on DTS behavior - the same APIs will need to be implemented). Also, we'll remove the pexpect implementation once Fabric is in place (unless we can think of a reason for pexpect to stay, in which case we'll need to refactor it). I think that instead of focusing on pexpect we could focus on making sure the replacement won't cause any issues. What do you think?
> 

Personally, I would be very keen to get the move of DTS to the main repo
underway, and so I wouldn't look to have too many massive changes required
before we start seeing patches merged in. Basic code cleanup and
refactoring is fine, but I would think that requiring massive changes like
replacing expect with fabric may be too big an ask. After all, the rest of
DPDK is moving on, meaning more and more DTS content is being added to the
separate DTS repo every release, making the job bigger each time. :-(

Tl;dr - I'm ok to leave fabric replacement for a release next year.

/Bruce