[libcamera-devel] [PATCH v2] libcamera: Rework automatic version generation to avoid rebuilds
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jul 9 11:11:54 CEST 2019
Hi Kieran,
On Tue, Jul 09, 2019 at 09:43:39AM +0100, Kieran Bingham wrote:
> 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.
This was on purpose to fix an issue I experienced with globbing and word
splitting, but it turns out I must have fixed something and it's not
needed anymore. I'll happily fix this, thanks :-)
> On 08/07/2019 16:05, 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...
> >
> >
> >> 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list