[libcamera-devel] [PATCH] libcamera: Auto generate version information
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jun 12 11:30:45 CEST 2019
Hi Laurent,
On 12/06/2019 09:39, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Tue, Jun 11, 2019 at 11:25:43PM +0100, Kieran Bingham wrote:
>> On 11/06/2019 21:32, Laurent Pinchart wrote:
>>> On Tue, Jun 11, 2019 at 09:14:11PM +0100, Kieran Bingham wrote:
>>>> Generate a version string, and provide a global singleton object which
>>>> allows applications to interogate the current libcamera version
>>>> information.
>>>>
>>>> The version header is automatically updated by meson on each build.
>>>> The string roughly follows the semver [0] conventions of
>>>> major.minor.patch-label as a value.
>>>>
>>>> [0] https://semver.org/
>>>>
>>>> The utils/version-gen script will look for tags in the form vX.Y as
>>>> starting points for the version string. While the repository does not
>>>> have any matching tags, v0.0 will be assumed, resulting in versions with
>>>> both major and minor being set to '0', and the patch count resulting
>>>> from the number of patches in the history to that point.
>>>>
>>>> Finally, a uniquely identifying shortened checksum is provided from git:
>>>>
>>>> v0.0.509.c544
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>> ---
>>>> Documentation/conf.py | 6 ++++--
>>>> include/libcamera/meson.build | 1 +
>>>> include/libcamera/version.h | 23 ++++++++++++++++++++
>>>> meson.build | 9 +++++++-
>>>> src/libcamera/meson.build | 1 +
>>>> src/libcamera/version.cpp | 40 +++++++++++++++++++++++++++++++++++
>>>> utils/version-gen | 36 +++++++++++++++++++++++++++++++
>>>> version.h.in | 3 +++
>>>> 8 files changed, 116 insertions(+), 3 deletions(-)
>>>> create mode 100644 include/libcamera/version.h
>>>> create mode 100644 src/libcamera/version.cpp
>>>> create mode 100755 utils/version-gen
>>>> create mode 100644 version.h.in
>>>>
>>>> diff --git a/Documentation/conf.py b/Documentation/conf.py
>>>> index 970edf3d7298..e5980b98216d 100644
>>>> --- a/Documentation/conf.py
>>>> +++ b/Documentation/conf.py
>>>> @@ -23,10 +23,12 @@ project = 'libcamera'
>>>> copyright = '2018-2019, The libcamera documentation authors'
>>>> author = u'Kieran Bingham, Jacopo Mondi, Laurent Pinchart, Niklas Söderlund'
>>>>
>>>> +# Vesion information is provided by the build environment.
>>>
>>> s/Vesion/Version/
>>
>> Eeep. Fixed.
>>
>>>> +#
>>>> # The short X.Y version
>>>> -version = ''
>>>> +# version = ''
>>>> # The full version, including alpha/beta/rc tags
>>>> -release = '0.1'
>>>> +# release = '0.1'
>>>
>>> Should we just delete the version and release lines ?
>>
>> I chose to keep these intentionally. The rest of the conf.py contains
>> 'template' lines.
>>
>> I can remove if you prefer, but the whole conf.py could probably be
>> stripped right down too.
>
> If you want to keep them please make sure they match the stock template
> file.
>
> By the way, where does the build environment provide version information
> to sphinx ?
I have no idea. It 'just works' with this update, when you rebuild the
documentation - the generated string is present in the output documentation.
I did nothing specific to make this happen. It just happens when you
don't override the version and release tags in this file.
>>>> # -- General configuration ---------------------------------------------------
>>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>>>> index 1b86fdc7fca4..201832105457 100644
>>>> --- a/include/libcamera/meson.build
>>>> +++ b/include/libcamera/meson.build
>>>> @@ -12,6 +12,7 @@ libcamera_api = files([
>>>> 'signal.h',
>>>> 'stream.h',
>>>> 'timer.h',
>>>> + 'version.h',
>>>> ])
>>>>
>>>> gen_header = join_paths(meson.current_source_dir(), 'gen-header.sh')
>>>> diff --git a/include/libcamera/version.h b/include/libcamera/version.h
>>>> new file mode 100644
>>>> index 000000000000..ad21f148e748
>>>> --- /dev/null
>>>> +++ b/include/libcamera/version.h
>>>> @@ -0,0 +1,23 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2019, Google Inc.
>>>> + *
>>>> + * version.h - Library version information
>>>> + */
>>>> +#ifndef __LIBCAMERA_VERSION_H__
>>>> +#define __LIBCAMERA_VERSION_H__
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +class LibcameraVersion
>>>> +{
>>>> +public:
>>>> + LibcameraVersion();
>>>> + std::string toString();
>>>> +};
>>>> +
>>>> +extern LibcameraVersion version;
>>>> +
>>>> +} /* namespace libcamera */
>>>> +
>>>> +#endif /* __LIBCAMERA_VERSION_H__ */
>>>
>>> This seems a bit overkill, can't we just install version.h ?
>>
>> I wanted to provide a way for an application or otherwise to obtain the
>> information at runtime.
>
> Sure, I think that's useful, but I think just moving version.h to
> include/libcamera/ and installing it would be better. Applications would
> have access to the raw data, and can do whatever they want with it. I'd
> go one step further, and provide numerical version information too, but
> that can be for later.
No - then they have access to the information they *compile against*
which (although it should be the same) might not be the same as the
version that they *run* against.
I agree, it might be worth providing this at compile time too though.
>> The static global instance 'version' prints a debug line through our LOG
>> interface at library start up, and I wanted to be able to expose that
>> string if needed.
>
> If you want to log the version I would do so in CameraManager. That's
> better than in a global constructor that would likely be called before
> the logging infrastructure is initialised.
OK - that sounds reasonable too.
>> Hrm... seems I messed up the patch anyway, I can't extern a static :-) -
>> Making it just 'global' to fix.
>>
>>>> diff --git a/meson.build b/meson.build
>>>> index 4d3e99d3e58f..5182546382c5 100644
>>>> --- a/meson.build
>>>> +++ b/meson.build
>>>> @@ -1,6 +1,8 @@
>>>> project('libcamera', 'c', 'cpp',
>>>> meson_version : '>= 0.40',
>>>> - version : '0.1',
>>>> + version : run_command('utils/version-gen',
>>>> + '@0@'.format(meson.source_root()),
>>>> + check : true).stdout().strip(),
>>>> default_options : [
>>>> 'werror=true',
>>>> 'warning_level=2',
>>>> @@ -13,6 +15,11 @@ project('libcamera', 'c', 'cpp',
>>>> # generated from this too.
>>>> api_version = '0.1'
>>>>
>>>> +vcs_tag(command: ['utils/version-gen', '.'],
>>>> + input: 'version.h.in',
>>>> + output: 'version.h',
>>>> + fallback: '')
>>>
>>> Mixing tabs and spaces.
>>
>> Oops! I thought I fixed that.
>>
>>> Don't you need to add the customtarget object returned by vcs_tag to
>>> dependencies or sources for something ? Otherwise meson won't know what
>>> depends on version.h.
>>
>> I think you're right, but I can't see how to express a dependency on
>> only a single object file.
>>
>> I'll have to ask that one in #mesonbuild.
>>
>> I don't know what magic is happening, but it does seem to be doing the
>> right thing anyway.
>
> Until we hit the right race :-)
Sure I love a day at the races ! :)
>> I've tested this with a few iterations of changing files that should not
>> directly recreate the version.o or libcamera.so - but does affect the
>> version-gen output, such as changing between a clean tree, and a -dirty
>> label - and the library does seem to be correctly updated.
>
> Note that once you have compiled the project, ninja will have generated
> dependency information, and will rebuild the .h and .cpp files
> correctly. To test for races you need to wipe the build directory. You
> can then use 'ninja target' with the target being set to a file that
> includes version.h, and watch it fail when building that target only.
OK - I haven't yet understood where that happens. My head is still stuck
in the dark ages with autoconf, and I know how make used to create .d
files - but ninja is still magic to me.
>> I'll call this one 'resolved by magic' for now.
>>
"Abracadabra, a dove" ...
<pulls white rabbit out of hat instead and looks perplexed>
>>>> +
>>>> cc = meson.get_compiler('c')
>>>> config_h = configuration_data()
>>>>
>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>>> index 1ca1083cf5c7..b9a4153839c9 100644
>>>> --- a/src/libcamera/meson.build
>>>> +++ b/src/libcamera/meson.build
>>>> @@ -25,6 +25,7 @@ libcamera_sources = files([
>>>> 'utils.cpp',
>>>> 'v4l2_device.cpp',
>>>> 'v4l2_subdevice.cpp',
>>>> + 'version.cpp',
>>>> ])
>>>>
>>>> libcamera_headers = files([
>>>> diff --git a/src/libcamera/version.cpp b/src/libcamera/version.cpp
>>>> new file mode 100644
>>>> index 000000000000..81f692f7cae7
>>>> --- /dev/null
>>>> +++ b/src/libcamera/version.cpp
>>>> @@ -0,0 +1,40 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2019, Google Inc.
>>>> + *
>>>> + * version.cpp - Version information
>>>> + */
>>>> +
>>>> +#include <libcamera/version.h>
>>>> +
>>>> +#include "log.h"
>>>> +
>>>> +/* The version header is automatically generated at the base of the project. */
>>>> +#include "../../version.h"
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +LOG_DEFINE_CATEGORY(Version)
>>>> +
>>>> +/**
>>>> + * \class LibcameraVersion
>>>> + *
>>>> + * Reports library version information
>>>> + */
>>>> +
>>>> +LibcameraVersion::LibcameraVersion()
>>>> +{
>>>> + LOG(Version, Info) << "Libcamera Version " << toString();
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Return the library version as a string
>>>> + */
>>>> +std::string LibcameraVersion::toString()
>>>> +{
>>>> + return LIBCAMERA_VERSION;
>>>> +}
>>>> +
>>>> +static LibcameraVersion version;
>>>> +
>>>> +} /* namespace libcamera */
>>>> diff --git a/utils/version-gen b/utils/version-gen
>>>> new file mode 100755
>>>> index 000000000000..b8e53d77b63e
>>>> --- /dev/null
>>>> +++ b/utils/version-gen
>>>> @@ -0,0 +1,36 @@
>>>> +#!/bin/sh
>>>
>>> Missing SPDX header.
>>
>> As a component of the build system, is this GPL? or LGPL?
>
> I'd say GPL-2+. I just realised that gen-headers doesn't have an SPDX
> header either. Feel free to fix both, or I can send a patch.
Lets fix gen-headers separately.
>>>> +
>>>> +if test -n "$1"
>>>
>>> Please use
>>>
>>> if [[ ]] ; then
>>>
>>> Our scripts rely on bash, so I think I would specify that explicitly
>>> instead of /bin/sh.
>>
>> I am intentionally trying to stay posix shell compliant.
>>
>> libcamera$ git grep bash
>> utils/ipu3/ipu3-capture.sh:#!/bin/bash
>> utils/ipu3/ipu3-process.sh:#!/bin/bash
>>
>> There's a big difference between non-essential helper scripts for a user
>> to run - and a script which is executed by the build system...
>>
>> Thus I'm not sure being reliant on bash is the right thing ...
>
> I believe we'll introduce bash-isms even if we try not to, and they will
> result in failures later. If we mandate usage of bash it should be less
> of an issue. If you're confident about being able to write 100%
> posix-compliant shell script, I would use [ instead of [[.
Shellcheck will check for posix compliance when scripts begin with
#!/bin/sh.
This might be another reason to just go back to importing the autoconf's
implementation:
http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=build-aux/git-version-gen
>> I should run shellcheck too :)
>> (Fixed those warnings too)
>>
>>>> +then
>>>> + cd "$1"
>>>> +fi
>>>> +
>>>> +# No fall back is provided for tarball releases or such, as we do not yet provide releases.
>>>
>>> Line break at 80 columns ?
>>
>> Fixed.
>>
>>> Tarball releases should just ship version.h.
>>
>> Sure, but the build system will have to know not to regenerate it.
>>
>> We'll sort that later.
>>
>>>> +
>>>> +# Get a short description from the tree.
>>>> +version=$(git describe --abbrev=4 --match "v[0-9]*" 2>/dev/null)
>>>> +
>>>> +if test -z "$version"
>>>> +then
>>>
>>> if [[ -z "$version" ]] ; then
>>>
>>>> + # Handle an un-tagged repository
>>>> + sha=$(git describe --abbrev=4 --always 2>/dev/null)
>>>> + commits=$(git log --oneline | wc -l 2>/dev/null)
>>>> + version=v0.0.$commits.$sha
>>>> +fi
>>>> +
>>>> +
>>>> +git update-index --refresh > /dev/null 2>&1
>>>
>>> What does this do ?
>>
>> Prevents changed timestamps causing a -dirty label. I've added a comment.
>>
>>>> +dirty=`sh -c 'git diff-index --name-only HEAD' 2>/dev/null` || dirty=
>>>
>>> $(...) instead of `...`
>>>
>>>> +
>>>> +# Strip the 'g'
>>>
>>> Anything wrong with the 'g' ? :-)
>>
>> Would you like to keep it?
>
> I don't mind either way, it's included in the Linux kernel version so I
> got used to it, I think it would confuse me a bit not to have it.
The 'methodology' I was following here was from the gnu git-version-gen
which removes the 'g', as does the original "GIT-VERSION-GEN" from git
itself.
I had wanted to simplify things, hence splitting out our needs - but now
I'm thinking it's better just to use the external script.
I'll look again and decide before reposting, but I'll re-back-burner
this task for this week. It's consumed too many cycles already.
>>>> +version=$(echo "$version" | sed -e 's/-g/-/g')
>>>> +
>>>> +# Fix the '-' (the patch count) to a '.' as a version increment.
>>>> +version=$(echo "$version" | sed -e 's/-/./g')
>>>> +
>>>> +if test -n "$dirty"
>>>> +then
>>>
>>> if [[ -n "$dirty" ]] ; then
>>>
>>>> + version=$version-dirty
>>>> +fi
>>>> +
>>>> +echo $version
>>>> diff --git a/version.h.in b/version.h.in
>>>> new file mode 100644
>>>> index 000000000000..142819645b64
>>>> --- /dev/null
>>>> +++ b/version.h.in
>>>> @@ -0,0 +1,3 @@
>>>
>>> Should this have a comment stating that the file is auto-generated ?
>>
>> Added.
>>
>>>> +#pragma once
>>>
>>> We usually use #ifdef #define #endif, is there a specific reason to use
>>> a #pragma instead ?
>>
>> Probably just because of the sample vcs_tag generator file I looked
>> at... It was in January so I don't recall anything specific.
>> (I'm trying to clear my backlog of misc patches)
>>
>> I'll change to #ifdefs
>>
>>>> +
>>>> +#define LIBCAMERA_VERSION "@VCS_TAG@"
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list