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

[Henning Schild]

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.

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

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

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)}"

--
2.16.1

[Jan Kiszka]

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.

Should we promote this pattern now or destsuffix?

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.

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

Jan

[Henning Schild]

Am Thu, 19 Apr 2018 11:45:05 +0200
schrieb Jan Kiszka <jan.k...@siemens.com>:

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

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

[Jan Kiszka]

On 2018-04-19 12:36, Henning Schild wrote:
>>> 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 ""'?

Problem is that I don't remember doing this intentionally. But if that
were the case (intermediate Null state), it should not longer harm us
now (no more "x + Null").

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[Henning Schild]

Am Thu, 19 Apr 2018 13:42:22 +0200
schrieb Jan Kiszka <jan.k...@siemens.com>:

It was + ed later, but never seem to be Null. V2 on the way.

Henning

> Jan
>

[Henning Schild]

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.

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

index 26a0c81..0bc449f 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)

[Henning Schild]

Hold this one, it still has issues building custom kernels. A path that
is not on CI testing ...

Henning

Am Thu, 19 Apr 2018 18:58:32 +0200
schrieb Henning Schild <henning...@siemens.com>:

[Henning Schild]

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.

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...@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 | 2 +-

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 | 4 ++--
11 files changed, 25 insertions(+), 22 deletions(-)

index 2c93d40..fc465fd 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
@@ -12,6 +12,6 @@ SRC_URI += " \

file://x86_64_defconfig"
SRC_URI[sha256sum] = "866a94c1c38d923ae18e74b683d7a8a79b674ebdfe7f40f1a3be9a27d39fe354"

-S = "linux-${PV}"

+S = "${WORKDIR}/linux-${PV}"

index 0498dfa..1176b25 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)}"

@@ -43,5 +43,5 @@ dpkg_runbuild() {
export KERNEL_DEBIAN_DEPENDS="${KERNEL_DEBIAN_DEPENDS}"
export KERNEL_HEADERS_DEBIAN_DEPENDS="${KERNEL_HEADERS_DEBIAN_DEPENDS}"

- sudo -E chroot ${BUILDCHROOT_DIR} /build-kernel.sh ${PP}/${S}
+ sudo -E chroot ${BUILDCHROOT_DIR} /build-kernel.sh ${PP}/${PPS}
}
--
2.16.1

[Henning Schild]

This one is now manually tested against linux-cip and -mainline recipes
from the Isar tree.

Henning

Am Fri, 20 Apr 2018 14:36:26 +0200
schrieb Henning Schild <henning...@siemens.com>:

[Jan Kiszka]

On 2018-04-20 14:37, Henning Schild wrote:
> This one is now manually tested against linux-cip and -mainline recipes
> from the Isar tree.

And the trick was...?

Jan

[Henning Schild]

Am Fri, 20 Apr 2018 14:39:21 +0200
schrieb Jan Kiszka <jan.k...@siemens.com>:

> On 2018-04-20 14:37, Henning Schild wrote:
> > This one is now manually tested against linux-cip and -mainline
> > recipes from the Isar tree.
>
> And the trick was...?

I had to keep setting S in the tar.gz case (mainline), subdir= will
actually create another folder hierarchy instead of defining the name.

And in build-kernel.sh has to go to ${PP}/${PPS}, not PP/S like in v2.

Henning

[Alexander Smirnov]

On 04/20/2018 03:36 PM, 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.
>
> 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.
>

Applied to next, thanks!

Alex