[libcamera-devel] [PATCH v2] libcamera: skip auto version generation when building for Chromium OS

[Laurent Pinchart]

Hi Kieran,

On Wed, Jul 10, 2019 at 09:32:31PM +0100, Kieran Bingham wrote:
> On 10/07/2019 15:23, Paul Elder wrote:
> > Commit b817bcec6b53 ("libcamera: Auto generate version information")
> > causes the build to fail in the Chromium OS build environment, because
> > git update-index tries to take a lock (ie. write) in the git repo that
> > is outside of the build directory.
> 
> ouch ...
> 
> > The solution is to simply skip git update-index if we are building in
> > the Chromium OS build environment, and this decision is made if the
> > build directory is not a subdirectory of the source directory.
> 
> Thanks for looking into a fix,
> 
> Running shellcheck highlights a few improvements on this patch
> 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> > Changes in v2:
> > - add quotes around variable accessess, and the string matcher
> > - make the two path arguments to gen-version.sh required
> > - actually run gen-version.sh from the needed place in meson
> > 
> >  meson.build               |  3 ++-
> >  src/libcamera/meson.build |  2 +-
> >  utils/gen-version.sh      | 14 +++++++++++---
> >  3 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index 8f3d0ce..99a3a80 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -15,7 +15,8 @@ project('libcamera', 'c', 'cpp',
> >  # git version tag, the build metadata (e.g. +211-c94a24f4) is omitted from
> >  # libcamera_git_version.
> >  libcamera_git_version = run_command('utils/gen-version.sh',
> > -                                    meson.source_root()).stdout().strip()
> > +                                    meson.source_root(),
> > +                                    meson.build_root()).stdout().strip()
> >  if libcamera_git_version == ''
> >      libcamera_git_version = meson.project_version()
> >  endif
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 97ff86e..4c442b9 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -81,7 +81,7 @@ libcamera_sources += control_types_cpp
> >  
> >  gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')
> >  
> > -version_cpp = vcs_tag(command : [gen_version, meson.source_root()],
> > +version_cpp = vcs_tag(command : [gen_version, meson.source_root(), meson.build_root()],
> >                        input : 'version.cpp.in',
> >                        output : 'version.cpp',
> >                        fallback : meson.project_version())
> > diff --git a/utils/gen-version.sh b/utils/gen-version.sh
> > index 708c01d..5005db9 100755
> > --- a/utils/gen-version.sh
> > +++ b/utils/gen-version.sh
> > @@ -3,11 +3,16 @@
> >  # SPDX-License-Identifier: GPL-2.0-or-later
> >  # Generate a version string using git describe
> >  
> > -if [ -n "$1" ]
> > +src_dir="$1"
> > +build_dir="$2"
> > +
> > +if [ -z "$src_dir" -o -z "$build_dir" ]
> 
> Shellcheck reports the following:
> 
> In utils/gen-version.sh line 9:
> if [ -z "$src_dir" -o -z "$build_dir" ]
>                    ^-- SC2166: Prefer [ p ] || [ q ] as [ p -o q ] is
> not well defined.
> 
> So perhaps this line could be:
> 
>  if [ -z "$src_dir" ] || [ -z "$build_dir" ]
> 
> >  then
> > -	cd "$1" 2>/dev/null || exit 1
> > +	exit
> >  fi
> 
> It's a shame that we can't just run the command from the source tree any
> more to get the output, but as that's just really for testing and
> development, that's not a real issue.

We could drop the src_dir argument as the script is always run from the
source directory by meson. Then we can make build_dir optional, as it's
only needed to skip git update-index.

> > +cd "$src_dir" 2>/dev/null || exit 1
> > +
> >  # Bail out if the directory isn't under git control
> >  git rev-parse --git-dir >/dev/null 2>&1 || exit 1
> >  
> > @@ -24,7 +29,10 @@ fi
> >  
> >  # Append a '-dirty' suffix if the working tree is dirty. Prevent false
> >  # positives due to changed timestamps by running git update-index.
> > -git update-index --refresh > /dev/null 2>&1
> > +if [ -n "$(echo "$build_dir" | grep "$src_dir")" ]
> 
> Shellcheck reports:
> 
> In utils/gen-version.sh line 32:
> if [ -n "$(echo "$build_dir" | grep "$src_dir")" ]
>      ^-- SC2143: Use grep -q instead of comparing output with [ -n .. ].
> 
> Perhaps this line could be:
> 
>  if (echo "$build_dir" | grep -vq "$src_dir")

Won't the ( ... ) create a subshell that prevents the status to be
reported up ?

> With shellcheck running cleanly on utils/gen-version.sh:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +then
> > +	git update-index --refresh > /dev/null 2>&1
> > +fi
> >  git diff-index --quiet HEAD || version="$version-dirty"
> >  
> >  # Replace first '-' with a '+' to denote build metadata, strip the 'g' in from

-- 
Regards,

Laurent Pinchart