Re: [PATCH v2] tests: force non-deterministic mode in non-deterministic tests

[Jan Beulich <jbeulich@suse.com>]

On 20.12.2023 18:40, Stephen Kitt wrote:
> On Wed, 20 Dec 2023 09:03:37 +0100, Jan Beulich <jbeulich@suse.com> wrote:
>> On 19.12.2023 22:53, Stephen Kitt wrote:
>>> Since ar can be built defaulting to deterministic mode, tests which
>>> expect non-deterministic behaviour need to explicitly set the U flag.
>>> They also need to run without SOURCE_DATE_EPOCH since that also
>>> enables deterministic mode.  
>>
>> Not quite - the env var controls only time stamps, not UID/GID or
>> permissions, aiui. That's merely a matter of changing the wording of
>> course.
> 
> Right, I’ll update the wording.
> 
>>> --- a/binutils/testsuite/binutils-all/ar.exp
>>> +++ b/binutils/testsuite/binutils-all/ar.exp
>>> @@ -571,6 +571,8 @@ proc replacing_non_deterministic_member { } {
>>>  	return
>>>      }
>>>  
>>> +    unsetenv SOURCE_DATE_EPOCH
>>> +
>>>      set archive tmpdir/artest.a
>>>      set older_objfile tmpdir/bintest.${obj}
>>>      set newer_objfile tmpdir/ar/bintest.${obj}  
>>
>> I think it would be nice if this was done in the place where,
>> respectively, replacing_sde_deterministic_member has its setenv
>> (and then, like that, if it also had a brief comment). Unless of
>> course there's an issue with moving this down by a few lines.
> 
> Yes, that makes sense.
> 
>> Furthermore I'm afraid I'm less comfortable approving this, as the
>> correctness of the unconditional unsetenv in
>> replacing_sde_deterministic_member isn't really clear to me: If that
>> variable was indeed set in the environment up front, it's not clear
>> to me whether it really is okay to permanently alter the environment.
>> Otoh of course your change merely moves the (possible) problem ahead
>> by a tiny bit, so yes - with the cosmetic adjustments done the change
>> is probably still okay.
> 
> The scenario I’m trying to address is indeed when SOURCE_DATE_EPOCH is set in
> the environment on entry to the tests. This is what happens for example in
> Debian builds, and that’s where I ran into this.
> https://buildd.debian.org/status/fetch.php?pkg=binutils-mingw-w64&arch=amd64&ver=11.1&stamp=1702977480&raw=0
> provides an example.
> 
> I think it’s fine for tests to make sure their environment is appropriate for
> their own tests, but I agree it would be better for them to restore the
> environment as it was on entry.
> 
> The Linaro build bot is complaining about this too; it’s failing because
> unsetenv is called on a variable which isn’t present in the environment. It
> seems I should check for the variable’s presence before unsetting it, or
> perhaps just set it to empty...
> 
> Incidentally, builds with this patch in Debian all pass these tests (with
> SOURCE_DATE_EPOCH set in the environment), but fail on i386 tests on
> big-endian systems; see
> https://buildd.debian.org/status/package.php?p=binutils-mingw-w64 for logs.
> git bisect tells me that the culprit is
> 734dfd1cc966aff736eaeda68bfa4807ee4b50c1 but I haven’t figured out why yet.

That has been there for a while. I would have looked at the build logs, but
the link above points only at their tails, and I can't spot how to obtain
them through just the browser (needing to obtain any debian specific software
for this purpose is a no-go imo).

Jan