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

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

[Stephen Kitt]

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.

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 binutils/testsuite/binutils-all/ar.exp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/binutils/testsuite/binutils-all/ar.exp b/binutils/testsuite/binutils-all/ar.exp
index aade419344e..3ac3c424447 100644
--- 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}
@@ -581,7 +583,7 @@ proc replacing_non_deterministic_member { } {
 
     # Build the archive with the *newer* object file.
     
-    set got [binutils_run $AR "rc $archive ${newer_objfile}"]
+    set got [binutils_run $AR "rcU $archive ${newer_objfile}"]
     if ![string match "" $got] {
 	fail "$testname: (could not build archive)"
 	return
@@ -589,7 +591,7 @@ proc replacing_non_deterministic_member { } {
 
     # Now try to replace the newer file with the older one.  This should not work.
     
-    set got [binutils_run $AR "ru $archive $older_objfile"]
+    set got [binutils_run $AR "ruU $archive $older_objfile"]
     if ![string match "" $got] {
 	fail "$testname: (failed to replace file)"
 	return
@@ -651,7 +653,7 @@ proc replacing_sde_deterministic_member { } {
     # Build the archive with the *newer* object file.
     setenv SOURCE_DATE_EPOCH "1000"
     
-    set got [binutils_run $AR "rc $archive ${newer_objfile}"]
+    set got [binutils_run $AR "rcU $archive ${newer_objfile}"]
     if ![string match "" $got] {
 	fail "$testname: (could not build archive)"
 	unsetenv SOURCE_DATE_EPOCH
@@ -662,7 +664,7 @@ proc replacing_sde_deterministic_member { } {
     # archive this will not work, but one created to be deterministic
     # should always replace its members.
     
-    set got [binutils_run $AR "ru $archive $older_objfile"]
+    set got [binutils_run $AR "ruU $archive $older_objfile"]
     if ![string match "" $got] {
 	fail "$testname: (failed to replace file)"
 	unsetenv SOURCE_DATE_EPOCH

base-commit: c4fb39bb31a53bbb2df3be3200d694f025c5b892
-- 
2.30.2

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

[Jan Beulich]

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.

> --- 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.

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.

Jan

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

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]

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.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jan Beulich]

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

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

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

On Thu, 21 Dec 2023 08:22:43 +0100, Jan Beulich <jbeulich@suse.com> wrote:
> On 20.12.2023 18:40, Stephen Kitt wrote:
> > 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).

Of course! It’s not obvious, you can get the full logs by following the
“Build-Attempted” links. For example,
https://buildd.debian.org/status/fetch.php?pkg=binutils-mingw-w64&arch=s390x&ver=11.2&stamp=1703021196&raw=0
on s390x.

To reproduce this, on any BE system (s390x, sparc64 etc.), on current trunk:

./configure --target=i686-w64-mingw32 && make && make -C gas check

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jan Beulich]

On 21.12.2023 09:13, Stephen Kitt wrote:
> On Thu, 21 Dec 2023 08:22:43 +0100, Jan Beulich <jbeulich@suse.com> wrote:
>> On 20.12.2023 18:40, Stephen Kitt wrote:
>>> 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).
> 
> Of course! It’s not obvious, you can get the full logs by following the
> “Build-Attempted” links. For example,
> https://buildd.debian.org/status/fetch.php?pkg=binutils-mingw-w64&arch=s390x&ver=11.2&stamp=1703021196&raw=0
> on s390x.

Ah, I see - it's really not the build, but the checking that fails.

> To reproduce this, on any BE system (s390x, sparc64 etc.), on current trunk:
> 
> ./configure --target=i686-w64-mingw32 && make && make -C gas check

Provided one has access to a BE system. Anyway, it'll be the transformation
cpu_flags_from_attr() does which needs further tweaking for being BE-
compatible. Just that right now I can't see yet what exactly needs changing
there in which way.

Jan

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

[Jan Beulich]

On 21.12.2023 09:13, Stephen Kitt wrote:
> On Thu, 21 Dec 2023 08:22:43 +0100, Jan Beulich <jbeulich@suse.com> wrote:
>> On 20.12.2023 18:40, Stephen Kitt wrote:
>>> 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).
> 
> Of course! It’s not obvious, you can get the full logs by following the
> “Build-Attempted” links. For example,
> https://buildd.debian.org/status/fetch.php?pkg=binutils-mingw-w64&arch=s390x&ver=11.2&stamp=1703021196&raw=0
> on s390x.
> 
> To reproduce this, on any BE system (s390x, sparc64 etc.), on current trunk:
> 
> ./configure --target=i686-w64-mingw32 && make && make -C gas check

As said, I don't have any suitable system for doing so. I've tried to
deal with it nevertheless, and while doing so I found further issues
with that earlier change of mine. Would it be possible for you to try
https://sourceware.org/pipermail/binutils/2023-December/131393.html
there?

Jan

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

[Stephen Kitt]

Le 22/12/2023 12:29, Jan Beulich a écrit :
> On 21.12.2023 09:13, Stephen Kitt wrote:
>> On Thu, 21 Dec 2023 08:22:43 +0100, Jan Beulich <jbeulich@suse.com> 
>> wrote:
>>> On 20.12.2023 18:40, Stephen Kitt wrote:
>>>> 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).
>> 
>> Of course! It’s not obvious, you can get the full logs by following 
>> the
>> “Build-Attempted” links. For example,
>> https://buildd.debian.org/status/fetch.php?pkg=binutils-mingw-w64&arch=s390x&ver=11.2&stamp=1703021196&raw=0
>> on s390x.
>> 
>> To reproduce this, on any BE system (s390x, sparc64 etc.), on current 
>> trunk:
>> 
>> ./configure --target=i686-w64-mingw32 && make && make -C gas check
> 
> As said, I don't have any suitable system for doing so. I've tried to
> deal with it nevertheless, and while doing so I found further issues
> with that earlier change of mine. Would it be possible for you to try
> https://sourceware.org/pipermail/binutils/2023-December/131393.html
> there?

Thanks, the tests pass with that patch.

Regards,

Stephen