Re: [PATCH] Correctly use the bitbake variable S from now on

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2018-04-19 11:26, Henning Schild wrote:
> S is defined as ${WORKDIR}/${P} so it already contains ${WORKDIR} by its
> definition. In Isar it was often concatinated with WORKDIR again. One
> example where this was a problem is if you specified a patch in SRC_URI
> but did not actually overwrite S.
> Align the use of the variable with OE and bitbake defaults again.

Yeah, I missed that when migrating Isar from SRC_DIR to S.

> 
> Impact: layers building on top of Isar will have to adjust their recipes
> like the internal ones needed modification. The suggestion is to not set
> S and make sure to unpack to ${WORKDIR}/${P}. When setting S make sure
> to include §{WORKDIR}. This patch also introduces a warning and tries to
> preserve the old behaviour a bit. However if your recipe uses patches in
> SRC_URI and sets S you will get the warning and do_patch will fail.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  doc/user_manual.md                                        |  2 +-
>  meta-isar/recipes-app/example-hello/example-hello.bb      |  4 +---
>  meta-isar/recipes-app/libhello/libhello.bb                |  4 +---
>  meta-isar/recipes-kernel/example-module/example-module.bb |  2 +-
>  meta-isar/recipes-kernel/linux/linux-cip_4.4.bb           |  4 +---
>  meta-isar/recipes-kernel/linux/linux-mainline_4.14.18.bb  |  4 +---
>  meta/classes/dpkg-base.bbclass                            | 13 +++++++++++--
>  meta/classes/dpkg.bbclass                                 |  2 +-
>  meta/classes/patch.bbclass                                |  2 +-
>  meta/recipes-kernel/linux-module/module.inc               |  8 ++++----
>  meta/recipes-kernel/linux/linux-custom.inc                |  2 +-
>  11 files changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/doc/user_manual.md b/doc/user_manual.md
> index 2bd3793..3072bd5 100644
> --- a/doc/user_manual.md
> +++ b/doc/user_manual.md
> @@ -511,7 +511,7 @@ PV = "1.0"
>  SRC_URI = "git://github.com/ilbers/hello.git"
>  SRCREV = "ad7065ecc4840cc436bfcdac427386dbba4ea719"
>  
> -S = "git"
> +S = "${WORKDIR}/git"

Should we promote this pattern now or destsuffix?

>  
>  inherit dpkg
>  ```
> diff --git a/meta-isar/recipes-app/example-hello/example-hello.bb b/meta-isar/recipes-app/example-hello/example-hello.bb
> index 9788ec0..d23ee6c 100644
> --- a/meta-isar/recipes-app/example-hello/example-hello.bb
> +++ b/meta-isar/recipes-app/example-hello/example-hello.bb
> @@ -15,11 +15,9 @@ PV = "0.2-86cc719"
>  DEPENDS += "libhello"
>  
>  SRC_URI = " \
> -    git://github.com/ilbers/hello.git;protocol=https \
> +    git://github.com/ilbers/hello.git;protocol=https;destsuffix=${P} \
>      file://0001-Add-some-help.patch \
>      file://yet-another-change.txt;apply=yes;striplevel=0"
>  SRCREV = "86cc719b3359adc3c4e243387feba50360a860f3"
>  
> -S = "git"
> -
>  inherit dpkg
> diff --git a/meta-isar/recipes-app/libhello/libhello.bb b/meta-isar/recipes-app/libhello/libhello.bb
> index 1875831..4e75f98 100644
> --- a/meta-isar/recipes-app/libhello/libhello.bb
> +++ b/meta-isar/recipes-app/libhello/libhello.bb
> @@ -10,9 +10,7 @@ LIC_FILES_CHKSUM = "file://${LAYERDIR_isar}/licenses/COPYING.GPLv2;md5=751419260
>  
>  PV = "0.1-98f2e41"
>  
> -SRC_URI = "git://github.com/ilbers/libhello.git;protocol=https"
> +SRC_URI = "git://github.com/ilbers/libhello.git;protocol=https;destsuffix=${P}"
>  SRCREV = "98f2e41e7d05ab8d19b0c5d160b104b725c8fd93"
>  
> -S = "git"
> -
>  inherit dpkg
> diff --git a/meta-isar/recipes-kernel/example-module/example-module.bb b/meta-isar/recipes-kernel/example-module/example-module.bb
> index dbaf5ac..98d0aaa 100644
> --- a/meta-isar/recipes-kernel/example-module/example-module.bb
> +++ b/meta-isar/recipes-kernel/example-module/example-module.bb
> @@ -9,6 +9,6 @@ include recipes-kernel/linux-module/module.inc
>  
>  SRC_URI += "file://src"
>  
> -S = "src"
> +S = "${WORKDIR}/src"
>  
>  AUTOLOAD = "1"
> diff --git a/meta-isar/recipes-kernel/linux/linux-cip_4.4.bb b/meta-isar/recipes-kernel/linux/linux-cip_4.4.bb
> index 7502f70..e9aaa9f 100644
> --- a/meta-isar/recipes-kernel/linux/linux-cip_4.4.bb
> +++ b/meta-isar/recipes-kernel/linux/linux-cip_4.4.bb
> @@ -8,12 +8,10 @@
>  require recipes-kernel/linux/linux-custom.inc
>  
>  SRC_URI += " \
> -    git://git.kernel.org/pub/scm/linux/kernel/git/bwh/linux-cip.git;branch=linux-4.4.y-cip \
> +    git://git.kernel.org/pub/scm/linux/kernel/git/bwh/linux-cip.git;branch=linux-4.4.y-cip;destsuffix=${P} \
>      file://x86_64_defconfig"
>  
>  SRCREV = "4e52cc5f668c4666e31a8485725b5f4e897b3baf"
>  PV = "4.4.112-cip18"
>  
> -S = "git"
> -
>  KERNEL_DEFCONFIG_qemuamd64 = "x86_64_defconfig"
> diff --git a/meta-isar/recipes-kernel/linux/linux-mainline_4.14.18.bb b/meta-isar/recipes-kernel/linux/linux-mainline_4.14.18.bb
> index 2c93d40..751912f 100644
> --- a/meta-isar/recipes-kernel/linux/linux-mainline_4.14.18.bb
> +++ b/meta-isar/recipes-kernel/linux/linux-mainline_4.14.18.bb
> @@ -8,10 +8,8 @@
>  require recipes-kernel/linux/linux-custom.inc
>  
>  SRC_URI += " \
> -    https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-${PV}.tar.xz \
> +    https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-${PV}.tar.xz;subdir=${P} \
>      file://x86_64_defconfig"
>  SRC_URI[sha256sum] = "866a94c1c38d923ae18e74b683d7a8a79b674ebdfe7f40f1a3be9a27d39fe354"
>  
> -S = "linux-${PV}"
> -
>  KERNEL_DEFCONFIG_qemuamd64 = "x86_64_defconfig"
> diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass
> index 3c3a484..3845d8c 100644
> --- a/meta/classes/dpkg-base.bbclass
> +++ b/meta/classes/dpkg-base.bbclass
> @@ -2,8 +2,8 @@
>  # Copyright (C) 2017 Siemens AG
>  
>  do_adjust_git() {
> -    if [ -f ${WORKDIR}/${S}/.git/objects/info/alternates ]; then
> -        sed -i ${WORKDIR}/${S}/.git/objects/info/alternates \
> +    if [ -f ${S}/.git/objects/info/alternates ]; then
> +        sed -i ${S}/.git/objects/info/alternates \
>              -e 's|${DL_DIR}|/downloads|'
>      fi
>  }
> @@ -21,9 +21,18 @@ do_build[depends] = "buildchroot:do_build"
>  DEPENDS ?= ""
>  do_build[deptask] = "do_deploy_deb"
>  
> +def get_package_srcdir(d):
> +    s = d.getVar("S", True)
> +    workdir = d.getVar("WORKDIR", True)
> +    if s.startswith(workdir):
> +        return s[len(workdir)+1:]
> +    bb.warn('S does not start with WORKDIR')
> +    return s
> +
>  # Each package should have its own unique build folder, so use
>  # recipe name as identifier
>  PP = "/home/builder/${PN}"
> +PPS ?= "${@get_package_srcdir(d)}"
>  
>  BUILDROOT = "${BUILDCHROOT_DIR}/${PP}"
>  do_build[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
> diff --git a/meta/classes/dpkg.bbclass b/meta/classes/dpkg.bbclass
> index 06f0579..c8d4ac5 100644
> --- a/meta/classes/dpkg.bbclass
> +++ b/meta/classes/dpkg.bbclass
> @@ -6,5 +6,5 @@ inherit dpkg-base
>  # Build package from sources using build script
>  dpkg_runbuild() {
>      E="${@ bb.utils.export_proxies(d)}"
> -    sudo -E chroot ${BUILDCHROOT_DIR} /build.sh ${PP}/${S}
> +    sudo -E chroot ${BUILDCHROOT_DIR} /build.sh ${PP}/${PPS}
>  }
> diff --git a/meta/classes/patch.bbclass b/meta/classes/patch.bbclass
> index 26a0c81..1559359 100644
> --- a/meta/classes/patch.bbclass
> +++ b/meta/classes/patch.bbclass
> @@ -5,7 +5,7 @@ python do_patch() {
>      import subprocess
>  
>      workdir = d.getVar("WORKDIR", True) + "/"
> -    src_dir = workdir + (d.getVar("S", True) or "")
> +    src_dir = (d.getVar("S", True) or "")

That's not equivalent, but I'm undecided if it matters. S should never
be unset (bitbake.conf holds the default). So we could just pull S and
let the build fail if that assumption should ever be wrong.

>  
>      for src_uri in (d.getVar("SRC_URI", True) or "").split():
>          try:
> diff --git a/meta/recipes-kernel/linux-module/module.inc b/meta/recipes-kernel/linux-module/module.inc
> index ec1c4b0..3075f44 100644
> --- a/meta/recipes-kernel/linux-module/module.inc
> +++ b/meta/recipes-kernel/linux-module/module.inc
> @@ -18,14 +18,14 @@ AUTOLOAD ?= "0"
>  inherit dpkg
>  
>  dpkg_runbuild_prepend() {
> -    cp -r ${WORKDIR}/debian ${WORKDIR}/${S}/
> +    cp -r ${WORKDIR}/debian ${S}/
>      sed -i -e 's/@PN@/${PN}/g' -e 's/@PV@/${PV}/g' \
>             -e 's/@KERNEL_NAME@/${KERNEL_NAME}/g' \
>             -e 's/@DESCRIPTION@/${DESCRIPTION}/g' \
> -        ${WORKDIR}/${S}/debian/changelog ${WORKDIR}/${S}/debian/control
> +        ${S}/debian/changelog ${S}/debian/control
>  
>      if [ ${AUTOLOAD} = "1" ]; then
> -        echo "echo ${PN} >> /etc/modules" >> ${WORKDIR}/${S}/debian/postinst
> -        chmod +x ${WORKDIR}/${S}/debian/postinst
> +        echo "echo ${PN} >> /etc/modules" >> ${S}/debian/postinst
> +        chmod +x ${S}/debian/postinst
>      fi
>  }
> diff --git a/meta/recipes-kernel/linux/linux-custom.inc b/meta/recipes-kernel/linux/linux-custom.inc
> index 0498dfa..2643fec 100644
> --- a/meta/recipes-kernel/linux/linux-custom.inc
> +++ b/meta/recipes-kernel/linux/linux-custom.inc
> @@ -32,7 +32,7 @@ dpkg_runbuild() {
>  	# Install package builder script
>  	sudo install -m 755 ${WORKDIR}/build-kernel.sh ${BUILDCHROOT_DIR}
>  
> -	sudo cp ${WORKDIR}/${KERNEL_DEFCONFIG} ${WORKDIR}/${S}/.config
> +	sudo cp ${WORKDIR}/${KERNEL_DEFCONFIG} ${S}/.config
>  
>  	E="${@ bb.utils.export_proxies(d)}"
>  
> 

Otherwise:
Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>

Jan