From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 1C98B42CB2;
	Mon, 19 Jun 2023 10:00:21 +0200 (CEST)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id AD95F40DF8;
	Mon, 19 Jun 2023 10:00:20 +0200 (CEST)
Received: from us-smtp-delivery-124.mimecast.com
 (us-smtp-delivery-124.mimecast.com [170.10.129.124])
 by mails.dpdk.org (Postfix) with ESMTP id E4F3A40A81
 for <dev@dpdk.org>; Mon, 19 Jun 2023 10:00:18 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
 s=mimecast20190719; t=1687161618;
 h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
 to:to:cc:cc:mime-version:mime-version:content-type:content-type:
 content-transfer-encoding:content-transfer-encoding:
 in-reply-to:in-reply-to:references:references;
 bh=eNDDKs/1HASQfoGrsfc0chUPkCDY5lxWNW2dMIwcAfs=;
 b=eUc6wz00SmPhYzirqS+8kk8Z9vK4LKuB5Xi0gYZO2z12bGv9VYIpV+S/fh/o3sttAxAILn
 xdPbUZGloh6ka7PTVLthXoCjS+yHylZjaqCfpGua+KAiuZQrTrf4aj5vo23KzdZ6W848J4
 n5oiysUbuACPeYwGbILqBJPh7rizEsc=
Received: from mail-pj1-f72.google.com (mail-pj1-f72.google.com
 [209.85.216.72]) by relay.mimecast.com with ESMTP with STARTTLS
 (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id
 us-mta-617-FDtYVAcXOm62ajdLZSQ7bw-1; Mon, 19 Jun 2023 04:00:15 -0400
X-MC-Unique: FDtYVAcXOm62ajdLZSQ7bw-1
Received: by mail-pj1-f72.google.com with SMTP id
 98e67ed59e1d1-25eb3db302eso2569292a91.3
 for <dev@dpdk.org>; Mon, 19 Jun 2023 01:00:14 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20221208; t=1687161614; x=1689753614;
 h=content-transfer-encoding:cc:to:subject:message-id:date:from
 :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc
 :subject:date:message-id:reply-to;
 bh=eNDDKs/1HASQfoGrsfc0chUPkCDY5lxWNW2dMIwcAfs=;
 b=L/FFodGfXCJ38nboi1L7v9V4+pazR7JkF5CYrRkwnXWV9KNPYV0tg2CDsBUwZpkcLR
 PMPJYzq/EA+kEE+ydDgM0WuGcpsTZ+juCbhrfGnu0kOXKKeRRfxPhtDJoKTdKxB924UB
 WDEhrxuTsV9Y7eFHb8Bf+UNPAs50X9N5U6Ppx+eU8Z8JDYaGEf8hJAAvOUAY6n1Z7Hr0
 Gno/seLhNLNYkMGVKKivWS4CnxD2P2BCRobNmneAhd9w7lDco8S9QYqMluSnAFeiR/9T
 Pfn+/gmWns1RgzZ9xq8kPuPs2HXoiDEU8z0f2KDlB3jXtQ2zeEnK4Gwg0k8yjfP4twvA
 BrAg==
X-Gm-Message-State: AC+VfDx+m+Eb8HdrT5eFQ7APGZ7trpiftIWbLrXz1bPvHxcKKIhhlqUf
 c+ouFcPSm2fvKbM5RtjJJWbt0tE4vLW3qjiP31THko1QARGvO1IkjIJCs07FpxkHgdFrDgk6+Bw
 +RHg7KltorRCIHWhefwY=
X-Received: by 2002:a17:90a:9ae:b0:25e:a643:adeb with SMTP id
 43-20020a17090a09ae00b0025ea643adebmr8660202pjo.39.1687161613802; 
 Mon, 19 Jun 2023 01:00:13 -0700 (PDT)
X-Google-Smtp-Source: ACHHUZ7t9f1q0UtkrTZvBarjg5qQQRfEjK1sr4OegeOVeYkqVC7smtUoAX8h8ZSOaqEYLNe5v9B73n+daHMlasXwViU=
X-Received: by 2002:a17:90a:9ae:b0:25e:a643:adeb with SMTP id
 43-20020a17090a09ae00b0025ea643adebmr8660181pjo.39.1687161613439; Mon, 19 Jun
 2023 01:00:13 -0700 (PDT)
MIME-Version: 1.0
References: <20211117112847.7362-6-david.marchand@redhat.com>
 <20230616071450.3542479-1-david.marchand@redhat.com>
 <ZIwuaz1eCSN3pyxy@bricha3-MOBL.ger.corp.intel.com>
In-Reply-To: <ZIwuaz1eCSN3pyxy@bricha3-MOBL.ger.corp.intel.com>
From: David Marchand <david.marchand@redhat.com>
Date: Mon, 19 Jun 2023 10:00:02 +0200
Message-ID: <CAJFAV8wUqn6UbngGB8G329vOJps=ZwzE=jKiLf18D-JJQ1Nepg@mail.gmail.com>
Subject: Re: [PATCH v3] build: select optional libraries
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org, thomas@monjalon.net, bluca@debian.org, tredaelli@redhat.com, 
 i.maximets@ovn.org, james.r.harris@intel.com, mohammed@hawari.fr, 
 Aaron Conole <aconole@redhat.com>, Michael Santana <maicolgabriel@hotmail.com>
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

On Fri, Jun 16, 2023 at 11:42=E2=80=AFAM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Fri, Jun 16, 2023 at 09:14:50AM +0200, David Marchand wrote:
> > There is currently no way to know which libraries are optional.
> > Introduce a enable_libs option (close to what we have for drivers) so
> > that packagers or projects consuming DPDK can more easily select the
> > optional libraries that matter to them and disable other optional
> > libraries.
> >
> > Deprecated libraries are handled via some logic in lib/meson.build
> > rather than a default value in meson_options.txt.
> > Enabling deprecated libraries is achieved by requesting all
> > libraries to be built in the CI.
> >
> > kni dependency to IOVA configuration is moved to its own meson.build.
> >
> > Note: the enabled_libs variable is renamed for sake of consistency wrt =
to
> > drivers and applications similar variables.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> Thanks David.
> Some comments inline below just on the usability aspects of this.
>
> /Bruce
>
> > The topic was raised again while Stephen was reviewing stale patches in
> > patchwork. The idea of this patch is the same as before.
> > I simply rebased the change (and dealt with the deprecated libraries th=
at
> > added some little complication).
> >
> > Changes since v2:
> > - moved the IOVA check for kni build in lib/kni/meson.build,
> > - reworked deprecated libraries handling by only considering them when
> >   no enable_libs option is set. With this, no need for a default value
> >   in meson_options.txt, yet configurations in the CI must be adjusted,
> > - moved mandatory libraries check after enable/disable_libs evaluation,
> >
> > ---
> >  .ci/linux-build.sh             |  2 +-
> >  app/test/meson.build           |  2 +-
> >  buildtools/chkincs/meson.build |  2 +-
> >  devtools/test-meson-builds.sh  |  4 ++--
> >  lib/kni/meson.build            |  5 ++++
> >  lib/meson.build                | 44 ++++++++++++++++++++--------------
> >  meson.build                    |  3 ++-
> >  meson_options.txt              |  4 +++-
> >  8 files changed, 41 insertions(+), 25 deletions(-)
> >
> > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> > index 9631e342b5..a4e474a900 100755
> > --- a/.ci/linux-build.sh
> > +++ b/.ci/linux-build.sh
> > @@ -97,7 +97,7 @@ if [ "$MINI" =3D "true" ]; then
> >      OPTS=3D"$OPTS -Denable_drivers=3Dnet/null"
> >      OPTS=3D"$OPTS -Ddisable_libs=3D*"
> >  else
> > -    OPTS=3D"$OPTS -Ddisable_libs=3D"
> > +    OPTS=3D"$OPTS -Denable_libs=3D*"
> >  fi
> >
> >  if [ "$ASAN" =3D "true" ]; then
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index d0fabcbb8b..f3217ae577 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -152,7 +152,7 @@ test_sources =3D files(
> >          'virtual_pmd.c',
> >  )
> >
> > -test_deps =3D enabled_libs
> > +test_deps =3D dpdk_libs_enabled
> >  # as well as libs, the pci and vdev bus drivers are needed for a lot o=
f tests
> >  test_deps +=3D ['bus_pci', 'bus_vdev']
> >
> > diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.=
build
> > index 378c2f19ef..f2dadcae18 100644
> > --- a/buildtools/chkincs/meson.build
> > +++ b/buildtools/chkincs/meson.build
> > @@ -22,7 +22,7 @@ sources +=3D gen_c_files.process(dpdk_chkinc_headers)
> >  # so we always include them in deps list
> >  deps =3D [get_variable('shared_rte_bus_vdev'), get_variable('shared_rt=
e_bus_pci')]
> >  # add the rest of the libs to the dependencies
> > -foreach l:enabled_libs
> > +foreach l:dpdk_libs_enabled
> >      deps +=3D get_variable('shared_rte_' + l)
> >  endforeach
> >
> > diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds=
.sh
> > index 9131088c9d..4f93702e1f 100755
> > --- a/devtools/test-meson-builds.sh
> > +++ b/devtools/test-meson-builds.sh
> > @@ -122,8 +122,8 @@ config () # <dir> <builddir> <meson options>
> >       options=3D
> >       # deprecated libs may be disabled by default, so for complete bui=
lds ensure
> >       # no libs are disabled
> > -     if ! echo $* | grep -q -- 'disable_libs' ; then
> > -             options=3D"$options -Ddisable_libs=3D"
> > +     if ! echo $* | grep -q -- 'enable_libs' ; then
> > +             options=3D"$options -Denable_libs=3D*"
> >       fi
> >       if echo $* | grep -qw -- '--default-library=3Dshared' ; then
> >               options=3D"$options -Dexamples=3Dall"
> > diff --git a/lib/kni/meson.build b/lib/kni/meson.build
> > index 8a71d8ba6f..5ce410f7f2 100644
> > --- a/lib/kni/meson.build
> > +++ b/lib/kni/meson.build
> > @@ -7,6 +7,11 @@ if is_windows
> >      subdir_done()
> >  endif
> >
> > +if dpdk_conf.get('RTE_IOVA_IN_MBUF') =3D=3D 0
> > +    build =3D false
> > +    reason =3D 'requires IOVA in mbuf (set enable_iova_as_pa option)'
> > +endif
> > +
> >  if not is_linux or not dpdk_conf.get('RTE_ARCH_64')
> >      build =3D false
> >      reason =3D 'only supported on 64-bit Linux'
> > diff --git a/lib/meson.build b/lib/meson.build
> > index 9677239236..6018f5230d 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -92,20 +92,18 @@ dpdk_libs_deprecated +=3D [
> >          'kni',
> >  ]
> >
> > -disabled_libs =3D []
> > -opt_disabled_libs =3D run_command(list_dir_globs, get_option('disable_=
libs'),
> > +disable_libs =3D run_command(list_dir_globs, get_option('disable_libs'=
),
> >          check: true).stdout().split()
> > -if dpdk_conf.get('RTE_IOVA_IN_MBUF') =3D=3D 0
> > -    opt_disabled_libs +=3D ['kni']
> > +enable_libs =3D run_command(list_dir_globs, get_option('enable_libs'),
> > +        check: true).stdout().split()
> > +if enable_libs.length() =3D=3D 0
> > +    enable_libs =3D []
> > +    foreach l:run_command(list_dir_globs, '*', check: true).stdout().s=
plit()
> > +        if l not in dpdk_libs_deprecated
> > +            enable_libs +=3D [l]
> > +        endif
> > +    endforeach
>
> The enable libraries option is really only used for enabling optional
> libraries - mandatory libs should not need to be specified. I'm wondering
> about if we should append the mandatory libs to the glob search
> automatically. This specifically concerns the error messsage about
> mandatory libs being disabled below....

Good point.
Let me relook at this.


>
> >  endif
> > -foreach l:opt_disabled_libs
> > -    if not optional_libs.contains(l)
> > -        warning('Cannot disable mandatory library "@0@"'.format(l))
> > -        continue
> > -    endif
> > -    disabled_libs +=3D l
> > -endforeach
> > -
> >
> >  default_cflags =3D machine_args
> >  default_cflags +=3D ['-DALLOW_EXPERIMENTAL_API']
> > @@ -115,8 +113,6 @@ if cc.has_argument('-Wno-format-truncation')
> >      default_cflags +=3D '-Wno-format-truncation'
> >  endif
> >
> > -enabled_libs =3D [] # used to print summary at the end
> > -
> >  foreach l:libraries
> >      build =3D true
> >      reason =3D '<unknown reason>' # set if build =3D=3D false to expla=
in why
> > @@ -141,13 +137,25 @@ foreach l:libraries
> >          deps +=3D ['eal']
> >      endif
> >
> > -    if disabled_libs.contains(l)
> > +    if not enable_libs.contains(l)
> > +        build =3D false
> > +        reason =3D 'not in enabled libraries build config'
> > +    elif disable_libs.contains(l)
> >          build =3D false
> >          reason =3D 'explicitly disabled via build config'
> > -        if dpdk_libs_deprecated.contains(l)
> > +    endif
> > +
> > +    if not build
> > +        if not optional_libs.contains(l)
> > +            warning('Cannot disable mandatory library "@0@"'.format(l)=
)
> > +            build =3D true
> > +            reason =3D '<unknown reason>'
> > +        elif dpdk_libs_deprecated.contains(l)
>
> This error message seems weird/wrong in the case that the user uses
> "enable_libs" option rather than "disable_libs". If the user specified he
> wanted to enable the vhost lib, for example, it would be strange getting =
an
> error about having disabled eal, mbuf etc. etc.

Ack.


>
> >              reason +=3D ' (deprecated lib)'
> >          endif
> > -    else
> > +    endif
> > +
> > +    if build
> >          if dpdk_libs_deprecated.contains(l)
> >              warning('Enabling deprecated library, "@0@"'.format(l))
> >          endif
> > @@ -183,7 +191,7 @@ foreach l:libraries
> >          continue
> >      endif
> >
> > -    enabled_libs +=3D name
> > +    dpdk_libs_enabled +=3D name
> >      dpdk_conf.set('RTE_LIB_' + name.to_upper(), 1)
> >      install_headers(headers)
> >      install_headers(indirect_headers)
> > diff --git a/meson.build b/meson.build
> > index 992ca91e88..39cb73846d 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -44,6 +44,7 @@ dpdk_extra_ldflags =3D []
> >  dpdk_libs_deprecated =3D []
> >  dpdk_apps_disabled =3D []
> >  dpdk_libs_disabled =3D []
> > +dpdk_libs_enabled =3D []
> >  dpdk_drvs_disabled =3D []
> >  testpmd_drivers_sources =3D []
> >  testpmd_drivers_deps =3D []
> > @@ -134,7 +135,7 @@ message(output_message + '\n')
> >  output_message =3D '\n=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D\nLibraries Enabled\n=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
\n'
> >  output_message +=3D '\nlibs:\n\t'
> >  output_count =3D 0
> > -foreach lib:enabled_libs
> > +foreach lib:dpdk_libs_enabled
> >      output_message +=3D lib + ', '
> >      output_count +=3D 1
> >      if output_count =3D=3D 8
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 82c8297065..af54f8b8d4 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -10,7 +10,7 @@ option('disable_apps', type: 'string', value: '', des=
cription:
> >         'Comma-separated list of apps to explicitly disable.')
> >  option('disable_drivers', type: 'string', value: '', description:
> >         'Comma-separated list of drivers to explicitly disable.')
> > -option('disable_libs', type: 'string', value: 'flow_classify,kni', des=
cription:
> > +option('disable_libs', type: 'string', value: '', description:
> >         'Comma-separated list of libraries to explicitly disable. [NOTE=
: not all libs can be disabled]')
> >  option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VE=
RSION>', description:
> >         'Subdirectory of libdir where to install PMDs. Defaults to usin=
g a versioned subdirectory.')
> > @@ -20,6 +20,8 @@ option('enable_apps', type: 'string', value: '', desc=
ription:
> >         'Comma-separated list of apps to build. If unspecified, build a=
ll apps.')
> >  option('enable_drivers', type: 'string', value: '', description:
> >         'Comma-separated list of drivers to build. If unspecified, buil=
d all drivers.')
> > +option('enable_libs', type: 'string', value: '', description:
> > +       'Comma-separated list of libraries to explicitly enable.')
>
> s/libraries/optional libraries/ ??
> Maybe like with the disable option, add a note that mandatory libs are
> always enabled?

I'll update this too.


>
> >  option('enable_driver_sdk', type: 'boolean', value: false, description=
:
> >         'Install headers to build drivers.')
> >  option('enable_kmods', type: 'boolean', value: false, description:
> > --
> > 2.40.1
> >
>

Thanks Bruce.


--=20
David Marchand