[PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[Bruce Richardson]

When the user passes a list of desired libraries to build via the
"enable_libs" option, the expectation is that those libraries should be
part of the build. However, if those libs have either external or
internal dependencies, they still may be silently disabled, for example:
running "meson setup -Denable_libs=security build" will successfully
run, but the security lib will not be configured as "cryptodev" is
missing.

We can fix this by setting a flag to indicate when the libraries are
specified via an enable_libs flag. If so, then we error out when a
library is unbuildable, giving a suitable error message. For the above
example case, the "meson setup" run fails with:

Message: Disabling security [lib/security]: missing internal dependency "cryptodev"

lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly requested lib "security".
	Please add missing dependency "cryptodev" to "enable_libs" option

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/meson.build | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/meson.build b/lib/meson.build
index 099b0ed18a..cf4aa63630 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -131,7 +131,9 @@ endforeach
 disable_libs = run_command(list_dir_globs, get_option('disable_libs'), check: true).stdout().split()
 
 enable_libs = run_command(list_dir_globs, get_option('enable_libs'), check: true).stdout().split()
+require_libs = true
 if enable_libs.length() == 0
+    require_libs = false
     enable_libs += optional_libs
 endif
 enable_libs += always_enable
@@ -189,6 +191,10 @@ foreach l:libraries
 
     if build
         subdir(l)
+        if not build and require_libs
+            error('Cannot build explicitly requested lib "@0@".\n'.format(name)
+                    +'\tReason: ' + reason)
+        endif
     endif
     if name != l
         warning('Library name, "@0@", and directory name, "@1@", do not match'.format(name, l))
@@ -208,6 +214,10 @@ foreach l:libraries
             endif
             message('Disabling @1@ [@2@]: missing internal dependency "@0@"'
                     .format(d, name, 'lib/' + l))
+            if require_libs
+                error('Cannot build explicitly requested lib "@0@".\n'.format(name)
+                        + '\tPlease add missing dependency "@0@" to "enable_libs" option'.format(d))
+            endif
         else
             shared_deps += [get_variable('shared_rte_' + d)]
             static_deps += [get_variable('static_rte_' + d)]
-- 
2.39.2

[PATCH 2/2] build: fail if explicitly requested driver is unbuildable

[Bruce Richardson]

When the user passes a list of desired drivers to build via the
"enable_drivers" option, the expectation is that those drivers should be
part of the build. However, if those drivers have either external or
internal dependencies, they still may be silently disabled, for example:
running "meson setup -Denable_drivers=net/iavf build" will successfully
run, but the iavf net driver will not be configured as "common/iavf" is
missing.

We can fix this by setting a flag to indicate when the drivers are
specified via an enable_drivers flag. However, unlike when erroring out
on missing libs, we don't error out if a driver in unbuildable, unless
the driver name explicitly appears in the "enable_drivers" list. This is
implemented this way to ensure that wildcarding still works. For
example: we still want to allow "meson setup -Denable_drivers=net/*" to
work, configuring only the buildable network drivers. While it's true
that this additional restriction may cause some builds to pass when they
should fail, e.g. if the wildcard refers only to a single driver,
implementing things this way avoids massive amounts of complexity, and
is still an improvement on the status-quo.

Suggested-by: Anatoly Burakov <anatoly.burakov@intel.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/meson.build | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/meson.build b/drivers/meson.build
index 417b64b8fc..8c775bbe62 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -41,7 +41,9 @@ disable_drivers = run_command(list_dir_globs, disable_drivers, check: true).stdo
 # add cmdline enabled drivers and meson enabled drivers together
 enable_drivers = ',' + get_option('enable_drivers')
 enable_drivers = run_command(list_dir_globs, enable_drivers, check: true).stdout().split()
+require_drivers = true
 if enable_drivers.length() == 0
+    require_drivers = false
     enable_drivers = run_command(list_dir_globs, '*/*', check: true).stdout().split()
 endif
 
@@ -155,6 +157,12 @@ foreach subpath:subdirs
                 build = false
                 reason = 'requires IOVA in mbuf (set enable_iova_as_pa option)'
             endif
+            # error out if we can't build a driver and that driver was explicitly requested,
+            # i.e. not via wildcard.
+            if not build and require_drivers and get_option('enable_drivers').contains(drv_path)
+                error('Cannot build explicitly requested driver "@0@".\n'.format(drv_path)
+                        +'\tReason: ' + reason)
+            endif
 
             # get dependency objs from strings
             shared_deps = ext_deps
@@ -171,6 +179,12 @@ foreach subpath:subdirs
                     endif
                     message('Disabling @1@ [@2@]: missing internal dependency "@0@"'
                             .format(d, name, 'drivers/' + drv_path))
+                    # error out if we can't build a driver and that driver was explicitly
+                    # requested, i.e. not via wildcard.
+                    if require_drivers and get_option('enable_drivers').contains(drv_path)
+                        error('Cannot build explicitly requested driver "@0@".\n'.format(drv_path)
+                                +'\tPlease enable missing dependency "@0@"'.format(d))
+                    endif
                 else
                     shared_deps += [get_variable('shared_rte_' + d)]
                     static_deps += [get_variable('static_rte_' + d)]
-- 
2.39.2

Re: [PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[David Marchand]

On Fri, Sep 1, 2023 at 4:23 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> When the user passes a list of desired libraries to build via the
> "enable_libs" option, the expectation is that those libraries should be
> part of the build. However, if those libs have either external or
> internal dependencies, they still may be silently disabled, for example:
> running "meson setup -Denable_libs=security build" will successfully
> run, but the security lib will not be configured as "cryptodev" is
> missing.
>
> We can fix this by setting a flag to indicate when the libraries are
> specified via an enable_libs flag. If so, then we error out when a
> library is unbuildable, giving a suitable error message. For the above
> example case, the "meson setup" run fails with:
>
> Message: Disabling security [lib/security]: missing internal dependency "cryptodev"
>
> lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly requested lib "security".
>         Please add missing dependency "cryptodev" to "enable_libs" option
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

I remember setting a enable_drivers=net/af_xdp on a system lacking
libbpf-devel and not getting warned about its absence.
So I suspect enable_drivers is affected by the same issue.


-- 
David Marchand

Re: [PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[David Marchand]

On Fri, Sep 1, 2023 at 4:29 PM David Marchand <david.marchand@redhat.com> wrote:
>
> On Fri, Sep 1, 2023 at 4:23 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > When the user passes a list of desired libraries to build via the
> > "enable_libs" option, the expectation is that those libraries should be
> > part of the build. However, if those libs have either external or
> > internal dependencies, they still may be silently disabled, for example:
> > running "meson setup -Denable_libs=security build" will successfully
> > run, but the security lib will not be configured as "cryptodev" is
> > missing.
> >
> > We can fix this by setting a flag to indicate when the libraries are
> > specified via an enable_libs flag. If so, then we error out when a
> > library is unbuildable, giving a suitable error message. For the above
> > example case, the "meson setup" run fails with:
> >
> > Message: Disabling security [lib/security]: missing internal dependency "cryptodev"
> >
> > lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly requested lib "security".
> >         Please add missing dependency "cryptodev" to "enable_libs" option
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>
> I remember setting a enable_drivers=net/af_xdp on a system lacking
> libbpf-devel and not getting warned about its absence.
> So I suspect enable_drivers is affected by the same issue.

Arf sorry, did not see it was a two patch series :-).

-- 
David Marchand

Re: [PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[Bruce Richardson]

On Fri, Sep 01, 2023 at 04:30:56PM +0200, David Marchand wrote:
> On Fri, Sep 1, 2023 at 4:29 PM David Marchand <david.marchand@redhat.com> wrote:
> >
> > On Fri, Sep 1, 2023 at 4:23 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > When the user passes a list of desired libraries to build via the
> > > "enable_libs" option, the expectation is that those libraries should be
> > > part of the build. However, if those libs have either external or
> > > internal dependencies, they still may be silently disabled, for example:
> > > running "meson setup -Denable_libs=security build" will successfully
> > > run, but the security lib will not be configured as "cryptodev" is
> > > missing.
> > >
> > > We can fix this by setting a flag to indicate when the libraries are
> > > specified via an enable_libs flag. If so, then we error out when a
> > > library is unbuildable, giving a suitable error message. For the above
> > > example case, the "meson setup" run fails with:
> > >
> > > Message: Disabling security [lib/security]: missing internal dependency "cryptodev"
> > >
> > > lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly requested lib "security".
> > >         Please add missing dependency "cryptodev" to "enable_libs" option
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> >
> > I remember setting a enable_drivers=net/af_xdp on a system lacking
> > libbpf-devel and not getting warned about its absence.
> > So I suspect enable_drivers is affected by the same issue.
> 
> Arf sorry, did not see it was a two patch series :-).
> 
No problem.

At least your response confirms to me that this is indeed a common issue,
that could do with a solution. As I explain in patch 2, fixing for drivers
is a little trickier than for libs, as for drivers we really need to take
account of wildcards to avoid breaking people's existing builds. [Or, at
minimum, my own builds! :-)].

BTW: while we could look to handle wildcards for libs, I think their use is
probably much more limited there, and I decided to go with the simplest
possible solution instead.

/Bruce

RE: [PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 1 September 2023 16.24
> 
> When the user passes a list of desired libraries to build via the
> "enable_libs" option, the expectation is that those libraries should be
> part of the build. However, if those libs have either external or
> internal dependencies, they still may be silently disabled, for example:
> running "meson setup -Denable_libs=security build" will successfully
> run, but the security lib will not be configured as "cryptodev" is
> missing.
> 
> We can fix this by setting a flag to indicate when the libraries are
> specified via an enable_libs flag. If so, then we error out when a
> library is unbuildable, giving a suitable error message. For the above
> example case, the "meson setup" run fails with:
> 
> Message: Disabling security [lib/security]: missing internal dependency
> "cryptodev"
> 
> lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly
> requested lib "security".
> 	Please add missing dependency "cryptodev" to "enable_libs" option
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Morten Brørup <mb@smartsharesystems.com>

RE: [PATCH 2/2] build: fail if explicitly requested driver is unbuildable

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 1 September 2023 16.24
> 
> When the user passes a list of desired drivers to build via the
> "enable_drivers" option, the expectation is that those drivers should be
> part of the build. However, if those drivers have either external or
> internal dependencies, they still may be silently disabled, for example:
> running "meson setup -Denable_drivers=net/iavf build" will successfully
> run, but the iavf net driver will not be configured as "common/iavf" is
> missing.
> 
> We can fix this by setting a flag to indicate when the drivers are
> specified via an enable_drivers flag. However, unlike when erroring out
> on missing libs, we don't error out if a driver in unbuildable, unless
> the driver name explicitly appears in the "enable_drivers" list. This is
> implemented this way to ensure that wildcarding still works. For
> example: we still want to allow "meson setup -Denable_drivers=net/*" to
> work, configuring only the buildable network drivers. While it's true
> that this additional restriction may cause some builds to pass when they
> should fail, e.g. if the wildcard refers only to a single driver,
> implementing things this way avoids massive amounts of complexity, and
> is still an improvement on the status-quo.
> 
> Suggested-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 2/2] build: fail if explicitly requested driver is unbuildable

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 136 bytes --]

ARM Ampere server test fails on this patch are lab infra-failures (I did
some updates on the server yesterday) and they can be ignored.

[-- Attachment #2: Type: text/html, Size: 212 bytes --]

Re: [PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[David Marchand]

On Fri, Sep 1, 2023 at 4:44 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Sep 01, 2023 at 04:30:56PM +0200, David Marchand wrote:
> > On Fri, Sep 1, 2023 at 4:29 PM David Marchand <david.marchand@redhat.com> wrote:
> > >
> > > On Fri, Sep 1, 2023 at 4:23 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > When the user passes a list of desired libraries to build via the
> > > > "enable_libs" option, the expectation is that those libraries should be
> > > > part of the build. However, if those libs have either external or
> > > > internal dependencies, they still may be silently disabled, for example:
> > > > running "meson setup -Denable_libs=security build" will successfully
> > > > run, but the security lib will not be configured as "cryptodev" is
> > > > missing.
> > > >
> > > > We can fix this by setting a flag to indicate when the libraries are
> > > > specified via an enable_libs flag. If so, then we error out when a
> > > > library is unbuildable, giving a suitable error message. For the above
> > > > example case, the "meson setup" run fails with:
> > > >
> > > > Message: Disabling security [lib/security]: missing internal dependency "cryptodev"
> > > >
> > > > lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly requested lib "security".
> > > >         Please add missing dependency "cryptodev" to "enable_libs" option
> > > >
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > I remember setting a enable_drivers=net/af_xdp on a system lacking
> > > libbpf-devel and not getting warned about its absence.
> > > So I suspect enable_drivers is affected by the same issue.
> >
> > Arf sorry, did not see it was a two patch series :-).
> >
> No problem.
>
> At least your response confirms to me that this is indeed a common issue,
> that could do with a solution. As I explain in patch 2, fixing for drivers
> is a little trickier than for libs, as for drivers we really need to take
> account of wildcards to avoid breaking people's existing builds. [Or, at
> minimum, my own builds! :-)].
>
> BTW: while we could look to handle wildcards for libs, I think their use is
> probably much more limited there, and I decided to go with the simplest
> possible solution instead.

We have categories (classes) of drivers, and for them, I can imagine
people using wildcards.
But for libraries, I don't see how wildcards could be used, so I think
we can ignore them for now (until proven wrong ;-).


Thanks Bruce, this series is also enhancing the situation for a user.
Like when wanting to enable explicity a driver or a library: the
explicit error message on the missing dependency is easier to catch
than scrolling back/parsing meson output.


I did not review in detail, but it lgtm for my few tests.

For the series,
Acked-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH 2/2] build: fail if explicitly requested driver is unbuildable

[Bruce Richardson]

On Sat, Sep 02, 2023 at 01:24:18PM -0400, Patrick Robb wrote:
>    ARM Ampere server test fails on this patch are lab infra-failures (I
>    did some updates on the server yesterday) and they can be ignored.

Can a retest be run on these now, to clear the errors in patchwork, or have
the issued been fixed?

Re: [PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[Bruce Richardson]

Recheck-request: iol-unit-arm64-testing, iol-intel-Performance

On Fri, Sep 01, 2023 at 03:23:31PM +0100, Bruce Richardson wrote:
> When the user passes a list of desired libraries to build via the
> "enable_libs" option, the expectation is that those libraries should be
> part of the build. However, if those libs have either external or
> internal dependencies, they still may be silently disabled, for example:
> running "meson setup -Denable_libs=security build" will successfully
> run, but the security lib will not be configured as "cryptodev" is
> missing.
> 
> We can fix this by setting a flag to indicate when the libraries are
> specified via an enable_libs flag. If so, then we error out when a
> library is unbuildable, giving a suitable error message. For the above
> example case, the "meson setup" run fails with:
> 
> Message: Disabling security [lib/security]: missing internal dependency "cryptodev"
> 
> lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly requested lib "security".
> 	Please add missing dependency "cryptodev" to "enable_libs" option
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/meson.build | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

Re: [PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[Bruce Richardson]

Recheck-request: iol-unit-arm64-testing, iol-intel-Performance

I'm informed now that there were infra issues which should now have been fixed,
so requesting a second re-check.

/Bruce
On Fri, Sep 01, 2023 at 03:23:31PM +0100, Bruce Richardson wrote:
> When the user passes a list of desired libraries to build via the
> "enable_libs" option, the expectation is that those libraries should be
> part of the build. However, if those libs have either external or
> internal dependencies, they still may be silently disabled, for example:
> running "meson setup -Denable_libs=security build" will successfully
> run, but the security lib will not be configured as "cryptodev" is
> missing.
> 
> We can fix this by setting a flag to indicate when the libraries are
> specified via an enable_libs flag. If so, then we error out when a
> library is unbuildable, giving a suitable error message. For the above
> example case, the "meson setup" run fails with:
> 
> Message: Disabling security [lib/security]: missing internal dependency "cryptodev"
> 
> lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly requested lib "security".
> 	Please add missing dependency "cryptodev" to "enable_libs" option
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/meson.build | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/meson.build b/lib/meson.build
> index 099b0ed18a..cf4aa63630 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -131,7 +131,9 @@ endforeach
>  disable_libs = run_command(list_dir_globs, get_option('disable_libs'), check: true).stdout().split()
>  
>  enable_libs = run_command(list_dir_globs, get_option('enable_libs'), check: true).stdout().split()
> +require_libs = true
>  if enable_libs.length() == 0
> +    require_libs = false
>      enable_libs += optional_libs
>  endif
>  enable_libs += always_enable
> @@ -189,6 +191,10 @@ foreach l:libraries
>  
>      if build
>          subdir(l)
> +        if not build and require_libs
> +            error('Cannot build explicitly requested lib "@0@".\n'.format(name)
> +                    +'\tReason: ' + reason)
> +        endif
>      endif
>      if name != l
>          warning('Library name, "@0@", and directory name, "@1@", do not match'.format(name, l))
> @@ -208,6 +214,10 @@ foreach l:libraries
>              endif
>              message('Disabling @1@ [@2@]: missing internal dependency "@0@"'
>                      .format(d, name, 'lib/' + l))
> +            if require_libs
> +                error('Cannot build explicitly requested lib "@0@".\n'.format(name)
> +                        + '\tPlease add missing dependency "@0@" to "enable_libs" option'.format(d))
> +            endif
>          else
>              shared_deps += [get_variable('shared_rte_' + d)]
>              static_deps += [get_variable('static_rte_' + d)]
> -- 
> 2.39.2
> 

Re: [PATCH 1/2] build: fail if explicitly requested lib is unbuildable

[David Marchand]

On Fri, Sep 1, 2023 at 4:23 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> When the user passes a list of desired libraries to build via the
> "enable_libs" option, the expectation is that those libraries should be
> part of the build. However, if those libs have either external or
> internal dependencies, they still may be silently disabled, for example:
> running "meson setup -Denable_libs=security build" will successfully
> run, but the security lib will not be configured as "cryptodev" is
> missing.
>
> We can fix this by setting a flag to indicate when the libraries are
> specified via an enable_libs flag. If so, then we error out when a
> library is unbuildable, giving a suitable error message. For the above
> example case, the "meson setup" run fails with:
>
> Message: Disabling security [lib/security]: missing internal dependency "cryptodev"
>
> lib/meson.build:218:16: ERROR: Problem encountered: Cannot build explicitly requested lib "security".
>         Please add missing dependency "cryptodev" to "enable_libs" option
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

CI now passes fine after a retest.
Series applied, thanks Bruce.


-- 
David Marchand