Re: [PATCH 6/6] meson: subprojects: replace submodules with wrap files

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, May 19, 2023 at 10:56:46AM +0200, Paolo Bonzini wrote:
> Compared to submodules, .wrap files have several advantages:
> 
> * option parsing and downloading is delegated to meson
> 
> * the commit is stored in a text file instead of a magic entry in the
>   git tree object

This avoids one of the big pain points with submodules, in that
when switching branches you often end up with dirty submodules,
which then get accidentally added to the commit. IIUC, with wrap,
when switching branches, meson should automatically update the
wrap to the right git hash and re-buld as needed. I think most
people would consider this along to be sufficient reason to
drop submodules.

> * we could stop shipping external dependencies that are only used as a
>   fallback, but not break compilation on platforms that lack them.
>   For example it may make sense to download dtc at build time,
>   controlled by --enable-download, even when building from a tarball.
>   This is _not_ done in this patch.

IIUC, you actually added code to make-release to avoid doing this.

Considering each lib

 * dtc - the distro should always have it anyway
 * libvfio-user - don't think this has found its way
                  into any distros yet. So if we don't
		  bundle it, distros have to pacakge it
		  or re-bundle with their QEMU bulds
 * keycodemapdb - has always been intended to be a copylib,
                  not packaged separately by distros. In
		  retrospect I think this wa probably a
		  mistake. None the less today, it will
		  need to be re-bundled by distros if we
		  omitted it.

I think there's a decent case to be made for 'dtc' to be dropped
from 'make-release', but keep the other two.

> dependency() can fall back to a wrap automatically.  However, this
> is only possible for libraries that come with a .pc file, and this
> is not very common for libfdt even though the upstream project in
> principle provides it; it also removes the control that we provide with
> --enable-fdt={system,internal}.  Therefore, the logic to pick system
> vs. internal libfdt is left untouched.
> 
> It is possible to use subprojects also for berkeley-softfloat-3
> and berkeley-testfloat-3, but this is left for later.
>mak 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .gitlab-ci.d/buildtest-template.yml |  1 +
>  .gitmodules                         |  9 -----
>  configure                           | 54 +++++------------------------
>  meson.build                         | 17 ++-------
>  meson_options.txt                   |  1 +
>  scripts/archive-source.sh           | 11 ++++--
>  scripts/make-release                |  5 +++
>  subprojects/.gitignore              |  3 ++
>  subprojects/dtc                     |  1 -
>  subprojects/dtc.wrap                |  4 +++
>  subprojects/keycodemapdb            |  1 -
>  subprojects/keycodemapdb.wrap       |  4 +++
>  subprojects/libvfio-user            |  1 -
>  subprojects/libvfio-user.wrap       |  4 +++
>  14 files changed, 41 insertions(+), 75 deletions(-)
>  delete mode 160000 subprojects/dtc
>  create mode 100644 subprojects/dtc.wrap
>  delete mode 160000 subprojects/keycodemapdb
>  create mode 100644 subprojects/keycodemapdb.wrap
>  delete mode 160000 subprojects/libvfio-user
>  create mode 100644 subprojects/libvfio-user.wrap
> 
> diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml
> index c9f2e737c09a..3c997d7265b2 100644
> --- a/.gitlab-ci.d/buildtest-template.yml
> +++ b/.gitlab-ci.d/buildtest-template.yml
> @@ -44,6 +44,7 @@
>    script:
>      - scripts/git-submodule.sh update
>          $(sed -n '/GIT_SUBMODULES=/ s/.*=// p' build/config-host.mak)
> +    - meson subprojects download $(cd build/subprojects && echo *)

Why is this addition needed ?  Isn't meson supposd to automatically
download the wrapped subprojects it needs without explicit user
action ?

> diff --git a/meson_options.txt b/meson_options.txt
> index 972c458b80b4..635a2bfd351f 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -296,6 +296,7 @@ option('capstone', type: 'feature', value: 'auto',
>         description: 'Whether and how to find the capstone library')
>  option('fdt', type: 'combo', value: 'auto',
>         choices: ['disabled', 'enabled', 'auto', 'system', 'internal'],
> +       deprecated: { 'git': 'internal' },
>         description: 'Whether and how to find the libfdt library')

Do we need to deprecate this, as opposed to removing it ?

We've considered build time options to not be subject to a deprecation
process, just documenting changes in the release notes.

>  
>  option('selinux', type: 'feature', value: 'auto',
> diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
> index dba5ae05b67e..b99cb66e4122 100755
> --- a/scripts/archive-source.sh
> +++ b/scripts/archive-source.sh
> @@ -26,8 +26,8 @@ sub_file="${sub_tdir}/submodule.tar"
>  # independent of what the developer currently has initialized
>  # in their checkout, because the build environment is completely
>  # different to the host OS.
> -submodules="subprojects/dtc subprojects/keycodemapdb"
> -submodules="$submodules tests/fp/berkeley-softfloat-3 tests/fp/berkeley-testfloat-3"
> +subprojects="dtc keycodemapdb libvfio-user"
> +submodules="tests/fp/berkeley-softfloat-3 tests/fp/berkeley-testfloat-3"
>  sub_deinit=""
>  
>  function cleanup() {
> @@ -70,4 +70,11 @@ for sm in $submodules; do
>      tar --concatenate --file "$tar_file" "$sub_file"
>      test $? -ne 0 && error "failed append submodule $sm to $tar_file"
>  done
> +
> +for sp in $subprojects; do
> +    meson subprojects download $sp
> +    test $? -ne 0 && error "failed to download subproject $sp"
> +    tar --append --file "$tar_file" --exclude=.git subprojects/$sp
> +    test $? -ne 0 && error "failed to append subproject $sp to $tar_file"
> +done
>  exit 0
> diff --git a/scripts/make-release b/scripts/make-release
> index 44a9d86a04a7..0604e61b8143 100755
> --- a/scripts/make-release
> +++ b/scripts/make-release
> @@ -16,6 +16,9 @@ if [ $# -ne 2 ]; then
>      exit 0
>  fi
>  
> +# Only include wraps that are invoked with subproject()
> +SUBPROJECTS="dtc libvfio-user keycodemapdb"
> +
>  src="$1"
>  version="$2"
>  destination=qemu-${version}
> @@ -26,6 +29,8 @@ git clone --single-branch -b "v${version}" -c advice.detachedHead=false \
>  pushd ${destination}
>  
>  git submodule update --init --single-branch
> +meson subprojects download $SUBPROJECTS
> +
>  (cd roms/seabios && git describe --tags --long --dirty > .version)
>  (cd roms/skiboot && ./make_version.sh > .version)
>  # Fetch edk2 submodule's submodules, since it won't have access to them via

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|