Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8; format=Flowed
Date: Mon, 22 Jan 2024 14:04:43 -0600
Message-Id: <CYLI6IZ5RAVR.3RBBF4F3P8MHX@neon.tech>
Subject: Re: make dist using git archive
Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>
To: "Peter Eisentraut" <peter@eisentraut.org>
From: "Tristan Partin" <tristan@neon.tech>
References: <40e80f77-a294-4f29-a16f-e21bc7bc75fc@eisentraut.org>
In-Reply-To: <40e80f77-a294-4f29-a16f-e21bc7bc75fc@eisentraut.org>
Archived-At: 
 <https://www.postgresql.org/message-id/CYLI6IZ5RAVR.3RBBF4F3P8MHX%40neon.tech>
Precedence: bulk

On Mon Jan 22, 2024 at 1:31 AM CST, Peter Eisentraut wrote:
> From 4b128faca90238d0a0bb6949a8050c2501d1bd67 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter@eisentraut.org>
> Date: Sat, 20 Jan 2024 21:54:36 +0100
> Subject: [PATCH v0] make dist uses git archive
>
> ---
>  GNUmakefile.in | 34 ++++++++++++----------------------
>  meson.build    | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/GNUmakefile.in b/GNUmakefile.in
> index eba569e930e..3e04785ada2 100644
> --- a/GNUmakefile.in
> +++ b/GNUmakefile.in
> @@ -87,29 +87,19 @@ update-unicode: | submake-generated-headers submake-l=
ibpgport
>  distdir	=3D postgresql-$(VERSION)
>  dummy	=3D =3Dinstall=3D
> =20
> +GIT =3D git
> +
>  dist: $(distdir).tar.gz $(distdir).tar.bz2
> -	rm -rf $(distdir)
> -
> -$(distdir).tar: distdir
> -	$(TAR) chf $@ $(distdir)
> -
> -.INTERMEDIATE: $(distdir).tar
> -
> -distdir-location:
> -	@echo $(distdir)
> -
> -distdir:
> -	rm -rf $(distdir)* $(dummy)
> -	for x in `cd $(top_srcdir) && find . \( -name CVS -prune \) -o \( -name=
 .git -prune \) -o -print`; do \
> -	  file=3D`expr X$$x : 'X\./\(.*\)'`; \
> -	  if test -d "$(top_srcdir)/$$file" ; then \
> -	    mkdir "$(distdir)/$$file" && chmod 777 "$(distdir)/$$file";	\
> -	  else \
> -	    ln "$(top_srcdir)/$$file" "$(distdir)/$$file" >/dev/null 2>&1 \
> -	      || cp "$(top_srcdir)/$$file" "$(distdir)/$$file"; \
> -	  fi || exit; \
> -	done
> -	$(MAKE) -C $(distdir) distclean
> +
> +.PHONY: check-dirty-index
> +check-dirty-index:
> +	$(GIT) diff-index --quiet HEAD
> +
> +$(distdir).tar.gz: check-dirty-index
> +	$(GIT) archive --format tar.gz --prefix $(distdir)/ HEAD -o $@
> +
> +$(distdir).tar.bz2: check-dirty-index
> +	$(GIT) -c tar.tar.bz2.command=3D'$(BZIP2) -c' archive --format tar.bz2 =
--prefix $(distdir)/ HEAD -o $@
> =20
>  distcheck: dist
>  	rm -rf $(dummy)
> diff --git a/meson.build b/meson.build
> index c317144b6bc..f0d870c5192 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3347,6 +3347,44 @@ run_target('help',
> =20
> =20
> =20
> +###############################################################
> +# Distribution archive
> +###############################################################
> +
> +git =3D find_program('git', required: false, native: true, disabler: tru=
e)
> +bzip2 =3D find_program('bzip2', required: false, native: true, disabler:=
 true)

This doesn't need to be a disabler. git is fine as-is. See later=20
comment. Disablers only work like you are expecting when they are used=20
like how git is used. Once you call a method like .path(), all bets are=20
off.

> +distdir =3D meson.project_name() + '-' + meson.project_version()
> +
> +check_dirty_index =3D run_target('check-dirty-index',
> +                               command: [git, 'diff-index', '--quiet', '=
HEAD'])

Seems like you might want to add -C here too?

> +
> +tar_gz =3D custom_target('tar.gz',
> +  build_always_stale: true,
> +  command: [git, '-C', '@SOURCE_ROOT@', 'archive',
> +            '--format', 'tar.gz',
> +            '--prefix', distdir + '/',
> +            '-o', '@BUILD_ROOT@/@OUTPUT@',
> +            'HEAD', '.'],
> +  install: false,
> +  output: distdir + '.tar.gz',
> +)
> +
> +tar_bz2 =3D custom_target('tar.bz2',
> +  build_always_stale: true,
> +  command: [git, '-C', '@SOURCE_ROOT@', '-c', 'tar.tar.bz2.command=3D' +=
 bzip2.path() + ' -c', 'archive',
> +            '--format', 'tar.bz2',
> +            '--prefix', distdir + '/',

-            '-o', '@BUILD_ROOT@/@OUTPUT@',
+            '-o', join_paths(meson.build_root(), '@OUTPUT@'),

This will generate the tarballs in the build directory. Do the same for=20
the previous target. Tested locally.

> +            'HEAD', '.'],
> +  install: false,
> +  output: distdir + '.tar.bz2',
> +)

The bz2 target should be wrapped in an `if bzip2.found()`. It is=20
possible for git to be found, but not bzip2. I might also define the bz2=20
command out of line. Also, you may want to add=20
these programs to meson_options.txt for overriding, even though the=20
"meson-ic" way is to use a machine file.

> +
> +alias_target('pgdist', [check_dirty_index, tar_gz, tar_bz2])

Are you intending for the check_dirty_index target to prohibit the other=20
two targets from running? Currently that is not the case. If it is what=20
you intend, use a stamp file or something to indicate a relationship.=20
Alternatively, inline the git diff-index into the other commands. These=20
might also do better as external scripts. It would reduce duplication=20
between the autotools and Meson builds.

> +
> +
> +
>  ###############################################################
>  # The End, The End, My Friend
>  ###############################################################

I am not really following why we can't use the builtin Meson dist=20
command. The only difference from my testing is it doesn't use=20
a --prefix argument.

--=20
Tristan Partin
Neon (https://neon.tech)