[libcamera-devel] [PATCH v2] libcamera: Rework automatic version generation to avoid rebuilds
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 9 10:43:39 CEST 2019
Hi Laurent,
Please also note the following shell-check warnings:
> shellcheck ./include/libcamera/gen-header.sh
>
> In ./include/libcamera/gen-header.sh line 19:
> headers=$(for header in $"$src_dir"/*.h ; do
> ^-- SC2039: In POSIX sh, $".." is undefined.
>
>
> In ./include/libcamera/gen-header.sh line 21:
> echo $header
> ^-- SC2086: Double quote to prevent globbing and word splitting.
>
>
> In ./include/libcamera/gen-header.sh line 24:
> for header in $(echo $headers) ; do
> ^-- SC2116: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'.
> ^-- SC2086: Double quote to prevent globbing and word splitting.
>
--
Regards
Kieran
On 08/07/2019 16:05, Kieran Bingham wrote:
> Hi Laurent,
>
> On 08/07/2019 14:12, Laurent Pinchart wrote:
>> Commit b817bcec6b53 ("libcamera: Auto generate version information")
>> generates version information in order to automatically include it
>> various locations (Sphinx and Doxygen documentation, libcamera::version
>> variable available at runtime, and version.h available at compile time).
>> Unfortunately this causes lots of unnecessary rebuilds when modifying
>> the git tree state, which hinders development.
>>
>> The problem is caused by the generated version.h being listed as a
>> dependency for the whole libcamera. This is required as meson (to the
>> best of my knowledge) doesn't provide a way to explicitly specify the
>> dependency of a single object file (camera_manager.o in this case, as
>> camera_manager.cpp is the only consumer of the generated version string)
>> on the custom target used to generate version.h. The dependency can't be
>> automatically detected at build time, like dependencies on normal
>> headers that are generated by parsing the source, because the version.h
>> header may not exist yet. The build could then fail in a racy way.
>>
>> This change attempts at solving the issue by generating a version.cpp
>> instead of a version.h to set the git-based version. This minimises the
>> number of files that need to be rebuild when then git tree state
>> changes, while retaining the main purpose of the original automatic
>> version generation, the ability to access the git-based version string
>> at runtime. We however lose the ability to access git-based version
>> information at build time in an application building against libcamera,
>> but there is no expected use case for this.
>>
>> On the other hand, major, minor and patch level version numbers are
>> useful at build time. This commit changes the generation of version.h in
>> order to add three macros named LIBCAMERA_VERSION_MAJOR,
>> LIBCAMERA_VERSION_MINOR and LIBCAMERA_VERSION_PATCH for this purpose.
>> version.h is not included by any other libcamera header or source file,
>> and thus doesn't force a rebuild of the library.
>>
>> The Sphinx and Doxygen documentation keep their git-based version
>> information, which is set during the configuration of the build and then
>> doesn't track git commits. We may want to investigate how to improve
>> this, but given that git-based version for the documentation has very
>> few use cases outside of tagging nightly builds, this isn't considered
>> an issue at the moment.
>>
>> The documentation install directory now uses the base version string, in
>> order to avoid increasing the number of documentation directories
>> needlessly. This shouldn't cause any issue as the API should not change
>> without a change to the version number.
>>
>> The version number generation and handling code now also standardises
>> the version variables to not start with a 'v' prefix in meson, in order
>> to simplify their handling. The prefix is added when generating the
>> relevant files.
>>
>> Note that we go back to specifying the fallback version in the main
>> meson.build, in the call to the project() function. For the time being I
>> believe this should be a good compromise to avoid unnecessary
>> recompilation, and moving the fallback version to a different file for
>> tarball releases can be built on top of this.
>>
>> Fixes: b817bcec6b53 ("libcamera: Auto generate version information")
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> When I apply your patch, or build from your branch I get the following:
>
>
> In file included from ../src/cam/main.cpp:13:
> include/libcamera/libcamera.h:11:10: fatal error: libcamera/config.h: No
> such file or directory
> #include <libcamera/config.h>
>
>
> But that seems unrelated???
>
> Perhaps it's uncovered some race or lack of dependency from a previous
> commit.
>
> I've removed my entire build directory, and I still get the config.h
> fault ... hrm.
>
> In fact, gen-header.sh seems to be broken for me:
>
> cat build/include/libcamera/libcamera.h
> /* SPDX-License-Identifier: LGPL-2.1-or-later */
> /* This file is auto-generated, do not edit! */
> /*
> * Copyright (C) 2018-2019, Google Inc.
> *
> * libcamera.h - libcamera public API
> */
> #ifndef __LIBCAMERA_LIBCAMERA_H__
> #define __LIBCAMERA_LIBCAMERA_H__
>
> #include <libcamera/config.h>
> #include <libcamera/version.h>
>
> #endif /* __LIBCAMERA_LIBCAMERA_H__ */
>
>
>
>> ---
>> Documentation/meson.build | 6 +++---
>> include/libcamera/camera_manager.h | 3 +++
>> include/libcamera/gen-header.sh | 6 +++++-
>> include/libcamera/meson.build | 18 +++++++++++-------
>> include/libcamera/version.h.in | 12 +++---------
>> meson.build | 19 ++++++++++++++++---
>> src/libcamera/camera_manager.cpp | 14 +++++++-------
>> src/libcamera/meson.build | 10 +++++++++-
>> src/libcamera/version.cpp.in | 16 ++++++++++++++++
>> src/qcam/main_window.cpp | 2 +-
>> utils/gen-version.sh | 23 ++++++++++-------------
>> 11 files changed, 84 insertions(+), 45 deletions(-)
>> create mode 100644 src/libcamera/version.cpp.in
>>
>> diff --git a/Documentation/meson.build b/Documentation/meson.build
>> index c355d5feb504..b1720b05f5ee 100644
>> --- a/Documentation/meson.build
>> +++ b/Documentation/meson.build
>> @@ -1,5 +1,5 @@
>> doc_install_dir = join_paths(get_option('datadir'), 'doc',
>> - 'libcamera- at 0@'.format(meson.project_version()))
>> + 'libcamera- at 0@'.format(libcamera_version))
>>
>> #
>> # Doxygen
>> @@ -9,7 +9,7 @@ doxygen = find_program('doxygen', required : false)
>>
>> if doxygen.found()
>> cdata = configuration_data()
>> - cdata.set('VERSION', meson.project_version())
>> + cdata.set('VERSION', 'v at 0@'.format(libcamera_git_version))
>> cdata.set('TOP_SRCDIR', meson.source_root())
>> cdata.set('TOP_BUILDDIR', meson.build_root())
>>
>> @@ -48,7 +48,7 @@ if sphinx.found()
>> 'index.rst',
>> ]
>>
>> - release = 'release=' + meson.project_version()
>> + release = 'release=v' + libcamera_git_version
>>
>> custom_target('documentation',
>> command : [sphinx, '-D', release, '-q', '-W', '-b', 'html',
>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
>> index cf3a85ae8fe3..633d27d17ebf 100644
>> --- a/include/libcamera/camera_manager.h
>> +++ b/include/libcamera/camera_manager.h
>> @@ -31,6 +31,7 @@ public:
>> void removeCamera(Camera *camera);
>>
>> static CameraManager *instance();
>> + static const std::string &version() { return version_; }
>>
>> void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
>> EventDispatcher *eventDispatcher();
>> @@ -46,6 +47,8 @@ private:
>> std::vector<std::shared_ptr<Camera>> cameras_;
>>
>> std::unique_ptr<EventDispatcher> dispatcher_;
>> +
>> + static const std::string version_;
>> };
>>
>> } /* namespace libcamera */
>> diff --git a/include/libcamera/gen-header.sh b/include/libcamera/gen-header.sh
>> index e171c08c20b8..ccd3414bd8ca 100755
>> --- a/include/libcamera/gen-header.sh
>> +++ b/include/libcamera/gen-header.sh
>> @@ -16,8 +16,12 @@ cat <<EOF > "$dst_file"
>>
>> EOF
>>
>> -for header in "$src_dir"/*.h ; do
>> +headers=$(for header in $"$src_dir"/*.h ; do
>
> I wonder if that extra $ is to blame...
>
>
>> header=$(basename "$header")
>> + echo $header
>> +done ; echo "version.h" | sort)
>> +
>> +for header in $(echo $headers) ; do
>> echo "#include <libcamera/$header>" >> "$dst_file"
>> done
>>
>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>> index 6f81f1117318..972513fc2df5 100644
>> --- a/include/libcamera/meson.build
>> +++ b/include/libcamera/meson.build
>> @@ -16,13 +16,6 @@ libcamera_api = files([
>> 'timer.h',
>> ])
>>
>> -gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')
>> -
>> -version_h = vcs_tag(command : [gen_version, meson.current_source_dir()],
>> - input : 'version.h.in',
>> - output : 'version.h',
>> - fallback : 'v0.0')
>> -
>> gen_header = files('gen-header.sh')
>>
>> libcamera_h = custom_target('gen-header',
>> @@ -32,5 +25,16 @@ libcamera_h = custom_target('gen-header',
>> install : true,
>> install_dir : 'include/libcamera')
>>
>> +version = libcamera_version.split('.')
>> +libcamera_version_config = configuration_data()
>> +libcamera_version_config.set('LIBCAMERA_VERSION_MAJOR', version[0])
>> +libcamera_version_config.set('LIBCAMERA_VERSION_MINOR', version[1])
>> +libcamera_version_config.set('LIBCAMERA_VERSION_PATCH', version[2])
>> +
>> +configure_file(input : 'version.h.in',
>> + output : 'version.h',
>> + configuration : libcamera_version_config,
>> + install_dir : 'include/libcamera')
>> +
>> install_headers(libcamera_api,
>> subdir : 'libcamera')
>> diff --git a/include/libcamera/version.h.in b/include/libcamera/version.h.in
>> index e49b36962aed..5e9a30911d12 100644
>> --- a/include/libcamera/version.h.in
>> +++ b/include/libcamera/version.h.in
>> @@ -9,14 +9,8 @@
>> #ifndef __LIBCAMERA_VERSION_H__
>> #define __LIBCAMERA_VERSION_H__
>>
>> -#include <string>
>> -
>> -#define LIBCAMERA_VERSION "@VCS_TAG@"
>> -
>> -namespace libcamera {
>> -
>> -extern const std::string version;
>> -
>> -} /* namespace libcamera */
>> +#define LIBCAMERA_VERSION_MAJOR @LIBCAMERA_VERSION_MAJOR@
>> +#define LIBCAMERA_VERSION_MINOR @LIBCAMERA_VERSION_MINOR@
>> +#define LIBCAMERA_VERSION_PATCH @LIBCAMERA_VERSION_PATCH@
>>
>> #endif /* __LIBCAMERA_VERSION_H__ */
>> diff --git a/meson.build b/meson.build
>> index 342b3cc76a93..8f3d0ce91cb8 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -1,8 +1,6 @@
>> project('libcamera', 'c', 'cpp',
>> meson_version : '>= 0.40',
>> - version : run_command('utils/gen-version.sh',
>> - '@0@'.format(meson.source_root()),
>> - check : true).stdout().strip(),
>> + version : '0.0.0',
>
> I dislike not being able to populate the meson.project_version().
>
> I still think that fallback should be handled in the gen-version.sh script.
>
>
>
>
>> default_options : [
>> 'werror=true',
>> 'warning_level=2',
>> @@ -10,6 +8,21 @@ project('libcamera', 'c', 'cpp',
>> ],
>> license : 'LGPL 2.1+')
>>
>> +# Generate version information. The libcamera_git_version variable contains the
>> +# full version with git patch count and SHA1 (e.g. 1.2.3+211-c94a24f4), while
>> +# the libcamera_version variable contains the major.minor.patch (e.g. 1.2.3)
>> +# only. If the source tree isn't under git control, or if it matches the last
>> +# 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()
>> +if libcamera_git_version == ''
>> + libcamera_git_version = meson.project_version()
>
> ...
>
>> +endif
>> +
>> +libcamera_version = libcamera_git_version.split('+')[0]
>> +
>> +# Configure the build environment.
>> cc = meson.get_compiler('c')
>> config_h = configuration_data()
>>
>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>> index c5da46b4062d..337496c21cfc 100644
>> --- a/src/libcamera/camera_manager.cpp
>> +++ b/src/libcamera/camera_manager.cpp
>> @@ -9,7 +9,6 @@
>>
>> #include <libcamera/camera.h>
>> #include <libcamera/event_dispatcher.h>
>> -#include <libcamera/version.h>
>>
>> #include "device_enumerator.h"
>> #include "event_dispatcher_poll.h"
>> @@ -26,11 +25,6 @@ namespace libcamera {
>>
>> LOG_DEFINE_CATEGORY(Camera)
>>
>> -/**
>> - * \brief The library global version string
>> - */
>> -const std::string version(LIBCAMERA_VERSION);
>> -
>> /**
>> * \class CameraManager
>> * \brief Provide access and manage all cameras in the system
>> @@ -85,7 +79,7 @@ int CameraManager::start()
>> if (enumerator_)
>> return -EBUSY;
>>
>> - LOG(Camera, Info) << "libcamera " << version;
>> + LOG(Camera, Info) << "libcamera " << version_;
>>
>> enumerator_ = DeviceEnumerator::create();
>> if (!enumerator_ || enumerator_->enumerate())
>> @@ -232,6 +226,12 @@ CameraManager *CameraManager::instance()
>> return &manager;
>> }
>>
>> +/**
>> + * \fn const std::string &CameraManager::version()
>> + * \brief Retrieve the libcamera version string
>> + * \return The libcamera version string
>> + */
>> +
>> /**
>> * \brief Set the event dispatcher
>> * \param[in] dispatcher Pointer to the event dispatcher
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 336f4f066fac..97ff86e2167f 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -79,8 +79,16 @@ control_types_cpp = custom_target('control_types_cpp',
>>
>> 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()],
>> + input : 'version.cpp.in',
>> + output : 'version.cpp',
>> + fallback : meson.project_version())
>
> No point specifying the fallback if that's the default fallback?
>
>> +
>> +libcamera_sources += version_cpp
>> +
>> libcamera_deps = [
>> - declare_dependency(sources : version_h),> cc.find_library('dl'),
>> libudev,
>> ]
>> diff --git a/src/libcamera/version.cpp.in b/src/libcamera/version.cpp.in
>> new file mode 100644
>> index 000000000000..5aec08a189f3
>> --- /dev/null
>> +++ b/src/libcamera/version.cpp.in
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * version.cpp - libcamera version
>> + *
>> + * This file is auto-generated. Do not edit.
>> + */
>> +
>> +#include <libcamera/camera_manager.h>
>> +
>> +namespace libcamera {
>> +
>> +const std::string CameraManager::version_("v at VCS_TAG@");
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>> index b2f3a1f324f2..907d2423ed15 100644
>> --- a/src/qcam/main_window.cpp
>> +++ b/src/qcam/main_window.cpp
>> @@ -26,7 +26,7 @@ MainWindow::MainWindow(const OptionsParser::Options &options)
>> {
>> int ret;
>>
>> - title_ = "QCam " + QString::fromStdString(libcamera::version);
>> + title_ = "QCam " + QString::fromStdString(CameraManager::version());
>
> Why do you prefer exposing the version in the CameraManager namespace,
> rather than libcamera?
>
> It's the version of libcamera, the CameraManager is not versioned
> differently from the rest of the library...
>
> Now that you're moving the version (back) to a version.cpp I don't see
> the need for it to be in the CameraManager namespace. I could have
> understood the reasoning while it was 'borrowing' the CameraManager C++
> object file perhaps...
>
>
> Not objecting to the namespace choice, I just haven't understood the
> rationale for the move.
>
>
>> setWindowTitle(title_);
>> connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
>>
>> diff --git a/utils/gen-version.sh b/utils/gen-version.sh
>> index b3003d7a80d3..1800d71642c0 100755
>> --- a/utils/gen-version.sh
>> +++ b/utils/gen-version.sh
>> @@ -8,6 +8,9 @@ then
>> cd "$1" 2>/dev/null || exit 1
>> fi
>>
>> +# Bail out if the directory isn't under git control
>> +git rev-parse --git-dir >/dev/null 2>&1 || exit 1
>> +
>
> This is where I believe the script should (in the future, or perhaps
> near future) identify the appropriate libcamera_version file and return
> that content.
>
> The file would be automatically generated when creating a release.
>
>
> I'll submit a patch to do all of this after you've got this one how you
> want it. I had always planned to do the 'release' stage after the
> initial versioning.
>
>
>
>> # Get a short description from the tree.
>> version=$(git describe --abbrev=8 --match "v[0-9]*" 2>/dev/null)
>>
>> @@ -16,22 +19,16 @@ then
>> # Handle an un-tagged repository
>> sha=$(git describe --abbrev=8 --always 2>/dev/null)
>> commits=$(git log --oneline | wc -l 2>/dev/null)
>> - version=v0.0.$commits.$sha
>> + version="v0.0.0-$commits-g$sha"
>> fi
>>
>> -# Prevent changed timestamps causing -dirty labels
>> +# Append a '-dirty' suffix is the working tree is dirty. Prevent false
>
> s/suffix is/suffix if/
>
>> +# positives due to changed timestamps by running git update-index.
>> git update-index --refresh > /dev/null 2>&1
>> -dirty=$(git diff-index --name-only HEAD 2>/dev/null) || dirty=
>> +git diff-index --quiet HEAD || version="$version-dirty"
>>
>> -# Strip the 'g', and replace the preceeding '-' with a '+' to denote a label
>> -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 [ -n "$dirty" ]
>> -then
>> - version=$version-dirty
>> -fi
>> +# Replace first '-' with a '+' to denote build metadata, strip the 'g' in from
>> +# of the git SHA1 and remove the initial 'v'.
>> +version=$(echo "$version" | sed -e 's/-/+/' | sed -e 's/-g/-/' | cut -c 2-)
>
> At first this was a bit painful to read. It looks like you've removed
> the '-' and replaced with '+', which would then be used by the second
> sed, however I see that they only affect the first instance.
>
> Not really anything to change though, just me misreading I guess.
>
>
>>
>> echo "$version"
>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list