Re: make installcheck-world in a clean environment - Mailing list pgsql-hackers

Alexander Lakhin <exclusion@gmail.com> writes:
> 12.09.2018 10:20, Michael Paquier wrote:
>> On Mon, May 07, 2018 at 01:07:15PM -0400, Tom Lane wrote:
>>> Nah, I disagree with this.  To me, the purpose of "make installcheck"
>>> is to verify the correctness of an installation, which I take to include
>>> the client programs as well as the server.  I think that "make
>>> installcheck" ought to use the installed version of any file that we
>>> actually install, and go to the build tree only for things we don't
>>> install (e.g. SQL test scripts).

>> I agree with Tom's position, and this is the behavior that Postgres is
>> using for ages for check and installcheck.  If there are no objections,
>> I'll mark the patch as rejected and move on to other things.

> It seems, that you miss a major part of the discussion (we have
> discussed the issue from other positions later).

I think the main reason this patch isn't moving forward is that it's not
clear to most people what you're trying to fix.  And I'd lay a big part of
the blame for that on the fact that the patch includes no documentation
changes at all, not even additional Makefile comments.  Perhaps the SGML
text about how to use "make installcheck" needs to be expanded to clarify
what we expect to be already installed.

The patch itself contains some pretty dubious/confusing things too.
For instance, the first hunk:

@@ -244,7 +244,13 @@ CPPFLAGS = @CPPFLAGS@

 override CPPFLAGS := $(ICU_CFLAGS) $(CPPFLAGS)

+ifdef USE_INSTALLED_ASSETS
+USE_INCLUDEDIR = 1
+endif
 ifdef PGXS
+USE_INCLUDEDIR = 1
+endif
+ifdef USE_INCLUDEDIR
 override CPPFLAGS := -I$(includedir_server) -I$(includedir_internal) $(CPPFLAGS)
 else # not PGXS
 override CPPFLAGS := -I$(top_srcdir)/src/include $(CPPFLAGS)

immediately leads the reader to wonder where else USE_INCLUDEDIR is used,
and the answer is nowhere, so why'd you define it?  I think it'd be better
to replace the first seven lines with the usual locution for OR:

ifneq (,$(PGXS)$(USE_INSTALLED_ASSETS))

and likewise further down for USE_LIBDIR.  I also note you didn't fix the
comment you falsified about "# not PGXS".

It seems wrong to have changed src/interfaces/ecpg/Makefile the way you
did.  Surely it is the responsibility of src/interfaces/ecpg/test/Makefile
to handle a "make installcheck" request correctly, whether it is issued
from the parent directory or locally.  (Personally I do "make
installcheck" in the test/ subdirectory quite often, so I'd be unhappy
if it doesn't work right when started from there.)

The change in pgxs.mk doesn't make a lot of sense to me either; even if it's
not actually syntactically wrong, what's the point given what you did in
Makefile.global?

I do not believe that the changes in either plpgsql/src or test/isolation
are appropriate.  If the former is needed then it would at least imply
that plperl, plpython, and pltcl are all broken too, and probably also
that all the contrib makefiles are broken, and I don't believe any of
that.  As for test/isolation, there is nothing that it installs, so why
would it need a change in behavior?

Nor do I understand the need for changes in test/regress.  I'm prepared to
believe that ECPG might need some work, because it's got a complicated
situation and few people pay any attention to it anyway.  But all this
other stuff works perfectly fine under "make installcheck" today, and
has done for years, and you have not explained why changing it would
be an improvement.

            regards, tom lane