Re: [PATCH v3] build: select optional libraries

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

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 that
> 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" = "true" ]; then
>      OPTS="$OPTS -Denable_drivers=net/null"
>      OPTS="$OPTS -Ddisable_libs=*"
>  else
> -    OPTS="$OPTS -Ddisable_libs="
> +    OPTS="$OPTS -Denable_libs=*"
>  fi
>  
>  if [ "$ASAN" = "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 = files(
>          'virtual_pmd.c',
>  )
>  
> -test_deps = enabled_libs
> +test_deps = dpdk_libs_enabled
>  # as well as libs, the pci and vdev bus drivers are needed for a lot of tests
>  test_deps += ['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 += gen_c_files.process(dpdk_chkinc_headers)
>  # so we always include them in deps list
>  deps = [get_variable('shared_rte_bus_vdev'), get_variable('shared_rte_bus_pci')]
>  # add the rest of the libs to the dependencies
> -foreach l:enabled_libs
> +foreach l:dpdk_libs_enabled
>      deps += 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=
>  	# deprecated libs may be disabled by default, so for complete builds ensure
>  	# no libs are disabled
> -	if ! echo $* | grep -q -- 'disable_libs' ; then
> -		options="$options -Ddisable_libs="
> +	if ! echo $* | grep -q -- 'enable_libs' ; then
> +		options="$options -Denable_libs=*"
>  	fi
>  	if echo $* | grep -qw -- '--default-library=shared' ; then
>  		options="$options -Dexamples=all"
> 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') == 0
> +    build = false
> +    reason = 'requires IOVA in mbuf (set enable_iova_as_pa option)'
> +endif
> +
>  if not is_linux or not dpdk_conf.get('RTE_ARCH_64')
>      build = false
>      reason = '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 += [
>          'kni',
>  ]
>  
> -disabled_libs = []
> -opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
> +disable_libs = run_command(list_dir_globs, get_option('disable_libs'),
>          check: true).stdout().split()
> -if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
> -    opt_disabled_libs += ['kni']
> +enable_libs = run_command(list_dir_globs, get_option('enable_libs'),
> +        check: true).stdout().split()
> +if enable_libs.length() == 0
> +    enable_libs = []
> +    foreach l:run_command(list_dir_globs, '*', check: true).stdout().split()
> +        if l not in dpdk_libs_deprecated
> +            enable_libs += [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....

>  endif
> -foreach l:opt_disabled_libs
> -    if not optional_libs.contains(l)
> -        warning('Cannot disable mandatory library "@0@"'.format(l))
> -        continue
> -    endif
> -    disabled_libs += l
> -endforeach
> -
>  
>  default_cflags = machine_args
>  default_cflags += ['-DALLOW_EXPERIMENTAL_API']
> @@ -115,8 +113,6 @@ if cc.has_argument('-Wno-format-truncation')
>      default_cflags += '-Wno-format-truncation'
>  endif
>  
> -enabled_libs = [] # used to print summary at the end
> -
>  foreach l:libraries
>      build = true
>      reason = '<unknown reason>' # set if build == false to explain why
> @@ -141,13 +137,25 @@ foreach l:libraries
>          deps += ['eal']
>      endif
>  
> -    if disabled_libs.contains(l)
> +    if not enable_libs.contains(l)
> +        build = false
> +        reason = 'not in enabled libraries build config'
> +    elif disable_libs.contains(l)
>          build = false
>          reason = '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 = true
> +            reason = '<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.

>              reason += ' (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 += name
> +    dpdk_libs_enabled += 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 = []
>  dpdk_libs_deprecated = []
>  dpdk_apps_disabled = []
>  dpdk_libs_disabled = []
> +dpdk_libs_enabled = []
>  dpdk_drvs_disabled = []
>  testpmd_drivers_sources = []
>  testpmd_drivers_deps = []
> @@ -134,7 +135,7 @@ message(output_message + '\n')
>  output_message = '\n=================\nLibraries Enabled\n=================\n'
>  output_message += '\nlibs:\n\t'
>  output_count = 0
> -foreach lib:enabled_libs
> +foreach lib:dpdk_libs_enabled
>      output_message += lib + ', '
>      output_count += 1
>      if output_count == 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: '', description:
>         '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', description:
> +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-<VERSION>', description:
>         'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
> @@ -20,6 +20,8 @@ option('enable_apps', type: 'string', value: '', description:
>         'Comma-separated list of apps to build. If unspecified, build all apps.')
>  option('enable_drivers', type: 'string', value: '', description:
>         'Comma-separated list of drivers to build. If unspecified, build 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?

>  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
>