[v14,04/11] app/test: skip interrupt tests on Windows

  Even though test_interrupts.c can compile on Windows, skip interrupt
tests for now since majority of eal_interrupt on Windows are stubs.
Will remove the skip after interrupt being fully enabled on Windows.

Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

---
 app/test/test_interrupts.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
  

On Thu, Dec 9, 2021 at 12:30 AM Jie Zhou <jizh@linux.microsoft.com> wrote:
>
> Even though test_interrupts.c can compile on Windows, skip interrupt
> tests for now since majority of eal_interrupt on Windows are stubs.
> Will remove the skip after interrupt being fully enabled on Windows.
>
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
>
> ---
>  app/test/test_interrupts.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/app/test/test_interrupts.c b/app/test/test_interrupts.c
> index 2a05399f96..eec9b2805b 100644
> --- a/app/test/test_interrupts.c
> +++ b/app/test/test_interrupts.c
> @@ -12,6 +12,15 @@
>
>  #include "test.h"
>
> +#ifdef RTE_EXEC_ENV_WINDOWS

Across the series,
Instead of adding conditional compilation everywhere, Why not disable
specific file
for compilation for windows?
Purpose of EAL to abstract the differences in execution environment
and application
should not know that.


> +static int
> +test_interrupt(void)
> +{
> +       printf("Interrupt on Windows is not fully supported yet, skipping test\n");
> +       return TEST_SKIPPED;
> +}
> +#else
> +
>  #define TEST_INTERRUPT_CHECK_INTERVAL 100 /* ms */
>
>  /* predefined interrupt handle types */
> @@ -590,5 +599,6 @@ test_interrupt(void)
>
>         return ret;
>  }
> +#endif /*ifdef RTE_EXEC_ENV_WINDOWS*/
>
>  REGISTER_TEST_COMMAND(interrupt_autotest, test_interrupt);
> --
> 2.31.0.vfs.0.1
>
  

Jerin Jacob <jerinjacobk@gmail.com> writes:

> On Thu, Dec 9, 2021 at 12:30 AM Jie Zhou <jizh@linux.microsoft.com> wrote:
>>
>> Even though test_interrupts.c can compile on Windows, skip interrupt
>> tests for now since majority of eal_interrupt on Windows are stubs.
>> Will remove the skip after interrupt being fully enabled on Windows.
>>
>> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
>> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
>>
>> ---
>>  app/test/test_interrupts.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/app/test/test_interrupts.c b/app/test/test_interrupts.c
>> index 2a05399f96..eec9b2805b 100644
>> --- a/app/test/test_interrupts.c
>> +++ b/app/test/test_interrupts.c
>> @@ -12,6 +12,15 @@
>>
>>  #include "test.h"
>>
>> +#ifdef RTE_EXEC_ENV_WINDOWS
>
> Across the series,
> Instead of adding conditional compilation everywhere, Why not disable
> specific file
> for compilation for windows?
> Purpose of EAL to abstract the differences in execution environment
> and application
> should not know that.

I think this was done because there would be two test lists in the meson
unit test file.  But this is the second comment about these ifdef's, and
maybe we should revisit that discussion.  Is there a different way to
accomplish not running the tests which are not appropriate for windows
builds, while not having two overlapping lists of unit tests in the
meson build file?

>> +static int
>> +test_interrupt(void)
>> +{
>> +       printf("Interrupt on Windows is not fully supported yet, skipping test\n");
>> +       return TEST_SKIPPED;
>> +}
>> +#else
>> +
>>  #define TEST_INTERRUPT_CHECK_INTERVAL 100 /* ms */
>>
>>  /* predefined interrupt handle types */
>> @@ -590,5 +599,6 @@ test_interrupt(void)
>>
>>         return ret;
>>  }
>> +#endif /*ifdef RTE_EXEC_ENV_WINDOWS*/
>>
>>  REGISTER_TEST_COMMAND(interrupt_autotest, test_interrupt);
>> --
>> 2.31.0.vfs.0.1
>>
  

On Thu, Dec 09, 2021 at 08:15:01AM -0500, Aaron Conole wrote:
> Jerin Jacob <jerinjacobk@gmail.com> writes:
> 
> > On Thu, Dec 9, 2021 at 12:30 AM Jie Zhou <jizh@linux.microsoft.com> wrote:
> >>
> >> Even though test_interrupts.c can compile on Windows, skip interrupt
> >> tests for now since majority of eal_interrupt on Windows are stubs.
> >> Will remove the skip after interrupt being fully enabled on Windows.
> >>
> >> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> >> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> >>
> >> ---
> >>  app/test/test_interrupts.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/app/test/test_interrupts.c b/app/test/test_interrupts.c
> >> index 2a05399f96..eec9b2805b 100644
> >> --- a/app/test/test_interrupts.c
> >> +++ b/app/test/test_interrupts.c
> >> @@ -12,6 +12,15 @@
> >>
> >>  #include "test.h"
> >>
> >> +#ifdef RTE_EXEC_ENV_WINDOWS
> >
> > Across the series,
> > Instead of adding conditional compilation everywhere, Why not disable
> > specific file
> > for compilation for windows?
> > Purpose of EAL to abstract the differences in execution environment
> > and application
> > should not know that.
> 
> I think this was done because there would be two test lists in the meson
> unit test file.  But this is the second comment about these ifdef's, and
> maybe we should revisit that discussion.  Is there a different way to
> accomplish not running the tests which are not appropriate for windows
> builds, while not having two overlapping lists of unit tests in the
> meson build file?
> 
I'm wondering if a reasonable compromise solution might be to have the
build system expose a usable RTE_EXEC_ENV symbol that can be used in C-code
if statements rather than just in ifdefs. That would allow us to easily add
e.g.

if (RTE_EXEC_ENV == rte_env_linux)
    return TEST_SKIPPED;

into each test function needing it. Two lines of C-code is a lot easier to
add and manage than #ifdefs covering the whole file, or alternative lists
in meson.

/Bruce
  

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Thursday, December 9, 2021 4:17 PM
> To: Aaron Conole <aconole@redhat.com>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Jie Zhou
> <jizh@linux.microsoft.com>; dpdk-dev <dev@dpdk.org>; Dmitry Kozlyuk
> <dmitry.kozliuk@gmail.com>; roretzla@microsoft.com; Narcisa Ana Maria
> Vasile <navasile@linux.microsoft.com>; Dmitry Malloy (MESHCHANINOV)
> <dmitrym@microsoft.com>; Kadam, Pallavi <pallavi.kadam@intel.com>;
> talshn@nvidia.com; Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [PATCH v14 04/11] app/test: skip interrupt tests on Windows
> 
> On Thu, Dec 09, 2021 at 08:15:01AM -0500, Aaron Conole wrote:
> > Jerin Jacob <jerinjacobk@gmail.com> writes:
> >
> > > On Thu, Dec 9, 2021 at 12:30 AM Jie Zhou <jizh@linux.microsoft.com>
> wrote:
> > >>
> > >> Even though test_interrupts.c can compile on Windows, skip interrupt
> > >> tests for now since majority of eal_interrupt on Windows are stubs.
> > >> Will remove the skip after interrupt being fully enabled on Windows.
> > >>
> > >> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > >> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > >>
> > >> ---
> > >>  app/test/test_interrupts.c | 10 ++++++++++
> > >>  1 file changed, 10 insertions(+)
> > >>
> > >> diff --git a/app/test/test_interrupts.c b/app/test/test_interrupts.c
> > >> index 2a05399f96..eec9b2805b 100644
> > >> --- a/app/test/test_interrupts.c
> > >> +++ b/app/test/test_interrupts.c
> > >> @@ -12,6 +12,15 @@
> > >>
> > >>  #include "test.h"
> > >>
> > >> +#ifdef RTE_EXEC_ENV_WINDOWS
> > >
> > > Across the series,
> > > Instead of adding conditional compilation everywhere, Why not disable
> > > specific file
> > > for compilation for windows?
> > > Purpose of EAL to abstract the differences in execution environment
> > > and application
> > > should not know that.
> >
> > I think this was done because there would be two test lists in the meson
> > unit test file.  But this is the second comment about these ifdef's, and
> > maybe we should revisit that discussion.  Is there a different way to
> > accomplish not running the tests which are not appropriate for windows
> > builds, while not having two overlapping lists of unit tests in the
> > meson build file?
> >
> I'm wondering if a reasonable compromise solution might be to have the
> build system expose a usable RTE_EXEC_ENV symbol that can be used in C-
> code
> if statements rather than just in ifdefs. That would allow us to easily
> add
> e.g.
> 
> if (RTE_EXEC_ENV == rte_env_linux)
>     return TEST_SKIPPED;

or even " == rte_env_windows" for this case! 😊
  

On Thu, Dec 09, 2021 at 04:17:08PM +0000, Bruce Richardson wrote:
> On Thu, Dec 09, 2021 at 08:15:01AM -0500, Aaron Conole wrote:
> > Jerin Jacob <jerinjacobk@gmail.com> writes:
> > 
> > > On Thu, Dec 9, 2021 at 12:30 AM Jie Zhou <jizh@linux.microsoft.com> wrote:
> > >>
> > >> Even though test_interrupts.c can compile on Windows, skip interrupt
> > >> tests for now since majority of eal_interrupt on Windows are stubs.
> > >> Will remove the skip after interrupt being fully enabled on Windows.
> > >>
> > >> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > >> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > >>
> > >> ---
> > >>  app/test/test_interrupts.c | 10 ++++++++++
> > >>  1 file changed, 10 insertions(+)
> > >>
> > >> diff --git a/app/test/test_interrupts.c b/app/test/test_interrupts.c
> > >> index 2a05399f96..eec9b2805b 100644
> > >> --- a/app/test/test_interrupts.c
> > >> +++ b/app/test/test_interrupts.c
> > >> @@ -12,6 +12,15 @@
> > >>
> > >>  #include "test.h"
> > >>
> > >> +#ifdef RTE_EXEC_ENV_WINDOWS
> > >
> > > Across the series,
> > > Instead of adding conditional compilation everywhere, Why not disable
> > > specific file
> > > for compilation for windows?
> > > Purpose of EAL to abstract the differences in execution environment
> > > and application
> > > should not know that.
> > 
> > I think this was done because there would be two test lists in the meson
> > unit test file.  But this is the second comment about these ifdef's, and
> > maybe we should revisit that discussion.  Is there a different way to
> > accomplish not running the tests which are not appropriate for windows
> > builds, while not having two overlapping lists of unit tests in the
> > meson build file?
> > 
> I'm wondering if a reasonable compromise solution might be to have the
> build system expose a usable RTE_EXEC_ENV symbol that can be used in C-code
> if statements rather than just in ifdefs. That would allow us to easily add
> e.g.
> 
> if (RTE_EXEC_ENV == rte_env_linux)
>     return TEST_SKIPPED;
> 
> into each test function needing it. Two lines of C-code is a lot easier to
> add and manage than #ifdefs covering the whole file, or alternative lists
> in meson.
> 
Quick patch to allow C-code comparisons:

diff --git a/lib/eal/meson.build b/lib/eal/meson.build
index 1722924f67..b5b9fa14b4 100644
--- a/lib/eal/meson.build
+++ b/lib/eal/meson.build
@@ -10,6 +10,12 @@ if not is_windows
     subdir('unix')
 endif
 
+exec_envs = {'freebsd': 0, 'linux': 1, 'windows': 2}
+foreach env, id:exec_envs
+    dpdk_conf.set('RTE_ENV_' + env.to_upper(), id)
+endforeach
+dpdk_conf.set('RTE_EXEC_ENV', exec_envs[exec_env])
+
 dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
 subdir(exec_env)

A slightly simpler patch would just expose the environment as a string as
e.g. "linux", but I think numeric ids just make the code better rather than
having string comparisons. Alternatively, this could also be done via
C-code with ifdefs in EAL, but as it stands this meson change allows:

  if (RTE_EXEC_ENV == RTE_ENV_WINDOWS)
     ...

or:

  switch (RTE_EXEC_ENV) {
    case RTE_ENV_LINUX: ... ; break;
    case RTE_ENV_FREEBSD: ... ; break;
    case RTE_ENV_WINDOWS: ... ; break;
  }

Thoughts?

/Bruce
  

2021-12-09 16:39 (UTC+0000), Bruce Richardson:
> On Thu, Dec 09, 2021 at 04:17:08PM +0000, Bruce Richardson wrote:
> [...]
> > I'm wondering if a reasonable compromise solution might be to have the
> > build system expose a usable RTE_EXEC_ENV symbol that can be used in C-code
> > if statements rather than just in ifdefs. That would allow us to easily add
> > e.g.
> > 
> > if (RTE_EXEC_ENV == rte_env_linux)
> >     return TEST_SKIPPED;
> > 
> > into each test function needing it. Two lines of C-code is a lot easier to
> > add and manage than #ifdefs covering the whole file, or alternative lists
> > in meson.
> >   
> Quick patch to allow C-code comparisons:
> 
> diff --git a/lib/eal/meson.build b/lib/eal/meson.build
> index 1722924f67..b5b9fa14b4 100644
> --- a/lib/eal/meson.build
> +++ b/lib/eal/meson.build
> @@ -10,6 +10,12 @@ if not is_windows
>      subdir('unix')
>  endif
>  
> +exec_envs = {'freebsd': 0, 'linux': 1, 'windows': 2}
> +foreach env, id:exec_envs
> +    dpdk_conf.set('RTE_ENV_' + env.to_upper(), id)
> +endforeach
> +dpdk_conf.set('RTE_EXEC_ENV', exec_envs[exec_env])
> +
>  dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
>  subdir(exec_env)
> 
> A slightly simpler patch would just expose the environment as a string as
> e.g. "linux", but I think numeric ids just make the code better rather than
> having string comparisons. Alternatively, this could also be done via
> C-code with ifdefs in EAL, but as it stands this meson change allows:
> 
>   if (RTE_EXEC_ENV == RTE_ENV_WINDOWS)
>      ...
> 
> or:
> 
>   switch (RTE_EXEC_ENV) {
>     case RTE_ENV_LINUX: ... ; break;
>     case RTE_ENV_FREEBSD: ... ; break;
>     case RTE_ENV_WINDOWS: ... ; break;
>   }
> 
> Thoughts?

I like this.
Even outside of tests more code can be made to compile on all platforms
(e.g. ixgbe_wait_for_link_up).
Alternative naming: RTE_EXEC_ENV_IS_* (similar to RTE_CC_IS_*),
which does not allow switch statements, but shortens most practical cases.
Will Coverity understand that if a condition is always false,
variables beneath still may be used on another platform?
  

On Fri, Dec 10, 2021 at 12:23:59PM +0300, Dmitry Kozlyuk wrote:
> 2021-12-09 16:39 (UTC+0000), Bruce Richardson:
> > On Thu, Dec 09, 2021 at 04:17:08PM +0000, Bruce Richardson wrote:
> > [...]
> > > I'm wondering if a reasonable compromise solution might be to have the
> > > build system expose a usable RTE_EXEC_ENV symbol that can be used in C-code
> > > if statements rather than just in ifdefs. That would allow us to easily add
> > > e.g.
> > > 
> > > if (RTE_EXEC_ENV == rte_env_linux)
> > >     return TEST_SKIPPED;
> > > 
> > > into each test function needing it. Two lines of C-code is a lot easier to
> > > add and manage than #ifdefs covering the whole file, or alternative lists
> > > in meson.
> > >   
> > Quick patch to allow C-code comparisons:
> > 
> > diff --git a/lib/eal/meson.build b/lib/eal/meson.build
> > index 1722924f67..b5b9fa14b4 100644
> > --- a/lib/eal/meson.build
> > +++ b/lib/eal/meson.build
> > @@ -10,6 +10,12 @@ if not is_windows
> >      subdir('unix')
> >  endif
> >  
> > +exec_envs = {'freebsd': 0, 'linux': 1, 'windows': 2}
> > +foreach env, id:exec_envs
> > +    dpdk_conf.set('RTE_ENV_' + env.to_upper(), id)
> > +endforeach
> > +dpdk_conf.set('RTE_EXEC_ENV', exec_envs[exec_env])
> > +
> >  dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
> >  subdir(exec_env)
> > 
> > A slightly simpler patch would just expose the environment as a string as
> > e.g. "linux", but I think numeric ids just make the code better rather than
> > having string comparisons. Alternatively, this could also be done via
> > C-code with ifdefs in EAL, but as it stands this meson change allows:
> > 
> >   if (RTE_EXEC_ENV == RTE_ENV_WINDOWS)
> >      ...
> > 
> > or:
> > 
> >   switch (RTE_EXEC_ENV) {
> >     case RTE_ENV_LINUX: ... ; break;
> >     case RTE_ENV_FREEBSD: ... ; break;
> >     case RTE_ENV_WINDOWS: ... ; break;
> >   }
> > 
> > Thoughts?
> 
> I like this.
> Even outside of tests more code can be made to compile on all platforms
> (e.g. ixgbe_wait_for_link_up).
> Alternative naming: RTE_EXEC_ENV_IS_* (similar to RTE_CC_IS_*),
> which does not allow switch statements, but shortens most practical cases.

Sure. I wonder if it is worthwhile implementing both, since it's not a
large amount of code.

> Will Coverity understand that if a condition is always false,
> variables beneath still may be used on another platform?

That I don't know, unfortunately. Perhaps some coverity experts can weigh
in.

/Bruce
  

The proposal below is now merged.
Please Jie, use it in a v15 of this series.


10/12/2021 10:30, Bruce Richardson:
> On Fri, Dec 10, 2021 at 12:23:59PM +0300, Dmitry Kozlyuk wrote:
> > 2021-12-09 16:39 (UTC+0000), Bruce Richardson:
> > > On Thu, Dec 09, 2021 at 04:17:08PM +0000, Bruce Richardson wrote:
> > > [...]
> > > > I'm wondering if a reasonable compromise solution might be to have the
> > > > build system expose a usable RTE_EXEC_ENV symbol that can be used in C-code
> > > > if statements rather than just in ifdefs. That would allow us to easily add
> > > > e.g.
> > > > 
> > > > if (RTE_EXEC_ENV == rte_env_linux)
> > > >     return TEST_SKIPPED;
> > > > 
> > > > into each test function needing it. Two lines of C-code is a lot easier to
> > > > add and manage than #ifdefs covering the whole file, or alternative lists
> > > > in meson.
> > > >   
> > > Quick patch to allow C-code comparisons:
> > > 
> > > diff --git a/lib/eal/meson.build b/lib/eal/meson.build
> > > index 1722924f67..b5b9fa14b4 100644
> > > --- a/lib/eal/meson.build
> > > +++ b/lib/eal/meson.build
> > > @@ -10,6 +10,12 @@ if not is_windows
> > >      subdir('unix')
> > >  endif
> > >  
> > > +exec_envs = {'freebsd': 0, 'linux': 1, 'windows': 2}
> > > +foreach env, id:exec_envs
> > > +    dpdk_conf.set('RTE_ENV_' + env.to_upper(), id)
> > > +endforeach
> > > +dpdk_conf.set('RTE_EXEC_ENV', exec_envs[exec_env])
> > > +
> > >  dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
> > >  subdir(exec_env)
> > > 
> > > A slightly simpler patch would just expose the environment as a string as
> > > e.g. "linux", but I think numeric ids just make the code better rather than
> > > having string comparisons. Alternatively, this could also be done via
> > > C-code with ifdefs in EAL, but as it stands this meson change allows:
> > > 
> > >   if (RTE_EXEC_ENV == RTE_ENV_WINDOWS)
> > >      ...
> > > 
> > > or:
> > > 
> > >   switch (RTE_EXEC_ENV) {
> > >     case RTE_ENV_LINUX: ... ; break;
> > >     case RTE_ENV_FREEBSD: ... ; break;
> > >     case RTE_ENV_WINDOWS: ... ; break;
> > >   }
> > > 
> > > Thoughts?
> > 
> > I like this.
> > Even outside of tests more code can be made to compile on all platforms
> > (e.g. ixgbe_wait_for_link_up).
> > Alternative naming: RTE_EXEC_ENV_IS_* (similar to RTE_CC_IS_*),
> > which does not allow switch statements, but shortens most practical cases.
> 
> Sure. I wonder if it is worthwhile implementing both, since it's not a
> large amount of code.
> 
> > Will Coverity understand that if a condition is always false,
> > variables beneath still may be used on another platform?
> 
> That I don't know, unfortunately. Perhaps some coverity experts can weigh
> in.