Re: [HACKERS] PROVE_FLAGS

2017-06-09 Thread Peter Eisentraut 
On 6/5/17 15:06, Andrew Dunstan wrote:
>> In that case we're going to need to invent a way to do this similarly
>> in vcregress.pl. I'm not simply going to revert to the situation where
>> it and the makefiles are completely out of sync on this. The previous
>> patch was made more or less by ignoring the existence of vcregress.pl.
>>
>> I will look at this again probably after pgcon. I don't think it's
>> terribly urgent.
>>
> 
> 
> Here's a patch that should do that.

I'm not able to test this, but it looks like a reasonable approach.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-06-05 Thread Andrew Dunstan 


On 05/23/2017 06:59 AM, Andrew Dunstan wrote:
> On 17 May 2017 at 14:30, Robert Haas  wrote:
>> On Tue, May 16, 2017 at 9:20 PM, Andrew Dunstan
>>  wrote:
>>> Inheriting variables from the environment is a part of make by design.
>>> We have PG_PROVE_FLAGS for our own forced settings.
>> I don't buy this argument.  We've had previous cases where we've gone
>> through and added -X to psql invocations in the regression tests
>> because, if you don't, some people's .psqlrc files may cause tests to
>> fail, which nobody wants.  The more general principle is that the
>> regression tests should ideally pass regardless of the local machine
>> configuration.  It's proven impractical to make that universally true,
>> but that doesn't make it a bad goal.  Now, for all I know it may be
>> that setting PROVE_FLAGS can never cause a regression failure, but I
>> still tend to agree with Peter that the regression tests framework
>> should insulate us from the surrounding environment rather than
>> absorbing settings from it.
>>
> In that case we're going to need to invent a way to do this similarly
> in vcregress.pl. I'm not simply going to revert to the situation where
> it and the makefiles are completely out of sync on this. The previous
> patch was made more or less by ignoring the existence of vcregress.pl.
>
> I will look at this again probably after pgcon. I don't think it's
> terribly urgent.
>


Here's a patch that should do that. If this is acceptable I'll do this
and put the makefiles back the way they were.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 468a62d..e2d225e 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -178,12 +178,18 @@ sub tap_check
 	die "Tap tests not enabled in configuration"
 	  unless $config->{tap_tests};
 
+	my @flags;
+	foreach my $arg (0 .. scalar(@_))
+	{
+		next unless $_[$arg] =~ /^PROVE_FLAGS=(.*)/;
+		@flags = split(/\s+/, $1};
+		splice(@_,$arg,1);
+		last;
+	}
+
 	my $dir = shift;
 	chdir $dir;
 
-	my @flags;
-	@flags = split(/\s+/, $ENV{PROVE_FLAGS}) if exists $ENV{PROVE_FLAGS};
-
 	my @args = ("prove", @flags, "t/*.pl");
 
 	# adjust the environment for just this test

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-23 Thread Andrew Dunstan 
On 17 May 2017 at 14:30, Robert Haas  wrote:
> On Tue, May 16, 2017 at 9:20 PM, Andrew Dunstan
>  wrote:
>> Inheriting variables from the environment is a part of make by design.
>> We have PG_PROVE_FLAGS for our own forced settings.
>
> I don't buy this argument.  We've had previous cases where we've gone
> through and added -X to psql invocations in the regression tests
> because, if you don't, some people's .psqlrc files may cause tests to
> fail, which nobody wants.  The more general principle is that the
> regression tests should ideally pass regardless of the local machine
> configuration.  It's proven impractical to make that universally true,
> but that doesn't make it a bad goal.  Now, for all I know it may be
> that setting PROVE_FLAGS can never cause a regression failure, but I
> still tend to agree with Peter that the regression tests framework
> should insulate us from the surrounding environment rather than
> absorbing settings from it.
>

In that case we're going to need to invent a way to do this similarly
in vcregress.pl. I'm not simply going to revert to the situation where
it and the makefiles are completely out of sync on this. The previous
patch was made more or less by ignoring the existence of vcregress.pl.

I will look at this again probably after pgcon. I don't think it's
terribly urgent.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-17 Thread Robert Haas 
On Tue, May 16, 2017 at 9:20 PM, Andrew Dunstan
 wrote:
> Inheriting variables from the environment is a part of make by design.
> We have PG_PROVE_FLAGS for our own forced settings.

I don't buy this argument.  We've had previous cases where we've gone
through and added -X to psql invocations in the regression tests
because, if you don't, some people's .psqlrc files may cause tests to
fail, which nobody wants.  The more general principle is that the
regression tests should ideally pass regardless of the local machine
configuration.  It's proven impractical to make that universally true,
but that doesn't make it a bad goal.  Now, for all I know it may be
that setting PROVE_FLAGS can never cause a regression failure, but I
still tend to agree with Peter that the regression tests framework
should insulate us from the surrounding environment rather than
absorbing settings from it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-16 Thread Andrew Dunstan 


On 05/16/2017 07:44 PM, Peter Eisentraut wrote:
> On 5/3/17 15:14, Andrew Dunstan wrote:
>> Can someone please explain to me why we have this in Makefile.global.in?
>> (from commit e9c81b60 )
>>
>> PROVE_FLAGS =
>>
>> ISTM it's unnecessary, and prevents us from using the same named value
>> in the environment. I want to be able to use the environment in
>> vcregress.pl, and I'd like the Make files to work the same way.
> I think relying on environment variables like this is a bad design.
> Someone could have something left in the environment and it changes
> things in mysterious ways.  For all other *FLAGS variables, we
> explicitly set them in Makefile.global, sometimes to empty values, as
> the case may be.
>
> Note that specifying a variable on the make command line overrides
> settings in the makefile under certain circumstances, which is a useful
> feature.
>
> So I think the above setting was correct, the new behavior is
> inconsistent and incorrect, and I think this change should be reverted.
>


It would have been nice if you'd spoken up when the topic was raised.

This doesn't rely on the environment variable, it just enables it. You
can still say "make PROVE_FLAGS=--whatever" and it will override the
environment.

Inheriting variables from the environment is a part of make by design.
We have PG_PROVE_FLAGS for our own forced settings.

Note that the previous setting was done without any thought being given
to how this works with vcregress.pl. I have been trying to make the two
systems more consistent. This was a part of that effort.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-16 Thread Peter Eisentraut 
On 5/3/17 15:14, Andrew Dunstan wrote:
> Can someone please explain to me why we have this in Makefile.global.in?
> (from commit e9c81b60 )
> 
> PROVE_FLAGS =
> 
> ISTM it's unnecessary, and prevents us from using the same named value
> in the environment. I want to be able to use the environment in
> vcregress.pl, and I'd like the Make files to work the same way.

I think relying on environment variables like this is a bad design.
Someone could have something left in the environment and it changes
things in mysterious ways.  For all other *FLAGS variables, we
explicitly set them in Makefile.global, sometimes to empty values, as
the case may be.

Note that specifying a variable on the make command line overrides
settings in the makefile under certain circumstances, which is a useful
feature.

So I think the above setting was correct, the new behavior is
inconsistent and incorrect, and I think this change should be reverted.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-12 Thread Tom Lane 
Michael Paquier  writes:
> On Fri, May 12, 2017 at 9:05 PM, Andrew Dunstan
>  wrote:
>> Does anyone object to me backpatching this? It seems to me kinda crazy
>> to have --verbose hardcoded on the back branches and not on the dev branch.

> +1. A maximum of consistency with the test code when possible is nice to have.

No objection here either.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-12 Thread Michael Paquier 
On Fri, May 12, 2017 at 9:05 PM, Andrew Dunstan
 wrote:
> Does anyone object to me backpatching this? It seems to me kinda crazy
> to have --verbose hardcoded on the back branches and not on the dev branch.

+1. A maximum of consistency with the test code when possible is nice to have.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-12 Thread Andrew Dunstan 


On 05/04/2017 12:50 AM, Stephen Frost wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Andrew Dunstan  writes:
>>> Can someone please explain to me why we have this in Makefile.global.in?
>>> (from commit e9c81b60 )
>>> PROVE_FLAGS =
>> Before that commit it was like
>>
>>  PROVE_FLAGS = --verbose
> right.
>
>> which had some value.  I agree that now we'd be better off to not
>> set it at all, especially since the convention now appears to be that
>> automatically-supplied prove options should be set in PG_PROVE_FLAGS.
> Good point.
>
>> I'd suggest that the comment just above be replaced by something like
>>
>> # User-supplied prove flags can be provided in PROVE_FLAGS.
> Works for me.
>


Does anyone object to me backpatching this? It seems to me kinda crazy
to have --verbose hardcoded on the back branches and not on the dev branch.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-03 Thread Stephen Frost 
* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Andrew Dunstan  writes:
> > Can someone please explain to me why we have this in Makefile.global.in?
> > (from commit e9c81b60 )
> > PROVE_FLAGS =
> 
> Before that commit it was like
> 
>   PROVE_FLAGS = --verbose

right.

> which had some value.  I agree that now we'd be better off to not
> set it at all, especially since the convention now appears to be that
> automatically-supplied prove options should be set in PG_PROVE_FLAGS.

Good point.

> I'd suggest that the comment just above be replaced by something like
> 
> # User-supplied prove flags can be provided in PROVE_FLAGS.

Works for me.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PROVE_FLAGS

2017-05-03 Thread Tom Lane 
Andrew Dunstan  writes:
> Can someone please explain to me why we have this in Makefile.global.in?
> (from commit e9c81b60 )
> PROVE_FLAGS =

Before that commit it was like

PROVE_FLAGS = --verbose

which had some value.  I agree that now we'd be better off to not
set it at all, especially since the convention now appears to be that
automatically-supplied prove options should be set in PG_PROVE_FLAGS.

I'd suggest that the comment just above be replaced by something like

# User-supplied prove flags can be provided in PROVE_FLAGS.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-03 Thread Andrew Dunstan 


On 05/03/2017 03:21 PM, Andres Freund wrote:
> On 2017-05-03 15:14:27 -0400, Andrew Dunstan wrote:
>> Can someone please explain to me why we have this in Makefile.global.in?
>> (from commit e9c81b60 )
>>
>>
>> PROVE_FLAGS =
>>
>>
>> ISTM it's unnecessary, and prevents us from using the same named value
>> in the environment. I want to be able to use the environment in
>> vcregress.pl, and I'd like the Make files to work the same way.
> Wouldn't it be better to append the environment to the flags here,
> that'd allow us to modify flags from both places?
>



The Makefile already has:

$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) ...

It doesn't set PROVE_FLAGS anywhere.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PROVE_FLAGS

2017-05-03 Thread Andres Freund 
On 2017-05-03 15:14:27 -0400, Andrew Dunstan wrote:
> 
> Can someone please explain to me why we have this in Makefile.global.in?
> (from commit e9c81b60 )
> 
> 
> PROVE_FLAGS =
> 
> 
> ISTM it's unnecessary, and prevents us from using the same named value
> in the environment. I want to be able to use the environment in
> vcregress.pl, and I'd like the Make files to work the same way.

Wouldn't it be better to append the environment to the flags here,
that'd allow us to modify flags from both places?

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] PROVE_FLAGS

2017-05-03 Thread Andrew Dunstan 

Can someone please explain to me why we have this in Makefile.global.in?
(from commit e9c81b60 )


PROVE_FLAGS =


ISTM it's unnecessary, and prevents us from using the same named value
in the environment. I want to be able to use the environment in
vcregress.pl, and I'd like the Make files to work the same way.


cheers


andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers