Thread: [HACKERS] PROVE_FLAGS
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 Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
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 Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
On 05/04/2017 12:50 AM, Stephen Frost wrote: > * Tom Lane (tgl@sss.pgh.pa.us) wrote: >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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 Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, May 12, 2017 at 9:05 PM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, May 12, 2017 at 9:05 PM, Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com> 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
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
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 Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, May 16, 2017 at 9:20 PM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
On 17 May 2017 at 14:30, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, May 16, 2017 at 9:20 PM, Andrew Dunstan > <andrew.dunstan@2ndquadrant.com> 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 Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 05/23/2017 06:59 AM, Andrew Dunstan wrote: > On 17 May 2017 at 14:30, Robert Haas <robertmhaas@gmail.com> wrote: >> On Tue, May 16, 2017 at 9:20 PM, Andrew Dunstan >> <andrew.dunstan@2ndquadrant.com> 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 Dunstan https://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
Attachment
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