| Message ID | 20211013122312.1943362-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, (CC'ing Kieran) Thank you for the patch. On Wed, Oct 13, 2021 at 01:23:12PM +0100, Naushir Patuck wrote: > When distributions build and package libcamera libraries, they may not > necessarily run the build in the upstream source tree. In these cases, the git > SHA1 versioning information will be lost. > > This change addresses that problem by requiring package managers to run > 'meson dist' to create a tarball of the source files and build from there. > On runing 'meson dist', the utils/run-dist.sh script will create a version.gen > file in the release tarball with the version string generated from the existing > utils/gen-version.sh script. > > The utils/gen-version.sh script has been updated to check for the presence of > this version.gen file and read the version string from it instead of creating > one. I came across https://sources.debian.org/src/libsmpp34/1.14.0-2/git-version-gen/ which seems to store the version in a file named .tarball-version. Should we follow the same convention ? > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > meson.build | 3 +++ > src/libcamera/meson.build | 11 +++++------ > utils/gen-version.sh | 9 +++++++++ > utils/run-dist.sh | 11 +++++++++++ > 4 files changed, 28 insertions(+), 6 deletions(-) > create mode 100644 utils/run-dist.sh > > diff --git a/meson.build b/meson.build > index a49c484fe64e..85ca0013733e 100644 > --- a/meson.build > +++ b/meson.build > @@ -24,6 +24,9 @@ endif > > libcamera_version = libcamera_git_version.split('+')[0] > > +# This script gererates the version.gen file on a 'meson dist' command. > +meson.add_dist_script('utils/run-dist.sh') > + > # Configure the build environment. > cc = meson.get_compiler('c') > cxx = meson.get_compiler('cpp') > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index 243dd3c180eb..360eaf80ecf1 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -93,12 +93,11 @@ endforeach > > libcamera_sources += control_sources > > -gen_version = meson.source_root() / 'utils' / 'gen-version.sh' > - > -version_cpp = vcs_tag(command : [gen_version, meson.build_root()], > - input : 'version.cpp.in', > - output : 'version.cpp', > - fallback : meson.project_version()) > +version_data = configuration_data() > +version_data.set('VCS_TAG', libcamera_git_version) > +version_cpp = configure_file(input : 'version.cpp.in', > + output : 'version.cpp', > + configuration : version_data) This doesn't seem strictly needed, but it avoids running gen-version.sh a second time, which is nice. I however have a nagging feeling that there was a reason why we used vcs_tag and not configure_file, but I can't recall it. We'll find out if it causes issues :-) > > libcamera_sources += version_cpp > > diff --git a/utils/gen-version.sh b/utils/gen-version.sh > index b09ad495f86a..09cede84c25e 100755 > --- a/utils/gen-version.sh > +++ b/utils/gen-version.sh > @@ -5,6 +5,15 @@ > > build_dir="$1" > > +# If version.gen exists, output the version string from the file and exit. > +# This file is auto-generated on a 'meson dist' command from the run-dist.sh > +# script. > +if [ -f "${MESON_SOURCE_ROOT}"/version.gen ] > +then > + cat "${MESON_SOURCE_ROOT}"/version.gen > + exit 0 > +fi > + Shouldn't we do this only if we're not under git control ? There should be no version.gen file in that case of course, but isn't still a better default ? > # Bail out if the directory isn't under git control > src_dir=$(git rev-parse --git-dir 2>&1) || exit 1 > src_dir=$(readlink -f "$src_dir/..") > diff --git a/utils/run-dist.sh b/utils/run-dist.sh > new file mode 100644 > index 000000000000..3b6c0adb05ed > --- /dev/null > +++ b/utils/run-dist.sh > @@ -0,0 +1,11 @@ > +#!/bin/sh > + > +# SPDX-License-Identifier: GPL-2.0-or-later > +# > +# On a meson dist run, generate the version string and store it in a file. > +# This will later be picked up by the utils/gen-version.sh script and used > +# instead of re-generating it. This way, if we are not building in the upstream > +# git source tree, the upstream version information will be preserved. > + > +cd "$MESON_SOURCE_ROOT" || return > +./utils/gen-version.sh > "$MESON_DIST_ROOT"/version.gen Do we need to handle the case of meson dist being run from a tarball instead of git ? It seems like a corner case to me, so probably not very useful, but we could possibly support it by copying $MESON_SOURCE_ROOT/version.gen to $MESON_DIST_ROOT/ if the source file exists. I'm fine either way.
Hi Laurent, Thank you for your feedback. On Thu, 14 Oct 2021 at 01:46, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > (CC'ing Kieran) > > Thank you for the patch. > > On Wed, Oct 13, 2021 at 01:23:12PM +0100, Naushir Patuck wrote: > > When distributions build and package libcamera libraries, they may not > > necessarily run the build in the upstream source tree. In these cases, > the git > > SHA1 versioning information will be lost. > > > > This change addresses that problem by requiring package managers to run > > 'meson dist' to create a tarball of the source files and build from > there. > > On runing 'meson dist', the utils/run-dist.sh script will create a > version.gen > > file in the release tarball with the version string generated from the > existing > > utils/gen-version.sh script. > > > > The utils/gen-version.sh script has been updated to check for the > presence of > > this version.gen file and read the version string from it instead of > creating > > one. > > I came across > https://sources.debian.org/src/libsmpp34/1.14.0-2/git-version-gen/ which > seems to store the version in a file named .tarball-version. Should we > follow the same convention ? > Sure, I can rename it to .tarball-version. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > meson.build | 3 +++ > > src/libcamera/meson.build | 11 +++++------ > > utils/gen-version.sh | 9 +++++++++ > > utils/run-dist.sh | 11 +++++++++++ > > 4 files changed, 28 insertions(+), 6 deletions(-) > > create mode 100644 utils/run-dist.sh > > > > diff --git a/meson.build b/meson.build > > index a49c484fe64e..85ca0013733e 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -24,6 +24,9 @@ endif > > > > libcamera_version = libcamera_git_version.split('+')[0] > > > > +# This script gererates the version.gen file on a 'meson dist' command. > > +meson.add_dist_script('utils/run-dist.sh') > > + > > # Configure the build environment. > > cc = meson.get_compiler('c') > > cxx = meson.get_compiler('cpp') > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index 243dd3c180eb..360eaf80ecf1 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -93,12 +93,11 @@ endforeach > > > > libcamera_sources += control_sources > > > > -gen_version = meson.source_root() / 'utils' / 'gen-version.sh' > > - > > -version_cpp = vcs_tag(command : [gen_version, meson.build_root()], > > - input : 'version.cpp.in', > > - output : 'version.cpp', > > - fallback : meson.project_version()) > > +version_data = configuration_data() > > +version_data.set('VCS_TAG', libcamera_git_version) > > +version_cpp = configure_file(input : 'version.cpp.in', > > + output : 'version.cpp', > > + configuration : version_data) > > This doesn't seem strictly needed, but it avoids running gen-version.sh > a second time, which is nice. I however have a nagging feeling that > there was a reason why we used vcs_tag and not configure_file, but I > can't recall it. We'll find out if it causes issues :-) > There seems to be a subtle difference from my brief messing around with this. configure_file() will execute on a meson config/setup command, and vcs_tag will run on the ninja build command. I did have a problem with my change using the latter (I cannot recall exactly right now, but it was something to do with the $MESON_SOURCE_ROOT env variable not setup when running the gen-version script). I thought this change would be good as you do not run the script twice either. > > > > > libcamera_sources += version_cpp > > > > diff --git a/utils/gen-version.sh b/utils/gen-version.sh > > index b09ad495f86a..09cede84c25e 100755 > > --- a/utils/gen-version.sh > > +++ b/utils/gen-version.sh > > @@ -5,6 +5,15 @@ > > > > build_dir="$1" > > > > +# If version.gen exists, output the version string from the file and > exit. > > +# This file is auto-generated on a 'meson dist' command from the > run-dist.sh > > +# script. > > +if [ -f "${MESON_SOURCE_ROOT}"/version.gen ] > > +then > > + cat "${MESON_SOURCE_ROOT}"/version.gen > > + exit 0 > > +fi > > + > > Shouldn't we do this only if we're not under git control ? There should > be no version.gen file in that case of course, but isn't still a better > default ? > In the original github issue, the reporter had a flow where the tarball would be initialised as a new git repo, and so the sha values extracted were valid values, but had no relation to the upstream sha values. So for those cases, we have to check the file unconditionally. > > > # Bail out if the directory isn't under git control > > src_dir=$(git rev-parse --git-dir 2>&1) || exit 1 > > src_dir=$(readlink -f "$src_dir/..") > > diff --git a/utils/run-dist.sh b/utils/run-dist.sh > > new file mode 100644 > > index 000000000000..3b6c0adb05ed > > --- /dev/null > > +++ b/utils/run-dist.sh > > @@ -0,0 +1,11 @@ > > +#!/bin/sh > > + > > +# SPDX-License-Identifier: GPL-2.0-or-later > > +# > > +# On a meson dist run, generate the version string and store it in a > file. > > +# This will later be picked up by the utils/gen-version.sh script and > used > > +# instead of re-generating it. This way, if we are not building in the > upstream > > +# git source tree, the upstream version information will be preserved. > > + > > +cd "$MESON_SOURCE_ROOT" || return > > +./utils/gen-version.sh > "$MESON_DIST_ROOT"/version.gen > > Do we need to handle the case of meson dist being run from a tarball > instead of git ? It seems like a corner case to me, so probably not very > useful, but we could possibly support it by copying > $MESON_SOURCE_ROOT/version.gen to $MESON_DIST_ROOT/ if the source file > exists. I'm fine either way. > Good point. Although, I cannot see why meson dist should be run from a tarball and not the upstream git tree :-) Thanks, Naush > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On Thu, 14 Oct 2021 at 07:04, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi Laurent, > > Thank you for your feedback. > > On Thu, 14 Oct 2021 at 01:46, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > >> Hi Naush, >> >> (CC'ing Kieran) >> >> Thank you for the patch. >> >> On Wed, Oct 13, 2021 at 01:23:12PM +0100, Naushir Patuck wrote: >> > When distributions build and package libcamera libraries, they may not >> > necessarily run the build in the upstream source tree. In these cases, >> the git >> > SHA1 versioning information will be lost. >> > >> > This change addresses that problem by requiring package managers to run >> > 'meson dist' to create a tarball of the source files and build from >> there. >> > On runing 'meson dist', the utils/run-dist.sh script will create a >> version.gen >> > file in the release tarball with the version string generated from the >> existing >> > utils/gen-version.sh script. >> > >> > The utils/gen-version.sh script has been updated to check for the >> presence of >> > this version.gen file and read the version string from it instead of >> creating >> > one. >> >> I came across >> https://sources.debian.org/src/libsmpp34/1.14.0-2/git-version-gen/ which >> seems to store the version in a file named .tarball-version. Should we >> follow the same convention ? >> > > Sure, I can rename it to .tarball-version. > > >> >> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> > --- >> > meson.build | 3 +++ >> > src/libcamera/meson.build | 11 +++++------ >> > utils/gen-version.sh | 9 +++++++++ >> > utils/run-dist.sh | 11 +++++++++++ >> > 4 files changed, 28 insertions(+), 6 deletions(-) >> > create mode 100644 utils/run-dist.sh >> > >> > diff --git a/meson.build b/meson.build >> > index a49c484fe64e..85ca0013733e 100644 >> > --- a/meson.build >> > +++ b/meson.build >> > @@ -24,6 +24,9 @@ endif >> > >> > libcamera_version = libcamera_git_version.split('+')[0] >> > >> > +# This script gererates the version.gen file on a 'meson dist' command. >> > +meson.add_dist_script('utils/run-dist.sh') >> > + >> > # Configure the build environment. >> > cc = meson.get_compiler('c') >> > cxx = meson.get_compiler('cpp') >> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build >> > index 243dd3c180eb..360eaf80ecf1 100644 >> > --- a/src/libcamera/meson.build >> > +++ b/src/libcamera/meson.build >> > @@ -93,12 +93,11 @@ endforeach >> > >> > libcamera_sources += control_sources >> > >> > -gen_version = meson.source_root() / 'utils' / 'gen-version.sh' >> > - >> > -version_cpp = vcs_tag(command : [gen_version, meson.build_root()], >> > - input : 'version.cpp.in', >> > - output : 'version.cpp', >> > - fallback : meson.project_version()) >> > +version_data = configuration_data() >> > +version_data.set('VCS_TAG', libcamera_git_version) >> > +version_cpp = configure_file(input : 'version.cpp.in', >> > + output : 'version.cpp', >> > + configuration : version_data) >> >> This doesn't seem strictly needed, but it avoids running gen-version.sh >> a second time, which is nice. I however have a nagging feeling that >> there was a reason why we used vcs_tag and not configure_file, but I >> can't recall it. We'll find out if it causes issues :-) >> > > There seems to be a subtle difference from my brief messing around with > this. configure_file() will execute on a meson config/setup command, and > vcs_tag will run on the ninja build command. I did have a problem with my > change using the latter (I cannot recall exactly right now, but it was > something > to do with the $MESON_SOURCE_ROOT env variable not setup when running > the gen-version script). I thought this change would be good as you do > not run > the script twice either. > > >> >> > >> > libcamera_sources += version_cpp >> > >> > diff --git a/utils/gen-version.sh b/utils/gen-version.sh >> > index b09ad495f86a..09cede84c25e 100755 >> > --- a/utils/gen-version.sh >> > +++ b/utils/gen-version.sh >> > @@ -5,6 +5,15 @@ >> > >> > build_dir="$1" >> > >> > +# If version.gen exists, output the version string from the file and >> exit. >> > +# This file is auto-generated on a 'meson dist' command from the >> run-dist.sh >> > +# script. >> > +if [ -f "${MESON_SOURCE_ROOT}"/version.gen ] >> > +then >> > + cat "${MESON_SOURCE_ROOT}"/version.gen >> > + exit 0 >> > +fi >> > + >> >> Shouldn't we do this only if we're not under git control ? There should >> be no version.gen file in that case of course, but isn't still a better >> default ? >> > > In the original github issue, the reporter had a flow where the tarball > would be initialised as a new git repo, and so the sha values extracted > were valid values, but had no relation to the upstream sha values. So for > those cases, we have to check the file unconditionally. > > >> >> > # Bail out if the directory isn't under git control >> > src_dir=$(git rev-parse --git-dir 2>&1) || exit 1 >> > src_dir=$(readlink -f "$src_dir/..") >> > diff --git a/utils/run-dist.sh b/utils/run-dist.sh >> > new file mode 100644 >> > index 000000000000..3b6c0adb05ed >> > --- /dev/null >> > +++ b/utils/run-dist.sh >> > @@ -0,0 +1,11 @@ >> > +#!/bin/sh >> > + >> > +# SPDX-License-Identifier: GPL-2.0-or-later >> > +# >> > +# On a meson dist run, generate the version string and store it in a >> file. >> > +# This will later be picked up by the utils/gen-version.sh script and >> used >> > +# instead of re-generating it. This way, if we are not building in the >> upstream >> > +# git source tree, the upstream version information will be preserved. >> > + >> > +cd "$MESON_SOURCE_ROOT" || return >> > +./utils/gen-version.sh > "$MESON_DIST_ROOT"/version.gen >> >> Do we need to handle the case of meson dist being run from a tarball >> instead of git ? It seems like a corner case to me, so probably not very >> useful, but we could possibly support it by copying >> $MESON_SOURCE_ROOT/version.gen to $MESON_DIST_ROOT/ if the source file >> exists. I'm fine either way. >> > > Good point. Although, I cannot see why meson dist should be run from a > tarball and not the upstream git tree :-) > So it turns out the above change is not needed after all: $ meson dist --no-tests Dist currently only works with Git or Mercurial repos Regards, Naush
Hi Naush, On Thu, Oct 14, 2021 at 07:04:40AM +0100, Naushir Patuck wrote: > On Thu, 14 Oct 2021 at 01:46, Laurent Pinchart wrote: > > > Hi Naush, > > > > (CC'ing Kieran) > > > > Thank you for the patch. > > > > On Wed, Oct 13, 2021 at 01:23:12PM +0100, Naushir Patuck wrote: > > > When distributions build and package libcamera libraries, they may not > > > necessarily run the build in the upstream source tree. In these cases, the git > > > SHA1 versioning information will be lost. > > > > > > This change addresses that problem by requiring package managers to run > > > 'meson dist' to create a tarball of the source files and build from there. > > > On runing 'meson dist', the utils/run-dist.sh script will create a version.gen > > > file in the release tarball with the version string generated from the existing > > > utils/gen-version.sh script. > > > > > > The utils/gen-version.sh script has been updated to check for the presence of > > > this version.gen file and read the version string from it instead of creating > > > one. > > > > I came across > > https://sources.debian.org/src/libsmpp34/1.14.0-2/git-version-gen/ which > > seems to store the version in a file named .tarball-version. Should we > > follow the same convention ? > > Sure, I can rename it to .tarball-version. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > meson.build | 3 +++ > > > src/libcamera/meson.build | 11 +++++------ > > > utils/gen-version.sh | 9 +++++++++ > > > utils/run-dist.sh | 11 +++++++++++ > > > 4 files changed, 28 insertions(+), 6 deletions(-) > > > create mode 100644 utils/run-dist.sh > > > > > > diff --git a/meson.build b/meson.build > > > index a49c484fe64e..85ca0013733e 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -24,6 +24,9 @@ endif > > > > > > libcamera_version = libcamera_git_version.split('+')[0] > > > > > > +# This script gererates the version.gen file on a 'meson dist' command. > > > +meson.add_dist_script('utils/run-dist.sh') > > > + > > > # Configure the build environment. > > > cc = meson.get_compiler('c') > > > cxx = meson.get_compiler('cpp') > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > index 243dd3c180eb..360eaf80ecf1 100644 > > > --- a/src/libcamera/meson.build > > > +++ b/src/libcamera/meson.build > > > @@ -93,12 +93,11 @@ endforeach > > > > > > libcamera_sources += control_sources > > > > > > -gen_version = meson.source_root() / 'utils' / 'gen-version.sh' > > > - > > > -version_cpp = vcs_tag(command : [gen_version, meson.build_root()], > > > - input : 'version.cpp.in', > > > - output : 'version.cpp', > > > - fallback : meson.project_version()) > > > +version_data = configuration_data() > > > +version_data.set('VCS_TAG', libcamera_git_version) > > > +version_cpp = configure_file(input : 'version.cpp.in', > > > + output : 'version.cpp', > > > + configuration : version_data) > > > > This doesn't seem strictly needed, but it avoids running gen-version.sh > > a second time, which is nice. I however have a nagging feeling that > > there was a reason why we used vcs_tag and not configure_file, but I > > can't recall it. We'll find out if it causes issues :-) > > There seems to be a subtle difference from my brief messing around with > this. configure_file() will execute on a meson config/setup command, and > vcs_tag will run on the ninja build command. I did have a problem with my > change using the latter (I cannot recall exactly right now, but it was something > to do with the $MESON_SOURCE_ROOT env variable not setup when running > the gen-version script). I thought this change would be good as you do not run > the script twice either. Doesn't this mean that we'll record the wrong version if one adds a new commit and recompiles without reconfiguring ? Adding the SHA1 to version.cpp, and printing it in the log, is meant to help catching issues when libcamera is being deployed to the wrong directory by mistake during development, so I think this is a very important use case. > > > libcamera_sources += version_cpp > > > > > > diff --git a/utils/gen-version.sh b/utils/gen-version.sh > > > index b09ad495f86a..09cede84c25e 100755 > > > --- a/utils/gen-version.sh > > > +++ b/utils/gen-version.sh > > > @@ -5,6 +5,15 @@ > > > > > > build_dir="$1" > > > > > > +# If version.gen exists, output the version string from the file and exit. > > > +# This file is auto-generated on a 'meson dist' command from the run-dist.sh > > > +# script. > > > +if [ -f "${MESON_SOURCE_ROOT}"/version.gen ] > > > +then > > > + cat "${MESON_SOURCE_ROOT}"/version.gen > > > + exit 0 > > > +fi > > > + > > > > Shouldn't we do this only if we're not under git control ? There should > > be no version.gen file in that case of course, but isn't still a better > > default ? > > In the original github issue, the reporter had a flow where the tarball > would be initialised as a new git repo, and so the sha values extracted > were valid values, but had no relation to the upstream sha values. So for > those cases, we have to check the file unconditionally. > > > > # Bail out if the directory isn't under git control > > > src_dir=$(git rev-parse --git-dir 2>&1) || exit 1 > > > src_dir=$(readlink -f "$src_dir/..") > > > diff --git a/utils/run-dist.sh b/utils/run-dist.sh > > > new file mode 100644 > > > index 000000000000..3b6c0adb05ed > > > --- /dev/null > > > +++ b/utils/run-dist.sh > > > @@ -0,0 +1,11 @@ > > > +#!/bin/sh > > > + > > > +# SPDX-License-Identifier: GPL-2.0-or-later > > > +# > > > +# On a meson dist run, generate the version string and store it in a file. > > > +# This will later be picked up by the utils/gen-version.sh script and used > > > +# instead of re-generating it. This way, if we are not building in the upstream > > > +# git source tree, the upstream version information will be preserved. > > > + > > > +cd "$MESON_SOURCE_ROOT" || return > > > +./utils/gen-version.sh > "$MESON_DIST_ROOT"/version.gen > > > > Do we need to handle the case of meson dist being run from a tarball > > instead of git ? It seems like a corner case to me, so probably not very > > useful, but we could possibly support it by copying > > $MESON_SOURCE_ROOT/version.gen to $MESON_DIST_ROOT/ if the source file > > exists. I'm fine either way. > > Good point. Although, I cannot see why meson dist should be run from a > tarball and not the upstream git tree :-)
On Thu, 14 Oct 2021 at 11:21, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Thu, Oct 14, 2021 at 07:04:40AM +0100, Naushir Patuck wrote: > > On Thu, 14 Oct 2021 at 01:46, Laurent Pinchart wrote: > > > > > Hi Naush, > > > > > > (CC'ing Kieran) > > > > > > Thank you for the patch. > > > > > > On Wed, Oct 13, 2021 at 01:23:12PM +0100, Naushir Patuck wrote: > > > > When distributions build and package libcamera libraries, they may > not > > > > necessarily run the build in the upstream source tree. In these > cases, the git > > > > SHA1 versioning information will be lost. > > > > > > > > This change addresses that problem by requiring package managers to > run > > > > 'meson dist' to create a tarball of the source files and build from > there. > > > > On runing 'meson dist', the utils/run-dist.sh script will create a > version.gen > > > > file in the release tarball with the version string generated from > the existing > > > > utils/gen-version.sh script. > > > > > > > > The utils/gen-version.sh script has been updated to check for the > presence of > > > > this version.gen file and read the version string from it instead of > creating > > > > one. > > > > > > I came across > > > https://sources.debian.org/src/libsmpp34/1.14.0-2/git-version-gen/ > which > > > seems to store the version in a file named .tarball-version. Should we > > > follow the same convention ? > > > > Sure, I can rename it to .tarball-version. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > meson.build | 3 +++ > > > > src/libcamera/meson.build | 11 +++++------ > > > > utils/gen-version.sh | 9 +++++++++ > > > > utils/run-dist.sh | 11 +++++++++++ > > > > 4 files changed, 28 insertions(+), 6 deletions(-) > > > > create mode 100644 utils/run-dist.sh > > > > > > > > diff --git a/meson.build b/meson.build > > > > index a49c484fe64e..85ca0013733e 100644 > > > > --- a/meson.build > > > > +++ b/meson.build > > > > @@ -24,6 +24,9 @@ endif > > > > > > > > libcamera_version = libcamera_git_version.split('+')[0] > > > > > > > > +# This script gererates the version.gen file on a 'meson dist' > command. > > > > +meson.add_dist_script('utils/run-dist.sh') > > > > + > > > > # Configure the build environment. > > > > cc = meson.get_compiler('c') > > > > cxx = meson.get_compiler('cpp') > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > > index 243dd3c180eb..360eaf80ecf1 100644 > > > > --- a/src/libcamera/meson.build > > > > +++ b/src/libcamera/meson.build > > > > @@ -93,12 +93,11 @@ endforeach > > > > > > > > libcamera_sources += control_sources > > > > > > > > -gen_version = meson.source_root() / 'utils' / 'gen-version.sh' > > > > - > > > > -version_cpp = vcs_tag(command : [gen_version, meson.build_root()], > > > > - input : 'version.cpp.in', > > > > - output : 'version.cpp', > > > > - fallback : meson.project_version()) > > > > +version_data = configuration_data() > > > > +version_data.set('VCS_TAG', libcamera_git_version) > > > > +version_cpp = configure_file(input : 'version.cpp.in', > > > > + output : 'version.cpp', > > > > + configuration : version_data) > > > > > > This doesn't seem strictly needed, but it avoids running gen-version.sh > > > a second time, which is nice. I however have a nagging feeling that > > > there was a reason why we used vcs_tag and not configure_file, but I > > > can't recall it. We'll find out if it causes issues :-) > > > > There seems to be a subtle difference from my brief messing around with > > this. configure_file() will execute on a meson config/setup command, and > > vcs_tag will run on the ninja build command. I did have a problem with > my > > change using the latter (I cannot recall exactly right now, but it was > something > > to do with the $MESON_SOURCE_ROOT env variable not setup when running > > the gen-version script). I thought this change would be good as you do > not run > > the script twice either. > > Doesn't this mean that we'll record the wrong version if one adds a new > commit and recompiles without reconfiguring ? Adding the SHA1 to > version.cpp, and printing it in the log, is meant to help catching > issues when libcamera is being deployed to the wrong directory by > mistake during development, so I think this is a very important use > case. > You are correct, that is a common use case that must be accounted for! Perhaps this was the reason the original code used vcs_tag :-) I'll revert this back and try to work through my issue I was seeing in another way. > > > > > libcamera_sources += version_cpp > > > > > > > > diff --git a/utils/gen-version.sh b/utils/gen-version.sh > > > > index b09ad495f86a..09cede84c25e 100755 > > > > --- a/utils/gen-version.sh > > > > +++ b/utils/gen-version.sh > > > > @@ -5,6 +5,15 @@ > > > > > > > > build_dir="$1" > > > > > > > > +# If version.gen exists, output the version string from the file > and exit. > > > > +# This file is auto-generated on a 'meson dist' command from the > run-dist.sh > > > > +# script. > > > > +if [ -f "${MESON_SOURCE_ROOT}"/version.gen ] > > > > +then > > > > + cat "${MESON_SOURCE_ROOT}"/version.gen > > > > + exit 0 > > > > +fi > > > > + > > > > > > Shouldn't we do this only if we're not under git control ? There should > > > be no version.gen file in that case of course, but isn't still a better > > > default ? > > > > In the original github issue, the reporter had a flow where the tarball > > would be initialised as a new git repo, and so the sha values extracted > > were valid values, but had no relation to the upstream sha values. So for > > those cases, we have to check the file unconditionally. > > > > > > # Bail out if the directory isn't under git control > > > > src_dir=$(git rev-parse --git-dir 2>&1) || exit 1 > > > > src_dir=$(readlink -f "$src_dir/..") > > > > diff --git a/utils/run-dist.sh b/utils/run-dist.sh > > > > new file mode 100644 > > > > index 000000000000..3b6c0adb05ed > > > > --- /dev/null > > > > +++ b/utils/run-dist.sh > > > > @@ -0,0 +1,11 @@ > > > > +#!/bin/sh > > > > + > > > > +# SPDX-License-Identifier: GPL-2.0-or-later > > > > +# > > > > +# On a meson dist run, generate the version string and store it in > a file. > > > > +# This will later be picked up by the utils/gen-version.sh script > and used > > > > +# instead of re-generating it. This way, if we are not building in > the upstream > > > > +# git source tree, the upstream version information will be > preserved. > > > > + > > > > +cd "$MESON_SOURCE_ROOT" || return > > > > +./utils/gen-version.sh > "$MESON_DIST_ROOT"/version.gen > > > > > > Do we need to handle the case of meson dist being run from a tarball > > > instead of git ? It seems like a corner case to me, so probably not > very > > > useful, but we could possibly support it by copying > > > $MESON_SOURCE_ROOT/version.gen to $MESON_DIST_ROOT/ if the source file > > > exists. I'm fine either way. > > > > Good point. Although, I cannot see why meson dist should be run from a > > tarball and not the upstream git tree :-) > > -- > Regards, > > Laurent Pinchart >