[PATCH] app/test: drop dependency on crypto scheduler driver

[PATCH] app/test: drop dependency on crypto scheduler driver

[Bruce Richardson]

The cryptodev autotests make use of the crypto scheduler driver when it
is available, but build fine without. We can therefore remove the hard
dependency on that driver when building the crypto test files.

Fixes: 50823f30f0c8 ("test: build using per-file dependencies")

Reported-by: Akhil Goyal <gakhil@marvell.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/meson.build b/app/test/meson.build
index 05bae9216d..29ddf18502 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -11,7 +11,7 @@ packet_burst_generator_deps = ['net']
 sample_packet_forward_deps = ['net_ring', 'ethdev', 'bus_vdev']
 virtual_pmd_deps = ['ethdev', 'net', 'bus_pci']
 # test_cryptodev has material that other crypto tests need
-test_cryptodev_deps = ['bus_vdev', 'net', 'cryptodev', 'crypto_scheduler', 'security']
+test_cryptodev_deps = ['bus_vdev', 'net', 'cryptodev', 'security']
 
 source_file_deps = {
     # The C files providing functionality to other test cases
-- 
2.39.2

RE: [EXT] [PATCH] app/test: drop dependency on crypto scheduler driver

[Akhil Goyal]

> The cryptodev autotests make use of the crypto scheduler driver when it
> is available, but build fine without. We can therefore remove the hard
> dependency on that driver when building the crypto test files.
> 
> Fixes: 50823f30f0c8 ("test: build using per-file dependencies")
> 
> Reported-by: Akhil Goyal <gakhil@marvell.com>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Akhil Goyal <gakhil@marvell.com>

Re: [EXT] [PATCH] app/test: drop dependency on crypto scheduler driver

[David Marchand]

On Thu, Sep 28, 2023 at 11:37 AM Akhil Goyal <gakhil@marvell.com> wrote:
>
> > The cryptodev autotests make use of the crypto scheduler driver when it
> > is available, but build fine without. We can therefore remove the hard
> > dependency on that driver when building the crypto test files.

loongarch CI does not seem to agree, can you have a look?

> >
> > Fixes: 50823f30f0c8 ("test: build using per-file dependencies")
> >
> > Reported-by: Akhil Goyal <gakhil@marvell.com>
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Akhil Goyal <gakhil@marvell.com>


-- 
David Marchand

Re: [EXT] [PATCH] app/test: drop dependency on crypto scheduler driver

[Bruce Richardson]

On Thu, Sep 28, 2023 at 11:45:37AM +0200, David Marchand wrote:
> On Thu, Sep 28, 2023 at 11:37 AM Akhil Goyal <gakhil@marvell.com> wrote:
> >
> > > The cryptodev autotests make use of the crypto scheduler driver when it
> > > is available, but build fine without. We can therefore remove the hard
> > > dependency on that driver when building the crypto test files.
> 
> loongarch CI does not seem to agree, can you have a look?
> 
Yes, although I find the error reported a little strange as it should not
be including the header if the component is not available.

However, first I'm looking into some other issues I caught post-post
of the patch. The dependency tracking is also not correct after this
change, so I need to do a little more digging to fix this. I probably need
a precursor patch to fix the dependency assignment for the binary, as
having optional dependencies means we can no longer use the dependency list
for determining what objects we need to link against (and what header paths
to search). Instead for the unit tests, we should just link against
everything we have built.

Please ignore this patch for now. V2 set coming soon...

/Bruce

[PATCH 1/2] app/test: add support for optional dependencies

[Bruce Richardson]

Some tests make optionally use a component but don't require it for
building. If we include the dependency in the per-file lists, then tests
may be unnecessarily omitted, as the dependency is not required.
On the other hand, removing the optional dependency from the list can
cause build failures, as the test case may include the optional code,
but then fail to build as the build system won't have added the necessary
paths for header inclusion, or the necessary libraries for linking.

Resolve this by explicitly providing a list of optional dependencies.
Any items in this list will be added to the dependency list if
available, but otherwise won't be involved in enable/disable of specific
tests.

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

diff --git a/app/test/meson.build b/app/test/meson.build
index 05bae9216d..80b60f68b2 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -5,6 +5,10 @@
 deps += ['cmdline', 'ring', 'mempool', 'mbuf']
 sources += files('commands.c', 'test.c')
 
+# optional dependencies: some files may use these - and so we should link them in -
+# but do not explicitly require them so they are not listed in the per-file lists below
+optional_deps = []
+
 # some other utility C files, providing functions used by various tests
 # so we need to include these deps in the dependency list for the files using those fns.
 packet_burst_generator_deps = ['net']
@@ -226,6 +230,12 @@ foreach f, f_deps : source_file_deps
         sources += files(f)
     endif
 endforeach
+# add the optional dependencies
+foreach d:optional_deps
+    if is_variable(def_lib + '_rte_' + d) and d not in deps
+        deps += d
+    endif
+endforeach
 
 if cc.has_argument('-Wno-format-truncation')
     cflags += '-Wno-format-truncation'
-- 
2.39.2

[PATCH 2/2] app/test: make crypto scheduler an optional dependency

[Bruce Richardson]

The cryptodev autotests make use of the crypto scheduler driver when it
is available, but build fine without. We can therefore remove the hard
dependency on that driver when building the crypto test files.

Fixes: 50823f30f0c8 ("test: build using per-file dependencies")

Reported-by: Akhil Goyal <gakhil@marvell.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/meson.build | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test/meson.build b/app/test/meson.build
index 80b60f68b2..bf9fc90612 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -7,7 +7,7 @@ sources += files('commands.c', 'test.c')
 
 # optional dependencies: some files may use these - and so we should link them in -
 # but do not explicitly require them so they are not listed in the per-file lists below
-optional_deps = []
+optional_deps = ['crypto_scheduler']
 
 # some other utility C files, providing functions used by various tests
 # so we need to include these deps in the dependency list for the files using those fns.
@@ -15,7 +15,7 @@ packet_burst_generator_deps = ['net']
 sample_packet_forward_deps = ['net_ring', 'ethdev', 'bus_vdev']
 virtual_pmd_deps = ['ethdev', 'net', 'bus_pci']
 # test_cryptodev has material that other crypto tests need
-test_cryptodev_deps = ['bus_vdev', 'net', 'cryptodev', 'crypto_scheduler', 'security']
+test_cryptodev_deps = ['bus_vdev', 'net', 'cryptodev', 'security']
 
 source_file_deps = {
     # The C files providing functionality to other test cases
-- 
2.39.2

Re: [PATCH 1/2] app/test: add support for optional dependencies

[David Marchand]

On Thu, Sep 28, 2023 at 1:05 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> Some tests make optionally use a component but don't require it for

s/make //

> building. If we include the dependency in the per-file lists, then tests
> may be unnecessarily omitted, as the dependency is not required.
> On the other hand, removing the optional dependency from the list can
> cause build failures, as the test case may include the optional code,
> but then fail to build as the build system won't have added the necessary
> paths for header inclusion, or the necessary libraries for linking.
>
> Resolve this by explicitly providing a list of optional dependencies.
> Any items in this list will be added to the dependency list if
> available, but otherwise won't be involved in enable/disable of specific
> tests.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

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

Akhil can you confirm this series works for you?

Thanks!


-- 
David Marchand

RE: [EXT] Re: [PATCH 1/2] app/test: add support for optional dependencies

[Akhil Goyal]

> On Thu, Sep 28, 2023 at 1:05 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > Some tests make optionally use a component but don't require it for
> 
> s/make //
> 
> > building. If we include the dependency in the per-file lists, then tests
> > may be unnecessarily omitted, as the dependency is not required.
> > On the other hand, removing the optional dependency from the list can
> > cause build failures, as the test case may include the optional code,
> > but then fail to build as the build system won't have added the necessary
> > paths for header inclusion, or the necessary libraries for linking.
> >
> > Resolve this by explicitly providing a list of optional dependencies.
> > Any items in this list will be added to the dependency list if
> > available, but otherwise won't be involved in enable/disable of specific
> > tests.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> For the series,
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> Akhil can you confirm this series works for you?
Tested-by: Akhil Goyal <gakhil@marvell.com>

Re: [EXT] Re: [PATCH 1/2] app/test: add support for optional dependencies

[David Marchand]

On Thu, Sep 28, 2023 at 3:06 PM Akhil Goyal <gakhil@marvell.com> wrote:
> > > Some tests make optionally use a component but don't require it for
> >
> > s/make //
> >
> > > building. If we include the dependency in the per-file lists, then tests
> > > may be unnecessarily omitted, as the dependency is not required.
> > > On the other hand, removing the optional dependency from the list can
> > > cause build failures, as the test case may include the optional code,
> > > but then fail to build as the build system won't have added the necessary
> > > paths for header inclusion, or the necessary libraries for linking.
> > >
> > > Resolve this by explicitly providing a list of optional dependencies.
> > > Any items in this list will be added to the dependency list if
> > > available, but otherwise won't be involved in enable/disable of specific
> > > tests.
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > Reviewed-by: David Marchand <david.marchand@redhat.com>
> Tested-by: Akhil Goyal <gakhil@marvell.com>

Series applied, thanks for the quick fix Bruce.


-- 
David Marchand