[libcamera-devel] [PATCH v2 1/1] build: Preserve upstream git versioning using meson dist

[Laurent Pinchart]

Hi Naush,

On Thu, Oct 14, 2021 at 07:04:40AM +0100, Naushir Patuck wrote:
> On Thu, 14 Oct 2021 at 01:46, Laurent Pinchart wrote:
> 
> > Hi Naush,
> >
> > (CC'ing Kieran)
> >
> > Thank you for the patch.
> >
> > On Wed, Oct 13, 2021 at 01:23:12PM +0100, Naushir Patuck wrote:
> > > When distributions build and package libcamera libraries, they may not
> > > necessarily run the build in the upstream source tree. In these cases, the git
> > > SHA1 versioning information will be lost.
> > >
> > > This change addresses that problem by requiring package managers to run
> > > 'meson dist' to create a tarball of the source files and build from there.
> > > On runing 'meson dist', the utils/run-dist.sh script will create a version.gen
> > > file in the release tarball with the version string generated from the existing
> > > utils/gen-version.sh script.
> > >
> > > The utils/gen-version.sh script has been updated to check for the presence of
> > > this version.gen file and read the version string from it instead of creating
> > > one.
> >
> > I came across
> > https://sources.debian.org/src/libsmpp34/1.14.0-2/git-version-gen/ which
> > seems to store the version in a file named .tarball-version. Should we
> > follow the same convention ?
> 
> Sure, I can rename it to .tarball-version.
> 
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  meson.build               |  3 +++
> > >  src/libcamera/meson.build | 11 +++++------
> > >  utils/gen-version.sh      |  9 +++++++++
> > >  utils/run-dist.sh         | 11 +++++++++++
> > >  4 files changed, 28 insertions(+), 6 deletions(-)
> > >  create mode 100644 utils/run-dist.sh
> > >
> > > diff --git a/meson.build b/meson.build
> > > index a49c484fe64e..85ca0013733e 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -24,6 +24,9 @@ endif
> > >
> > >  libcamera_version = libcamera_git_version.split('+')[0]
> > >
> > > +# This script gererates the version.gen file on a 'meson dist' command.
> > > +meson.add_dist_script('utils/run-dist.sh')
> > > +
> > >  # Configure the build environment.
> > >  cc = meson.get_compiler('c')
> > >  cxx = meson.get_compiler('cpp')
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 243dd3c180eb..360eaf80ecf1 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -93,12 +93,11 @@ endforeach
> > >
> > >  libcamera_sources += control_sources
> > >
> > > -gen_version = meson.source_root() / 'utils' / 'gen-version.sh'
> > > -
> > > -version_cpp = vcs_tag(command : [gen_version, meson.build_root()],
> > > -                      input : 'version.cpp.in',
> > > -                      output : 'version.cpp',
> > > -                      fallback : meson.project_version())
> > > +version_data = configuration_data()
> > > +version_data.set('VCS_TAG', libcamera_git_version)
> > > +version_cpp = configure_file(input : 'version.cpp.in',
> > > +                             output : 'version.cpp',
> > > +                             configuration : version_data)
> >
> > This doesn't seem strictly needed, but it avoids running gen-version.sh
> > a second time, which is nice. I however have a nagging feeling that
> > there was a reason why we used vcs_tag and not configure_file, but I
> > can't recall it. We'll find out if it causes issues :-)
> 
> There seems to be a subtle difference from my brief messing around with
> this.  configure_file() will execute on a meson config/setup command, and
> vcs_tag will run on the ninja build command.  I did have a problem with my
> change using the latter (I cannot recall exactly right now, but it was something
> to do with the $MESON_SOURCE_ROOT env variable not setup when running
> the gen-version script).  I thought this change would be good as you do not run
> the script twice either.

Doesn't this mean that we'll record the wrong version if one adds a new
commit and recompiles without reconfiguring ? Adding the SHA1 to
version.cpp, and printing it in the log, is meant to help catching
issues when libcamera is being deployed to the wrong directory by
mistake during development, so I think this is a very important use
case.

> > >  libcamera_sources += version_cpp
> > >
> > > diff --git a/utils/gen-version.sh b/utils/gen-version.sh
> > > index b09ad495f86a..09cede84c25e 100755
> > > --- a/utils/gen-version.sh
> > > +++ b/utils/gen-version.sh
> > > @@ -5,6 +5,15 @@
> > >
> > >  build_dir="$1"
> > >
> > > +# If version.gen exists, output the version string from the file and exit.
> > > +# This file is auto-generated on a 'meson dist' command from the run-dist.sh
> > > +# script.
> > > +if [ -f "${MESON_SOURCE_ROOT}"/version.gen ]
> > > +then
> > > +     cat "${MESON_SOURCE_ROOT}"/version.gen
> > > +     exit 0
> > > +fi
> > > +
> >
> > Shouldn't we do this only if we're not under git control ? There should
> > be no version.gen file in that case of course, but isn't still a better
> > default ?
> 
> In the original github issue, the reporter had a flow where the tarball
> would be initialised as a new git repo, and so the sha values extracted
> were valid values, but had no relation to the upstream sha values. So for
> those cases, we have to check the file unconditionally.
> 
> > >  # Bail out if the directory isn't under git control
> > >  src_dir=$(git rev-parse --git-dir 2>&1) || exit 1
> > >  src_dir=$(readlink -f "$src_dir/..")
> > > diff --git a/utils/run-dist.sh b/utils/run-dist.sh
> > > new file mode 100644
> > > index 000000000000..3b6c0adb05ed
> > > --- /dev/null
> > > +++ b/utils/run-dist.sh
> > > @@ -0,0 +1,11 @@
> > > +#!/bin/sh
> > > +
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +#
> > > +# On a meson dist run, generate the version string and store it in a file.
> > > +# This will later be picked up by the utils/gen-version.sh script and used
> > > +# instead of re-generating it. This way, if we are not building in the upstream
> > > +# git source tree, the upstream version information will be preserved.
> > > +
> > > +cd "$MESON_SOURCE_ROOT" || return
> > > +./utils/gen-version.sh > "$MESON_DIST_ROOT"/version.gen
> >
> > Do we need to handle the case of meson dist being run from a tarball
> > instead of git ? It seems like a corner case to me, so probably not very
> > useful, but we could possibly support it by copying
> > $MESON_SOURCE_ROOT/version.gen to $MESON_DIST_ROOT/ if the source file
> > exists. I'm fine either way.
> 
> Good point.  Although, I cannot see why meson dist should be run from a
> tarball and not the upstream git tree :-)

-- 
Regards,

Laurent Pinchart