[v3] build: select optional libraries

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

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
>
  

On Fri, Jun 16, 2023 at 11:42 AM 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 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....

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 += 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.

Ack.


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

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.
  

On Fri, Jun 16, 2023 at 9:21 AM David Marchand
<david.marchand@redhat.com> wrote:
> @@ -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

There is also a change in behavior for current users of the
-Ddisable_libs= configuration (which was used for enabling deprecated
libraries, for example).
My current solution resides in making disable_libs and enable_libs
options being mutually exclusive (meaning that presence of a value for
enable_libs will ignore any configuration around disable_libs).

Does it look ok to you?
  

On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> <david.marchand@redhat.com> wrote:
> > @@ -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
> 
> There is also a change in behavior for current users of the
> -Ddisable_libs= configuration (which was used for enabling deprecated
> libraries, for example).

I notice the change in behaviour for enabling the deprecated libs. Is there
any other change in behaviour for current users?

> My current solution resides in making disable_libs and enable_libs
> options being mutually exclusive (meaning that presence of a value for
> enable_libs will ignore any configuration around disable_libs).
> 
> Does it look ok to you?
> 
Do we need to make them mutually exclusive? The current drivers
implementation allows them to be used together, I think.

/Bruce
  

On Mon, Jun 19, 2023 at 4:26 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> > On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> > <david.marchand@redhat.com> wrote:
> > > @@ -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
> >
> > There is also a change in behavior for current users of the
> > -Ddisable_libs= configuration (which was used for enabling deprecated
> > libraries, for example).
>
> I notice the change in behaviour for enabling the deprecated libs. Is there
> any other change in behaviour for current users?

The only change I see, is that this implementation breaks enabling
deprecated libs via disable_libs.
It may break existing developer build directory and maybe some
packaging scripts, this is why I am a bit puzzled.

Relooking at the disable_libs option current implementation, it seems
backward to pass a disable_libs option when you want to build some
deprecated library.
It is more straightforward to request building libraries via
-Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
implicitly.

But again, we may be breaking something for people who relied on this behavior.

>
> > My current solution resides in making disable_libs and enable_libs
> > options being mutually exclusive (meaning that presence of a value for
> > enable_libs will ignore any configuration around disable_libs).
> >
> > Does it look ok to you?
> >
> Do we need to make them mutually exclusive? The current drivers
> implementation allows them to be used together, I think.

I would prefer we are consistent with the drivers options.
  

On Tue, Jun 20, 2023 at 10:31:19AM +0200, David Marchand wrote:
> On Mon, Jun 19, 2023 at 4:26 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> > > On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> > > <david.marchand@redhat.com> wrote:
> > > > @@ -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
> > >
> > > There is also a change in behavior for current users of the
> > > -Ddisable_libs= configuration (which was used for enabling deprecated
> > > libraries, for example).
> >
> > I notice the change in behaviour for enabling the deprecated libs. Is there
> > any other change in behaviour for current users?
> 
> The only change I see, is that this implementation breaks enabling
> deprecated libs via disable_libs.
> It may break existing developer build directory and maybe some
> packaging scripts, this is why I am a bit puzzled.
> 
> Relooking at the disable_libs option current implementation, it seems
> backward to pass a disable_libs option when you want to build some
> deprecated library.
> It is more straightforward to request building libraries via
> -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> implicitly.
> 
> But again, we may be breaking something for people who relied on this behavior.
> 

That's what I expected, and I think that is ok. I just wanted to check that
the change in behaviour was only for the deprecated libs case.

> >
> > > My current solution resides in making disable_libs and enable_libs
> > > options being mutually exclusive (meaning that presence of a value for
> > > enable_libs will ignore any configuration around disable_libs).
> > >
> > > Does it look ok to you?
> > >
> > Do we need to make them mutually exclusive? The current drivers
> > implementation allows them to be used together, I think.
> 
> I would prefer we are consistent with the drivers options.
> 
Yep, I definitely agree. Both drivers and libs should have the same
behaviour.
  

On Tue, Jun 20, 2023 at 10:36 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Tue, Jun 20, 2023 at 10:31:19AM +0200, David Marchand wrote:
> > On Mon, Jun 19, 2023 at 4:26 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> > > > On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> > > > <david.marchand@redhat.com> wrote:
> > > > > @@ -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
> > > >
> > > > There is also a change in behavior for current users of the
> > > > -Ddisable_libs= configuration (which was used for enabling deprecated
> > > > libraries, for example).
> > >
> > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > any other change in behaviour for current users?
> >
> > The only change I see, is that this implementation breaks enabling
> > deprecated libs via disable_libs.
> > It may break existing developer build directory and maybe some
> > packaging scripts, this is why I am a bit puzzled.
> >
> > Relooking at the disable_libs option current implementation, it seems
> > backward to pass a disable_libs option when you want to build some
> > deprecated library.
> > It is more straightforward to request building libraries via
> > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > implicitly.
> >
> > But again, we may be breaking something for people who relied on this behavior.
> >
>
> That's what I expected, and I think that is ok. I just wanted to check that
> the change in behaviour was only for the deprecated libs case.

Thomas, wdyt?
It requires some release note, at least.
  

On Tue, Jun 20, 2023 at 10:38:33AM +0200, David Marchand wrote:
> On Tue, Jun 20, 2023 at 10:36 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > On Tue, Jun 20, 2023 at 10:31:19AM +0200, David Marchand wrote:
> > > On Mon, Jun 19, 2023 at 4:26 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> > > > > On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> > > > > <david.marchand@redhat.com> wrote:
> > > > > > @@ -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
> > > > >
> > > > > There is also a change in behavior for current users of the
> > > > > -Ddisable_libs= configuration (which was used for enabling deprecated
> > > > > libraries, for example).
> > > >
> > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > any other change in behaviour for current users?
> > >
> > > The only change I see, is that this implementation breaks enabling
> > > deprecated libs via disable_libs.
> > > It may break existing developer build directory and maybe some
> > > packaging scripts, this is why I am a bit puzzled.
> > >
> > > Relooking at the disable_libs option current implementation, it seems
> > > backward to pass a disable_libs option when you want to build some
> > > deprecated library.
> > > It is more straightforward to request building libraries via
> > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > implicitly.
> > >
> > > But again, we may be breaking something for people who relied on this behavior.
> > >
> >
> > That's what I expected, and I think that is ok. I just wanted to check that
> > the change in behaviour was only for the deprecated libs case.
> 
> Thomas, wdyt?
> It requires some release note, at least.
> 
I am assuming this is not targetting this release though, right? Assuming
23.11, we can put in a deprecation note informing of the change ahead of
time too.

/Bruce
  

On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > any other change in behaviour for current users?
> > > >
> > > > The only change I see, is that this implementation breaks enabling
> > > > deprecated libs via disable_libs.
> > > > It may break existing developer build directory and maybe some
> > > > packaging scripts, this is why I am a bit puzzled.
> > > >
> > > > Relooking at the disable_libs option current implementation, it seems
> > > > backward to pass a disable_libs option when you want to build some
> > > > deprecated library.
> > > > It is more straightforward to request building libraries via
> > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > implicitly.
> > > >
> > > > But again, we may be breaking something for people who relied on this behavior.
> > > >
> > >
> > > That's what I expected, and I think that is ok. I just wanted to check that
> > > the change in behaviour was only for the deprecated libs case.
> >
> > Thomas, wdyt?
> > It requires some release note, at least.
> >
> I am assuming this is not targetting this release though, right? Assuming
> 23.11, we can put in a deprecation note informing of the change ahead of
> time too.

I was hoping to get it in this release.
But I am fine with postponing and announcing the change beforehand.
  

On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > any other change in behaviour for current users?
> > > > >
> > > > > The only change I see, is that this implementation breaks enabling
> > > > > deprecated libs via disable_libs.
> > > > > It may break existing developer build directory and maybe some
> > > > > packaging scripts, this is why I am a bit puzzled.
> > > > >
> > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > backward to pass a disable_libs option when you want to build some
> > > > > deprecated library.
> > > > > It is more straightforward to request building libraries via
> > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > implicitly.
> > > > >
> > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > >
> > > >
> > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > the change in behaviour was only for the deprecated libs case.
> > >
> > > Thomas, wdyt?
> > > It requires some release note, at least.
> > >
> > I am assuming this is not targetting this release though, right? Assuming
> > 23.11, we can put in a deprecation note informing of the change ahead of
> > time too.
> 
> I was hoping to get it in this release.
> But I am fine with postponing and announcing the change beforehand.
> 
Given the fact that we are likely changing behaviour, and the fact that the
deprecated libs makes it more complicated than the drivers one (since we
have always on, default on and default off cases to consider), I think it's
best we don't rush this.

/Bruce
  

20/06/2023 11:03, Bruce Richardson:
> On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > any other change in behaviour for current users?
> > > > > >
> > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > deprecated libs via disable_libs.
> > > > > > It may break existing developer build directory and maybe some
> > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > >
> > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > deprecated library.
> > > > > > It is more straightforward to request building libraries via
> > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > implicitly.
> > > > > >
> > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > >
> > > > >
> > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > the change in behaviour was only for the deprecated libs case.
> > > >
> > > > Thomas, wdyt?
> > > > It requires some release note, at least.
> > > >
> > > I am assuming this is not targetting this release though, right? Assuming
> > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > time too.
> > 
> > I was hoping to get it in this release.
> > But I am fine with postponing and announcing the change beforehand.
> > 
> Given the fact that we are likely changing behaviour, and the fact that the
> deprecated libs makes it more complicated than the drivers one (since we
> have always on, default on and default off cases to consider), I think it's
> best we don't rush this.

I'm not sure what is the best behaviour.
I tend to think such options should be simple to understand
with only 3 cases:
- no option -> default
- enable option -> only core mandatory and listed libraries
- disable option -> all but the listed libraries
It looks simpler to forbid having both enable and disable libraries.

Would you be open to change the behaviour of the drivers options?
  

On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> 20/06/2023 11:03, Bruce Richardson:
> > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > > any other change in behaviour for current users?
> > > > > > >
> > > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > > deprecated libs via disable_libs.
> > > > > > > It may break existing developer build directory and maybe some
> > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > >
> > > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > > deprecated library.
> > > > > > > It is more straightforward to request building libraries via
> > > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > > implicitly.
> > > > > > >
> > > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > > >
> > > > > >
> > > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > > the change in behaviour was only for the deprecated libs case.
> > > > >
> > > > > Thomas, wdyt?
> > > > > It requires some release note, at least.
> > > > >
> > > > I am assuming this is not targetting this release though, right? Assuming
> > > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > > time too.
> > > 
> > > I was hoping to get it in this release.
> > > But I am fine with postponing and announcing the change beforehand.
> > > 
> > Given the fact that we are likely changing behaviour, and the fact that the
> > deprecated libs makes it more complicated than the drivers one (since we
> > have always on, default on and default off cases to consider), I think it's
> > best we don't rush this.
> 
> I'm not sure what is the best behaviour.
> I tend to think such options should be simple to understand
> with only 3 cases:
> - no option -> default
> - enable option -> only core mandatory and listed libraries
> - disable option -> all but the listed libraries
> It looks simpler to forbid having both enable and disable libraries.
> 
> Would you be open to change the behaviour of the drivers options?
> 
Yes, no issue there. I don't see a problem with disallowing having both
enable and disable options set.
  

On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> 20/06/2023 11:03, Bruce Richardson:
> > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > > any other change in behaviour for current users?
> > > > > > >
> > > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > > deprecated libs via disable_libs.
> > > > > > > It may break existing developer build directory and maybe some
> > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > >
> > > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > > deprecated library.
> > > > > > > It is more straightforward to request building libraries via
> > > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > > implicitly.
> > > > > > >
> > > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > > >
> > > > > >
> > > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > > the change in behaviour was only for the deprecated libs case.
> > > > >
> > > > > Thomas, wdyt?
> > > > > It requires some release note, at least.
> > > > >
> > > > I am assuming this is not targetting this release though, right? Assuming
> > > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > > time too.
> > > 
> > > I was hoping to get it in this release.
> > > But I am fine with postponing and announcing the change beforehand.
> > > 
> > Given the fact that we are likely changing behaviour, and the fact that the
> > deprecated libs makes it more complicated than the drivers one (since we
> > have always on, default on and default off cases to consider), I think it's
> > best we don't rush this.
> 
> I'm not sure what is the best behaviour.
> I tend to think such options should be simple to understand
> with only 3 cases:
> - no option -> default
> - enable option -> only core mandatory and listed libraries
> - disable option -> all but the listed libraries
> It looks simpler to forbid having both enable and disable libraries.
> 
> Would you be open to change the behaviour of the drivers options?
> 

[reducing CC list a bit]

As a further follow-up, I really think we need to move slowly and more
carefully on this. While I can see the simplicity in disallowing the two
options to be specified, depending on how we go about choosing the
enabling of the deprecated libs, we may want to keep support for allowing
both.

Specifically, my current concern/thinking is:
* David points out that using the "disabled_libs" options may not make the
  most logic sense for *enabling* deprecated libs.
* While that is true, I think the usability of enabling them via
  "enabled_libs" could be pretty terrible - unless we start adding more
  complications. Specifically, if someone wants to just enable KNI in the
  build using "enable_libs", specifying "-Denable_libs=kni" will not do
  what they want - sure it will enable kni, but will disable dozens of
  other parts of DPDK.
* Therefore, I think keeping the disabling of deprecated parts of DPDK via
  disabled_libs is easier on users - though agreed it is slightly less
  logical. However, if we forbid using enabled and disabled options
  together, that would mean that to switch to enabled libs style, the user
  has to set both enabled libs, *and* clear the default disabled libs option
  of the deprecated ones.
* Therefore, right now I'm tending more towards something like the status
  quo - disabling deprecated via disable option, but allowing both enable
  and disable options together. This hasn't caused us issues with drivers
  thus far, so I'd be hopeful for using it for libs.

The other alternative, is we come up with:
* another scheme for managing deprecated libs
* a special keyword for enabled libs to cover the default case, that one
  can use to add on the deprecated libs, without having to call out each
  and every other optional library.
  

On Tue, Jun 20, 2023 at 5:05 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> > 20/06/2023 11:03, Bruce Richardson:
> > > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > > > any other change in behaviour for current users?
> > > > > > > >
> > > > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > > > deprecated libs via disable_libs.
> > > > > > > > It may break existing developer build directory and maybe some
> > > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > > >
> > > > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > > > deprecated library.
> > > > > > > > It is more straightforward to request building libraries via
> > > > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > > > implicitly.
> > > > > > > >
> > > > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > > > >
> > > > > > >
> > > > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > > > the change in behaviour was only for the deprecated libs case.
> > > > > >
> > > > > > Thomas, wdyt?
> > > > > > It requires some release note, at least.
> > > > > >
> > > > > I am assuming this is not targetting this release though, right? Assuming
> > > > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > > > time too.
> > > >
> > > > I was hoping to get it in this release.
> > > > But I am fine with postponing and announcing the change beforehand.
> > > >
> > > Given the fact that we are likely changing behaviour, and the fact that the
> > > deprecated libs makes it more complicated than the drivers one (since we
> > > have always on, default on and default off cases to consider), I think it's
> > > best we don't rush this.
> >
> > I'm not sure what is the best behaviour.
> > I tend to think such options should be simple to understand
> > with only 3 cases:
> > - no option -> default
> > - enable option -> only core mandatory and listed libraries
> > - disable option -> all but the listed libraries
> > It looks simpler to forbid having both enable and disable libraries.
> >
> > Would you be open to change the behaviour of the drivers options?
> >
>
> [reducing CC list a bit]
>
> As a further follow-up, I really think we need to move slowly and more
> carefully on this. While I can see the simplicity in disallowing the two
> options to be specified, depending on how we go about choosing the
> enabling of the deprecated libs, we may want to keep support for allowing
> both.

I agree, we need more time on this topic, sorry for being pushy earlier :-).


>
> Specifically, my current concern/thinking is:
> * David points out that using the "disabled_libs" options may not make the
>   most logic sense for *enabling* deprecated libs.
> * While that is true, I think the usability of enabling them via
>   "enabled_libs" could be pretty terrible - unless we start adding more
>   complications. Specifically, if someone wants to just enable KNI in the
>   build using "enable_libs", specifying "-Denable_libs=kni" will not do
>   what they want - sure it will enable kni, but will disable dozens of
>   other parts of DPDK.
> * Therefore, I think keeping the disabling of deprecated parts of DPDK via
>   disabled_libs is easier on users - though agreed it is slightly less
>   logical. However, if we forbid using enabled and disabled options
>   together, that would mean that to switch to enabled libs style, the user
>   has to set both enabled libs, *and* clear the default disabled libs option
>   of the deprecated ones.
> * Therefore, right now I'm tending more towards something like the status
>   quo - disabling deprecated via disable option, but allowing both enable
>   and disable options together. This hasn't caused us issues with drivers
>   thus far, so I'd be hopeful for using it for libs.

Mixing enable/disable_drivers has been possible since the introduction
of enable_drivers.
Looking at the code, it is unclear the intention was to make them exclusive.
Is there no usecase for using both options in some soc configuration?
Juraj, do you have an opinion?


>
> The other alternative, is we come up with:
> * another scheme for managing deprecated libs
> * a special keyword for enabled libs to cover the default case, that one
>   can use to add on the deprecated libs, without having to call out each
>   and every other optional library.

I think a separate option to control deprecated libraries would be better.
This option would control whether the deprecation libs are part of the
optional libraries list.
The optional libraries list would then be evaluated according to
enable/disable_libs.

I'll try this to see how it behaves.
  

On Wed, Jun 21, 2023 at 11:54:31AM +0200, David Marchand wrote:
> On Tue, Jun 20, 2023 at 5:05 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> > > 20/06/2023 11:03, Bruce Richardson:
> > > > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > > > <bruce.richardson@intel.com> wrote:
> > > > > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > > > > any other change in behaviour for current users?
> > > > > > > > >
> > > > > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > > > > deprecated libs via disable_libs.
> > > > > > > > > It may break existing developer build directory and maybe some
> > > > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > > > >
> > > > > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > > > > deprecated library.
> > > > > > > > > It is more straightforward to request building libraries via
> > > > > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > > > > implicitly.
> > > > > > > > >
> > > > > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > > > > >
> > > > > > > >
> > > > > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > > > > the change in behaviour was only for the deprecated libs case.
> > > > > > >
> > > > > > > Thomas, wdyt?
> > > > > > > It requires some release note, at least.
> > > > > > >
> > > > > > I am assuming this is not targetting this release though, right? Assuming
> > > > > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > > > > time too.
> > > > >
> > > > > I was hoping to get it in this release.
> > > > > But I am fine with postponing and announcing the change beforehand.
> > > > >
> > > > Given the fact that we are likely changing behaviour, and the fact that the
> > > > deprecated libs makes it more complicated than the drivers one (since we
> > > > have always on, default on and default off cases to consider), I think it's
> > > > best we don't rush this.
> > >
> > > I'm not sure what is the best behaviour.
> > > I tend to think such options should be simple to understand
> > > with only 3 cases:
> > > - no option -> default
> > > - enable option -> only core mandatory and listed libraries
> > > - disable option -> all but the listed libraries
> > > It looks simpler to forbid having both enable and disable libraries.
> > >
> > > Would you be open to change the behaviour of the drivers options?
> > >
> >
> > [reducing CC list a bit]
> >
> > As a further follow-up, I really think we need to move slowly and more
> > carefully on this. While I can see the simplicity in disallowing the two
> > options to be specified, depending on how we go about choosing the
> > enabling of the deprecated libs, we may want to keep support for allowing
> > both.
> 
> I agree, we need more time on this topic, sorry for being pushy earlier :-).
> 

No problem at all. I too was keen to get this done, but what initially we
thought was simple turns out (as usual!) to have hidden complexity. :-)

> 
> >
> > Specifically, my current concern/thinking is:
> > * David points out that using the "disabled_libs" options may not make the
> >   most logic sense for *enabling* deprecated libs.
> > * While that is true, I think the usability of enabling them via
> >   "enabled_libs" could be pretty terrible - unless we start adding more
> >   complications. Specifically, if someone wants to just enable KNI in the
> >   build using "enable_libs", specifying "-Denable_libs=kni" will not do
> >   what they want - sure it will enable kni, but will disable dozens of
> >   other parts of DPDK.
> > * Therefore, I think keeping the disabling of deprecated parts of DPDK via
> >   disabled_libs is easier on users - though agreed it is slightly less
> >   logical. However, if we forbid using enabled and disabled options
> >   together, that would mean that to switch to enabled libs style, the user
> >   has to set both enabled libs, *and* clear the default disabled libs option
> >   of the deprecated ones.
> > * Therefore, right now I'm tending more towards something like the status
> >   quo - disabling deprecated via disable option, but allowing both enable
> >   and disable options together. This hasn't caused us issues with drivers
> >   thus far, so I'd be hopeful for using it for libs.
> 
> Mixing enable/disable_drivers has been possible since the introduction
> of enable_drivers.
> Looking at the code, it is unclear the intention was to make them exclusive.
> Is there no usecase for using both options in some soc configuration?

If we can solve the deprecated libs management separately, having them
exclusive may well make sense. Since we can wildcard, I think the cases
where we need both simultaneously should be few.

> Juraj, do you have an opinion?
> 
> 
> >
> > The other alternative, is we come up with:
> > * another scheme for managing deprecated libs
> > * a special keyword for enabled libs to cover the default case, that one
> >   can use to add on the deprecated libs, without having to call out each
> >   and every other optional library.
> 
> I think a separate option to control deprecated libraries would be better.
> This option would control whether the deprecation libs are part of the
> optional libraries list.
> The optional libraries list would then be evaluated according to
> enable/disable_libs.
> 
> I'll try this to see how it behaves.
> 
Ok, let's see how that goes. It does likely mean another build config flag
though, which I was hoping to avoid, but if it simplifies everything else
it may be worth it.

/Bruce
  

I haven't followed this discussion in detail, so this might have been mentioned already...

Omitting deprecated libs when building will not omit deprecated functions in other libs.

In other words: Omitting deprecated libs only achieves part of what we really want to achieve (omitting everything deprecated), so special handling of deprecated libs might not be that important.


PS: I appreciate the effort to make the libs optional at build time, like the drivers!
  

On Wed, Jun 21, 2023 at 11:54 AM David Marchand <david.marchand@redhat.com>
wrote:

> On Tue, Jun 20, 2023 at 5:05 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> > > 20/06/2023 11:03, Bruce Richardson:
> > > > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > > > <bruce.richardson@intel.com> wrote:
> > > > > > > > > > I notice the change in behaviour for enabling the
> deprecated libs. Is there
> > > > > > > > > > any other change in behaviour for current users?
> > > > > > > > >
> > > > > > > > > The only change I see, is that this implementation breaks
> enabling
> > > > > > > > > deprecated libs via disable_libs.
> > > > > > > > > It may break existing developer build directory and maybe
> some
> > > > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > > > >
> > > > > > > > > Relooking at the disable_libs option current
> implementation, it seems
> > > > > > > > > backward to pass a disable_libs option when you want to
> build some
> > > > > > > > > deprecated library.
> > > > > > > > > It is more straightforward to request building libraries
> via
> > > > > > > > > -Denable_libs=<deprecated_lib> explicitly or
> -Denable_libs=*
> > > > > > > > > implicitly.
> > > > > > > > >
> > > > > > > > > But again, we may be breaking something for people who
> relied on this behavior.
> > > > > > > > >
> > > > > > > >
> > > > > > > > That's what I expected, and I think that is ok. I just
> wanted to check that
> > > > > > > > the change in behaviour was only for the deprecated libs
> case.
> > > > > > >
> > > > > > > Thomas, wdyt?
> > > > > > > It requires some release note, at least.
> > > > > > >
> > > > > > I am assuming this is not targetting this release though, right?
> Assuming
> > > > > > 23.11, we can put in a deprecation note informing of the change
> ahead of
> > > > > > time too.
> > > > >
> > > > > I was hoping to get it in this release.
> > > > > But I am fine with postponing and announcing the change beforehand.
> > > > >
> > > > Given the fact that we are likely changing behaviour, and the fact
> that the
> > > > deprecated libs makes it more complicated than the drivers one
> (since we
> > > > have always on, default on and default off cases to consider), I
> think it's
> > > > best we don't rush this.
> > >
> > > I'm not sure what is the best behaviour.
> > > I tend to think such options should be simple to understand
> > > with only 3 cases:
> > > - no option -> default
> > > - enable option -> only core mandatory and listed libraries
> > > - disable option -> all but the listed libraries
> > > It looks simpler to forbid having both enable and disable libraries.
> > >
> > > Would you be open to change the behaviour of the drivers options?
> > >
> >
> > [reducing CC list a bit]
> >
> > As a further follow-up, I really think we need to move slowly and more
> > carefully on this. While I can see the simplicity in disallowing the two
> > options to be specified, depending on how we go about choosing the
> > enabling of the deprecated libs, we may want to keep support for allowing
> > both.
>
> I agree, we need more time on this topic, sorry for being pushy earlier
> :-).
>
>
> >
> > Specifically, my current concern/thinking is:
> > * David points out that using the "disabled_libs" options may not make
> the
> >   most logic sense for *enabling* deprecated libs.
> > * While that is true, I think the usability of enabling them via
> >   "enabled_libs" could be pretty terrible - unless we start adding more
> >   complications. Specifically, if someone wants to just enable KNI in the
> >   build using "enable_libs", specifying "-Denable_libs=kni" will not do
> >   what they want - sure it will enable kni, but will disable dozens of
> >   other parts of DPDK.
> > * Therefore, I think keeping the disabling of deprecated parts of DPDK
> via
> >   disabled_libs is easier on users - though agreed it is slightly less
> >   logical. However, if we forbid using enabled and disabled options
> >   together, that would mean that to switch to enabled libs style, the
> user
> >   has to set both enabled libs, *and* clear the default disabled libs
> option
> >   of the deprecated ones.
> > * Therefore, right now I'm tending more towards something like the status
> >   quo - disabling deprecated via disable option, but allowing both enable
> >   and disable options together. This hasn't caused us issues with drivers
> >   thus far, so I'd be hopeful for using it for libs.
>
> Mixing enable/disable_drivers has been possible since the introduction
> of enable_drivers.
> Looking at the code, it is unclear the intention was to make them
> exclusive.
> Is there no usecase for using both options in some soc configuration?
> Juraj, do you have an opinion?
>
>
I seem to remember that there wasn't any intention of using them together -
I think I did it this way because it was essentially "free" (i.e. the
ability to use them together was an extra feature on top of what we needed).

The SoC considerations were for cross compilation between Arm
microarchitectures/SoCs - use enable_drivers mainly to enable only a
handful of relevant drivers for small SoC's when building on a server grade
SoC. The same applies for disable_drivers - the server grade SoC where
we're building may have extra dependencies that we may not want/need on the
target (possibly server-grade) system. They don't need to be used in
conjunction in these scenarios.


>
> >
> > The other alternative, is we come up with:
> > * another scheme for managing deprecated libs
> > * a special keyword for enabled libs to cover the default case, that one
> >   can use to add on the deprecated libs, without having to call out each
> >   and every other optional library.
>
> I think a separate option to control deprecated libraries would be better.
> This option would control whether the deprecation libs are part of the
> optional libraries list.
> The optional libraries list would then be evaluated according to
> enable/disable_libs.
>
> I'll try this to see how it behaves.
>
>
> --
> David Marchand
>
>