Re: [PATCH] configure: allow adding a custom string to PG_VERSION

This makes it easy to see if the binaries were built from a real release
or if they were built from a custom git tree.
---
configure.in | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/configure.in b/configure.in
index a4baeaf..7c5b3ce 100644
--- a/configure.in
+++ b/configure.in
@@ -29,7 +29,14 @@ AC_CONFIG_AUX_DIR(config)
AC_PREFIX_DEFAULT(/usr/local/pgsql)
AC_SUBST(configure_args, [$ac_configure_args])

-AC_DEFINE_UNQUOTED(PG_VERSION, "$PACKAGE_VERSION", [PostgreSQL version as a string])
+# Append git tag based version to PG_VERSION if we're building from a git
+# checkout, but don't touch PACKAGE_VERSION which is used to create other
+# version variables (major version and numeric version.)
+PG_VERSION="$PACKAGE_VERSION"
+if test -d .git; then
+ PG_VERSION="$PG_VERSION (`git describe --tags --long --dirty HEAD || echo unknown`)"
+fi
+AC_DEFINE_UNQUOTED(PG_VERSION, "$PG_VERSION", [PostgreSQL version as a string])
[PG_MAJORVERSION=`expr "$PACKAGE_VERSION" : '\([0-9][0-9]*\.[0-9][0-9]*\)'`]
AC_SUBST(PG_MAJORVERSION)
AC_DEFINE_UNQUOTED(PG_MAJORVERSION, "$PG_MAJORVERSION", [PostgreSQL major version as a string])
--
1.8.4.2

On 05.11.2013 12:22, Oskari Saarenmaa wrote:
> This makes it easy to see if the binaries were built from a real release
> or if they were built from a custom git tree.

Hmm, that would mean that a build from "git checkout REL9_3_1" produces
a different binary than one built from postgresql-9.3.1.tar.gz tarball.

I can see some value in that kind of information, ie. knowing what
patches a binary was built with, but this would only solve the problem
for git checkouts. Even for a git checkout, the binaries won't be
automatically updated unless you run "configure" again, which makes it
pretty unreliable.

-1 from me.

PS, the git command in the patch doesn't work with my version of git:

$ git describe --tags --long --dirty HEAD
fatal: --dirty is incompatible with committishes
$ git --version
git version 1.8.4.rc3

- Heikki

On Tue, Nov 05, 2013 at 02:06:26PM +0200, Heikki Linnakangas wrote:
> On 05.11.2013 12:22, Oskari Saarenmaa wrote:
> >This makes it easy to see if the binaries were built from a real release
> >or if they were built from a custom git tree.
>
> Hmm, that would mean that a build from "git checkout REL9_3_1"
> produces a different binary than one built from
> postgresql-9.3.1.tar.gz tarball.

Good point - how about adding git describe information only if the checkout
doesn't match a tag exactly? So when you build REL9_3_1 there would be no
extra information, but when you have any local changes on top of it we'd add
the git describe output.

> I can see some value in that kind of information, ie. knowing what
> patches a binary was built with, but this would only solve the
> problem for git checkouts. Even for a git checkout, the binaries
> won't be automatically updated unless you run "configure" again,
> which makes it pretty unreliable.
>
> -1 from me.

I don't think we can solve the problem of finding local changes for all the
things people may do, but I'd guess it's pretty common to build postgresql
from a clean local git checkout and with this change at least some portion
of users would get some extra information. To solve the "rerun configure"
thing we could put git version in a new header file and have a makefile
dependency on .git/index for regenerating that file when needed.

We could also let users override the extra version with a command line
argument for configure so distributions could put the distribution package
version there, for example "9.3.1-2.fc20" on my Fedora system.

> PS, the git command in the patch doesn't work with my version of git:
>
> $ git describe --tags --long --dirty HEAD
> fatal: --dirty is incompatible with committishes
> $ git --version
> git version 1.8.4.rc3

I initially used HEAD when looking at the git describe values, but then
added --dirty which obviously doesn't make sense when describing a ref.

Sorry about the broken patch, I was applying these changes manually from
configure.in to configure because I didn't have the proper autoconf version
installed (autoconf 2.63 doesn't seem to be available in many distributions
anymore, maybe the required version could be upgraded at some point..)

How about the patch below to fix the exact tag and --dirty issues?

/ Oskari

diff --git a/configure.in b/configure.in
index a4baeaf..d253286 100644
--- a/configure.in
+++ b/configure.in
@@ -29,7 +29,18 @@ AC_CONFIG_AUX_DIR(config)
AC_PREFIX_DEFAULT(/usr/local/pgsql)
AC_SUBST(configure_args, [$ac_configure_args])

-AC_DEFINE_UNQUOTED(PG_VERSION, "$PACKAGE_VERSION", [PostgreSQL version as a string])
+# Append git tag based version to PG_VERSION if we're building from a git
+# checkout that doesn't match a tag exactly. Don't touch PACKAGE_VERSION
+# which is used to create other version variables (major version and numeric
+# version.)
+PG_VERSION="$PACKAGE_VERSION"
+if test -d .git; then
+ exact="`git describe --tags --exact-match --dirty 2>/dev/null || echo dirty`"
+ if echo "$exact" | grep -q dirty; then
+ PG_VERSION="$PG_VERSION (`git describe --tags --long --dirty || echo unknown`)"
+ fi
+fi
+AC_DEFINE_UNQUOTED(PG_VERSION, "$PG_VERSION", [PostgreSQL version as a string])
[PG_MAJORVERSION=`expr "$PACKAGE_VERSION" : '\([0-9][0-9]*\.[0-9][0-9]*\)'`]
AC_SUBST(PG_MAJORVERSION)
AC_DEFINE_UNQUOTED(PG_MAJORVERSION, "$PG_MAJORVERSION", [PostgreSQL major version as a string])
--
1.8.4.2

On Tue, Nov 5, 2013 at 2:05 PM, Oskari Saarenmaa <os(at)ohmu(dot)fi> wrote:
>> I can see some value in that kind of information, ie. knowing what
>> patches a binary was built with, but this would only solve the
>> problem for git checkouts. Even for a git checkout, the binaries
>> won't be automatically updated unless you run "configure" again,
>> which makes it pretty unreliable.
>>
>> -1 from me.
>
> I don't think we can solve the problem of finding local changes for all the
> things people may do, but I'd guess it's pretty common to build postgresql
> from a clean local git checkout and with this change at least some portion
> of users would get some extra information. To solve the "rerun configure"
> thing we could put git version in a new header file and have a makefile
> dependency on .git/index for regenerating that file when needed.
>
> We could also let users override the extra version with a command line
> argument for configure so distributions could put the distribution package
> version there, for example "9.3.1-2.fc20" on my Fedora system.
>
>> PS, the git command in the patch doesn't work with my version of git:
>>
>> $ git describe --tags --long --dirty HEAD
>> fatal: --dirty is incompatible with committishes
>> $ git --version
>> git version 1.8.4.rc3
>
> I initially used HEAD when looking at the git describe values, but then
> added --dirty which obviously doesn't make sense when describing a ref.
>
> Sorry about the broken patch, I was applying these changes manually from
> configure.in to configure because I didn't have the proper autoconf version
> installed (autoconf 2.63 doesn't seem to be available in many distributions
> anymore, maybe the required version could be upgraded at some point..)
>
> How about the patch below to fix the exact tag and --dirty issues?
A similar thing has been discussed a couple of months ago, and the
idea to add any git-related information in configure has been
rejected. You can have a look at the discussion here:
http://www.postgresql.org/message-id/3038.1374031659@sss.pgh.pa.us
As well as a potential solution here:
http://www.postgresql.org/message-id/c51433da5e804767724d60eea57f4178.squirrel@webmail.xs4all.nl

Regards,
--
Michael

Oskari Saarenmaa <os(at)ohmu(dot)fi> writes:
> On Tue, Nov 05, 2013 at 02:06:26PM +0200, Heikki Linnakangas wrote:
>> I can see some value in that kind of information, ie. knowing what
>> patches a binary was built with, but this would only solve the
>> problem for git checkouts. Even for a git checkout, the binaries
>> won't be automatically updated unless you run "configure" again,
>> which makes it pretty unreliable.
>>
>> -1 from me.

> I don't think we can solve the problem of finding local changes for all the
> things people may do, but I'd guess it's pretty common to build postgresql
> from a clean local git checkout and with this change at least some portion
> of users would get some extra information.

I agree with Heikki that this is basically useless. Most of my builds are
from git + uncommitted changes, so telling me what the top commit was has
no notable value. Even if I always committed before building, the hash
tags are only decipherable until I discard that branch. So basically, this
would only be useful to people building production servers from random git
pulls from development or release-branch mainline. How many people really
do that, and should we inconvenience everybody else to benefit them?
(Admittedly, the proposed patch is only marginally inconvenient as-is,
but anything that would force a header update after any commit would
definitely put me on the warpath.)

BTW, I don't think the proposed patch works at all in a VPATH build.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I agree with Heikki that this is basically useless. Most of my builds are
> from git + uncommitted changes, so telling me what the top commit was has
> no notable value.

The focus of this change would really be, imv anyway, for more casual
PG developers, particularly those familiar with github and who work with
branches pushed there by other developers.

> Even if I always committed before building, the hash
> tags are only decipherable until I discard that branch.

Certainly true- but then, are you typically keeping binaries around
after you discard the branch? Or distributing those binaries to others?
Seems unlikely. However, if you're pulling in many branches from remote
locations and building lots of PG binaries, keeping it all straight and
being confident you didn't mix anything can be a bit more challenging.

> So basically, this
> would only be useful to people building production servers from random git
> pulls from development or release-branch mainline. How many people really
> do that, and should we inconvenience everybody else to benefit them?

Not many do it today because we actively discourage it by requiring
patches to be posted to the mailing list and the number of people
writing PG patches is relatively small. Even so though, I can see folks
like certain PG-on-cloud providers, who are doing testing, or even
deployments, with various patches to provide us feedback on them, and
therefore have to manage a bunch of different binaries, might find it
useful.

> (Admittedly, the proposed patch is only marginally inconvenient as-is,
> but anything that would force a header update after any commit would
> definitely put me on the warpath.)
>
> BTW, I don't think the proposed patch works at all in a VPATH build.

Clearly, that would need to be addressed.

All-in-all, I'm not super excited about this but I also wouldn't mind,
so while not really a '+1', I'd say '+0'. Nice idea, if it isn't
painful to deal with and maintain.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> So basically, this
>> would only be useful to people building production servers from random git
>> pulls from development or release-branch mainline. How many people really
>> do that, and should we inconvenience everybody else to benefit them?

> Not many do it today because we actively discourage it by requiring
> patches to be posted to the mailing list and the number of people
> writing PG patches is relatively small. Even so though, I can see folks
> like certain PG-on-cloud providers, who are doing testing, or even
> deployments, with various patches to provide us feedback on them, and
> therefore have to manage a bunch of different binaries, might find it
> useful.

I can see that there might be a use for tagging multiple binaries,
I just don't believe that this is a particularly helpful way to do it.
The last-commit tag is neither exactly the right data nor even a little
bit user-friendly. What about, say, a configure option to add a
user-specified string to the version() result?

regards, tom lane

On 11/05/2013 10:32 AM, Stephen Frost wrote:
>
> All-in-all, I'm not super excited about this but I also wouldn't mind,
> so while not really a '+1', I'd say '+0'. Nice idea, if it isn't
> painful to deal with and maintain.
>
>

I doubt it's buying us anything worth having. What's more, it might get
in the road of some other gadget that would be worth having.

cheers

andrew

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> What about, say, a configure option to add a
> user-specified string to the version() result?

I quite like that idea, personally. Folks who care about it being a git
tag could then trivially get that also.

Thanks,

Stephen

On 11/05/2013 10:53 AM, Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> What about, say, a configure option to add a
>> user-specified string to the version() result?
> I quite like that idea, personally. Folks who care about it being a git
> tag could then trivially get that also.
>

+1.

cheers

andrew

This can be used to tag custom built packages with an extra version string
such as the git describe id or distribution package release version.

Based on pgsql-hackers discussion:
http://www.postgresql.org/message-id/20131105102226.GA26836@saarenmaa.fi

Signed-off-by: Oskari Saarenmaa <os(at)ohmu(dot)fi>
---
configure.in | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/configure.in b/configure.in
index a4baeaf..5459c71 100644
--- a/configure.in
+++ b/configure.in
@@ -29,11 +29,15 @@ AC_CONFIG_AUX_DIR(config)
AC_PREFIX_DEFAULT(/usr/local/pgsql)
AC_SUBST(configure_args, [$ac_configure_args])

-AC_DEFINE_UNQUOTED(PG_VERSION, "$PACKAGE_VERSION", [PostgreSQL version as a string])
[PG_MAJORVERSION=`expr "$PACKAGE_VERSION" : '\([0-9][0-9]*\.[0-9][0-9]*\)'`]
AC_SUBST(PG_MAJORVERSION)
AC_DEFINE_UNQUOTED(PG_MAJORVERSION, "$PG_MAJORVERSION", [PostgreSQL major version as a string])

+# Allow adding a custom string to PG_VERSION
+PGAC_ARG_REQ(with, extra-version, [NAME], [extra version information],
+[PG_VERSION="$PACKAGE_VERSION ($withval)"], [PG_VERSION="$PACKAGE_VERSION"])
+AC_DEFINE_UNQUOTED(PG_VERSION, "$PG_VERSION", [PostgreSQL version as a string])
+
AC_CANONICAL_HOST

template=
@@ -1920,7 +1924,7 @@ else
fi

AC_DEFINE_UNQUOTED(PG_VERSION_STR,
- ["PostgreSQL $PACKAGE_VERSION on $host, compiled by $cc_string, `expr $ac_cv_sizeof_void_p \* 8`-bit"],
+ ["PostgreSQL $PG_VERSION on $host, compiled by $cc_string, `expr $ac_cv_sizeof_void_p \* 8`-bit"],
[A string containing the version number, platform, and C compiler])

# Supply a numeric version string for use by 3rd party add-ons
--
1.8.4.2

On Tue, Nov 5, 2013 at 6:35 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Oskari Saarenmaa <os(at)ohmu(dot)fi> writes:
> > On Tue, Nov 05, 2013 at 02:06:26PM +0200, Heikki Linnakangas wrote:
> >> I can see some value in that kind of information, ie. knowing what
> >> patches a binary was built with, but this would only solve the
> >> problem for git checkouts. Even for a git checkout, the binaries
> >> won't be automatically updated unless you run "configure" again,
> >> which makes it pretty unreliable.
> >>
> >> -1 from me.
>
> > I don't think we can solve the problem of finding local changes for all
> the
> > things people may do, but I'd guess it's pretty common to build
> postgresql
> > from a clean local git checkout and with this change at least some
> portion
> > of users would get some extra information.
>
> I agree with Heikki that this is basically useless. Most of my builds are
> from git + uncommitted changes, so telling me what the top commit was has
> no notable value. Even if I always committed before building, the hash
> tags are only decipherable until I discard that branch.

I nearly always remember to set config's "prefix" to some directory name
that describes the uncommitted changes which I am reviewing or testing.
Also including into the directory name the git commit to which those
changes were applied is awkward and easy to forgot to do--the kind of thing
best done by software. (And if I've discarded the branch, that pretty much
tells me what I need to know about the binary built from it--obsolete.)

Cheers,

Jeff

On Tue, Nov 5, 2013 at 4:29 PM, Oskari Saarenmaa <os(at)ohmu(dot)fi> wrote:
> This can be used to tag custom built packages with an extra version string
> such as the git describe id or distribution package release version.
Could you attach a proper patch to your email and register it to the
next commit fest? Typically here:
https://commitfest.postgresql.org/action/commitfest_view?id=20
This idea is more adaptable than your latest proposition, so some
other people might be interested to review it. At least I like this
idea.

Regards,
--
Michael

On Tue, 2013-11-05 at 18:29 +0200, Oskari Saarenmaa wrote:
> This can be used to tag custom built packages with an extra version string
> such as the git describe id or distribution package release version.

I think this is a reasonable feature, and the implementation is OK, but
I don't see why the format of the extra version information needs to be
exactly

PG_VERSION="$PACKAGE_VERSION ($withval)"

I'd imagine, for example, that someone will want to do -something or
+something. So I'd just make this

PG_VERSION="$PACKAGE_VERSION$withval"

Comments?

> +# Allow adding a custom string to PG_VERSION
> +PGAC_ARG_REQ(with, extra-version, [NAME], [extra version information],
> +[PG_VERSION="$PACKAGE_VERSION ($withval)"], [PG_VERSION="$PACKAGE_VERSION"])

This could be indented better. It was a bit confusing at first.

On Tue, Nov 19, 2013 at 10:48 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> I think this is a reasonable feature, and the implementation is OK, but
> I don't see why the format of the extra version information needs to be
> exactly
>
> PG_VERSION="$PACKAGE_VERSION ($withval)"
>
> I'd imagine, for example, that someone will want to do -something or
> +something. So I'd just make this
>
> PG_VERSION="$PACKAGE_VERSION$withval"
>
> Comments?
It makes sense and brings more flexibility. So +1 for this modification.
--
Michael

On Mon, Nov 18, 2013 at 08:48:13PM -0500, Peter Eisentraut wrote:
> On Tue, 2013-11-05 at 18:29 +0200, Oskari Saarenmaa wrote:
> > This can be used to tag custom built packages with an extra version string
> > such as the git describe id or distribution package release version.
>
> I think this is a reasonable feature, and the implementation is OK, but
> I don't see why the format of the extra version information needs to be
> exactly
>
> PG_VERSION="$PACKAGE_VERSION ($withval)"
>
> I'd imagine, for example, that someone will want to do -something or
> +something. So I'd just make this
>
> PG_VERSION="$PACKAGE_VERSION$withval"
>
> Comments?

Sounds reasonable.

> > +# Allow adding a custom string to PG_VERSION
> > +PGAC_ARG_REQ(with, extra-version, [NAME], [extra version information],
> > +[PG_VERSION="$PACKAGE_VERSION ($withval)"], [PG_VERSION="$PACKAGE_VERSION"])
>
> This could be indented better. It was a bit confusing at first.

Agreed. Attached an updated patch, or you can grab it from
https://github.com/saaros/postgres/compare/extra-version

Thanks,
Oskari

On Wed, Nov 20, 2013 at 8:08 AM, Oskari Saarenmaa <os(at)ohmu(dot)fi> wrote:
>
> On Mon, Nov 18, 2013 at 08:48:13PM -0500, Peter Eisentraut wrote:
> > On Tue, 2013-11-05 at 18:29 +0200, Oskari Saarenmaa wrote:
> > > This can be used to tag custom built packages with an extra version string
> > > such as the git describe id or distribution package release version.
> >
> > I think this is a reasonable feature, and the implementation is OK, but
> > I don't see why the format of the extra version information needs to be
> > exactly
> >
> > PG_VERSION="$PACKAGE_VERSION ($withval)"
> >
> > I'd imagine, for example, that someone will want to do -something or
> > +something. So I'd just make this
> >
> > PG_VERSION="$PACKAGE_VERSION$withval"
> >
> > Comments?
>
> Sounds reasonable.
>
> > > +# Allow adding a custom string to PG_VERSION
> > > +PGAC_ARG_REQ(with, extra-version, [NAME], [extra version information],
> > > +[PG_VERSION="$PACKAGE_VERSION ($withval)"], [PG_VERSION="$PACKAGE_VERSION"])
> >
> > This could be indented better. It was a bit confusing at first.
>
> Agreed. Attached an updated patch, or you can grab it from
> https://github.com/saaros/postgres/compare/extra-version

Here are a couple of comments about the patch:
1) I think that you should regenerate ./configure as well with this
patch to include all the changes together (someone correct me if I am
wrong here!)
2) This new option should be added in the section "## Command line
options" in configure.in
3) PG_VERSION is not a variable name adapted IMO, as it might contain
custom information. Something like PG_VERSION_TOTAL perhaps?
regards,
--
Michael

On Wed, 2013-11-20 at 22:41 +0900, Michael Paquier wrote:
> Here are a couple of comments about the patch:
> 1) I think that you should regenerate ./configure as well with this
> patch to include all the changes together (someone correct me if I am
> wrong here!)

Doesn't matter either way.

> 2) This new option should be added in the section "## Command line
> options" in configure.in

Yes.

> 3) PG_VERSION is not a variable name adapted IMO, as it might contain
> custom information. Something like PG_VERSION_TOTAL perhaps?

I don't think it's necessary to split this up further. We have
PG_VERSION and PG_MAJORVERSION. What's the use for one more level?

On Wed, 2013-11-20 at 01:08 +0200, Oskari Saarenmaa wrote:
> Agreed. Attached an updated patch, or you can grab it from
> https://github.com/saaros/postgres/compare/extra-version

Committed.

On Fri, Dec 13, 2013 at 12:03 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Wed, 2013-11-20 at 01:08 +0200, Oskari Saarenmaa wrote:
>> Agreed. Attached an updated patch, or you can grab it from
>> https://github.com/saaros/postgres/compare/extra-version
>
> Committed.
Thanks.
--
Michael