<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 14 Oct 2021 at 11:21, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Thu, Oct 14, 2021 at 07:04:40AM +0100, Naushir Patuck wrote:<br>
> On Thu, 14 Oct 2021 at 01:46, Laurent Pinchart wrote:<br>
> <br>
> > Hi Naush,<br>
> ><br>
> > (CC'ing Kieran)<br>
> ><br>
> > Thank you for the patch.<br>
> ><br>
> > On Wed, Oct 13, 2021 at 01:23:12PM +0100, Naushir Patuck wrote:<br>
> > > When distributions build and package libcamera libraries, they may not<br>
> > > necessarily run the build in the upstream source tree. In these cases, the git<br>
> > > SHA1 versioning information will be lost.<br>
> > ><br>
> > > This change addresses that problem by requiring package managers to run<br>
> > > 'meson dist' to create a tarball of the source files and build from there.<br>
> > > On runing 'meson dist', the utils/run-dist.sh script will create a version.gen<br>
> > > file in the release tarball with the version string generated from the existing<br>
> > > utils/gen-version.sh script.<br>
> > ><br>
> > > The utils/gen-version.sh script has been updated to check for the presence of<br>
> > > this version.gen file and read the version string from it instead of creating<br>
> > > one.<br>
> ><br>
> > I came across<br>
> > <a href="https://sources.debian.org/src/libsmpp34/1.14.0-2/git-version-gen/" rel="noreferrer" target="_blank">https://sources.debian.org/src/libsmpp34/1.14.0-2/git-version-gen/</a> which<br>
> > seems to store the version in a file named .tarball-version. Should we<br>
> > follow the same convention ?<br>
> <br>
> Sure, I can rename it to .tarball-version.<br>
> <br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > > meson.build | 3 +++<br>
> > > src/libcamera/meson.build | 11 +++++------<br>
> > > utils/gen-version.sh | 9 +++++++++<br>
> > > utils/run-dist.sh | 11 +++++++++++<br>
> > > 4 files changed, 28 insertions(+), 6 deletions(-)<br>
> > > create mode 100644 utils/run-dist.sh<br>
> > ><br>
> > > diff --git a/meson.build b/meson.build<br>
> > > index a49c484fe64e..85ca0013733e 100644<br>
> > > --- a/meson.build<br>
> > > +++ b/meson.build<br>
> > > @@ -24,6 +24,9 @@ endif<br>
> > ><br>
> > > libcamera_version = libcamera_git_version.split('+')[0]<br>
> > ><br>
> > > +# This script gererates the version.gen file on a 'meson dist' command.<br>
> > > +meson.add_dist_script('utils/run-dist.sh')<br>
> > > +<br>
> > > # Configure the build environment.<br>
> > > cc = meson.get_compiler('c')<br>
> > > cxx = meson.get_compiler('cpp')<br>
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build<br>
> > > index 243dd3c180eb..360eaf80ecf1 100644<br>
> > > --- a/src/libcamera/meson.build<br>
> > > +++ b/src/libcamera/meson.build<br>
> > > @@ -93,12 +93,11 @@ endforeach<br>
> > ><br>
> > > libcamera_sources += control_sources<br>
> > ><br>
> > > -gen_version = meson.source_root() / 'utils' / 'gen-version.sh'<br>
> > > -<br>
> > > -version_cpp = vcs_tag(command : [gen_version, meson.build_root()],<br>
> > > - input : '<a href="http://version.cpp.in" rel="noreferrer" target="_blank">version.cpp.in</a>',<br>
> > > - output : 'version.cpp',<br>
> > > - fallback : meson.project_version())<br>
> > > +version_data = configuration_data()<br>
> > > +version_data.set('VCS_TAG', libcamera_git_version)<br>
> > > +version_cpp = configure_file(input : '<a href="http://version.cpp.in" rel="noreferrer" target="_blank">version.cpp.in</a>',<br>
> > > + output : 'version.cpp',<br>
> > > + configuration : version_data)<br>
> ><br>
> > This doesn't seem strictly needed, but it avoids running gen-version.sh<br>
> > a second time, which is nice. I however have a nagging feeling that<br>
> > there was a reason why we used vcs_tag and not configure_file, but I<br>
> > can't recall it. We'll find out if it causes issues :-)<br>
> <br>
> There seems to be a subtle difference from my brief messing around with<br>
> this. configure_file() will execute on a meson config/setup command, and<br>
> vcs_tag will run on the ninja build command. I did have a problem with my<br>
> change using the latter (I cannot recall exactly right now, but it was something<br>
> to do with the $MESON_SOURCE_ROOT env variable not setup when running<br>
> the gen-version script). I thought this change would be good as you do not run<br>
> the script twice either.<br>
<br>
Doesn't this mean that we'll record the wrong version if one adds a new<br>
commit and recompiles without reconfiguring ? Adding the SHA1 to<br>
version.cpp, and printing it in the log, is meant to help catching<br>
issues when libcamera is being deployed to the wrong directory by<br>
mistake during development, so I think this is a very important use<br>
case.<br></blockquote><div><br></div><div>You are correct, that is a common use case that must be accounted for!</div><div>Perhaps this was the reason the original code used vcs_tag :-)</div><div><br></div><div>I'll revert this back and try to work through my issue I was seeing in another</div><div>way.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > > libcamera_sources += version_cpp<br>
> > ><br>
> > > diff --git a/utils/gen-version.sh b/utils/gen-version.sh<br>
> > > index b09ad495f86a..09cede84c25e 100755<br>
> > > --- a/utils/gen-version.sh<br>
> > > +++ b/utils/gen-version.sh<br>
> > > @@ -5,6 +5,15 @@<br>
> > ><br>
> > > build_dir="$1"<br>
> > ><br>
> > > +# If version.gen exists, output the version string from the file and exit.<br>
> > > +# This file is auto-generated on a 'meson dist' command from the run-dist.sh<br>
> > > +# script.<br>
> > > +if [ -f "${MESON_SOURCE_ROOT}"/version.gen ]<br>
> > > +then<br>
> > > + cat "${MESON_SOURCE_ROOT}"/version.gen<br>
> > > + exit 0<br>
> > > +fi<br>
> > > +<br>
> ><br>
> > Shouldn't we do this only if we're not under git control ? There should<br>
> > be no version.gen file in that case of course, but isn't still a better<br>
> > default ?<br>
> <br>
> In the original github issue, the reporter had a flow where the tarball<br>
> would be initialised as a new git repo, and so the sha values extracted<br>
> were valid values, but had no relation to the upstream sha values. So for<br>
> those cases, we have to check the file unconditionally.<br>
> <br>
> > > # Bail out if the directory isn't under git control<br>
> > > src_dir=$(git rev-parse --git-dir 2>&1) || exit 1<br>
> > > src_dir=$(readlink -f "$src_dir/..")<br>
> > > diff --git a/utils/run-dist.sh b/utils/run-dist.sh<br>
> > > new file mode 100644<br>
> > > index 000000000000..3b6c0adb05ed<br>
> > > --- /dev/null<br>
> > > +++ b/utils/run-dist.sh<br>
> > > @@ -0,0 +1,11 @@<br>
> > > +#!/bin/sh<br>
> > > +<br>
> > > +# SPDX-License-Identifier: GPL-2.0-or-later<br>
> > > +#<br>
> > > +# On a meson dist run, generate the version string and store it in a file.<br>
> > > +# This will later be picked up by the utils/gen-version.sh script and used<br>
> > > +# instead of re-generating it. This way, if we are not building in the upstream<br>
> > > +# git source tree, the upstream version information will be preserved.<br>
> > > +<br>
> > > +cd "$MESON_SOURCE_ROOT" || return<br>
> > > +./utils/gen-version.sh > "$MESON_DIST_ROOT"/version.gen<br>
> ><br>
> > Do we need to handle the case of meson dist being run from a tarball<br>
> > instead of git ? It seems like a corner case to me, so probably not very<br>
> > useful, but we could possibly support it by copying<br>
> > $MESON_SOURCE_ROOT/version.gen to $MESON_DIST_ROOT/ if the source file<br>
> > exists. I'm fine either way.<br>
> <br>
> Good point. Although, I cannot see why meson dist should be run from a<br>
> tarball and not the upstream git tree :-)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>