Warning from libgomp during make check

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Warning from libgomp during make check

Rik-4
All,

Is anyone else seeing this warning during the test for imfinfo.m during
'make check'?

libgomp: Invalid value for environment variable OMP_NUM_THREADS

I can manually reproduce it outside of 'make check' by doing

./run-octave -cli -f
test nproc.cc
test imfinfo

The issue seems to be the test code in nproc.cc which is

%!test
%! c = nproc ("current");
%! unwind_protect
%!   old_val = getenv ("OMP_NUM_THREADS");
%!   new_val = c + 1;
%!   setenv ("OMP_NUM_THREADS", num2str (new_val));
%!   assert (nproc ("overridable"), new_val);
%! unwind_protect_cleanup
%!   if (! isempty (old_val))
%!     setenv ("OMP_NUM_THREADS", old_val);
%!   else
%!     setenv ("OMP_NUM_THREADS", "");
%!   endif
%! end_unwind_protect

At the end of the test, because we don't have unsetenv(), the environment
variable is cleared to the empty string.  According to the Matlab
documentation, "setenv (name)" is equivalent to "setenv (name, "")" which
on a Windows system undefines the variable.  On a UNIX system, however, one
can have a defined environment variable with an empty value.

Perhaps we should deviate from Matlab here and if the two argument form is
used, then set the environment variable to whatever value is offered
including the null string.  But if the number of arguments to the function
is 1 then we call unsetenv with the name of the variable.

Thoughts,
Rik

Reply | Threaded
Open this post in threaded view
|

Re: Warning from libgomp during make check

Mike Miller
On Fri, Sep 26, 2014 at 10:22:41 -0700, Rik wrote:

> All,
>
> Is anyone else seeing this warning during the test for imfinfo.m during
> 'make check'?
>
> libgomp: Invalid value for environment variable OMP_NUM_THREADS
>
> I can manually reproduce it outside of 'make check' by doing
>
> ./run-octave -cli -f
> test nproc.cc
> test imfinfo

I don't know that I saw that during the code sprint when I added the
setenv-to-empty-string, but it makes sense.

> The issue seems to be the test code in nproc.cc which is
>
> %!test
> %! c = nproc ("current");
> %! unwind_protect
> %!   old_val = getenv ("OMP_NUM_THREADS");
> %!   new_val = c + 1;
> %!   setenv ("OMP_NUM_THREADS", num2str (new_val));
> %!   assert (nproc ("overridable"), new_val);
> %! unwind_protect_cleanup
> %!   if (! isempty (old_val))
> %!     setenv ("OMP_NUM_THREADS", old_val);
> %!   else
> %!     setenv ("OMP_NUM_THREADS", "");
> %!   endif
> %! end_unwind_protect
>
> At the end of the test, because we don't have unsetenv(), the environment
> variable is cleared to the empty string.  According to the Matlab
> documentation, "setenv (name)" is equivalent to "setenv (name, "")" which
> on a Windows system undefines the variable.  On a UNIX system, however, one
> can have a defined environment variable with an empty value.
>
> Perhaps we should deviate from Matlab here and if the two argument form is
> used, then set the environment variable to whatever value is offered
> including the null string.  But if the number of arguments to the function
> is 1 then we call unsetenv with the name of the variable.

How about keeping setenv the way it is and adding an unsetenv
built-in? This would follow the existing practice of adding standard C
library functions that make sense to have in Octave for completeness,
even if they are not available in Matlab, and keep setenv
Matlab-compatible.

I also think it might be useful to allow setenv (name, "") as it is
now. There might be reasons to want to set an empty value rather than
delete a variable. When I added that cleanup, I wasn't thinking that
it would delete the variable, but that it would set it to an empty
string (which it does) and that OpenMP wouldn't care (but it does).

--
mike

Reply | Threaded
Open this post in threaded view
|

Re: Warning from libgomp during make check

Rik-4
On 09/26/2014 12:43 PM, Mike Miller wrote:

> On Fri, Sep 26, 2014 at 10:22:41 -0700, Rik wrote:
>> All,
>>
>> Is anyone else seeing this warning during the test for imfinfo.m during
>> 'make check'?
>>
>> libgomp: Invalid value for environment variable OMP_NUM_THREADS
>>
>> I can manually reproduce it outside of 'make check' by doing
>>
>> ./run-octave -cli -f
>> test nproc.cc
>> test imfinfo
> I don't know that I saw that during the code sprint when I added the
> setenv-to-empty-string, but it makes sense.
>
>> The issue seems to be the test code in nproc.cc which is
>>
>> %!test
>> %! c = nproc ("current");
>> %! unwind_protect
>> %!   old_val = getenv ("OMP_NUM_THREADS");
>> %!   new_val = c + 1;
>> %!   setenv ("OMP_NUM_THREADS", num2str (new_val));
>> %!   assert (nproc ("overridable"), new_val);
>> %! unwind_protect_cleanup
>> %!   if (! isempty (old_val))
>> %!     setenv ("OMP_NUM_THREADS", old_val);
>> %!   else
>> %!     setenv ("OMP_NUM_THREADS", "");
>> %!   endif
>> %! end_unwind_protect
>>
>> At the end of the test, because we don't have unsetenv(), the environment
>> variable is cleared to the empty string.  According to the Matlab
>> documentation, "setenv (name)" is equivalent to "setenv (name, "")" which
>> on a Windows system undefines the variable.  On a UNIX system, however, one
>> can have a defined environment variable with an empty value.
>>
>> Perhaps we should deviate from Matlab here and if the two argument form is
>> used, then set the environment variable to whatever value is offered
>> including the null string.  But if the number of arguments to the function
>> is 1 then we call unsetenv with the name of the variable.
> How about keeping setenv the way it is and adding an unsetenv
> built-in? This would follow the existing practice of adding standard C
> library functions that make sense to have in Octave for completeness,
> even if they are not available in Matlab, and keep setenv
> Matlab-compatible.

That probably makes the most sense.  I did a quick hack and put in unsetenv
and it does, indeed, get rid of the error.

>
> I also think it might be useful to allow setenv (name, "") as it is
> now. There might be reasons to want to set an empty value rather than
> delete a variable. When I added that cleanup, I wasn't thinking that
> it would delete the variable, but that it would set it to an empty
> string (which it does) and that OpenMP wouldn't care (but it does).
>

My suggestion would still preserve this.  The two-input form of setenv
would remain exactly the same.  It would only be the one argument form,
setenv (name), which today expands to setenv (name, "") that would change.
I would have it expand to unsetenv (name) instead.

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: Warning from libgomp during make check

John W. Eaton
Administrator
On 09/26/2014 08:52 PM, Rik wrote:
> On 09/26/2014 12:43 PM, Mike Miller wrote:

>> I also think it might be useful to allow setenv (name, "") as it is
>> now. There might be reasons to want to set an empty value rather than
>> delete a variable. When I added that cleanup, I wasn't thinking that
>> it would delete the variable, but that it would set it to an empty
>> string (which it does) and that OpenMP wouldn't care (but it does).

If I understand correctly, setenv (name, "") does remove the variable
from the environment on Windows systems.

> My suggestion would still preserve this.  The two-input form of setenv
> would remain exactly the same.  It would only be the one argument form,
> setenv (name), which today expands to setenv (name, "") that would change.
> I would have it expand to unsetenv (name) instead.

If you do that, you'll make Octave's setenv incompatible with Matlab
since it is documented that setenv (name) is exactly equivlent to setenv
(name, '').

Is the gnulib unsetenv function portable to Windows?

jwe



Reply | Threaded
Open this post in threaded view
|

Re: Warning from libgomp during make check

Rik-4
On 09/30/2014 01:08 PM, John W. Eaton wrote:

> On 09/26/2014 08:52 PM, Rik wrote:
>> On 09/26/2014 12:43 PM, Mike Miller wrote:
>
>>> I also think it might be useful to allow setenv (name, "") as it is
>>> now. There might be reasons to want to set an empty value rather than
>>> delete a variable. When I added that cleanup, I wasn't thinking that
>>> it would delete the variable, but that it would set it to an empty
>>> string (which it does) and that OpenMP wouldn't care (but it does).
>
> If I understand correctly, setenv (name, "") does remove the variable
> from the environment on Windows systems.
>
>> My suggestion would still preserve this.  The two-input form of setenv
>> would remain exactly the same.  It would only be the one argument form,
>> setenv (name), which today expands to setenv (name, "") that would change.
>> I would have it expand to unsetenv (name) instead.
>
> If you do that, you'll make Octave's setenv incompatible with Matlab
> since it is documented that setenv (name) is exactly equivlent to setenv
> (name, '').
>
> Is the gnulib unsetenv function portable to Windows?

I guess we'll see.  I left setenv the same and added unsetenv via gnulib in
this cset (http://hg.savannah.gnu.org/hgweb/octave/rev/6f0290863d50).
Since this function isn't supported by Matlab there isn't a direct
compatibility concern.  I then used the new unsetenv function to take care
of the warning in cset
(http://hg.savannah.gnu.org/hgweb/octave/rev/9163a6e9b096).

--Rik