[libcamera-devel] [PATCH v2] libcamera: Rework automatic version generation to avoid rebuilds
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jul 9 10:48:37 CEST 2019
Hi Kieran,
On Mon, Jul 08, 2019 at 04:05:22PM +0100, Kieran Bingham wrote:
> 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...
It indeed seems incorrect. I've removed the extra $ and it still works
for me, but it didn't break in the first place :-)
> > 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.
I'm not opposed to that, but I think we should do so on top of this
patch.
> > 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?
The default fallback was introduced in meson > 0.40.
> > +
> > +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.
You're right, I'll change that back. I think I made the change in an
intermediate refactoring step, and it's not needed anymore.
> > 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.
Sure. I think it will be a small change if applied on top of this one.
> > # 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list