[1/1] build: allow disabling libs
diff mbox series

Message ID 20200918084924.31784-2-mohammed@hawari.fr
State New
Delegated to: Thomas Monjalon
Headers show
Series
  • build: allow disabling libs
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/checkpatch success coding style OK

Commit Message

Mohammed Hawari Sept. 18, 2020, 8:49 a.m. UTC
Similarly to the disable_drivers option, the disable_libs option is
introduced. This allows to selectively disable the build of elements
in libs to speed-up the build process.

Signed-off-by: Mohammed Hawari <mohammed@hawari.fr>
---
 app/meson.build   | 12 +++++++++++-
 lib/meson.build   |  7 +++++++
 meson.build       |  9 +++++++++
 meson_options.txt |  2 ++
 4 files changed, 29 insertions(+), 1 deletion(-)

Comments

Bruce Richardson Sept. 18, 2020, 11:43 a.m. UTC | #1
On Fri, Sep 18, 2020 at 10:49:23AM +0200, Mohammed Hawari wrote:
> Similarly to the disable_drivers option, the disable_libs option is
> introduced. This allows to selectively disable the build of elements
> in libs to speed-up the build process.
> 
> Signed-off-by: Mohammed Hawari <mohammed@hawari.fr>
> ---

While I don't particularly like allowing libs to be enabled and disabled
since it complicates the build, I can see why it's necessary. This is an
area that does need some discussion, as I believe others have some opinions
in this area too.

However, for now, some additional thoughts, both on this patch and in
general:

1. I see you included disabling apps if their required libs are not
   available. What about the drivers though?
2. A bigger issue is whether this is really what we want to do, guarantee a
   passing build even if vast chunks of DPDK are actually enabled? I'd tend
   towards "no" in this case, and I'd rather see disabling of libs more
   constrained.
3. To this end, I think I'd rather see us maintain a set of libs which are
   allowed to be disabled, and prevent the rest from being so. For example,
   it makes no sense in DPDK to disable the EAL or mempool libs, since nothing
   will build, while the bitrate_stats or latency_stats libs could likely
   be disabled with little or no impact.

Therefore, I think a better implementation is to start as in this patch
with a new config parameter to disable libs, but as part of that patch add
in an internal list of the libs which are allowed to be disabled (initially
empty). Telling the build system to disable a lib not on that list should
raise a configuration time error. As for how a lib gets on the list - that
should be done once the build has been tested with that lib disabled, i.e.
once testpmd and other apps have got #defines in the code for stripping out
the disabled blocks, and any drivers which depend on the lib have proper
checks and warnings in place about it being disabled (or also #defines in
the code if that can be done).

The other advantage of maintaining such a list is that it then becomes
somewhat feasible to test these build settings, in that (maybe once per
release), iterate through the list of disable-able libs and test that the
build passed with each one disabled individually. [I think for this purpose
we can ignore interactions of having two disabled simultaneously, in order
to have something testable]

What do others in the community think?

Regards,
/Bruce
Mohammed Hawari Sept. 18, 2020, 12:54 p.m. UTC | #2
Hello Bruce,

Thanks for the quick response, see inline

Best regards,

Mohammed

> On 18 Sep 2020, at 13:43, Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> On Fri, Sep 18, 2020 at 10:49:23AM +0200, Mohammed Hawari wrote:
>> Similarly to the disable_drivers option, the disable_libs option is
>> introduced. This allows to selectively disable the build of elements
>> in libs to speed-up the build process.
>> 
>> Signed-off-by: Mohammed Hawari <mohammed@hawari.fr>
>> ---
> 
> While I don't particularly like allowing libs to be enabled and disabled
> since it complicates the build, I can see why it's necessary. This is an
> area that does need some discussion, as I believe others have some opinions
> in this area too.
> 
> However, for now, some additional thoughts, both on this patch and in
> general:
> 
> 1. I see you included disabling apps if their required libs are not
>   available. What about the drivers though?
To my understanding, in the current code, the drivers/meson.build file already
does that check with:

foreach d:deps
                if not is_variable('shared_rte_' + d)
                    build = false

> 2. A bigger issue is whether this is really what we want to do, guarantee a
>   passing build even if vast chunks of DPDK are actually enabled? I'd tend
>   towards "no" in this case, and I'd rather see disabling of libs more
>   constrained.
> 3. To this end, I think I'd rather see us maintain a set of libs which are
>   allowed to be disabled, and prevent the rest from being so. For example,
>   it makes no sense in DPDK to disable the EAL or mempool libs, since nothing
>   will build, while the bitrate_stats or latency_stats libs could likely
>   be disabled with little or no impact.
I tend to agree with that more structured approach, but I am going to wait until
we get some more thoughts from the community before starting that work.

> Therefore, I think a better implementation is to start as in this patch
> with a new config parameter to disable libs, but as part of that patch add
> in an internal list of the libs which are allowed to be disabled (initially
> empty). Telling the build system to disable a lib not on that list should
> raise a configuration time error. As for how a lib gets on the list - that
> should be done once the build has been tested with that lib disabled, i.e.
> once testpmd and other apps have got #defines in the code for stripping out
> the disabled blocks, and any drivers which depend on the lib have proper
> checks and warnings in place about it being disabled (or also #defines in
> the code if that can be done).
> 
> The other advantage of maintaining such a list is that it then becomes
> somewhat feasible to test these build settings, in that (maybe once per
> release), iterate through the list of disable-able libs and test that the
> build passed with each one disabled individually. [I think for this purpose
> we can ignore interactions of having two disabled simultaneously, in order
> to have something testable]
> 
> What do others in the community think?
> 
> Regards,
> /Bruce
Bruce Richardson Sept. 18, 2020, 1:57 p.m. UTC | #3
On Fri, Sep 18, 2020 at 02:54:21PM +0200, Mohammed Hawari wrote:
> Hello Bruce,
> 
> Thanks for the quick response, see inline
> 
> Best regards,
> 
> Mohammed
> 
> > On 18 Sep 2020, at 13:43, Bruce Richardson <bruce.richardson@intel.com> wrote:
> > 
> > On Fri, Sep 18, 2020 at 10:49:23AM +0200, Mohammed Hawari wrote:
> >> Similarly to the disable_drivers option, the disable_libs option is
> >> introduced. This allows to selectively disable the build of elements
> >> in libs to speed-up the build process.
> >> 
> >> Signed-off-by: Mohammed Hawari <mohammed@hawari.fr>
> >> ---
> > 
> > While I don't particularly like allowing libs to be enabled and disabled
> > since it complicates the build, I can see why it's necessary. This is an
> > area that does need some discussion, as I believe others have some opinions
> > in this area too.
> > 
> > However, for now, some additional thoughts, both on this patch and in
> > general:
> > 
> > 1. I see you included disabling apps if their required libs are not
> >   available. What about the drivers though?
> To my understanding, in the current code, the drivers/meson.build file already
> does that check with:
> 
> foreach d:deps
>                 if not is_variable('shared_rte_' + d)
>                     build = false
> 

Yes, my mistake, I forgot that that was added as one driver could depend
upon another. :-(

> > 2. A bigger issue is whether this is really what we want to do, guarantee a
> >   passing build even if vast chunks of DPDK are actually enabled? I'd tend
> >   towards "no" in this case, and I'd rather see disabling of libs more
> >   constrained.
> > 3. To this end, I think I'd rather see us maintain a set of libs which are
> >   allowed to be disabled, and prevent the rest from being so. For example,
> >   it makes no sense in DPDK to disable the EAL or mempool libs, since nothing
> >   will build, while the bitrate_stats or latency_stats libs could likely
> >   be disabled with little or no impact.
> I tend to agree with that more structured approach, but I am going to wait until
> we get some more thoughts from the community before starting that work.
> 

That seems a wise approach. If there is no consensus after a while here, it
probably needs to go to the technical board.

Patch
diff mbox series

diff --git a/app/meson.build b/app/meson.build
index eb74f215a..93affefa3 100644
--- a/app/meson.build
+++ b/app/meson.build
@@ -42,7 +42,17 @@  foreach app:apps
 
 	subdir(name)
 
-	if build
+	foreach d:deps
+		if dpdk_libs_disabled.contains(d)
+			build = false
+			reason = 'missing dependency, "@0@" '.format (d)
+		endif
+	endforeach
+
+	if not build
+		dpdk_apps_disabled += name
+		set_variable(name.underscorify() + '_disable_reason', reason)
+	else
 		dep_objs = []
 		foreach d:deps
 			dep_objs += get_variable(get_option('default_library')
diff --git a/lib/meson.build b/lib/meson.build
index d8b358e5f..c8507fda1 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -45,6 +45,8 @@  if is_windows
 	] # only supported libraries for windows
 endif
 
+disabled_libs = get_option('disable_libs').split(',')
+
 default_cflags = machine_args
 default_cflags += ['-DALLOW_EXPERIMENTAL_API']
 default_cflags += ['-DALLOW_INTERNAL_API']
@@ -79,6 +81,11 @@  foreach l:libraries
 	dir_name = 'librte_' + l
 	subdir(dir_name)
 
+	if disabled_libs.contains(l)
+		build = false
+		reason = 'Explicitly disabled via build config'
+	endif
+
 	if build
 		shared_deps = ext_deps
 		static_deps = ext_deps
diff --git a/meson.build b/meson.build
index 61d9a4f5f..cf04f0e0e 100644
--- a/meson.build
+++ b/meson.build
@@ -21,6 +21,7 @@  dpdk_drivers = []
 dpdk_extra_ldflags = []
 dpdk_libs_disabled = []
 dpdk_drvs_disabled = []
+dpdk_apps_disabled = []
 abi_version_file = files('ABI_VERSION')
 
 if host_machine.cpu_family().startswith('x86')
@@ -106,6 +107,14 @@  foreach class:dpdk_driver_classes
 endforeach
 message(output_message + '\n')
 
+output_message = '\n===============\nApplications Disabled\n===============\n'
+foreach app:dpdk_apps_disabled
+	reason = get_variable(app.underscorify() + '_disable_reason')
+	output_message += app + ':\t' + reason + '\n\t'
+endforeach
+
+message(output_message + '\n')
+
 output_message = '\n=================\nContent Skipped\n=================\n'
 output_message += '\nlibs:\n\t'
 foreach lib:dpdk_libs_disabled
diff --git a/meson_options.txt b/meson_options.txt
index 9bf18ab6b..d1aa46b8d 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -4,6 +4,8 @@  option('armv8_crypto_dir', type: 'string', value: '',
 	description: 'path to the armv8_crypto library installation directory')
 option('disable_drivers', type: 'string', value: '',
 	description: 'Comma-separated list of drivers to explicitly disable.')
+option('disable_libs', type: 'string', value: '',
+	description: 'Comma-separated list of libs to explicitly disable.')
 option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>',
 	description: 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
 option('enable_docs', type: 'boolean', value: false,