Thread: set TESTDIR from perl rather than Makefile
Forking: <E6CA9D45-96EA-4037-8BEB-344CCA75A38C@anarazel.de> On Tue, Feb 15, 2022 at 10:42:09PM -0800, Andres Freund wrote: > >> I was thinking that we should make Utils.pm's INIT block responsible for > >> figuring out both the directory a test should run in and the log location, > >> instead having that in vcregress.pl and Makefile.global.in. Mostly because > >> doing it in the latter means we can't start tests with different TESTDIR and > >> working dir at the same time. > >> > >> If instead we pass the location of the top-level build and top-level source > >> directory from vcregress.pl / Makefile.global, the tap test infrastructure can > >> figure out that stuff themselves, on a per-test basis. > >> > >> For msvc builds we probably would need to pass in some information that allow > >> Utils.pm to set up PATH appropriately. I think that might just require knowing > >> that a) msvc build system is used b) Release vs Debug. > > > >I'm totally unsure if this resembles what you're thinking of, and I'm surprised > >I got it working so easily. But it gets the tap test output in separate dirs, > >and CI is passing for everyone (windows failed because I injected a "false" to > >force it to upload artifacts). > > > >https://github.com/justinpryzby/postgres/runs/5211673291 > > Yes, that's along the lines I was thinking. I only checked it on my phone, so it certainly isn't a careful look... > > I think this should be discussed in a separate thread, for visibility. I rebased and fixed the check-guc script to work, made it work with vpath builds, and cleaned it up some. There may be other reasons to do this, but the reason I did it is to implement an alltaptests target for vcregress, for cirrus (and everyone else). If all the tap tests are run serially, it takes ~16min on cirrus; it takes ~13 to run in parallel (the below run is slower than that since all the slow tests were scheduled at once - which isn't always a good idea). Running the tests in parallel uses a single invocation of prove, and starts all the TAP tests from the same dir rather than their own dir. But without this patch, all the tap test are output to the same dir, which is a pain to look through. This (and other) patches ran here. https://github.com/justinpryzby/postgres/runs/5261323874 ... e806bcb280 wip: set TESTDIR from src/test/perl rather than Makefile/vcregress a1bfa8e1a6 cirrus/windows: increase timout to 20min 5479b44198 vcregress: add alltaptests fcef696c7d vcregress: run alltaptests in parallel 2d7dba13dd cirrus/windows: prove --state to run tests in order 7edf835d43 tmp: run tap tests first 6fb010c137 cirrus: include hints how to install OS packages.. 28a25f12c3 cirrus/windows: add compiler_warnings_script 75bc8cff69 cirrus: upload changed html docs as artifacts 8008af2480 s!build docs as a separate task.. a3bf699a0e vcregress/ci: test modules/contrib with NO_INSTALLCHECK=1 d28ce46c2f wip: cirrus: code coverage -- Justin
On Sat, Feb 19, 2022 at 05:41:49PM -0600, Justin Pryzby wrote: > I rebased and fixed the check-guc script to work, made it work with vpath > builds, and cleaned it up some. I also meant to also attach it. > This (and other) patches ran here. > https://github.com/justinpryzby/postgres/runs/5261323874 > ... > e806bcb280 wip: set TESTDIR from src/test/perl rather than Makefile/vcregress
Attachment
Hi,
On 2022-02-19 17:53:09 -0600, Justin Pryzby wrote:
> I also meant to also attach it.
Is the patch actually independent of the other patches in your stack?
I like this concept a lot:
- I've had to use a wrapper around individual tap tests for meson, just to set
the CWD etc.
- Being able to run all tap tests at once, instead of many separate prove
invocations is a lot more readable. And can be faster.
- makes it easier to invoke tap tests "manually", which can be useful when
debugging problems (not fun to run make in valgrind or rr)
- I'd like to put test data and test log files in different places than they
are eventually. This seems like it gets us a tiny bit closer to that.
> - $expected = slurp_file_eval("traces/$testname.trace");
> + my $inputdir = "$ENV{'TESTDIR'}/tmp_check";
> + $expected = slurp_file_eval("$inputdir/traces/$testname.trace");
Why is this needed? Shouldn't we end up in exactly the same dir with/without
this patch?
> diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
> index 31e2b0315e..8a8d95ca8c 100644
> --- a/src/test/perl/PostgreSQL/Test/Utils.pm
> +++ b/src/test/perl/PostgreSQL/Test/Utils.pm
> @@ -184,19 +184,21 @@ INIT
> # test may still fail, but it's more likely to report useful facts.
> $SIG{PIPE} = 'IGNORE';
>
> - # Determine output directories, and create them. The base path is the
> - # TESTDIR environment variable, which is normally set by the invoking
> - # Makefile.
> - $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}/tmp_check" : "tmp_check";
> + my $test_dir = File::Spec->rel2abs(dirname($0));
> + my $test_name = basename($0);
> + $test_name =~ s/\.[^.]+$//;
> +
> + # Determine output directories, and create them.
> + # TODO: set srcdir?
> + $tmp_check = "$test_dir/tmp_check";
> $log_path = "$tmp_check/log";
> + $ENV{TESTDIR} = $test_dir;
>
> mkdir $tmp_check;
> mkdir $log_path;
>
> # Open the test log file, whose name depends on the test name.
> - $test_logfile = basename($0);
> - $test_logfile =~ s/\.[^.]+$//;
> - $test_logfile = "$log_path/regress_log_$test_logfile";
> + $test_logfile = "$log_path/regress_log_$test_name";
> open my $testlog, '>', $test_logfile
> or die "could not open STDOUT to logfile \"$test_logfile\": $!";
>
> diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
> index e2b0db0879..63085506e0 100644
> --- a/src/tools/msvc/vcregress.pl
> +++ b/src/tools/msvc/vcregress.pl
> @@ -261,10 +261,8 @@ sub tap_check
> $ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
> $ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
>
> - $ENV{TESTDIR} = "$dir";
> my $module = basename $dir;
> - # add the module build dir as the second element in the PATH
> - $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
> + #$ENV{VCREGRESS_MODE} = $Config;
Hm. How does the module build dir end up on PATH after this?
Greetings,
Andres Freund
On 2/19/22 18:53, Justin Pryzby wrote:
> On Sat, Feb 19, 2022 at 05:41:49PM -0600, Justin Pryzby wrote:
>> I rebased and fixed the check-guc script to work, made it work with vpath
>> builds, and cleaned it up some.
> I also meant to also attach it.
This is going to break a bunch of stuff as written.
First, it's not doing the same thing. The current system sets TESTDIR to
be the parent of the directory that holds the test. e.g. for
src/bin/pg_ctl/t/001_start_stop.pl it's src/bin/pg_ctl in the build
tree, not the 't' subdirectory. This patch apparently sets it to the 't'
subdirectory. That will break anything that goes looking for log files
in the current location, like the buildfarm client, and possibly some CI
setups as well.
Also, unless I'm mistaken it appears to to the wrong thing for vpath
builds:
my $test_dir = File::Spec->rel2abs(dirname($0));
is completely wrong for vpaths, since that will place it in the source
tree, not the build tree.
Last, and for no explained reason that I can see, the patch undoes
commit f4ce6c4d3a, but only for msvc builds. Even if that's justified it
appears to have no relevance to this patch.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Mon, Feb 21, 2022 at 07:00:54AM -0500, Andrew Dunstan wrote:
> On 2/19/22 18:53, Justin Pryzby wrote:
> > On Sat, Feb 19, 2022 at 05:41:49PM -0600, Justin Pryzby wrote:
> >> I rebased and fixed the check-guc script to work, made it work with vpath
> >> builds, and cleaned it up some.
> > I also meant to also attach it.
>
> This is going to break a bunch of stuff as written.
>
> First, it's not doing the same thing. The current system sets TESTDIR to
> be the parent of the directory that holds the test. e.g. for
> src/bin/pg_ctl/t/001_start_stop.pl it's src/bin/pg_ctl in the build
> tree, not the 't' subdirectory. This patch apparently sets it to the 't'
> subdirectory. That will break anything that goes looking for log files
> in the current location, like the buildfarm client, and possibly some CI
> setups as well.
Yes, thanks.
I changed the patch to use ENV{CURDIR} || dirname(dirname($0)). If I'm not
wrong, that seems to be doing the right thing.
> Also, unless I'm mistaken it appears to to the wrong thing for vpath
> builds:
>
> my $test_dir = File::Spec->rel2abs(dirname($0));
>
> is completely wrong for vpaths, since that will place it in the source
> tree, not the build tree.
>
> Last, and for no explained reason that I can see, the patch undoes
> commit f4ce6c4d3a, but only for msvc builds. Even if that's justified it
> appears to have no relevance to this patch.
Andres' idea is that perl should set TESTDIR and PATH. Here I commented out
PATH, and had the improbable issue that nothing seemed to be breaking,
including the pipeline test under msvc. It'd be helpful to know what
configuration that breaks so I can test that it's broken and then test that
it's fixed when set from within perl...
I got busy here, and may not be able to progress this for awhile.
Attachment
On 2/24/22 20:17, Justin Pryzby wrote:
> On Mon, Feb 21, 2022 at 07:00:54AM -0500, Andrew Dunstan wrote:
>> On 2/19/22 18:53, Justin Pryzby wrote:
>>> On Sat, Feb 19, 2022 at 05:41:49PM -0600, Justin Pryzby wrote:
>>>> I rebased and fixed the check-guc script to work, made it work with vpath
>>>> builds, and cleaned it up some.
>>> I also meant to also attach it.
>> This is going to break a bunch of stuff as written.
>>
>> First, it's not doing the same thing. The current system sets TESTDIR to
>> be the parent of the directory that holds the test. e.g. for
>> src/bin/pg_ctl/t/001_start_stop.pl it's src/bin/pg_ctl in the build
>> tree, not the 't' subdirectory. This patch apparently sets it to the 't'
>> subdirectory. That will break anything that goes looking for log files
>> in the current location, like the buildfarm client, and possibly some CI
>> setups as well.
> Yes, thanks.
>
> I changed the patch to use ENV{CURDIR} || dirname(dirname($0)). If I'm not
> wrong, that seems to be doing the right thing.
>
>> Also, unless I'm mistaken it appears to to the wrong thing for vpath
>> builds:
>>
>> my $test_dir = File::Spec->rel2abs(dirname($0));
>>
>> is completely wrong for vpaths, since that will place it in the source
>> tree, not the build tree.
>>
>> Last, and for no explained reason that I can see, the patch undoes
>> commit f4ce6c4d3a, but only for msvc builds. Even if that's justified it
>> appears to have no relevance to this patch.
> Andres' idea is that perl should set TESTDIR and PATH. Here I commented out
> PATH, and had the improbable issue that nothing seemed to be breaking,
> including the pipeline test under msvc. It'd be helpful to know what
> configuration that breaks so I can test that it's broken and then test that
> it's fixed when set from within perl...
I'm fairly sure what's broken here is the MSVC install procedure, which
is massively overeager. We should fix that rather than take it for granted.
>
> I got busy here, and may not be able to progress this for awhile.
You have fixed the vpath issue. But the changes in vcregress.pl look
wrong to me.
- $ENV{TESTDIR} = "$dir";
If we set PG_SUBDIR in the Makefile shouldn't we set it here too? Yes it
probably doesn't matter as our MSVC build system doesn't support vpath
builds, but it's good to maintain as much equivalence between the two as
possible. (Supporting vpath builds for MSVC might be a nice
student/intern project)
my $module = basename $dir;
- # add the module build dir as the second element in the PATH
- $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
See above comment re msvc install. If we're not reverting f4ce6c4d3a in
the Makefile we shouldn't be reverting here either.
+ # $ENV{VCREGRESS_MODE} = $Config;
Why is this commented out line here?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Sun, Feb 20, 2022 at 04:39:08PM -0800, Andres Freund wrote:
> On 2022-02-19 17:53:09 -0600, Justin Pryzby wrote:
> > I also meant to also attach it.
>
> Is the patch actually independent of the other patches in your stack?
Yes - I rearranged it that way for this thread.
However, it's best served/combined with the alltaptests patch :)
> > - $expected = slurp_file_eval("traces/$testname.trace");
> > + my $inputdir = "$ENV{'TESTDIR'}/tmp_check";
> > + $expected = slurp_file_eval("$inputdir/traces/$testname.trace");
>
> Why is this needed? Shouldn't we end up in exactly the same dir with/without
> this patch?
Right - I'd incorrectly set test_dir to t/ rather than the parent dir.
> > +++ b/src/tools/msvc/vcregress.pl
> > @@ -261,10 +261,8 @@ sub tap_check
> > $ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
> > $ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
> >
> > - $ENV{TESTDIR} = "$dir";
> > my $module = basename $dir;
> > - # add the module build dir as the second element in the PATH
> > - $ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
> > + #$ENV{VCREGRESS_MODE} = $Config;
>
> Hm. How does the module build dir end up on PATH after this?
That patch only dealt with TESTDIR. PATH was still set by makefiles.
For MSVC, it was being set to top_builddir/tmp_install.
I've added a 2nd patch to also set PATH. Maybe it should also set
PATH=$bindir:$PATH - I'm not sure.
https://github.com/justinpryzby/postgres/runs/5340884168
Attachment
- 0001-wip-set-TESTDIR-from-src-test-perl-rather-than-Makef.patch
- 0002-s-also-remove-PATH.patch
- 0003-cirrus-windows-increase-timeout-to-20min.patch
- 0004-vcregress-add-alltaptests.patch
- 0005-cirrus-include-hints-how-to-install-OS-packages.patch
- 0006-vcregress-run-alltaptests-in-parallel.patch
- 0007-tmp-run-tap-tests-first.patch