Remove distprep

Hi,

On 2023-06-09 11:10:14 +0200, Peter Eisentraut wrote:

Per discussion at the unconference[0], I started to write a patch that
removes the make distprep target. A more detailed explanation of the
rationale is also in the patch.

Thanks for tackling this!

It needs some polishing around the edges, but I wanted to put it out here to
get things moving and avoid duplicate work.

It'd be nice if we could get this in soon, so we could move ahead with the
src/tools/msvc removal.

One thing in particular that isn't clear right now is how "make world"
should behave if the documentation tools are not found. Maybe we should
make a build option, like "--with-docs", to mirror the meson behavior.

Isn't that somewhat unrelated to distprep? I see that you removed missing,
but I don't really understand why as part of this commit?

-# If there are any files in the source directory that we also generate in the
-# build directory, they might get preferred over the newly generated files,
-# e.g. because of a #include "file", which always will search in the current
-# directory first.
-message('checking for file conflicts between source and build directory')

You're thinking this can be removed because distclean is now reliable? There
were some pretty annoying to debug issues early on, where people switched from
an in-tree autoconf build to meson, with some files left over from the source
build, causing problems at a *later* time (when the files should have changed,
but the wrong ones were picked up). That's not really related distprep etc,
so I'd change this separately, if we want to change it.

diff --git a/src/tools/pginclude/cpluspluscheck b/src/tools/pginclude/cpluspluscheck
index 4e09c4686b..287395887c 100755
--- a/src/tools/pginclude/cpluspluscheck
+++ b/src/tools/pginclude/cpluspluscheck
@@ -134,6 +134,9 @@ do
test "$f" = src/interfaces/ecpg/preproc/preproc.h && continue
test "$f" = src/test/isolation/specparse.h && continue

+	# FIXME
+	test "$f" = src/backend/utils/adt/jsonpath_internal.h && continue
+
# ppport.h is not under our control, so we can't make it standalone.
test "$f" = src/pl/plperl/ppport.h && continue

Hm, what's that about?

We really ought to replace these scripts with something better, which
understands concurrency...

Greetings,

Andres Freund

On 13.07.23 01:19, Andres Freund wrote:

One thing in particular that isn't clear right now is how "make world"
should behave if the documentation tools are not found. Maybe we should
make a build option, like "--with-docs", to mirror the meson behavior.

Isn't that somewhat unrelated to distprep? I see that you removed missing,
but I don't really understand why as part of this commit?

Ok, I put the docs stuff back the way it was and put "missing" back in.

-# If there are any files in the source directory that we also generate in the
-# build directory, they might get preferred over the newly generated files,
-# e.g. because of a #include "file", which always will search in the current
-# directory first.
-message('checking for file conflicts between source and build directory')

You're thinking this can be removed because distclean is now reliable? There
were some pretty annoying to debug issues early on, where people switched from
an in-tree autoconf build to meson, with some files left over from the source
build, causing problems at a *later* time (when the files should have changed,
but the wrong ones were picked up). That's not really related distprep etc,
so I'd change this separately, if we want to change it.

Ok, I kept it in.

diff --git a/src/tools/pginclude/cpluspluscheck b/src/tools/pginclude/cpluspluscheck
index 4e09c4686b..287395887c 100755
--- a/src/tools/pginclude/cpluspluscheck
+++ b/src/tools/pginclude/cpluspluscheck
@@ -134,6 +134,9 @@ do
test "$f" = src/interfaces/ecpg/preproc/preproc.h && continue
test "$f" = src/test/isolation/specparse.h && continue

+	# FIXME
+	test "$f" = src/backend/utils/adt/jsonpath_internal.h && continue
+
# ppport.h is not under our control, so we can't make it standalone.
test "$f" = src/pl/plperl/ppport.h && continue

Hm, what's that about?

Don't remember ... ;-) I removed this.

Attached is a new version with the above changes, also updated for the
recently added generate-wait_event_types.pl, and I have adjusted all the
header file linking to use relative paths consistently. This addresses
all issues known to me.

On 2023-Jul-14, Peter Eisentraut wrote:

diff --git a/src/backend/parser/Makefile b/src/backend/parser/Makefile
index 9f1c4022bb..3d33b082f2 100644
--- a/src/backend/parser/Makefile
+++ b/src/backend/parser/Makefile
@@ -64,8 +64,8 @@ scan.c: FLEX_FIX_WARNING=yes
# Force these dependencies to be known even without dependency info built:
gram.o scan.o parser.o: gram.h

-
-# gram.c, gram.h, and scan.c are in the distribution tarball, so they
-# are not cleaned here.
-clean distclean maintainer-clean:
+clean:
+	rm -f parser/gram.c \
+	      parser/gram.h \
+	      parser/scan.c
rm -f lex.backup

Hmm, this hunk and the equivalents in src/backend/bootstrap and
src/backend/replication are wrong: you moved the rule from the parent
directory's makefile to the directory where the files reside, but didn't
remove the directory name from the command arguments, so the files
aren't actually deleted.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos" (A. Schopenhauer)

On 14.07.23 09:54, Peter Eisentraut wrote:

diff --git a/src/tools/pginclude/cpluspluscheck 
b/src/tools/pginclude/cpluspluscheck
index 4e09c4686b..287395887c 100755
--- a/src/tools/pginclude/cpluspluscheck
+++ b/src/tools/pginclude/cpluspluscheck
@@ -134,6 +134,9 @@ do
      test "$f" = src/interfaces/ecpg/preproc/preproc.h && continue
      test "$f" = src/test/isolation/specparse.h && continue

+    # FIXME
+    test "$f" = src/backend/utils/adt/jsonpath_internal.h && continue
+
      # ppport.h is not under our control, so we can't make it 
standalone.
      test "$f" = src/pl/plperl/ppport.h && continue

Hm, what's that about?

Don't remember ... ;-)  I removed this.

Ah, there was a reason. The headerscheck and cpluspluscheck targets
need jsonpath_gram.h to be built first. Which previously happened
indirectly somehow? I have fixed this in the new patch version. I also
fixed the issue that Álvaro reported nearby.

Peter Eisentraut <peter@eisentraut.org> writes:

Ah, there was a reason. The headerscheck and cpluspluscheck targets
need jsonpath_gram.h to be built first. Which previously happened
indirectly somehow? I have fixed this in the new patch version. I also
fixed the issue that Álvaro reported nearby.

Have we yet run this concept past the packagers list? I'm still
wondering if they will raise any objection to getting rid of all
the prebuilt files.

Also, I personally don't like the fact that you have removed the
distinction between distclean and maintainer-clean. I use
distclean-and-reconfigure quite a lot to avoid having to rebuild
bison/flex outputs. This patch seems to have destroyed that
workflow optimization in return for not much.

regards, tom lane

Re: Tom Lane

Have we yet run this concept past the packagers list? I'm still
wondering if they will raise any objection to getting rid of all
the prebuilt files.

No problem for Debian, we are building snapshot releases from plain
git already without issues. In fact, there are already some explicit
rm/clean rules in the packages to force rebuilding some (most?) of the
pre-built files.

Most notably, we are also rebuilding "configure" using autoconf 2.71
without issues. Perhaps we can get rid of the 2.69 hardcoding there?

Thanks for the heads-up,
Christoph

Christoph Berg <myon@debian.org> writes:

Most notably, we are also rebuilding "configure" using autoconf 2.71
without issues. Perhaps we can get rid of the 2.69 hardcoding there?

Meh ... the fact that it works fine for you doesn't mean it will work
fine elsewhere. Since we're trying to get out from under maintaining
the autoconf build system, I don't think it makes sense to open
ourselves up to having to do more work on it. A policy of benign
neglect seems appropriate to me.

regards, tom lane

Re: Tom Lane

Meh ... the fact that it works fine for you doesn't mean it will work
fine elsewhere. Since we're trying to get out from under maintaining
the autoconf build system, I don't think it makes sense to open
ourselves up to having to do more work on it. A policy of benign
neglect seems appropriate to me.

Understood, I was just pointing out there are more types of generated
files in there.

Christoph

Hi,

Thanks for sending the -packagers email Peter!

On 2023-08-09 16:25:28 +0200, Christoph Berg wrote:

Re: Tom Lane

Meh ... the fact that it works fine for you doesn't mean it will work
fine elsewhere. Since we're trying to get out from under maintaining
the autoconf build system, I don't think it makes sense to open
ourselves up to having to do more work on it. A policy of benign
neglect seems appropriate to me.

Understood, I was just pointing out there are more types of generated
files in there.

The situation for configure is somewhat different, due to being maintained in
the repository, rather than just being included in the tarball...

Greetings,

Andres Freund

On 14.07.23 11:48, Tom Lane wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

Ah, there was a reason. The headerscheck and cpluspluscheck targets
need jsonpath_gram.h to be built first. Which previously happened
indirectly somehow? I have fixed this in the new patch version. I also
fixed the issue that Álvaro reported nearby.

Have we yet run this concept past the packagers list? I'm still
wondering if they will raise any objection to getting rid of all
the prebuilt files.

So far there hasn't been any feedback from packagers that would appear
to affect the outcome here.

Also, I personally don't like the fact that you have removed the
distinction between distclean and maintainer-clean. I use
distclean-and-reconfigure quite a lot to avoid having to rebuild
bison/flex outputs. This patch seems to have destroyed that
workflow optimization in return for not much.

The distclean target has a standard meaning that is baked into
downstream build systems, so it would be pretty disruptive if distclean
didn't actually clean everything back down to what was in the
distribution tarball. We could add a different clean target that cleans
not quite everything, if you can suggest a definition of what that
should do.

On Wed, Aug 09, 2023 at 07:38:40AM -0700, Andres Freund wrote:

On 2023-08-09 16:25:28 +0200, Christoph Berg wrote:

Understood, I was just pointing out there are more types of generated
files in there.

The situation for configure is somewhat different, due to being maintained in
the repository, rather than just being included in the tarball...

This one comes down to Debian that patches autoconf with its own set
of options, requiring a new ./configure in the tree, right?
--
Michael

On 2023-08-18 10:11:12 +0900, Michael Paquier wrote:

On Wed, Aug 09, 2023 at 07:38:40AM -0700, Andres Freund wrote:

On 2023-08-09 16:25:28 +0200, Christoph Berg wrote:

Understood, I was just pointing out there are more types of generated
files in there.

The situation for configure is somewhat different, due to being maintained in
the repository, rather than just being included in the tarball...

This one comes down to Debian that patches autoconf with its own set
of options, requiring a new ./configure in the tree, right?

I'm not sure what you're really asking here?

Re: Michael Paquier

This one comes down to Debian that patches autoconf with its own set
of options, requiring a new ./configure in the tree, right?

Yes, mostly. Since autoconf had not seen a new release for so long,
everyone started to patch it, and one of the things that Debian and
others added was --runstatedir=DIR. The toolchain is also using it,
it's part of the default set of options used by dh_auto_configure.

In parallel, the standard debhelper toolchain also started to run
autoreconf by default, so instead of telling dh_auto_configure to omit
--runstatedir, it was really easier to patch configure.ac to remove
the autoconf 2.69 check.

Two of the other patches are touching configure(.ac) anyway to tweak
compiler flags (reproducibility, aarch64 tweaks).

Christoph

On Fri, Aug 18, 2023 at 10:22:47AM +0200, Christoph Berg wrote:

Yes, mostly. Since autoconf had not seen a new release for so long,
everyone started to patch it, and one of the things that Debian and
others added was --runstatedir=DIR. The toolchain is also using it,
it's part of the default set of options used by dh_auto_configure.

In parallel, the standard debhelper toolchain also started to run
autoreconf by default, so instead of telling dh_auto_configure to omit
--runstatedir, it was really easier to patch configure.ac to remove
the autoconf 2.69 check.

Ah, I didn't know this part of the story. Thanks for the insights.

Two of the other patches are touching configure(.ac) anyway to tweak
compiler flags (reproducibility, aarch64 tweaks).

Is reproducibility something you've brought to a separate thread?
FWIW, I'd be interested in improving this area for the in-core code,
if need be. (Not material for this thread, of course).
--
Michael

Re: Michael Paquier

Is reproducibility something you've brought to a separate thread?
FWIW, I'd be interested in improving this area for the in-core code,
if need be. (Not material for this thread, of course).

All the "normal" things like C compilation are actually already
reproducible.

The bit addressed by the mentioned patch is that the compiler flags
are recorded for later output by pg_config, and that includes
-ffile-prefix-map=/path/to/source=. which does improve C
reproducibility, but ironically is itself not reproducible when the
source is then compiled in a different directory. The patch simply
removes that flag from the information stored.

https://salsa.debian.org/postgresql/postgresql/-/blob/15/debian/patches/filter-debug-prefix-map

Not sure not much of that would be material for inclusion in PG.

This fix made PG 10 reproducible in Debian for about a week. Then LLVM
happened :D. The .bc files still record the build path, and so far no
one has found a way to prevent that:

https://tests.reproducible-builds.org/debian/rb-pkg/unstable/arm64/diffoscope-results/postgresql-15.html

Afaict that's the last part to be resolved (but it's been a while
since I checked). clang seems to have learned about -ffile-prefix-map=
in the meantime, that needs to be tested.

Christoph

On 14.07.23 10:56, Peter Eisentraut wrote:

On 14.07.23 09:54, Peter Eisentraut wrote:

diff --git a/src/tools/pginclude/cpluspluscheck 
b/src/tools/pginclude/cpluspluscheck
index 4e09c4686b..287395887c 100755
--- a/src/tools/pginclude/cpluspluscheck
+++ b/src/tools/pginclude/cpluspluscheck
@@ -134,6 +134,9 @@ do
      test "$f" = src/interfaces/ecpg/preproc/preproc.h && continue
      test "$f" = src/test/isolation/specparse.h && continue

+    # FIXME
+    test "$f" = src/backend/utils/adt/jsonpath_internal.h && continue
+
      # ppport.h is not under our control, so we can't make it 
standalone.
      test "$f" = src/pl/plperl/ppport.h && continue

Hm, what's that about?

Don't remember ... ;-)  I removed this.

Ah, there was a reason.  The headerscheck and cpluspluscheck targets
need jsonpath_gram.h to be built first.  Which previously happened
indirectly somehow?  I have fixed this in the new patch version.  I also
fixed the issue that Álvaro reported nearby.

Apparently, the headerscheck and cpluspluscheck targets still didn't
work right in the Cirrus jobs. Here is an updated patch to address
that. This is also rebased over some recent changes that affected this
patch (generated wait events stuff).

On Wed, Aug 23, 2023 at 12:46:45PM +0200, Peter Eisentraut wrote:

Apparently, the headerscheck and cpluspluscheck targets still didn't work
right in the Cirrus jobs. Here is an updated patch to address that. This
is also rebased over some recent changes that affected this patch (generated
wait events stuff).

-gettext-files: distprep
+gettext-files: postgres

This bit in src/backend/nls.mk does not seem right to me. The
following sequence works on HEAD, but breaks with the patch because
the files that should be automatically generated from the perl scripts
aren't anymore:
$ ./configure ...
$ make -C src/backend/ gettext-files
[...]
In file included from ../../../../src/include/postgres.h:46,
from brin.c:16:
../../../../src/include/utils/elog.h:79:10: fatal error:
utils/errcodes.h: No such file or directory
79 | #include "utils/errcodes.h"

# Technically, this should depend on Makefile.global, but then
# version.sgml would need to be rebuilt after every configure run,
# even in distribution tarballs. So this is cheating a bit, but it

There is this comment in doc/src/sgml/Makefile. Does it still apply?

Note that building <productname>PostgreSQL</productname> from the source
repository requires reasonably up-to-date versions of <application>bison</application>,
<application>flex</application>, and <application>Perl</application>.
These tools are not needed to build from a distribution tarball, because
the files generated with these tools are included in the tarball.
Other tool requirements

This paragraph exists in sourcerepo.sgml, but it should be updated, I
guess, because now these three binaries would be required when
building from a tarball.

# specparse.c and specscanner.c are in the distribution tarball,
# so do not clean them here

This comment in src/test/isolation/Makefile should be removed.
--
Michael

Hi,

On 2023-08-23 12:46:45 +0200, Peter Eisentraut wrote:

Subject: [PATCH v4] Remove distprep

A PostgreSQL release tarball contains a number of prebuilt files, in
particular files produced by bison, flex, perl, and well as html and
man documentation. We have done this consistent with established
practice at the time to not require these tools for building from a
tarball. Some of these tools were hard to get, or get the right
version of, from time to time, and shipping the prebuilt output was a
convenience to users.

Now this has at least two problems:

One, we have to make the build system(s) work in two modes: Building
from a git checkout and building from a tarball. This is pretty
complicated, but it works so far for autoconf/make. It does not
currently work for meson; you can currently only build with meson from
a git checkout. Making meson builds work from a tarball seems very
difficult or impossible. One particular problem is that since meson
requires a separate build directory, we cannot make the build update
files like gram.h in the source tree. So if you were to build from a
tarball and update gram.y, you will have a gram.h in the source tree
and one in the build tree, but the way things work is that the
compiler will always use the one in the source tree. So you cannot,
for example, make any gram.y changes when building from a tarball.
This seems impossible to fix in a non-horrible way.

I think it might be possible to fix in a non-horrible way, just that the
effort doing so could be much better spent on other things.

It's maybe also worth mentioning that this does *not* work reliably with vpath
make builds either...

The make maintainer-clean target, whose job it is to remove the
prebuilt files in addition to what make distclean does, is now just an
alias to make distprep. (In practice, it is probably obsolete given
that git clean is available.)

FWIW, I find a "full clean" target useful to be sure that we don't produce
"untracked" build products. Do a full build, then run "full clean", then see
what remains.

88 files changed, 169 insertions(+), 409 deletions(-)

It might be worthwhile to split this into a bit smaller chunks, e.g. depending
on perl, bison, flex, and then separately the various makefile bits that are
all over the tree.

Greetings,

Andres Freund

On 26.09.23 06:49, Michael Paquier wrote:

On Wed, Aug 23, 2023 at 12:46:45PM +0200, Peter Eisentraut wrote:

Apparently, the headerscheck and cpluspluscheck targets still didn't work
right in the Cirrus jobs. Here is an updated patch to address that. This
is also rebased over some recent changes that affected this patch (generated
wait events stuff).

-gettext-files: distprep
+gettext-files: postgres

This bit in src/backend/nls.mk does not seem right to me. The
following sequence works on HEAD, but breaks with the patch because
the files that should be automatically generated from the perl scripts
aren't anymore:
$ ./configure ...
$ make -C src/backend/ gettext-files
[...]
In file included from ../../../../src/include/postgres.h:46,
from brin.c:16:
../../../../src/include/utils/elog.h:79:10: fatal error:
utils/errcodes.h: No such file or directory
79 | #include "utils/errcodes.h"

Ok, I think I found a better way to address this. It requires keeping a
subset of the old distprep target in src/backend/Makefile for use by
nls.mk. I have checked that the above sequence now works, and that the
generated .pot files are identical to before this patch.

# Technically, this should depend on Makefile.global, but then
# version.sgml would need to be rebuilt after every configure run,
# even in distribution tarballs. So this is cheating a bit, but it

There is this comment in doc/src/sgml/Makefile. Does it still apply?

I have removed the "even in distribution tarballs" bit, but the general
idea is still valid I think.

Note that building <productname>PostgreSQL</productname> from the source
repository requires reasonably up-to-date versions of <application>bison</application>,
<application>flex</application>, and <application>Perl</application>.
These tools are not needed to build from a distribution tarball, because
the files generated with these tools are included in the tarball.
Other tool requirements

This paragraph exists in sourcerepo.sgml, but it should be updated, I
guess, because now these three binaries would be required when
building from a tarball.

I have removed that paragraph.

# specparse.c and specscanner.c are in the distribution tarball,
# so do not clean them here

This comment in src/test/isolation/Makefile should be removed.

done

The attached updated patch is also split up like Andres suggested
nearby. (Not sure if it would be good to commit it that way, but it's
easier to look at for now for sure.)