DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] app/test: drop dependency on crypto scheduler driver
@ 2023-09-28  9:26 Bruce Richardson
  2023-09-28  9:36 ` [EXT] " Akhil Goyal
  2023-09-28 11:04 ` [PATCH 1/2] app/test: add support for optional dependencies Bruce Richardson
  0 siblings, 2 replies; 9+ messages in thread
From: Bruce Richardson @ 2023-09-28  9:26 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, 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>
---
 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


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

* RE: [EXT] [PATCH] app/test: drop dependency on crypto scheduler driver
  2023-09-28  9:26 [PATCH] app/test: drop dependency on crypto scheduler driver Bruce Richardson
@ 2023-09-28  9:36 ` Akhil Goyal
  2023-09-28  9:45   ` David Marchand
  2023-09-28 11:04 ` [PATCH 1/2] app/test: add support for optional dependencies Bruce Richardson
  1 sibling, 1 reply; 9+ messages in thread
From: Akhil Goyal @ 2023-09-28  9:36 UTC (permalink / raw)
  To: Bruce Richardson, dev

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

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

* Re: [EXT] [PATCH] app/test: drop dependency on crypto scheduler driver
  2023-09-28  9:36 ` [EXT] " Akhil Goyal
@ 2023-09-28  9:45   ` David Marchand
  2023-09-28  9:55     ` Bruce Richardson
  0 siblings, 1 reply; 9+ messages in thread
From: David Marchand @ 2023-09-28  9:45 UTC (permalink / raw)
  To: Bruce Richardson, Akhil Goyal; +Cc: dev

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


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

* Re: [EXT] [PATCH] app/test: drop dependency on crypto scheduler driver
  2023-09-28  9:45   ` David Marchand
@ 2023-09-28  9:55     ` Bruce Richardson
  0 siblings, 0 replies; 9+ messages in thread
From: Bruce Richardson @ 2023-09-28  9:55 UTC (permalink / raw)
  To: David Marchand; +Cc: Akhil Goyal, dev

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

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

* [PATCH 1/2] app/test: add support for optional dependencies
  2023-09-28  9:26 [PATCH] app/test: drop dependency on crypto scheduler driver Bruce Richardson
  2023-09-28  9:36 ` [EXT] " Akhil Goyal
@ 2023-09-28 11:04 ` Bruce Richardson
  2023-09-28 11:04   ` [PATCH 2/2] app/test: make crypto scheduler an optional dependency Bruce Richardson
  2023-09-28 12:50   ` [PATCH 1/2] app/test: add support for optional dependencies David Marchand
  1 sibling, 2 replies; 9+ messages in thread
From: Bruce Richardson @ 2023-09-28 11:04 UTC (permalink / raw)
  To: dev; +Cc: gakhil, 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


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

* [PATCH 2/2] app/test: make crypto scheduler an optional dependency
  2023-09-28 11:04 ` [PATCH 1/2] app/test: add support for optional dependencies Bruce Richardson
@ 2023-09-28 11:04   ` Bruce Richardson
  2023-09-28 12:50   ` [PATCH 1/2] app/test: add support for optional dependencies David Marchand
  1 sibling, 0 replies; 9+ messages in thread
From: Bruce Richardson @ 2023-09-28 11:04 UTC (permalink / raw)
  To: dev; +Cc: gakhil, 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


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

* Re: [PATCH 1/2] app/test: add support for optional dependencies
  2023-09-28 11:04 ` [PATCH 1/2] app/test: add support for optional dependencies Bruce Richardson
  2023-09-28 11:04   ` [PATCH 2/2] app/test: make crypto scheduler an optional dependency Bruce Richardson
@ 2023-09-28 12:50   ` David Marchand
  2023-09-28 13:06     ` [EXT] " Akhil Goyal
  1 sibling, 1 reply; 9+ messages in thread
From: David Marchand @ 2023-09-28 12:50 UTC (permalink / raw)
  To: Bruce Richardson, gakhil; +Cc: dev

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


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

* RE: [EXT] Re: [PATCH 1/2] app/test: add support for optional dependencies
  2023-09-28 12:50   ` [PATCH 1/2] app/test: add support for optional dependencies David Marchand
@ 2023-09-28 13:06     ` Akhil Goyal
  2023-09-28 14:42       ` David Marchand
  0 siblings, 1 reply; 9+ messages in thread
From: Akhil Goyal @ 2023-09-28 13:06 UTC (permalink / raw)
  To: David Marchand, Bruce Richardson; +Cc: dev

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


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

* Re: [EXT] Re: [PATCH 1/2] app/test: add support for optional dependencies
  2023-09-28 13:06     ` [EXT] " Akhil Goyal
@ 2023-09-28 14:42       ` David Marchand
  0 siblings, 0 replies; 9+ messages in thread
From: David Marchand @ 2023-09-28 14:42 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Akhil Goyal, dev

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


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

end of thread, other threads:[~2023-09-28 14:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-28  9:26 [PATCH] app/test: drop dependency on crypto scheduler driver Bruce Richardson
2023-09-28  9:36 ` [EXT] " Akhil Goyal
2023-09-28  9:45   ` David Marchand
2023-09-28  9:55     ` Bruce Richardson
2023-09-28 11:04 ` [PATCH 1/2] app/test: add support for optional dependencies Bruce Richardson
2023-09-28 11:04   ` [PATCH 2/2] app/test: make crypto scheduler an optional dependency Bruce Richardson
2023-09-28 12:50   ` [PATCH 1/2] app/test: add support for optional dependencies David Marchand
2023-09-28 13:06     ` [EXT] " Akhil Goyal
2023-09-28 14:42       ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).