[libcamera-devel,v2] libcamera: skip auto version generation when building for Chromium OS
diff mbox series

Message ID 20190710142301.16340-1-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v2] libcamera: skip auto version generation when building for Chromium OS
Related show

Commit Message

Paul Elder July 10, 2019, 2:23 p.m. UTC 
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.

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.

Signed-off-by: Paul Elder <paul.elder@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(-)

Comments

Kieran Bingham July 10, 2019, 8:32 p.m. UTC | #1 
Hi Paul,

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



>  
> +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")



With shellcheck running cleanly on utils/gen-version.sh:

Reviewed-by: Kieran Bingham <kieran.bingham@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
>
Laurent Pinchart July 11, 2019, 2:23 a.m. UTC | #2 
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@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@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
Kieran Bingham July 11, 2019, 7:46 a.m. UTC | #3 
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@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@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
>

Patch
diff mbox series 

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" ]
 then
-	cd "$1" 2>/dev/null || exit 1
+	exit
 fi
 
+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")" ]
+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