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

[Kieran Bingham]

Hi Laurent,

On 11/07/2019 03:23, Laurent Pinchart wrote:
> 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.

Either route is fine with me, I'm not worried about this part at the
moment. We're in control of the calling parameters at the points that we
need this.


>>> +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 ?

The subshell should return the status should it not?

Here's a basic test to verify this:


~~~~~~~~~~~~~ check-subshell.sh ~~~~~~~~~~~~~
srcdir=/path/to/src
intreebuild=/path/to/src/build
outtreebuild=/path/to/build

# '-vq' is a quiet negated search.
# True if needle is not found in haystack
# (the logic our script wants)
if (echo "$outtreebuild" | grep -vq "$srcdir");
then
  echo "OutOfTree";
  echo "Test Pass";
else
  echo "InTree";
  echo "TEST FAIL";
fi


if (echo "$intreebuild" | grep -vq "$srcdir");
then
  echo "OutOfTree";
  echo "TEST FAIL";
else
  echo "InTree";
  echo "Test Pass";
fi
~~~~~~~~~~~~~ check-subshell.sh ~~~~~~~~~~~~~


$ /bin/bash ./check-subshell.sh
OutOfTree
Test Pass
InTree
Test Pass

$ /bin/dash ./check-subshell.sh
OutOfTree
Test Pass
InTree
Test Pass





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