Re: Warning from libgomp during make check

[Rik]

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