DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: David Marchand <david.marchand@redhat.com>,
	"Jiang, Cheng1" <cheng1.jiang@intel.com>,
	Maxime Coquelin <maxime.coquelin@redhat.com>,
	"Xia, Chenbo" <chenbo.xia@intel.com>, dev <dev@dpdk.org>,
	"Fu, Patrick" <patrick.fu@intel.com>,
	"Yang, YvonneX" <yvonnex.yang@intel.com>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"Hu, Jiayu" <jiayu.hu@intel.com>
Subject: Re: [dpdk-dev] [PATCH v10 0/4] add async data path in vhost sample
Date: Tue, 10 Nov 2020 14:34:43 +0000	[thread overview]
Message-ID: <20201110143442.GI1641@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <5954084.PnIyprfhLy@thomas>

On Tue, Nov 10, 2020 at 02:37:02PM +0100, Thomas Monjalon wrote:
> 10/11/2020 12:19, Bruce Richardson:
> > On Tue, Nov 10, 2020 at 09:17:36AM +0100, David Marchand wrote:
> > > On Tue, Nov 10, 2020 at 4:02 AM Jiang, Cheng1 <cheng1.jiang@intel.com> wrote:
> > > > > - This series breaks external compilation, as the external Makefile was not
> > > > > updated.
> > > > >
> > > >
> > > > I'm not sure I understand what you mean by external Makefile,
> > > > because as far as I know, makefile has been deprecated.
> > > 
> > > make support is dropped for dpdk compilation itself.
> > > 
> > > For the examples, the users will use make to compile them, as this is
> > > the only way provided to users *out of* dpdk.
> > > But the examples are compiled too via meson when compiling dpdk itself
> > > if you pass -Dexamples= options.
> > > 
> > > 
> > > Bruce,
> > > 
> > > I want to avoid more misses like this one.
> > > 
> > > If we want to keep the examples compilation in meson, we need a
> > > consistent framework to compile in both cases.
> > > Right now, we don't export meson for examples, and it makes no sense
> > > in their current form.
> > > It seems simpler to me to only maintain make support, and meson can
> > > still call make for each example.
> > > 
> > > Another solution is to rely only on test-meson-builds.sh, but then it
> > > ends up on the maintainer shoulders, so I prefer the solution above.
> > > 
> > > Other ideas?
> > > 
> > 
> > Hi David,
> > 
> > I've been thinking a bit about this since I got your email, but inspiration
> > for new ideas has yet to strike me.
> > 
> > While I can see the downside of having both make and meson build files for
> > the examples, I'm loath to see one of them dropped. Here is my current
> > thinking on it:
> > 
> > * We obviously need to keep the basic makefiles so as to make it easy for
> >   end-users to compile up and use the examples separate from DPDK itself.
> >   The makefiles serve as good examples as to how to pull the DPDK info from
> >   pkg-config.
> 
> The external compilation is part of the example, yes.
> 
> > * On the other hand, being able to build the examples as part of a regular
> >   meson build is a big advantage over having to build them separately, and
> >   allows errors with examples to be caught quicker.
> 
> In the past we had a Makefile which builds all examples.
> If you want, it can even been called at the end of meson DPDK compilation.
> 

Yes, we can do that, but my concern is not so much about having them built
as part of build tests, but rather having them part of a build workflow for
developers during development.  Thinking here in particular of those coding
with IDEs such as eclipse or VScode, which I know a lot of people use -
including myself from time to time, depending on what I am working on.

> >   It's also useful for those of us working on specific components
> >   with associated sample apps to have just that one app built constantly
> >   as we work.
> 
> I don't understand this point:
> 	ninja -C build && make -C examples/myexample
> 

That works fine for building on the commandline, but does not work well for
building as part of "build-on-save" inside an IDE, which is the biggest
reason I want to keep support for building examples using meson. For doing
development with a feature and associated sample app, being able to
configure your build folder to rebuild a particular sample app as part of a
main infrastructure rebuild is really useful - and works really quickly
too, since incremental builds of C file changes happen really fast with
meson.

> > * Therefore, while it looks like the more logical part to drop is indeed
> >   the meson support for the examples,
> 
> Yes, building the examples from the inside build system is strange,
> and hide issues.
> 

Sorry, while it may hide issues with the makefile because people weren't
really aware that they were sticking around, it definitely helps pick up
issues with C code changes as you are developing.

> >   we may struggle to implement clean
> >   building of the examples from meson using make, at least until meson 0.54
> >   becomes our standard. Before that version, we don't have a
> >   "meson-uninstalled" folder with build-time package-config file to use as
> >   source of build flags for each example.
> 
> We don't have to use meson at all.
> 

No, we don't, so long as we don't miss out on the benefits we currently get
from having it integrated.

> > * One final idea I had and investigated in the past was whether we could at
> >   build or install time auto-generate the Makefile for each example from
> >   the meson.build file. Unfortunately, nothing came of that idea the first
> >   time I tried it, but it might still be worth looking at. Even if it works
> >   for 80-90% of cases, it means that we have a much smaller subset of
> >   examples where we need to test independently the make and meson builds.
> 
> Hand-crafted Makefile is enough. They may be improved.
> If we feel it is too hard, we can use another build system
> in examples, like cmake.
> 
> > So overall my assessment is that it needs a bit of investigation and
> > prototyping to see what we can come up with.
> 
> I think testing external build + removing build from internal meson
> would be a good start to ensure quality of examples maintenance.
> 
If we remove the meson build of the examples, I think we need equivalent
functionality provided some other way. Calling make for examples from
within meson may be good enough, so long as it's not too slow.

> > On a semi-related note, it's perhaps a bigger problem that we cannot rely
> > on test-meson-builds and CI infrastructure to prevent issues like this.
> 
> We can. We just have to add all examples in test-meson-builds.sh.
> 

Yep, definite +1 here.
Once that is done, I'd then like to wait and see what future issues crop up
before we start ripping out other bits. This is the first release where we
have removed the make build system, so there are lots of teething issues
all over the place, of which this is but one example!

> > Surely this is what CI is there for - to help reduce the workload for
> > maintainers. The fact that we are considering removing the meson build of
> > examples because we cannot rely on CI is surely more worthy of a solution
> > than trying to find a way to build examples with make from within meson?
> 
> No the concern is to have all contributors work on the same
> single build path.
> 
Building all examples from test-meson-build should probably be sufficient
here, I think.

> > Perhaps we need to see about stricter measures from CI failure, e.g.
> > anything failing travis build automatically gets marked as changes
> > requested in patchwork, and the author gets an email informing them of
> > such?
> 
> When there is a failure, authors receive an email,
> and maintainers can see a red flag. I think it's OK.
> 
> The only issue was that this build path was not tested.
> I think David is going to fix it by compiling all possible examples
> with external make build from test-meson-builds.sh.
> 

Seems like we are all aligned on the first step anyway. Let's see beyond
that what needs to be done in 21.02.

/Bruce

  reply	other threads:[~2020-11-10 14:34 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-10  6:43 [dpdk-dev] [PATCH v1 " Cheng Jiang
2020-09-10  6:43 ` [dpdk-dev] [PATCH v1 1/4] example/vhost: add async vhost driver args parsing function Cheng Jiang
2020-09-23  8:25   ` Maxime Coquelin
2020-09-28  6:09     ` Jiang, Cheng1
2020-09-10  6:43 ` [dpdk-dev] [PATCH v1 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-09-10  6:43 ` [dpdk-dev] [PATCH v1 3/4] doc: update vhost sample doc " Cheng Jiang
2020-09-10  6:43 ` [dpdk-dev] [PATCH v1 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-09-29  6:42 ` [dpdk-dev] [PATCH v2 0/4] add async data path in " Cheng Jiang
2020-09-29  6:42   ` [dpdk-dev] [PATCH v2 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-09-29  6:42   ` [dpdk-dev] [PATCH v2 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-09-29  6:42   ` [dpdk-dev] [PATCH v2 3/4] doc: update vhost sample doc " Cheng Jiang
2020-09-29  6:42   ` [dpdk-dev] [PATCH v2 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-09-30  3:08   ` [dpdk-dev] [PATCH v3 0/4] add async data path in " Cheng Jiang
2020-09-30  3:08     ` [dpdk-dev] [PATCH v3 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-09-30  3:08     ` [dpdk-dev] [PATCH v3 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-09-30  3:08     ` [dpdk-dev] [PATCH v3 3/4] doc: update vhost sample doc " Cheng Jiang
2020-09-30  3:08     ` [dpdk-dev] [PATCH v3 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-10-12  4:54     ` [dpdk-dev] [PATCH v4 0/4] add async data path in " Cheng Jiang
2020-10-12  4:54       ` [dpdk-dev] [PATCH v4 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-10-14  9:23         ` Maxime Coquelin
2020-10-15  5:11           ` Jiang, Cheng1
2020-10-12  4:54       ` [dpdk-dev] [PATCH v4 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-10-12  4:54       ` [dpdk-dev] [PATCH v4 3/4] doc: update vhost sample doc " Cheng Jiang
2020-10-12  4:54       ` [dpdk-dev] [PATCH v4 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-10-15  4:54       ` [dpdk-dev] [PATCH v5 0/4] add async data path in " Cheng Jiang
2020-10-15  4:54         ` [dpdk-dev] [PATCH v5 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-10-15 15:13           ` Maxime Coquelin
2020-10-15  4:54         ` [dpdk-dev] [PATCH v5 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-10-15  4:54         ` [dpdk-dev] [PATCH v5 3/4] doc: update vhost sample doc " Cheng Jiang
2020-10-15  4:54         ` [dpdk-dev] [PATCH v5 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-10-16  4:29         ` [dpdk-dev] [PATCH v6 0/4] add async data path in " Cheng Jiang
2020-10-16  4:29           ` [dpdk-dev] [PATCH v6 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-10-16  4:29           ` [dpdk-dev] [PATCH v6 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-10-16  4:29           ` [dpdk-dev] [PATCH v6 3/4] doc: update vhost sample doc " Cheng Jiang
2020-10-16  4:29           ` [dpdk-dev] [PATCH v6 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-10-19  5:49             ` Jiang, Cheng1
2020-10-20 11:20           ` [dpdk-dev] [PATCH v7 0/4] add async data path in " Cheng Jiang
2020-10-20 11:20             ` [dpdk-dev] [PATCH v7 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-10-20 11:20             ` [dpdk-dev] [PATCH v7 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-10-20 11:20             ` [dpdk-dev] [PATCH v7 3/4] doc: update vhost sample doc " Cheng Jiang
2020-10-20 11:20             ` [dpdk-dev] [PATCH v7 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-10-21  6:50             ` [dpdk-dev] [PATCH v8 0/4] add async data path in " Cheng Jiang
2020-10-21  6:50               ` [dpdk-dev] [PATCH v8 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-10-21  6:50               ` [dpdk-dev] [PATCH v8 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-10-21  6:50               ` [dpdk-dev] [PATCH v8 3/4] doc: update vhost sample doc " Cheng Jiang
2020-10-21  6:50               ` [dpdk-dev] [PATCH v8 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-10-22  6:46               ` [dpdk-dev] [PATCH v9 0/4] add async data path in " Cheng Jiang
2020-10-22  6:46                 ` [dpdk-dev] [PATCH v9 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-10-22  6:46                 ` [dpdk-dev] [PATCH v9 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-10-22  6:46                 ` [dpdk-dev] [PATCH v9 3/4] doc: update vhost sample doc " Cheng Jiang
2020-10-22  6:46                 ` [dpdk-dev] [PATCH v9 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-10-22  9:10                 ` [dpdk-dev] [PATCH v9 0/4] add async data path in " Maxime Coquelin
2020-10-22  9:14                   ` Jiang, Cheng1
2020-10-22  8:59 ` [dpdk-dev] [PATCH v10 " Cheng Jiang
2020-10-22  8:59   ` [dpdk-dev] [PATCH v10 1/4] example/vhost: add async vhost args parsing function Cheng Jiang
2020-10-23 11:08     ` Maxime Coquelin
2020-10-22  8:59   ` [dpdk-dev] [PATCH v10 2/4] example/vhost: add support for vhost async data path Cheng Jiang
2020-10-23 11:12     ` Maxime Coquelin
2020-10-22  8:59   ` [dpdk-dev] [PATCH v10 3/4] doc: update vhost sample doc " Cheng Jiang
2020-10-23 11:15     ` Maxime Coquelin
2020-10-22  8:59   ` [dpdk-dev] [PATCH v10 4/4] doc: update release notes for vhost sample Cheng Jiang
2020-10-23 11:15     ` Maxime Coquelin
2020-10-23 11:23   ` [dpdk-dev] [PATCH v10 0/4] add async data path in " Maxime Coquelin
2020-10-23 13:20     ` Ferruh Yigit
2020-11-09 12:40     ` David Marchand
2020-11-10  3:02       ` Jiang, Cheng1
2020-11-10  8:17         ` David Marchand
2020-11-10 11:19           ` Bruce Richardson
2020-11-10 13:37             ` Thomas Monjalon
2020-11-10 14:34               ` Bruce Richardson [this message]
2020-11-10 14:40                 ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201110143442.GI1641@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=chenbo.xia@intel.com \
    --cc=cheng1.jiang@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jiayu.hu@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=patrick.fu@intel.com \
    --cc=thomas@monjalon.net \
    --cc=yvonnex.yang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).