[1/1] build: allow disabling libs
Checks
Commit Message
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
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
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
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.
On Fri, 18 Sep 2020 14:57:50 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:
> 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.
Marking current patch as "Changes requested".
Assume that if someone wants to go further then and propose a more
targeted build setting. Something like minimal??
On Wed, Jun 14, 2023 at 12:09:51PM -0700, Stephen Hemminger wrote:
> On Fri, 18 Sep 2020 14:57:50 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
>
> > 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.
>
>
> Marking current patch as "Changes requested".
> Assume that if someone wants to go further then and propose a more
> targeted build setting. Something like minimal??
The more targetted approach has been implemented and can constantly be
improved upon. We can already disable a set of libraries, with only those
validated as being ok to disable on that list. Therefore, I think this
patch can just be rejected as obsolete. Any additional work in this area
should be:
* increasing list of optional libs
* looking again at adding an "enable_libs" flag. I was against this
previously, but now think it's time may have come!
/Bruce
On Thu, Jun 15, 2023 at 10:43 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Jun 14, 2023 at 12:09:51PM -0700, Stephen Hemminger wrote:
> > On Fri, 18 Sep 2020 14:57:50 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >
> > > 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.
> >
> >
> > Marking current patch as "Changes requested".
> > Assume that if someone wants to go further then and propose a more
> > targeted build setting. Something like minimal??
>
> The more targetted approach has been implemented and can constantly be
> improved upon. We can already disable a set of libraries, with only those
> validated as being ok to disable on that list. Therefore, I think this
> patch can just be rejected as obsolete. Any additional work in this area
> should be:
> * increasing list of optional libs
> * looking again at adding an "enable_libs" flag. I was against this
> previously, but now think it's time may have come!
I still have my patch on enable_libs that I rebased not too long ago
(around the time the iova va build option was touched by Thomas).
I could retest it and post it if it helps.
On Thu, Jun 15, 2023 at 05:43:56PM +0200, David Marchand wrote:
> On Thu, Jun 15, 2023 at 10:43 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Wed, Jun 14, 2023 at 12:09:51PM -0700, Stephen Hemminger wrote:
> > > On Fri, 18 Sep 2020 14:57:50 +0100
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > >
> > > > 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.
> > >
> > >
> > > Marking current patch as "Changes requested".
> > > Assume that if someone wants to go further then and propose a more
> > > targeted build setting. Something like minimal??
> >
> > The more targetted approach has been implemented and can constantly be
> > improved upon. We can already disable a set of libraries, with only those
> > validated as being ok to disable on that list. Therefore, I think this
> > patch can just be rejected as obsolete. Any additional work in this area
> > should be:
> > * increasing list of optional libs
> > * looking again at adding an "enable_libs" flag. I was against this
> > previously, but now think it's time may have come!
>
> I still have my patch on enable_libs that I rebased not too long ago
> (around the time the iova va build option was touched by Thomas).
> I could retest it and post it if it helps.
>
Probably worth another look at some point. I'm happy to relook at it
whenever you have a version to post.
/Bruce
@@ -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')
@@ -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
@@ -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
@@ -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,