[1/2] tests: Fix unit tests for shared builds
diff mbox series

Message ID 20190731145030.19956-2-aconole@redhat.com
State Superseded, archived
Headers show
Series
  • Enable fast-unit tests under travis
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-Compile-Testing success Compile Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

Aaron Conole July 31, 2019, 2:50 p.m. UTC
From: Michael Santana <msantana@redhat.com>

Currently many unit tests fail when running tests under shared builds.
This happens because of missing driver dependencies. This is fixed by
explicitly linking in missing drivers for the test application.

before and after (clang):
https://travis-ci.com/Maickii/dpdk-2/jobs/212329160#L623
https://travis-ci.com/Maickii/dpdk-2/jobs/212335912#L620

Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Suggested-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Michael Santana <msantana@redhat.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 app/test/meson.build | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Bruce Richardson July 31, 2019, 3:36 p.m. UTC | #1
On Wed, Jul 31, 2019 at 10:50:29AM -0400, Aaron Conole wrote:
> From: Michael Santana <msantana@redhat.com>
> 
> Currently many unit tests fail when running tests under shared builds.
> This happens because of missing driver dependencies. This is fixed by
> explicitly linking in missing drivers for the test application.
> 
> before and after (clang):
> https://travis-ci.com/Maickii/dpdk-2/jobs/212329160#L623
> https://travis-ci.com/Maickii/dpdk-2/jobs/212335912#L620
> 
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Suggested-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Michael Santana <msantana@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
Rather than linking in the libraries explicitly, can you have the build do
a "ninja install" at the end to place the libraries and drivers in their
correct paths. That should mean that the test app (via eal) auto-loads all
drivers from EAL_PMD_PATH (/usr/local/...). It would save having to make
further changes to this file to link in any additional drivers.

EAL_PMD_PATH is based off of $prefix for the build or install, so you can
adjust that using meson options, if putting the drivers in /usr/local is
not desirable for your test environments.

/Bruce
Aaron Conole July 31, 2019, 4:07 p.m. UTC | #2
Bruce Richardson <bruce.richardson@intel.com> writes:

> On Wed, Jul 31, 2019 at 10:50:29AM -0400, Aaron Conole wrote:
>> From: Michael Santana <msantana@redhat.com>
>> 
>> Currently many unit tests fail when running tests under shared builds.
>> This happens because of missing driver dependencies. This is fixed by
>> explicitly linking in missing drivers for the test application.
>> 
>> before and after (clang):
>> https://travis-ci.com/Maickii/dpdk-2/jobs/212329160#L623
>> https://travis-ci.com/Maickii/dpdk-2/jobs/212335912#L620
>> 
>> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
>> Suggested-by: David Marchand <david.marchand@redhat.com>
>> Signed-off-by: Michael Santana <msantana@redhat.com>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
> Rather than linking in the libraries explicitly, can you have the build do
> a "ninja install" at the end to place the libraries and drivers in their
> correct paths. That should mean that the test app (via eal) auto-loads all
> drivers from EAL_PMD_PATH (/usr/local/...). It would save having to make
> further changes to this file to link in any additional drivers.
>
> EAL_PMD_PATH is based off of $prefix for the build or install, so you can
> adjust that using meson options, if putting the drivers in /usr/local is
> not desirable for your test environments.

The downside of not explicitly linking this way is that a developer
won't be able to do something like:

  meson build
  ninja -C build

  ... development work

  ninja -C build test

Rather, they will notice broken tests and not care about testing any
more (since that's what we're dealing with now).

Requiring that a developer do the install (ninja -C build install) then
presents a different problem: before doing the 'test' I *must* remember
to do the install.  Otherwise I won't be testing with the correct
version of the libraries.

Is there a way to adjust the library search path?  Maybe we can solve
this by making that search path adjusted during the unit tests to look
in all the various build directories?

> /Bruce
Aaron Conole July 31, 2019, 4:10 p.m. UTC | #3
Aaron Conole <aconole@redhat.com> writes:

> Bruce Richardson <bruce.richardson@intel.com> writes:
>
>> On Wed, Jul 31, 2019 at 10:50:29AM -0400, Aaron Conole wrote:
>>> From: Michael Santana <msantana@redhat.com>
>>> 
>>> Currently many unit tests fail when running tests under shared builds.
>>> This happens because of missing driver dependencies. This is fixed by
>>> explicitly linking in missing drivers for the test application.
>>> 
>>> before and after (clang):
>>> https://travis-ci.com/Maickii/dpdk-2/jobs/212329160#L623
>>> https://travis-ci.com/Maickii/dpdk-2/jobs/212335912#L620
>>> 
>>> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>> Signed-off-by: Michael Santana <msantana@redhat.com>
>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>> ---
>> Rather than linking in the libraries explicitly, can you have the build do
>> a "ninja install" at the end to place the libraries and drivers in their
>> correct paths. That should mean that the test app (via eal) auto-loads all
>> drivers from EAL_PMD_PATH (/usr/local/...). It would save having to make
>> further changes to this file to link in any additional drivers.

Oops, forgot to include a link to a build where I did this.  It's
failing (you can see the corresponding commit at
https://github.com/orgcandman/dpdk/commit/ccba975bdfe851b4c8ec3f208451bb105317b76d):

  https://travis-ci.org/orgcandman/dpdk/jobs/566044409

>> EAL_PMD_PATH is based off of $prefix for the build or install, so you can
>> adjust that using meson options, if putting the drivers in /usr/local is
>> not desirable for your test environments.
>
> The downside of not explicitly linking this way is that a developer
> won't be able to do something like:
>
>   meson build
>   ninja -C build
>
>   ... development work
>
>   ninja -C build test
>
> Rather, they will notice broken tests and not care about testing any
> more (since that's what we're dealing with now).
>
> Requiring that a developer do the install (ninja -C build install) then
> presents a different problem: before doing the 'test' I *must* remember
> to do the install.  Otherwise I won't be testing with the correct
> version of the libraries.
>
> Is there a way to adjust the library search path?  Maybe we can solve
> this by making that search path adjusted during the unit tests to look
> in all the various build directories?
>
>> /Bruce
Bruce Richardson Aug. 1, 2019, 9:19 a.m. UTC | #4
On Wed, Jul 31, 2019 at 12:10:55PM -0400, Aaron Conole wrote:
> Aaron Conole <aconole@redhat.com> writes:
> 
> > Bruce Richardson <bruce.richardson@intel.com> writes:
> >
> >> On Wed, Jul 31, 2019 at 10:50:29AM -0400, Aaron Conole wrote:
> >>> From: Michael Santana <msantana@redhat.com>
> >>> 
> >>> Currently many unit tests fail when running tests under shared builds.
> >>> This happens because of missing driver dependencies. This is fixed by
> >>> explicitly linking in missing drivers for the test application.
> >>> 
> >>> before and after (clang):
> >>> https://travis-ci.com/Maickii/dpdk-2/jobs/212329160#L623
> >>> https://travis-ci.com/Maickii/dpdk-2/jobs/212335912#L620
> >>> 
> >>> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> >>> Suggested-by: David Marchand <david.marchand@redhat.com>
> >>> Signed-off-by: Michael Santana <msantana@redhat.com>
> >>> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >>> ---
> >> Rather than linking in the libraries explicitly, can you have the build do
> >> a "ninja install" at the end to place the libraries and drivers in their
> >> correct paths. That should mean that the test app (via eal) auto-loads all
> >> drivers from EAL_PMD_PATH (/usr/local/...). It would save having to make
> >> further changes to this file to link in any additional drivers.
> 
> Oops, forgot to include a link to a build where I did this.  It's
> failing (you can see the corresponding commit at
> https://github.com/orgcandman/dpdk/commit/ccba975bdfe851b4c8ec3f208451bb105317b76d):
> 
>   https://travis-ci.org/orgcandman/dpdk/jobs/566044409
> 
I think the error may be due to having some drivers already linked in while
trying to reload all drivers dynamically. The dynamic loading of drivers I
actually think we could make more robust. For example:

* only load .so files. If you pass in the drivers path from the build, it
  errors out trying to load .a files
* when loading directories, maybe skip any files which don't have a "librte"
  prefix - anyone who has their own drivers they want loaded can add the
  prefix or load it explicitly via -d
* alternatively, skip files which don't have a pmdinfo section in them
* allow skipping of drivers which are already linked in
* don't fail the whole process if one driver fails to load from a
  directory.

Regards,
/Bruce
Aaron Conole Aug. 1, 2019, 3:40 p.m. UTC | #5
Bruce Richardson <bruce.richardson@intel.com> writes:

> On Wed, Jul 31, 2019 at 12:10:55PM -0400, Aaron Conole wrote:
>> Aaron Conole <aconole@redhat.com> writes:
>> 
>> > Bruce Richardson <bruce.richardson@intel.com> writes:
>> >
>> >> On Wed, Jul 31, 2019 at 10:50:29AM -0400, Aaron Conole wrote:
>> >>> From: Michael Santana <msantana@redhat.com>
>> >>> 
>> >>> Currently many unit tests fail when running tests under shared builds.
>> >>> This happens because of missing driver dependencies. This is fixed by
>> >>> explicitly linking in missing drivers for the test application.
>> >>> 
>> >>> before and after (clang):
>> >>> https://travis-ci.com/Maickii/dpdk-2/jobs/212329160#L623
>> >>> https://travis-ci.com/Maickii/dpdk-2/jobs/212335912#L620
>> >>> 
>> >>> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
>> >>> Suggested-by: David Marchand <david.marchand@redhat.com>
>> >>> Signed-off-by: Michael Santana <msantana@redhat.com>
>> >>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> >>> ---
>> >> Rather than linking in the libraries explicitly, can you have the build do
>> >> a "ninja install" at the end to place the libraries and drivers in their
>> >> correct paths. That should mean that the test app (via eal) auto-loads all
>> >> drivers from EAL_PMD_PATH (/usr/local/...). It would save having to make
>> >> further changes to this file to link in any additional drivers.
>> 
>> Oops, forgot to include a link to a build where I did this.  It's
>> failing (you can see the corresponding commit at
>> https://github.com/orgcandman/dpdk/commit/ccba975bdfe851b4c8ec3f208451bb105317b76d):
>> 
>>   https://travis-ci.org/orgcandman/dpdk/jobs/566044409
>> 
> I think the error may be due to having some drivers already linked in while
> trying to reload all drivers dynamically. The dynamic loading of drivers I
> actually think we could make more robust. For example:
>
> * only load .so files. If you pass in the drivers path from the build, it
>   errors out trying to load .a files
> * when loading directories, maybe skip any files which don't have a "librte"
>   prefix - anyone who has their own drivers they want loaded can add the
>   prefix or load it explicitly via -d
> * alternatively, skip files which don't have a pmdinfo section in them
> * allow skipping of drivers which are already linked in
> * don't fail the whole process if one driver fails to load from a
>   directory.

What change do you suggest I make to the steps I outlined?  I think it
seems to be more complex than a simple 'ninja install'.

Maybe it makes sense to merge these patches now and fix the library
loading code as a separate action?  Or is there a different procedure I
(and other developers) should follow?  Does it really need a 'ninja
install' even for a drive-by developer to make a small patch and test
before submitting?

> Regards,
> /Bruce
Bruce Richardson Aug. 1, 2019, 4:51 p.m. UTC | #6
On Thu, Aug 01, 2019 at 11:40:33AM -0400, Aaron Conole wrote:
> Bruce Richardson <bruce.richardson@intel.com> writes:
> 
> > On Wed, Jul 31, 2019 at 12:10:55PM -0400, Aaron Conole wrote:
> >> Aaron Conole <aconole@redhat.com> writes:
> >> 
> >> > Bruce Richardson <bruce.richardson@intel.com> writes:
> >> >
> >> >> On Wed, Jul 31, 2019 at 10:50:29AM -0400, Aaron Conole wrote:
> >> >>> From: Michael Santana <msantana@redhat.com>
> >> >>> 
> >> >>> Currently many unit tests fail when running tests under shared builds.
> >> >>> This happens because of missing driver dependencies. This is fixed by
> >> >>> explicitly linking in missing drivers for the test application.
> >> >>> 
> >> >>> before and after (clang):
> >> >>> https://travis-ci.com/Maickii/dpdk-2/jobs/212329160#L623
> >> >>> https://travis-ci.com/Maickii/dpdk-2/jobs/212335912#L620
> >> >>> 
> >> >>> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> >> >>> Suggested-by: David Marchand <david.marchand@redhat.com>
> >> >>> Signed-off-by: Michael Santana <msantana@redhat.com>
> >> >>> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> >>> ---
> >> >> Rather than linking in the libraries explicitly, can you have the build do
> >> >> a "ninja install" at the end to place the libraries and drivers in their
> >> >> correct paths. That should mean that the test app (via eal) auto-loads all
> >> >> drivers from EAL_PMD_PATH (/usr/local/...). It would save having to make
> >> >> further changes to this file to link in any additional drivers.
> >> 
> >> Oops, forgot to include a link to a build where I did this.  It's
> >> failing (you can see the corresponding commit at
> >> https://github.com/orgcandman/dpdk/commit/ccba975bdfe851b4c8ec3f208451bb105317b76d):
> >> 
> >>   https://travis-ci.org/orgcandman/dpdk/jobs/566044409
> >> 
> > I think the error may be due to having some drivers already linked in while
> > trying to reload all drivers dynamically. The dynamic loading of drivers I
> > actually think we could make more robust. For example:
> >
> > * only load .so files. If you pass in the drivers path from the build, it
> >   errors out trying to load .a files
> > * when loading directories, maybe skip any files which don't have a "librte"
> >   prefix - anyone who has their own drivers they want loaded can add the
> >   prefix or load it explicitly via -d
> > * alternatively, skip files which don't have a pmdinfo section in them
> > * allow skipping of drivers which are already linked in
> > * don't fail the whole process if one driver fails to load from a
> >   directory.
> 
> What change do you suggest I make to the steps I outlined?  I think it
> seems to be more complex than a simple 'ninja install'.
> 
> Maybe it makes sense to merge these patches now and fix the library
> loading code as a separate action?  Or is there a different procedure I
> (and other developers) should follow?  Does it really need a 'ninja
> install' even for a drive-by developer to make a small patch and test
> before submitting?
> 
I think your patch is the best solution for 19.08. Fixing the library
loading for developers is a larger job beyond that. 

Another item for future consideration: since meson/ninja automatically sets
the rpath for binaries in build directory - and then strips that out on
install - it would be really nice if we could leverage that somehow for
finding drivers.

/Bruce
Bruce Richardson Aug. 1, 2019, 4:52 p.m. UTC | #7
On Wed, Jul 31, 2019 at 10:50:29AM -0400, Aaron Conole wrote:
> From: Michael Santana <msantana@redhat.com>
> 
> Currently many unit tests fail when running tests under shared builds.
> This happens because of missing driver dependencies. This is fixed by
> explicitly linking in missing drivers for the test application.
> 
> before and after (clang):
> https://travis-ci.com/Maickii/dpdk-2/jobs/212329160#L623
> https://travis-ci.com/Maickii/dpdk-2/jobs/212335912#L620
> 
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Suggested-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Michael Santana <msantana@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  app/test/meson.build | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Thomas Monjalon Aug. 2, 2019, 8:32 p.m. UTC | #8
31/07/2019 16:50, Aaron Conole:
> From: Michael Santana <msantana@redhat.com>
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> +if dpdk_conf.has('RTE_LIBRTE_RING_MEMPOOL')
> +	test_deps += 'mempool_ring'
> +endif
> +if dpdk_conf.has('RTE_LIBRTE_STACK_MEMPOOL')
> +	test_deps += 'mempool_stack'
> +endif
> +if dpdk_conf.has('RTE_LIBRTE_SKELETON_EVENTDEV_PMD')
> +	test_deps += 'pmd_skeleton_event'
> +endif
>  if dpdk_conf.has('RTE_LIBRTE_PDUMP')
>  	test_deps += 'pdump'
>  endif

As these libraries are dynamically loadable as plugins,
it should be explained in comments that this explicit linking
is an exception to allow the developer run the unit tests
without intalling the libraries.
Otherwise, other applications will copy it.
Aaron Conole Aug. 2, 2019, 8:43 p.m. UTC | #9
Thomas Monjalon <thomas@monjalon.net> writes:

> 31/07/2019 16:50, Aaron Conole:
>> From: Michael Santana <msantana@redhat.com>
>> --- a/app/test/meson.build
>> +++ b/app/test/meson.build
>> +if dpdk_conf.has('RTE_LIBRTE_RING_MEMPOOL')
>> +	test_deps += 'mempool_ring'
>> +endif
>> +if dpdk_conf.has('RTE_LIBRTE_STACK_MEMPOOL')
>> +	test_deps += 'mempool_stack'
>> +endif
>> +if dpdk_conf.has('RTE_LIBRTE_SKELETON_EVENTDEV_PMD')
>> +	test_deps += 'pmd_skeleton_event'
>> +endif
>>  if dpdk_conf.has('RTE_LIBRTE_PDUMP')
>>  	test_deps += 'pdump'
>>  endif
>
> As these libraries are dynamically loadable as plugins,
> it should be explained in comments that this explicit linking
> is an exception to allow the developer run the unit tests
> without intalling the libraries.
> Otherwise, other applications will copy it.

Okay.  I'll add it.

Patch
diff mbox series

diff --git a/app/test/meson.build b/app/test/meson.build
index c50b20275..7bf700f29 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -297,6 +297,15 @@  dump_test_names = [
         'dump_memzone',
 ]
 
+if dpdk_conf.has('RTE_LIBRTE_RING_MEMPOOL')
+	test_deps += 'mempool_ring'
+endif
+if dpdk_conf.has('RTE_LIBRTE_STACK_MEMPOOL')
+	test_deps += 'mempool_stack'
+endif
+if dpdk_conf.has('RTE_LIBRTE_SKELETON_EVENTDEV_PMD')
+	test_deps += 'pmd_skeleton_event'
+endif
 if dpdk_conf.has('RTE_LIBRTE_PDUMP')
 	test_deps += 'pdump'
 endif