[PATCH v2 0/4] Feature to use symbolic links to binaries for ptest

[PATCH v2 0/4] Feature to use symbolic links to binaries for ptest

[Mariano Lopez]

Some binaries are renamed by update-alternatives class at build time
and some ptest run against a binary belonging to another package. Take
for example busybox or util-linux.

This series adds a directory within the ptest with symlinks to the
binaries produced by the package, so just adding this directory to the
PATH will test the correct binaries.

v2 changes:

Add busybox recipe to use this method of symlinks generation.

Refactor the way the binary ptest directory is generated. Now the
directory will contain only symlinks to binaries that were renamed by
update-alternatives and not symlinks to all the package's binaries. This
have some advantages:
- Less lines and the code is cleaner.
- There is no need to have symlinks for every binary.
- Reuse existing update-alternatives functionality, don't need to add
  new features to the class.

To use this feature just add PTEST_BINDIR = "1" to the recipe.

The following changes since commit ffa5a1bda6741f5dc9f1b8db1bb37b0c6f103c99:

  asciidoc: specify XML catalogue to use (2019-04-03 14:56:27 +0100)

are available in the Git repository at:

  git://github.com/justanotherboy/poky bug13238
  https://github.com/justanotherboy/poky/tree/bug13238

Mariano Lopez (4):
  update-alternatives.bbclass: Add function to get metadata
  ptest.bbclass: Add feature to populate a binary directory
  util-linux: Use PTEST binary directory
  busybox: Use PTEST binary directory

 meta/classes/ptest.bbclass                    | 31 ++++++++
 meta/classes/update-alternatives.bbclass      | 74 ++++++++++++-------
 meta/recipes-core/busybox/busybox.inc         | 16 +---
 meta/recipes-core/util-linux/util-linux.inc   |  5 +-
 .../util-linux/util-linux/run-ptest           |  4 +
 5 files changed, 87 insertions(+), 43 deletions(-)

-- 
2.19.2

[PATCH v2 1/4] update-alternatives.bbclass: Add function to get metadata

[Mariano Lopez]

This adds update_alternatives_alt_targets function to get the metadata
for a package. This is for code reuse because the metadata would help
other classes that needs to be aware of how update-alternatives modify
the final package.

[YOCTO #12597]
[YOCTO #13238]

Signed-off-by: Mariano Lopez <just.another.mariano@gmail.com>
---
 meta/classes/update-alternatives.bbclass | 74 +++++++++++++++---------
 1 file changed, 47 insertions(+), 27 deletions(-)

diff --git a/meta/classes/update-alternatives.bbclass b/meta/classes/update-alternatives.bbclass
index 537e85d9a3..bc19373348 100644
--- a/meta/classes/update-alternatives.bbclass
+++ b/meta/classes/update-alternatives.bbclass
@@ -216,44 +216,64 @@ python apply_update_alternative_renames () {
             update_files(alt_target, alt_target_rename, pkg, d)
 }
 
+def update_alternatives_alt_targets(d, pkg):
+    """
+    Returns the update-alternatives metadata for a package.
+
+    The returned format is a list of tuples where the tuple contains:
+    alt_name:     The binary name
+    alt_link:     The path for the binary (Shared by different packages)
+    alt_target:   The path for the renamed binary (Unique per package)
+    alt_priority: The priority of the alt_target
+
+    All the alt_targets will be installed into the sysroot. The alt_link is
+    a symlink pointing to the alt_target with the highest priority.
+    """
+
+    pn = d.getVar('BPN')
+    pkgdest = d.getVar('PKGD')
+    updates = list()
+    for alt_name in (d.getVar('ALTERNATIVE_%s' % pkg) or "").split():
+        alt_link     = d.getVarFlag('ALTERNATIVE_LINK_NAME', alt_name)
+        alt_target   = d.getVarFlag('ALTERNATIVE_TARGET_%s' % pkg, alt_name) or \
+                       d.getVarFlag('ALTERNATIVE_TARGET', alt_name) or \
+                       d.getVar('ALTERNATIVE_TARGET_%s' % pkg) or \
+                       d.getVar('ALTERNATIVE_TARGET') or \
+                       alt_link
+        alt_priority = d.getVarFlag('ALTERNATIVE_PRIORITY_%s' % pkg,  alt_name) or \
+                       d.getVarFlag('ALTERNATIVE_PRIORITY',  alt_name) or \
+                       d.getVar('ALTERNATIVE_PRIORITY_%s' % pkg) or  \
+                       d.getVar('ALTERNATIVE_PRIORITY')
+
+        # This shouldn't trigger, as it should have been resolved earlier!
+        if alt_link == alt_target:
+            bb.note('alt_link == alt_target: %s == %s -- correcting, this should not happen!' % (alt_link, alt_target))
+            alt_target = '%s.%s' % (alt_target, pn)
+
+        if not os.path.lexists('%s/%s' % (pkgdest, alt_target)):
+            bb.warn('%s: NOT adding alternative provide %s: %s does not exist' % (pn, alt_link, alt_target))
+            continue
+
+        alt_target = os.path.normpath(alt_target)
+        updates.append( (alt_name, alt_link, alt_target, alt_priority) )
+
+    return updates
+
 PACKAGESPLITFUNCS_prepend = "populate_packages_updatealternatives "
 
 python populate_packages_updatealternatives () {
     if not update_alternatives_enabled(d):
         return
 
-    pn = d.getVar('BPN')
-
     # Do actual update alternatives processing
-    pkgdest = d.getVar('PKGD')
     for pkg in (d.getVar('PACKAGES') or "").split():
         # Create post install/removal scripts
         alt_setup_links = ""
         alt_remove_links = ""
-        for alt_name in (d.getVar('ALTERNATIVE_%s' % pkg) or "").split():
-            alt_link     = d.getVarFlag('ALTERNATIVE_LINK_NAME', alt_name)
-            alt_target   = d.getVarFlag('ALTERNATIVE_TARGET_%s' % pkg, alt_name) or d.getVarFlag('ALTERNATIVE_TARGET', alt_name)
-            alt_target   = alt_target or d.getVar('ALTERNATIVE_TARGET_%s' % pkg) or d.getVar('ALTERNATIVE_TARGET') or alt_link
-            # Sometimes alt_target is specified as relative to the link name.
-            alt_target   = os.path.join(os.path.dirname(alt_link), alt_target)
-
-            alt_priority = d.getVarFlag('ALTERNATIVE_PRIORITY_%s' % pkg,  alt_name) or d.getVarFlag('ALTERNATIVE_PRIORITY',  alt_name)
-            alt_priority = alt_priority or d.getVar('ALTERNATIVE_PRIORITY_%s' % pkg) or d.getVar('ALTERNATIVE_PRIORITY')
-
-            # This shouldn't trigger, as it should have been resolved earlier!
-            if alt_link == alt_target:
-                bb.note('alt_link == alt_target: %s == %s -- correcting, this should not happen!' % (alt_link, alt_target))
-                alt_target = '%s.%s' % (alt_target, pn)
-
-            if not os.path.lexists('%s/%s' % (pkgdest, alt_target)):
-                bb.warn('%s: NOT adding alternative provide %s: %s does not exist' % (pn, alt_link, alt_target))
-                continue
-
-            # Default to generate shell script.. eventually we may want to change this...
-            alt_target = os.path.normpath(alt_target)
-
-            alt_setup_links  += '\tupdate-alternatives --install %s %s %s %s\n' % (alt_link, alt_name, alt_target, alt_priority)
-            alt_remove_links += '\tupdate-alternatives --remove  %s %s\n' % (alt_name, alt_target)
+        updates = update_alternatives_alt_targets(d, pkg)
+        for alt_name, alt_link, alt_target, alt_priority in updates:
+           alt_setup_links  += '\tupdate-alternatives --install %s %s %s %s\n' % (alt_link, alt_name, alt_target, alt_priority)
+           alt_remove_links += '\tupdate-alternatives --remove  %s %s\n' % (alt_name, alt_target)
 
         if alt_setup_links:
             # RDEPENDS setup
-- 
2.19.2

Re: [PATCH v2 1/4] update-alternatives.bbclass: Add function to get metadata

[Richard Purdie]

On Sun, 2019-04-07 at 12:15 -0500, Mariano Lopez wrote:
>  python populate_packages_updatealternatives () {
>      if not update_alternatives_enabled(d):
>          return
>  
> -    pn = d.getVar('BPN')
> -
>      # Do actual update alternatives processing
> -    pkgdest = d.getVar('PKGD')
>      for pkg in (d.getVar('PACKAGES') or "").split():
>          # Create post install/removal scripts
>          alt_setup_links = ""
>          alt_remove_links = ""
> -        for alt_name in (d.getVar('ALTERNATIVE_%s' % pkg) or "").split():
> -            alt_link     = d.getVarFlag('ALTERNATIVE_LINK_NAME', alt_name)
> -            alt_target   = d.getVarFlag('ALTERNATIVE_TARGET_%s' % pkg, alt_name) or d.getVarFlag('ALTERNATIVE_TARGET', alt_name)
> -            alt_target   = alt_target or d.getVar('ALTERNATIVE_TARGET_%s' % pkg) or d.getVar('ALTERNATIVE_TARGET') or alt_link
> -            # Sometimes alt_target is specified as relative to the link name.
> -            alt_target   = os.path.join(os.path.dirname(alt_link), alt_target)
> -
> -            alt_priority = d.getVarFlag('ALTERNATIVE_PRIORITY_%s' % pkg,  alt_name) or d.getVarFlag('ALTERNATIVE_PRIORITY',  alt_name)
> -            alt_priority = alt_priority or d.getVar('ALTERNATIVE_PRIORITY_%s' % pkg) or d.getVar('ALTERNATIVE_PRIORITY')
> -
> -            # This shouldn't trigger, as it should have been resolved earlier!
> -            if alt_link == alt_target:
> -                bb.note('alt_link == alt_target: %s == %s -- correcting, this should not happen!' % (alt_link, alt_target))
> -                alt_target = '%s.%s' % (alt_target, pn)
> -
> -            if not os.path.lexists('%s/%s' % (pkgdest, alt_target)):
> -                bb.warn('%s: NOT adding alternative provide %s: %s does not exist' % (pn, alt_link, alt_target))
> -                continue
> -
> -            # Default to generate shell script.. eventually we may want to change this...
> -            alt_target = os.path.normpath(alt_target)
> -
> -            alt_setup_links  += '\tupdate-alternatives --install %s %s %s %s\n' % (alt_link, alt_name, alt_target, alt_priority)
> -            alt_remove_links += '\tupdate-alternatives --remove  %s %s\n' % (alt_name, alt_target)
> +        updates = update_alternatives_alt_targets(d, pkg)
> +        for alt_name, alt_link, alt_target, alt_priority in updates:
> +           alt_setup_links  += '\tupdate-alternatives --install %s %s %s %s\n' % (alt_link, alt_name, alt_target, alt_priority)
> +           alt_remove_links += '\tupdate-alternatives --remove  %s %s\n' % (alt_name, alt_target)

I think the above is three whitespaces, not four? I know its minor but
it jumped out as I was comparing the code before/after!

Cheers,

Richard

Re: [PATCH v2 1/4] update-alternatives.bbclass: Add function to get metadata

[Mariano Lopez]

On 4/7/19 4:32 PM, Richard Purdie wrote:
> On Sun, 2019-04-07 at 12:15 -0500, Mariano Lopez wrote:
>> -
>> -            # Default to generate shell script.. eventually we may want to change this...
>> -            alt_target = os.path.normpath(alt_target)
>> -
>> -            alt_setup_links  += '\tupdate-alternatives --install %s %s %s %s\n' % (alt_link, alt_name, alt_target, alt_priority)
>> -            alt_remove_links += '\tupdate-alternatives --remove  %s %s\n' % (alt_name, alt_target)
>> +        updates = update_alternatives_alt_targets(d, pkg)
>> +        for alt_name, alt_link, alt_target, alt_priority in updates:
>> +           alt_setup_links  += '\tupdate-alternatives --install %s %s %s %s\n' % (alt_link, alt_name, alt_target, alt_priority)
>> +           alt_remove_links += '\tupdate-alternatives --remove  %s %s\n' % (alt_name, alt_target)
> I think the above is three whitespaces, not four? I know its minor but
> it jumped out as I was comparing the code before/after!
>
> Cheers,
>
> Richard
>

Good catch! I have sent the the fixed version but I messed up with the 
cover letter, you can find the correct version here:
https://patchwork.openembedded.org/patch/160236/


I think I don't even need to put that link there :)

Thanks,

Mariano

Re: [PATCH v2 1/4] update-alternatives.bbclass: Add function to get metadata

[richard.purdie]

On Sun, 2019-04-07 at 21:23 -0500, Mariano Lopez wrote:
> On 4/7/19 4:32 PM, Richard Purdie wrote:
> > On Sun, 2019-04-07 at 12:15 -0500, Mariano Lopez wrote:
> > > -
> > > -            # Default to generate shell script.. eventually we
> > > may want to change this...
> > > -            alt_target = os.path.normpath(alt_target)
> > > -
> > > -            alt_setup_links  += '\tupdate-alternatives --install 
> > > %s %s %s %s\n' % (alt_link, alt_name, alt_target, alt_priority)
> > > -            alt_remove_links += '\tupdate-alternatives --
> > > remove  %s %s\n' % (alt_name, alt_target)
> > > +        updates = update_alternatives_alt_targets(d, pkg)
> > > +        for alt_name, alt_link, alt_target, alt_priority in
> > > updates:
> > > +           alt_setup_links  += '\tupdate-alternatives --install
> > > %s %s %s %s\n' % (alt_link, alt_name, alt_target, alt_priority)
> > > +           alt_remove_links += '\tupdate-alternatives --
> > > remove  %s %s\n' % (alt_name, alt_target)
> > I think the above is three whitespaces, not four? I know its minor
> > but
> > it jumped out as I was comparing the code before/after!
> > 
> > Cheers,
> > 
> > Richard
> > 
> 
> Good catch! I have sent the the fixed version but I messed up with
> the 
> cover letter, you can find the correct version here:
> https://patchwork.openembedded.org/patch/160236/
> 
> 
> I think I don't even need to put that link there :)

Thanks, unfortunately nativesdk-util-linux fails to build :(

https://autobuilder.yoctoproject.org/typhoon/#/builders/20/builds/711

Cheers,

Richard

[PATCH v2 2/4] ptest.bbclass: Add feature to populate a binary directory

[Mariano Lopez]

This adds the functionality to create a binary directory within
PTEST_PATH directory. This directory will be populated with
symlinks pointing to the binaries installed by the package and
then renamed by update-alternatives. This way the ptest only needs
to source this binary directory in order to use the expected
binaries.

To enable this feature just add PTEST_BINDIR = "1" to the recipe.

[YOCTO #12597]
[YOCTO #13238]

Signed-off-by: Mariano Lopez <just.another.mariano@gmail.com>
---
 meta/classes/ptest.bbclass | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/meta/classes/ptest.bbclass b/meta/classes/ptest.bbclass
index 97865c9338..0c37447881 100644
--- a/meta/classes/ptest.bbclass
+++ b/meta/classes/ptest.bbclass
@@ -65,6 +65,37 @@ do_install_ptest_base() {
     done
 }
 
+PTEST_BINDIR_PKGD_PATH = "${PKGD}${PTEST_PATH}/bin"
+
+# This function needs to run after apply_update_alternative_renames because the
+# aforementioned function will update the ALTERNATIVE_LINK_NAME flag. Append is
+# used here to make this function to run as late as possible.
+PACKAGE_PREPROCESS_FUNCS_append = "${@bb.utils.contains("PTEST_BINDIR", "1", " ptest_update_alternatives", "", d)}"
+
+python ptest_update_alternatives() {
+    """
+    This function will generate the symlinks in the PTEST_BINDIR_PKGD_PATH
+    to match the renamed binaries by update-alternatives.
+    """
+
+    if not bb.data.inherits_class('update-alternatives', d) \
+           or not update_alternatives_enabled(d):
+        return
+
+    bb.note("Generating symlinks for ptest")
+    bin_paths = { os.environ["bindir"], os.environ["base_bindir"],
+                  os.environ["sbindir"], os.environ["base_sbindir"] }
+    ptest_bindir = d.getVar("PTEST_BINDIR_PKGD_PATH")
+    os.mkdir(ptest_bindir)
+    for pkg in (d.getVar('PACKAGES') or "").split():
+        alternatives = update_alternatives_alt_targets(d, pkg)
+        for alt_name, alt_link, alt_target, _ in alternatives:
+            # Some alternatives are for man pages,
+            # check if the alternative is in PATH
+            if os.path.dirname(alt_link) in bin_paths:
+                os.symlink(alt_target, os.path.join(ptest_bindir, alt_name))
+}
+
 do_configure_ptest_base[dirs] = "${B}"
 do_compile_ptest_base[dirs] = "${B}"
 do_install_ptest_base[dirs] = "${B}"
-- 
2.19.2

[PATCH v2 3/4] util-linux: Use PTEST binary directory

[Mariano Lopez]

Some binaries generated by util-linux will be replaced by core-utils
in the final image by update-alternatives, so use a dedicated directory
with symlinks to avoid using a binary generated by another package.

This will solve the issue with the ptest runner timing out when
running the kill ptests for util-linux.

[YOCTO #13238]

Signed-off-by: Mariano Lopez <just.another.mariano@gmail.com>
---
 meta/recipes-core/util-linux/util-linux.inc       | 5 +++--
 meta/recipes-core/util-linux/util-linux/run-ptest | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/meta/recipes-core/util-linux/util-linux.inc b/meta/recipes-core/util-linux/util-linux.inc
index 18c3af240e..a67318e84e 100644
--- a/meta/recipes-core/util-linux/util-linux.inc
+++ b/meta/recipes-core/util-linux/util-linux.inc
@@ -284,6 +284,7 @@ ALTERNATIVE_LINK_NAME[wall.1] = "${mandir}/man1/wall.1"
 
 BBCLASSEXTEND = "native nativesdk"
 
+PTEST_BINDIR = "1"
 do_compile_ptest() {
     oe_runmake buildtest-TESTS
 }
@@ -312,8 +313,8 @@ do_install_ptest() {
         '/^\tif[[:space:]]\[[[:space:]]![[:space:]]-x[[:space:]]"$1"/s|$1|`which $1 2>/dev/null`|g' \
          ${D}${PTEST_PATH}/tests/functions.sh
 
-    # "kill -L" behaves differently than "/bin/kill -L" so we need an additional fix
+    # Running "kill" without the the complete path would use the shell's built-in kill
     sed -i -e \
-         '/^TS_CMD_KILL/ s|kill|/bin/kill|g' \
+         '/^TS_CMD_KILL/ s|kill|${PTEST_PATH}/bin/kill|g' \
          ${D}${PTEST_PATH}/tests/commands.sh
 }
diff --git a/meta/recipes-core/util-linux/util-linux/run-ptest b/meta/recipes-core/util-linux/util-linux/run-ptest
index fbc2f9b56a..2178ab8fef 100644
--- a/meta/recipes-core/util-linux/util-linux/run-ptest
+++ b/meta/recipes-core/util-linux/util-linux/run-ptest
@@ -1,5 +1,9 @@
 #!/bin/sh
 
+current_path=$(readlink -f $0)
+export bindir=$(dirname $current_path)
+export PATH=$bindir/bin:$PATH
+
 cd tests || exit 1                                                          
 
 comps=$(find ts/ -type f -perm -111 -regex ".*/[^\.~]*" |  sort)
-- 
2.19.2

[PATCH v2 4/4] busybox: Use PTEST binary directory

[Mariano Lopez]

This will generate the symlinks in the ptest binary directory using the
ptest class functionality instead of generating them manually. Because
the ptest class uses update-alternatives to get the metadata for the
symlinks it will respect the use of BUSYBOX_SPLIT_SUID automatically.

[YOCTO #12597]

Signed-off-by: Mariano Lopez <just.another.mariano@gmail.com>
---
 meta/recipes-core/busybox/busybox.inc | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/meta/recipes-core/busybox/busybox.inc b/meta/recipes-core/busybox/busybox.inc
index c9d25ff1ca..174ce5a8c0 100644
--- a/meta/recipes-core/busybox/busybox.inc
+++ b/meta/recipes-core/busybox/busybox.inc
@@ -347,24 +347,12 @@ do_install () {
 	fi
 }
 
+PTEST_BINDIR = "1"
+
 do_install_ptest () {
 	cp -r ${B}/testsuite ${D}${PTEST_PATH}/
 	cp ${B}/.config      ${D}${PTEST_PATH}/
 	ln -s /bin/busybox   ${D}${PTEST_PATH}/busybox
-
-	mkdir ${D}${PTEST_PATH}/bin
-	if [ "${BUSYBOX_SPLIT_SUID}" = "1" ]; then
-		while read link; do
-			ln -s ${base_bindir}/busybox.suid ${D}${PTEST_PATH}/bin/$(basename $link)
-		done <${D}${sysconfdir}/busybox.links.suid
-		while read link; do
-			ln -s ${base_bindir}/busybox.nosuid ${D}${PTEST_PATH}/bin/$(basename $link)
-		done <${D}${sysconfdir}/busybox.links.nosuid
-	else
-		while read link; do
-			ln -s ${base_bindir}/busybox ${D}${PTEST_PATH}/bin/$(basename $link)
-		done <${D}${sysconfdir}/busybox.links
-	fi
 }
 
 inherit update-alternatives
-- 
2.19.2