DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Bruce Richardson <bruce.richardson@intel.com>,
	Neil Horman <nhorman@tuxdriver.com>, Ray Kinsella <mdr@ashroe.eu>
Cc: dev@dpdk.org, david.marchand@redhat.com
Subject: Re: [dpdk-dev] [PATCHv2] Remove validate-abi.sh from tree
Date: Thu, 09 Apr 2020 18:51:06 +0200	[thread overview]
Message-ID: <1831256.eGJsNajkDb@thomas> (raw)
In-Reply-To: <c328f842-2985-e713-be1a-26e33696c3b2@ashroe.eu>

09/04/2020 18:29, Ray Kinsella:
> On 09/04/2020 16:18, Thomas Monjalon wrote:
> > 09/04/2020 16:52, Ray Kinsella:
> >> On 09/04/2020 11:59, Thomas Monjalon wrote:
> >>> 09/04/2020 12:45, Ray Kinsella:
> >>>> On 09/04/2020 11:43, Bruce Richardson wrote:
> >>>>> On Thu, Apr 09, 2020 at 06:39:54AM -0400, Neil Horman wrote:
> >>>>>> On Thu, Apr 09, 2020 at 08:57:34AM +0100, Ray Kinsella wrote:
> >>>>>>> On 08/04/2020 20:56, Neil Horman wrote:
> >>>>>>>> +The syntax of the ``check-abi.sh`` utility is::
> >>>>>>>> +
> >>>>>>>> +   ./devtools/check-abi.sh <refdir> <newdir>
> >>>>>>>
> >>>>>>> (from v1 feedback)
> >>>>>>> Could we simplify this all greatly, by telling people to use the meson/ninja build,
> >>>>>>> so they get this checking out of the box, without all the headache below?
> >>>>>>>
> >>>>>> I think bruce noted that was never merged, correct?
> >>>>>>
> >>>>> Yep, correct. :-(
> >>>>
> >>>> apologies, was there a reason?
> >>>
> >>> Because build tool job is building, not checking.
> >>> It would be wrong to make (slow) checks mandatory in all builds.
> >>>
> >>> The need is to enforce checking ABI.
> >>> The result is already published by Travis in patchwork and in an
> >>> email to the author I believe.
> >>> Not checking email and patchwork is not a good excuse.
> >>>
> >>> Patchwork must be a mandatory read for everybody for all checks
> >>> in general. Let's not give up on general CI workflow.
> >>>
> >>
> >> Thomas 
> >>
> >> You are trying to solve two problems at once; CI tooling and ABI.
> >> Let's try to solve one at a time.
> > 
> > No, you want to mix two problems in a single tool :-)
> > 
> > 
> >> 1. The ABI check, will make the build _marginally_ slower.
> >> You _should_ only need to rebuild the changes between A and B.
> > 
> > Not so marginal.
> > A re-build takes less than a second. A mandatory check takes 10 secs
> > on my machine.
> > 
> > 
> >> 2. The meson/ninja are an order of magnitude faster than GNU Make. 
> >> We can afford this check.
> > 
> > I am doing such build 10 times (each target) per patch.
> > But that's not the main issue.
> > 
> > 
> >> 3. If we want to lessen the ABI burden and send the correct message.
> >> It should be a build blocker, contributors need to hear the message loud and clear.
> > 
> > The developer needs to get or build/save the ABI reference.
> > Making such ABI reference for each target is not so obvious:
> >   - all symbols must be enabled (dependencies)
> >   - some fixes may be needed for some compilers
> > 
> > 
> >> Most important people _consuming_ DPDK will never see this message.
> >> Only people _changing_ the ABI will see it - the people we want to hear the message loud and clear.
> > 
> > No, they will have issue in DPDK compilation if something in the check
> > goes wrong. We should not bother end users with internal checks.
> > 
> > 
> > The message is 
> > a) run the check by
> >   1) setting DPDK_ABI_REF_VERSION on command line or in devel.config file
> >   2) running devtools/test-build.sh or devtools/test-meson-builds.sh
> > b) check what Travis is reporting in
> >   - email to you
> >   - patchwork reports
> > 
> > I think Travis report is convenient to use.
> > The local check is integrated in build scripts
> > but cannot be run by default because of the reasons above.
> 
> Thomas the reality on this is that people have a tendency to filter
> this messages into an email folder and don't always see them. 
> 
> My 2c is that this will always be a struggle unless we find a way
> to make it un-ignore-able.
> Hence my build-wiring suggestion. 

My other concern is that we will have the same issue with all checks
done in a CI.
I think the right approach is to enforce people checking CI results.
They will be used to check CI in patchwork because the patches will
be blocked.



  reply	other threads:[~2020-04-09 16:51 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-08 19:56 Neil Horman
2020-04-09  7:57 ` Ray Kinsella
2020-04-09 10:39   ` Neil Horman
2020-04-09 10:43     ` Bruce Richardson
2020-04-09 10:45       ` Ray Kinsella
2020-04-09 10:59         ` Thomas Monjalon
2020-04-09 13:02           ` Neil Horman
2020-04-09 13:37             ` Thomas Monjalon
2020-04-09 14:52           ` Ray Kinsella
2020-04-09 15:18             ` Thomas Monjalon
2020-04-09 16:29               ` Ray Kinsella
2020-04-09 16:51                 ` Thomas Monjalon [this message]
2020-04-10  6:26                   ` Ray Kinsella
2020-04-10  7:57                     ` 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=1831256.eGJsNajkDb@thomas \
    --to=thomas@monjalon.net \
    --cc=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=mdr@ashroe.eu \
    --cc=nhorman@tuxdriver.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).