Date: Wed, 16 Nov 2022 22:18:05 -0800
From: Noah Misch <noah@leadboat.com>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: Michael Paquier <michael.paquier@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: Supporting TAP tests with MSVC and Windows
Message-ID: <20221117061805.GA4020280@rfd.leadboat.com>
References: 
 <CAB7nPqRvsJ7WvgNDQk+yFZGNUbAbMi-LRCwgcWaAcfV3R7V2nw@mail.gmail.com>
 <CAB7nPqSmCHA726iMtVuZZ4t0Rd4jkqZhDf52dTv4138iDXy7aQ@mail.gmail.com>
 <55B2758E.1010205@iki.fi>
 <20150725071408.GA1442444@tornado.leadboat.com>
 <CAB7nPqRYn4_AA270BJ8VXryGV_9mzeLMTvdC2LhXJk0CxJx2+A@mail.gmail.com>
 <CAB7nPqR4YxKOSW3zUNLwEexNqOLLFtCVWb0cTF1VKx6q53=pAA@mail.gmail.com>
 <55B90161.5090506@iki.fi>
 <CAB7nPqRSKMCKP-0C_Z1HPCWU_06XDVTQNNgnZAmb4XLryra-cg@mail.gmail.com>
 <20220822004052.GA646896@rfd.leadboat.com>
 <2856ec20-157c-8119-4f4d-fc15e23efe20@dunslane.net>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="IJpNTDwzlM2Ie8A6"
Content-Disposition: inline
In-Reply-To: <2856ec20-157c-8119-4f4d-fc15e23efe20@dunslane.net>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20221117061805.GA4020280%40rfd.leadboat.com>
Precedence: bulk


--IJpNTDwzlM2Ie8A6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Aug 22, 2022 at 09:44:42AM -0400, Andrew Dunstan wrote:
> On 2022-08-21 Su 20:40, Noah Misch wrote:
> > This (commit 13d856e of 2015-07-29) added the following:
> >
> > --- a/src/test/perl/TestLib.pm
> > +++ b/src/test/perl/TestLib.pm
> > @@ -242,7 +288,17 @@ sub command_exit_is
> >  	print("# Running: " . join(" ", @{$cmd}) ."\n");
> >  	my $h = start $cmd;
> >  	$h->finish();
> > -	is($h->result(0), $expected, $test_name);
> > +
> > +	# On Windows, the exit status of the process is returned directly as the
> > +	# process's exit code, while on Unix, it's returned in the high bits
> > +	# of the exit code (see WEXITSTATUS macro in the standard <sys/wait.h>
> > +	# header file). IPC::Run's result function always returns exit code >> 8,
> > +	# assuming the Unix convention, which will always return 0 on Windows as
> > +	# long as the process was not terminated by an exception. To work around
> > +	# that, use $h->full_result on Windows instead.
> > +	my $result = ($Config{osname} eq "MSWin32") ?
> > +		($h->full_results)[0] : $h->result(0);
> > +	is($result, $expected, $test_name);
> >  }
> >
> > That behavior came up again in the context of a newer IPC::Run test case.  I'm
> > considering changing the IPC::Run behavior such that the above would have been
> > unnecessary.  However, if I do, the above code would want to adapt to handle
> > pre-change and post-change IPC::Run versions.  If you have an opinion on
> > whether or how IPC::Run should change, I welcome comments on
> > https://github.com/toddr/IPC-Run/issues/161.
> 
> Assuming it changes, we'll have to have a version test here. I don't
> think we can have a flag day where we suddenly require IPC::Run's
> bleeding edge on Windows. So changing it is a good thing, but it won't
> help us much.

IPC::Run git now has the change, and we may release soon.  Here's what I plan
to push to make PostgreSQL cope.

--IJpNTDwzlM2Ie8A6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="ipc-run-result-w32-v1.patch"

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    Account for IPC::Run::result() Windows behavior change.
    
    This restores compatibility with the not-yet-released successor of
    version 20220807.0.  Back-patch to 9.4, which introduced this code.
    
    Reviewed by FIXME.
    
    Discussion: https://postgr.es/m/FIXME

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 99d3345..b139190 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -782,15 +782,11 @@ sub command_exit_is
 	my $h = IPC::Run::start $cmd;
 	$h->finish();
 
-	# On Windows, the exit status of the process is returned directly as the
-	# process's exit code, while on Unix, it's returned in the high bits
-	# of the exit code (see WEXITSTATUS macro in the standard <sys/wait.h>
-	# header file). IPC::Run's result function always returns exit code >> 8,
-	# assuming the Unix convention, which will always return 0 on Windows as
-	# long as the process was not terminated by an exception. To work around
-	# that, use $h->full_results on Windows instead.
+	# Normally, if the child called exit(N), IPC::Run::result() returns N.  On
+	# Windows, with IPC::Run v20220807.0 and earlier, full_results() is the
+	# method that returns N (https://github.com/toddr/IPC-Run/issues/161).
 	my $result =
-	    ($Config{osname} eq "MSWin32")
+	  ($Config{osname} eq "MSWin32" && $IPC::Run::VERSION <= 20220807.0)
 	  ? ($h->full_results)[0]
 	  : $h->result(0);
 	is($result, $expected, $test_name);

--IJpNTDwzlM2Ie8A6--