[PATCH] math: Add LIBM_TEST_VERBOSE environment variable support.

[PATCH] math: Add LIBM_TEST_VERBOSE environment variable support.

[Joe Simmons-Talbott]

From: Joe Talbott <joetalbott@gmail.com>

Allow the libm-test-driver based tests to have their verbosity set based
on the LIMB_TESt_VERBOSE environment variable.  This allows the entire
testsuite to be run with a non-default verbosity.
---
 math/libm-test-support.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/math/libm-test-support.c b/math/libm-test-support.c
index 1d60ac783b..a45686054f 100644
--- a/math/libm-test-support.c
+++ b/math/libm-test-support.c
@@ -1139,6 +1139,7 @@ libm_test_init (int argc, char **argv)
   int remaining;
   char *ulps_file_path;
   size_t dir_len = 0;
+  char *verbose_env;
 
   verbose = 1;
   output_ulps = 0;
@@ -1148,6 +1149,10 @@ libm_test_init (int argc, char **argv)
   /* XXX set to 0 for releases.  */
   ignore_max_ulp = 0;
 
+  verbose_env = getenv("LIBM_TEST_VERBOSE");
+  if (verbose_env != NULL)
+    verbose = (unsigned int) strtoul (verbose_env, NULL, 0);
+
   /* Parse and process arguments.  */
   argp_parse (&argp, argc, argv, 0, &remaining, NULL);
 
-- 
2.44.0

Re: [PATCH] math: Add LIBM_TEST_VERBOSE environment variable support.

[Carlos O'Donell]

On 5/6/24 20:34, Joe Simmons-Talbott wrote:
> From: Joe Talbott <joetalbott@gmail.com>
> 
> Allow the libm-test-driver based tests to have their verbosity set based
> on the LIMB_TESt_VERBOSE environment variable.  This allows the entire

s/LIMB_TESt_VERBOSE/LIBM_TEST_VERBOSE/g

Please call it GLIBC_TEST_LIBM_VERBOSE.

We should really use a known conventional prefix e.g. GLIBC_TEST.

We don't currently, but lets move in that direction.

At worse I see WAIT_FOR_DEBUGGER, PID_OUTSIDE_CONTAINER, TIMEOUTFACTOR,
TEST_DIRECT and TEST_COREDUMPS, which could all get cleaned up.

> testsuite to be run with a non-default verbosity.
> ---
>  math/libm-test-support.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/math/libm-test-support.c b/math/libm-test-support.c
> index 1d60ac783b..a45686054f 100644
> --- a/math/libm-test-support.c
> +++ b/math/libm-test-support.c
> @@ -1139,6 +1139,7 @@ libm_test_init (int argc, char **argv)
>    int remaining;
>    char *ulps_file_path;
>    size_t dir_len = 0;
> +  char *verbose_env;
>  
>    verbose = 1;
>    output_ulps = 0;
> @@ -1148,6 +1149,10 @@ libm_test_init (int argc, char **argv)
>    /* XXX set to 0 for releases.  */
>    ignore_max_ulp = 0;
>  
> +  verbose_env = getenv("LIBM_TEST_VERBOSE");

Adjust as above.

> +  if (verbose_env != NULL)
> +    verbose = (unsigned int) strtoul (verbose_env, NULL, 0);

What happens if this conversion fails?

Please see how we do this with TIMEOUTFACOR:

326   /* If set, read the test TIMEOUTFACTOR value from the environment.
327      This value is used to scale the default test timeout values. */
328   char *envstr_timeoutfactor = getenv ("TIMEOUTFACTOR");
329   if (envstr_timeoutfactor != NULL)
330     {
331       char *envstr_conv = envstr_timeoutfactor;
332       unsigned long int env_fact;
333 
334       env_fact = strtoul (envstr_timeoutfactor, &envstr_conv, 0);
335       if (*envstr_conv == '\0' && envstr_conv != envstr_timeoutfactor)
336         timeoutfactor = MAX (env_fact, 1);
337     }


> +
>    /* Parse and process arguments.  */
>    argp_parse (&argp, argc, argv, 0, &remaining, NULL);
>  

-- 
Cheers,
Carlos.

Re: [PATCH] math: Add LIBM_TEST_VERBOSE environment variable support.

[Joe Simmons-Talbott]

On Wed, May 8, 2024 at 9:29 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 5/6/24 20:34, Joe Simmons-Talbott wrote:
> > From: Joe Talbott <joetalbott@gmail.com>
> >
> > Allow the libm-test-driver based tests to have their verbosity set based
> > on the LIMB_TESt_VERBOSE environment variable.  This allows the entire
>
> s/LIMB_TESt_VERBOSE/LIBM_TEST_VERBOSE/g
>
> Please call it GLIBC_TEST_LIBM_VERBOSE.

Ok

>
> We should really use a known conventional prefix e.g. GLIBC_TEST.
>
> We don't currently, but lets move in that direction.
>
> At worse I see WAIT_FOR_DEBUGGER, PID_OUTSIDE_CONTAINER, TIMEOUTFACTOR,
> TEST_DIRECT and TEST_COREDUMPS, which could all get cleaned up.
>
> > testsuite to be run with a non-default verbosity.
> > ---
> >  math/libm-test-support.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/math/libm-test-support.c b/math/libm-test-support.c
> > index 1d60ac783b..a45686054f 100644
> > --- a/math/libm-test-support.c
> > +++ b/math/libm-test-support.c
> > @@ -1139,6 +1139,7 @@ libm_test_init (int argc, char **argv)
> >    int remaining;
> >    char *ulps_file_path;
> >    size_t dir_len = 0;
> > +  char *verbose_env;
> >
> >    verbose = 1;
> >    output_ulps = 0;
> > @@ -1148,6 +1149,10 @@ libm_test_init (int argc, char **argv)
> >    /* XXX set to 0 for releases.  */
> >    ignore_max_ulp = 0;
> >
> > +  verbose_env = getenv("LIBM_TEST_VERBOSE");
>
> Adjust as above.
>
> > +  if (verbose_env != NULL)
> > +    verbose = (unsigned int) strtoul (verbose_env, NULL, 0);
>
> What happens if this conversion fails?
>
> Please see how we do this with TIMEOUTFACOR:
>
> 326   /* If set, read the test TIMEOUTFACTOR value from the environment.
> 327      This value is used to scale the default test timeout values. */
> 328   char *envstr_timeoutfactor = getenv ("TIMEOUTFACTOR");
> 329   if (envstr_timeoutfactor != NULL)
> 330     {
> 331       char *envstr_conv = envstr_timeoutfactor;
> 332       unsigned long int env_fact;
> 333
> 334       env_fact = strtoul (envstr_timeoutfactor, &envstr_conv, 0);
> 335       if (*envstr_conv == '\0' && envstr_conv != envstr_timeoutfactor)
> 336         timeoutfactor = MAX (env_fact, 1);
> 337     }
>

Thank you for the review.  I've posted a v2[1] with the changes you've
suggested as well as handling the strtoul() call in the option parsing
as well.

Thanks,
Joe

[1] https://patchwork.sourceware.org/project/glibc/patch/20240509022714.558030-1-josimmon@redhat.com/
>
> > +
> >    /* Parse and process arguments.  */
> >    argp_parse (&argp, argc, argv, 0, &remaining, NULL);
> >
>
> --
> Cheers,
> Carlos.
>


-- 
Joe Simmons-Talbott