DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [PATCH 0/1] build: allow disabling libs
@ 2020-09-18  8:49 Mohammed Hawari
  2020-09-18  8:49 ` [dpdk-dev] [PATCH 1/1] " Mohammed Hawari
  0 siblings, 1 reply; 5+ messages in thread
From: Mohammed Hawari @ 2020-09-18  8:49 UTC (permalink / raw)
  Cc: dev

This patch introduces the disable_libs option in the meson build system.
This options allows to select a set of elements in libs/ that will not 
be compiled. Dependency tracking is also extended so that, if a lib is
disabled, the build of all apps depending on it is also disabled.

Mohammed Hawari (1):
  build: allow disabling libs

 app/meson.build   | 12 +++++++++++-
 lib/meson.build   |  7 +++++++
 meson.build       |  9 +++++++++
 meson_options.txt |  2 ++
 4 files changed, 29 insertions(+), 1 deletion(-)

-- 
2.28.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [dpdk-dev] [PATCH 1/1] build: allow disabling libs
  2020-09-18  8:49 [dpdk-dev] [PATCH 0/1] build: allow disabling libs Mohammed Hawari
@ 2020-09-18  8:49 ` " Mohammed Hawari
  2020-09-18 11:43   ` Bruce Richardson
  0 siblings, 1 reply; 5+ messages in thread
From: Mohammed Hawari @ 2020-09-18  8:49 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

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

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] build: allow disabling libs
  2020-09-18  8:49 ` [dpdk-dev] [PATCH 1/1] " Mohammed Hawari
@ 2020-09-18 11:43   ` Bruce Richardson
  2020-09-18 12:54     ` Mohammed Hawari
  0 siblings, 1 reply; 5+ messages in thread
From: Bruce Richardson @ 2020-09-18 11:43 UTC (permalink / raw)
  To: Mohammed Hawari; +Cc: dev

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] build: allow disabling libs
  2020-09-18 11:43   ` Bruce Richardson
@ 2020-09-18 12:54     ` Mohammed Hawari
  2020-09-18 13:57       ` Bruce Richardson
  0 siblings, 1 reply; 5+ messages in thread
From: Mohammed Hawari @ 2020-09-18 12:54 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] build: allow disabling libs
  2020-09-18 12:54     ` Mohammed Hawari
@ 2020-09-18 13:57       ` Bruce Richardson
  0 siblings, 0 replies; 5+ messages in thread
From: Bruce Richardson @ 2020-09-18 13:57 UTC (permalink / raw)
  To: Mohammed Hawari; +Cc: dev

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-18  8:49 [dpdk-dev] [PATCH 0/1] build: allow disabling libs Mohammed Hawari
2020-09-18  8:49 ` [dpdk-dev] [PATCH 1/1] " Mohammed Hawari
2020-09-18 11:43   ` Bruce Richardson
2020-09-18 12:54     ` Mohammed Hawari
2020-09-18 13:57       ` Bruce Richardson

DPDK patches and discussions

Archives are clonable:
	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox