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

Hello Tom,

Thank you for the very detailed review.
Please look at the improved version of the patch (and the `make
installcheck` log with it).

16.11.2018 04:19, Tom Lane writes:
>> 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.
Now I've tried to describe in regress.sgml the behavior, which I want to
implement.
I'm not sure that we should name all the assets, that we expect to be
installed, because it can change.
> 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".
Thanks for the notice. I fixed it.
> 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.)
Thanks again, I've moved the change into ecpg/test.
> 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've modified the patch to use the installed version of pg_regress. It
simplifies a lot. The main idea of the change is to not build pg_regress.
> 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?
Regarding test/isolation, we need to change the behavior to build
pg_isolation_regress and isolationtester only. We don't need to build
all the libraries needed as they are already installed in $(DESTDIR)/lib/.
By the way, I wonder why "pg_isolation_regress" and "isolationtester"
binaries are not installed as "pg_regress" is.

As to plpgsql and other PLs, yes, it was my omission. We don't need to
build pg_regress when we use $(pg_regress_installcheck) for any of these,
In contrib/ I've found only one usage of pg_regress_installcheck
(contrib/test_decoding). Fixed it too. We don't need to build the
test-decoding lib as it's installed too.

Also there is an issue with src/test/regress/. We install refint.so and
autoinc.so into $(DESTDIR)/lib/, but don't install regress.so. It makes
impossible to pass "--dlpath='$(DESTDIR)$(libdir)/'" to pg_regress. So
for now I leave dlpath unchanged and it requires building of all the .so's.
> 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.
The main purpose of the change is to make it possible to perform `make
installcheck` for the installation produced by the binary packages
(pgdg, for example).
The scenario is simple. If I've installed binary packages, how can I
check that the installation is working as expected? (Assuming that I
have corresponding source tarball.)
Currently, I can remove bin/ecpg, lib/libgport, lib/pgxs/, include/ from
the installation directory and `make installcheck-world` will succeed.
But it will not, if I remove bin/psql or lib/adminpack.so.
So the current behavior is at least inconsistent but moreover it's
unsuitable for the user-centric testing of installation. I do understand
that the developer-centric approach was working for years, but may be it
can (or should) be changed someday?

Best regards,
Alexander