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

[Naushir Patuck]

On Thu, 14 Oct 2021 at 11:21, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> 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.
>

You are correct, that is a common use case that must be accounted for!
Perhaps this was the reason the original code used vcs_tag :-)

I'll revert this back and try to work through my issue I was seeing in
another
way.



>
> > > >  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211014/4da07d00/attachment-0001.htm>