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

[Henning Schild <henning.schild@siemens.com>]

Am Thu, 19 Apr 2018 11:45:05 +0200
schrieb 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?

Both are fine. That is why i kind of mixed them in this commit. I am
not sure which one to promote. One tells people to modify an
internal variable the other one is not consistent between fetchers.
git uses destsuffix while tar.gz uses subdir

> >  
> >  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.

I was wondering why the or "" was there in the first place, S was
always set even before recipes decided to overwrite it. Some python
sniplets seem to return Null on some intermediate steps when using
layers. Might that be why you introduced the 'or ""'?

Henning

> >  
> >      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