[master][PATCH v2 0/6] Allow PR Service and hash equiv togther

[master][PATCH v2 0/6] Allow PR Service and hash equiv togther

[Mark Hatle]

v2:

Most comments have been addressed to create a v2 version.  I've refactored
the commits to make a few things more clear, basically moving code around
and fixing minor issues BEFORE the big patch.

Before there were two patches that together implemented the PR Serv/Hash
work.  This has been combined into a single patch and the oe_nohash stuff
has simply been removed as no longer applicable.

The only comment that was NOT addressed in this was the suggestion to
sed the pkgdata files in do_packagedata.  I'm hesitent to do this as
sed with ${...} in them has proven to be fragile for me in the past.  If
we decide that is necessary, I'd suggest we start with this set and then
make that change after this proves to work (or not).

v1:

Before PR service didn't work reliably with hash equivalency.  Generally
you ended up with results that may not be reproducible, even if you
started with the same PR service database and hash equivalency database.

Overtime, intermediate PR values would be created that would cause thing
to get out of sync in the case of certain rebuilds or other corner cases.

The set refactors the PR service to work along side the new hash equiv
system.  It moves the PR and AUTOINC lookup to AFTER the do_package task
is complete.  This allows us to use the do_package unihash for lookup.

Additionally this fixed a small issue with the kernel, where the PR value
could get incremented twice.  The fix is an artifact of the other changes
that cause us to only run the PR service work once per recipe.

This has been tested with the following workflow, which covers one of
the critical corner cases for me:

configure local.conf with:

   BB_HASHSERVE = "auto"
   BB_SIGNATURE_HANDLER = "OEEquivHash"
   
   PRSERV_HOST ??= "localhost:0"
   
   INHERIT += "reproducible_build"
   INHERIT += "buildhistory"

bitbake glibc linux-yocto

# Modify meta/recipes-core/glibc/glibc_2.32.bb, add a comment
# to the do_patch_append().  This will taint the hash of this
# function.

bitbake glibc linux-yocto

# The system should have detected the output was the same, and
# no proceed past do_package in glibc.  The kernel should not
# have built at all.

# Store/mv the tmp and sstate-cache from that build elsewhere
# repeat the run

bitbake glibc linux-yocto

# Compare the results of tmp/deploy/<package>/* between last
# and current run.
#
# The contents should be the same (filenames specifically).
#
# Also the kernel should be r0.0, not r0.1.

Note: if the hash equivalency database or PR server database (located
in the cache directory) is removed, the values may not be the same
as the previous run.

Additionally while testing the various package_*.bbclass files, it
was noted that package_tar.bbclass was not working the same way as
the others.  This was correct as a standalone patch.

Mark Hatle (6):
  package_tar.bbclass: Sync to the other package_* classes
  kernel.bbclass: Remove do_install[prefunc] no longer needed
  buildhistory.bbclass: Rework to use read_subpackage_metadata
  package.bbclass: Move package_get_auto_pr to packagedata.bbclass
  kernel.bbclass: Move away from calling package_get_auto_pr
  package.bbclass: hash equivalency and pr service

 meta/classes/buildhistory.bbclass |  49 ++++++-------
 meta/classes/kernel.bbclass       |   6 +-
 meta/classes/nopackages.bbclass   |   1 +
 meta/classes/package.bbclass      | 111 ++++++++++++++----------------
 meta/classes/package_tar.bbclass  |   6 +-
 meta/classes/packagedata.bbclass  |  68 ++++++++++++++++++
 meta/conf/bitbake.conf            |   1 +
 7 files changed, 148 insertions(+), 94 deletions(-)

-- 
2.17.1

[master][PATCH v2 1/6] package_tar.bbclass: Sync to the other package_* classes

[Mark Hatle]

Sync up the anonymous python definition with the other package_*.bbclass
files.  This should make future maintenance easier, even though it has
no difference in behavior from what was there.

Additional, there was a missing deltask in the nopackages.bbclass related
to the package_tar which has been corrected.  This could cause problems on
native recipes when package_tar was enabled.

Signed-off-by: Mark Hatle <mark.hatle@kernel.crashing.org>
---
 meta/classes/nopackages.bbclass  | 1 +
 meta/classes/package_tar.bbclass | 6 ++----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/meta/classes/nopackages.bbclass b/meta/classes/nopackages.bbclass
index 559f5078bd..7a4f632d71 100644
--- a/meta/classes/nopackages.bbclass
+++ b/meta/classes/nopackages.bbclass
@@ -2,6 +2,7 @@ deltask do_package
 deltask do_package_write_rpm
 deltask do_package_write_ipk
 deltask do_package_write_deb
+deltask do_package_write_tar
 deltask do_package_qa
 deltask do_packagedata
 deltask do_package_setscene
diff --git a/meta/classes/package_tar.bbclass b/meta/classes/package_tar.bbclass
index ce3ab4c8e2..d6c1b306fc 100644
--- a/meta/classes/package_tar.bbclass
+++ b/meta/classes/package_tar.bbclass
@@ -57,10 +57,8 @@ python do_package_tar () {
 
 python () {
     if d.getVar('PACKAGES') != '':
-        deps = (d.getVarFlag('do_package_write_tar', 'depends') or "").split()
-        deps.append('tar-native:do_populate_sysroot')
-        deps.append('virtual/fakeroot-native:do_populate_sysroot')
-        d.setVarFlag('do_package_write_tar', 'depends', " ".join(deps))
+        deps = ' tar-native:do_populate_sysroot virtual/fakeroot-native:do_populate_sysroot'
+        d.appendVarFlag('do_package_write_tar', 'depends', deps)
         d.setVarFlag('do_package_write_tar', 'fakeroot', "1")
 }
 
-- 
2.17.1

[master][PATCH v2 2/6] kernel.bbclass: Remove do_install[prefunc] no longer needed

[Mark Hatle]

Prior work has refactored the do_install task multiple times, and any
references to PKGV and PKGR (even indirect ones) have been removed.

Signed-off-by: Mark Hatle <mark.hatle@kernel.crashing.org>
---
 meta/classes/kernel.bbclass | 1 -
 1 file changed, 1 deletion(-)

diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
index cba77daa7a..7869184b94 100644
--- a/meta/classes/kernel.bbclass
+++ b/meta/classes/kernel.bbclass
@@ -416,7 +416,6 @@ kernel_do_install() {
 	install -d ${D}${sysconfdir}/modules-load.d
 	install -d ${D}${sysconfdir}/modprobe.d
 }
-do_install[prefuncs] += "package_get_auto_pr"
 
 # Must be ran no earlier than after do_kernel_checkout or else Makefile won't be in ${S}/Makefile
 do_kernel_version_sanity_check() {
-- 
2.17.1

[master][PATCH v2 3/6] buildhistory.bbclass: Rework to use read_subpackage_metadata

[Mark Hatle]

Using this mechanism ensures that we have a single point to implement
the loading of the package and subpackage meta data.  This also then
allows the buildhistory class to use the regular datastore vs it's
own custom arrays for processing history items.

Signed-off-by: Mark Hatle <mark.hatle@kernel.crashing.org>
---
 meta/classes/buildhistory.bbclass | 49 ++++++++++++++-----------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/meta/classes/buildhistory.bbclass b/meta/classes/buildhistory.bbclass
index 805e976ac5..fbdb1d3161 100644
--- a/meta/classes/buildhistory.bbclass
+++ b/meta/classes/buildhistory.bbclass
@@ -258,20 +258,15 @@ python buildhistory_emit_pkghistory() {
     rcpinfo.config = sortlist(oe.utils.squashspaces(d.getVar('PACKAGECONFIG') or ""))
     write_recipehistory(rcpinfo, d)
 
-    pkgdest = d.getVar('PKGDEST')
+    bb.build.exec_func("read_subpackage_metadata", d)
+
     for pkg in packagelist:
-        pkgdata = {}
-        with open(os.path.join(pkgdata_dir, 'runtime', pkg)) as f:
-            for line in f.readlines():
-                item = line.rstrip('\n').split(': ', 1)
-                key = item[0]
-                if key.endswith('_' + pkg):
-                    key = key[:-len(pkg)-1]
-                pkgdata[key] = item[1].encode('latin-1').decode('unicode_escape')
-
-        pkge = pkgdata.get('PKGE', '0')
-        pkgv = pkgdata['PKGV']
-        pkgr = pkgdata['PKGR']
+        localdata = d.createCopy()
+        localdata.setVar('OVERRIDES', d.getVar("OVERRIDES", False) + ":" + pkg)
+
+        pkge = localdata.getVar("PKGE") or '0'
+        pkgv = localdata.getVar("PKGV")
+        pkgr = localdata.getVar("PKGR")
         #
         # Find out what the last version was
         # Make sure the version did not decrease
@@ -288,31 +283,31 @@ python buildhistory_emit_pkghistory() {
 
         pkginfo = PackageInfo(pkg)
         # Apparently the version can be different on a per-package basis (see Python)
-        pkginfo.pe = pkgdata.get('PE', '0')
-        pkginfo.pv = pkgdata['PV']
-        pkginfo.pr = pkgdata['PR']
-        pkginfo.pkg = pkgdata['PKG']
+        pkginfo.pe = localdata.getVar("PE") or '0'
+        pkginfo.pv = localdata.getVar("PV")
+        pkginfo.pr = localdata.getVar("PR")
+        pkginfo.pkg = localdata.getVar("PKG")
         pkginfo.pkge = pkge
         pkginfo.pkgv = pkgv
         pkginfo.pkgr = pkgr
-        pkginfo.rprovides = sortpkglist(oe.utils.squashspaces(pkgdata.get('RPROVIDES', "")))
-        pkginfo.rdepends = sortpkglist(oe.utils.squashspaces(pkgdata.get('RDEPENDS', "")))
-        pkginfo.rrecommends = sortpkglist(oe.utils.squashspaces(pkgdata.get('RRECOMMENDS', "")))
-        pkginfo.rsuggests = sortpkglist(oe.utils.squashspaces(pkgdata.get('RSUGGESTS', "")))
-        pkginfo.rreplaces = sortpkglist(oe.utils.squashspaces(pkgdata.get('RREPLACES', "")))
-        pkginfo.rconflicts = sortpkglist(oe.utils.squashspaces(pkgdata.get('RCONFLICTS', "")))
-        pkginfo.files = oe.utils.squashspaces(pkgdata.get('FILES', ""))
+        pkginfo.rprovides = sortpkglist(oe.utils.squashspaces(localdata.getVar("RPROVIDES") or ""))
+        pkginfo.rdepends = sortpkglist(oe.utils.squashspaces(localdata.getVar("RDEPENDS") or ""))
+        pkginfo.rrecommends = sortpkglist(oe.utils.squashspaces(localdata.getVar("RRECOMMENDS") or ""))
+        pkginfo.rsuggests = sortpkglist(oe.utils.squashspaces(localdata.getVar("RSUGGESTS") or ""))
+        pkginfo.replaces = sortpkglist(oe.utils.squashspaces(localdata.getVar("RREPLACES") or ""))
+        pkginfo.rconflicts = sortpkglist(oe.utils.squashspaces(localdata.getVar("RCONFLICTS") or ""))
+        pkginfo.files = oe.utils.squashspaces(localdata.getVar("FILES") or "")
         for filevar in pkginfo.filevars:
-            pkginfo.filevars[filevar] = pkgdata.get(filevar, "")
+            pkginfo.filevars[filevar] = localdata.getVar(filevar) or ""
 
         # Gather information about packaged files
-        val = pkgdata.get('FILES_INFO', '')
+        val = localdata.getVar('FILES_INFO') or ''
         dictval = json.loads(val)
         filelist = list(dictval.keys())
         filelist.sort()
         pkginfo.filelist = " ".join([shlex.quote(x) for x in filelist])
 
-        pkginfo.size = int(pkgdata['PKGSIZE'])
+        pkginfo.size = int(localdata.getVar('PKGSIZE') or '0')
 
         write_pkghistory(pkginfo, d)
 
-- 
2.17.1

[master][PATCH v2 4/6] package.bbclass: Move package_get_auto_pr to packagedata.bbclass

[Mark Hatle]

Running package_get_auto_pr during do_package is 'too early' when we need
to evaluate the do_package output to get it's unihash to look up the
right value in the PR database.

Move this function packagedata.bbclass (no changes yet), as subsequent
patches will refactor this function and the order in which it's called.

Signed-off-by: Mark Hatle <mark.hatle@kernel.crashing.org>
---
 meta/classes/package.bbclass     | 50 ---------------------------
 meta/classes/packagedata.bbclass | 59 ++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 50 deletions(-)

diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
index 7a36262eb6..d44c359e94 100644
--- a/meta/classes/package.bbclass
+++ b/meta/classes/package.bbclass
@@ -667,56 +667,6 @@ def runtime_mapping_rename (varname, pkg, d):
 # Package functions suitable for inclusion in PACKAGEFUNCS
 #
 
-python package_get_auto_pr() {
-    import oe.prservice
-    import re
-
-    # Support per recipe PRSERV_HOST
-    pn = d.getVar('PN')
-    host = d.getVar("PRSERV_HOST_" + pn)
-    if not (host is None):
-        d.setVar("PRSERV_HOST", host)
-
-    pkgv = d.getVar("PKGV")
-
-    # PR Server not active, handle AUTOINC
-    if not d.getVar('PRSERV_HOST'):
-        if 'AUTOINC' in pkgv:
-            d.setVar("PKGV", pkgv.replace("AUTOINC", "0"))
-        return
-
-    auto_pr = None
-    pv = d.getVar("PV")
-    version = d.getVar("PRAUTOINX")
-    pkgarch = d.getVar("PACKAGE_ARCH")
-    checksum = d.getVar("BB_TASKHASH")
-
-    if d.getVar('PRSERV_LOCKDOWN'):
-        auto_pr = d.getVar('PRAUTO_' + version + '_' + pkgarch) or d.getVar('PRAUTO_' + version) or None
-        if auto_pr is None:
-            bb.fatal("Can NOT get PRAUTO from lockdown exported file")
-        d.setVar('PRAUTO',str(auto_pr))
-        return
-
-    try:
-        conn = d.getVar("__PRSERV_CONN")
-        if conn is None:
-            conn = oe.prservice.prserv_make_conn(d)
-        if conn is not None:
-            if "AUTOINC" in pkgv:
-                srcpv = bb.fetch2.get_srcrev(d)
-                base_ver = "AUTOINC-%s" % version[:version.find(srcpv)]
-                value = conn.getPR(base_ver, pkgarch, srcpv)
-                d.setVar("PKGV", pkgv.replace("AUTOINC", str(value)))
-
-            auto_pr = conn.getPR(version, pkgarch, checksum)
-    except Exception as e:
-        bb.fatal("Can NOT get PRAUTO, exception %s" %  str(e))
-    if auto_pr is None:
-        bb.fatal("Can NOT get PRAUTO from remote PR service")
-    d.setVar('PRAUTO',str(auto_pr))
-}
-
 LOCALEBASEPN ??= "${PN}"
 
 python package_do_split_locales() {
diff --git a/meta/classes/packagedata.bbclass b/meta/classes/packagedata.bbclass
index a903e5cfd2..5475d5c0da 100644
--- a/meta/classes/packagedata.bbclass
+++ b/meta/classes/packagedata.bbclass
@@ -32,3 +32,62 @@ python read_subpackage_metadata () {
             else:
                 d.setVar(key, sdata[key], parsing=True)
 }
+
+package_get_auto_pr[vardepsexclude] = "BB_TASKDEPDATA"
+python package_get_auto_pr() {
+    import oe.prservice
+    import re
+
+    def get_do_package_hash(pn):
+        if d.getVar("BB_RUNTASK") != "do_package":
+            taskdepdata = d.getVar("BB_TASKDEPDATA", False)
+            for dep in taskdepdata:
+                if taskdepdata[dep][1] == "do_package" and taskdepdata[dep][0] == pn:
+                    return taskdepdata[dep][6]
+        return d.getVar("BB_TASKHASH")
+
+    # Support per recipe PRSERV_HOST
+    pn = d.getVar('PN')
+    host = d.getVar("PRSERV_HOST_" + pn)
+    if not (host is None):
+        d.setVar("PRSERV_HOST", host)
+
+    pkgv = d.getVar("PKGV")
+
+    # PR Server not active, handle AUTOINC
+    if not d.getVar('PRSERV_HOST'):
+        if 'AUTOINC' in pkgv:
+            d.setVar("PKGV", pkgv.replace("AUTOINC", "0"))
+        return
+
+    auto_pr = None
+    pv = d.getVar("PV")
+    version = d.getVar("PRAUTOINX")
+    pkgarch = d.getVar("PACKAGE_ARCH")
+    checksum = get_do_package_hash(pn)
+
+    if d.getVar('PRSERV_LOCKDOWN'):
+        auto_pr = d.getVar('PRAUTO_' + version + '_' + pkgarch) or d.getVar('PRAUTO_' + version) or None
+        if auto_pr is None:
+            bb.fatal("Can NOT get PRAUTO from lockdown exported file")
+        d.setVar('PRAUTO',str(auto_pr))
+        return
+
+    try:
+        conn = d.getVar("__PRSERV_CONN")
+        if conn is None:
+            conn = oe.prservice.prserv_make_conn(d)
+        if conn is not None:
+            if "AUTOINC" in pkgv:
+                srcpv = bb.fetch2.get_srcrev(d)
+                base_ver = "AUTOINC-%s" % version[:version.find(srcpv)]
+                value = conn.getPR(base_ver, pkgarch, srcpv)
+                d.setVar("PKGV", pkgv.replace("AUTOINC", str(value)))
+
+            auto_pr = conn.getPR(version, pkgarch, checksum)
+    except Exception as e:
+        bb.fatal("Can NOT get PRAUTO, exception %s" %  str(e))
+    if auto_pr is None:
+        bb.fatal("Can NOT get PRAUTO from remote PR service")
+    d.setVar('PRAUTO',str(auto_pr))
+}
-- 
2.17.1

Re: [OE-core] [master][PATCH v2 4/6] package.bbclass: Move package_get_auto_pr to packagedata.bbclass

[Richard Purdie]

On Wed, 2020-08-26 at 06:27 -0500, Mark Hatle wrote:
> Running package_get_auto_pr during do_package is 'too early' when we
> need to evaluate the do_package output to get it's unihash to look up
> the right value in the PR database.
> 
> Move this function packagedata.bbclass (no changes yet), as 

The "no changes yet" isn't true as the new function is using
BB_TASKDEPDATA?

Cheers,

Richard

Re: [OE-core] [master][PATCH v2 4/6] package.bbclass: Move package_get_auto_pr to packagedata.bbclass

[Mark Hatle]

On 8/26/20 6:44 AM, Richard Purdie wrote:
> On Wed, 2020-08-26 at 06:27 -0500, Mark Hatle wrote:
>> Running package_get_auto_pr during do_package is 'too early' when we
>> need to evaluate the do_package output to get it's unihash to look up
>> the right value in the PR database.
>>
>> Move this function packagedata.bbclass (no changes yet), as 
> 
> The "no changes yet" isn't true as the new function is using
> BB_TASKDEPDATA?

Sorry, that's a merge error on my part.  I'll get that fixed.  That move to
taskdepdata was supposed to be in the final patch.

--Mark

> Cheers,
> 
> Richard
> 

[master][PATCH v2 5/6] kernel.bbclass: Move away from calling package_get_auto_pr

[Mark Hatle]

...instead we call read_subpackage_metadata.

Calling package_get_auto_pr *should* result in the same PKGV AUTOINC
replacement.  However, it will also end up changing PKGR differently
then do_package as the BB_TASKHASH used will be for the wrong task.

Generally this won't cause any real-world issue, but it could cause
problems.

Moving to read_subpackage_metadata ensures that the values used
in do_package will be read in and used for kernel deployment.

Signed-off-by: Mark Hatle <mark.hatle@kernel.crashing.org>
---
 meta/classes/kernel.bbclass | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
index 7869184b94..14c22da306 100644
--- a/meta/classes/kernel.bbclass
+++ b/meta/classes/kernel.bbclass
@@ -748,7 +748,10 @@ kernel_do_deploy() {
 		done
 	fi
 }
-do_deploy[prefuncs] += "package_get_auto_pr"
+
+# We deploy to filenames that include PKGV and PKGR, read the saved data to
+# ensure we get the right values for both
+do_deploy[prefuncs] += "read_subpackage_metadata"
 
 addtask deploy after do_populate_sysroot do_packagedata
 
-- 
2.17.1

[master][PATCH v2 6/6] package.bbclass: hash equivalency and pr service

[Mark Hatle]

When the PR service is enabled a number of small changes may happen
to variables.  In the do_package step a call to package_get_auto_pr
will end up setting PRAUTO and modifying PKGV (if AUTOINC is there).

PRAUTO is then used by EXTENDPRAUTO, which is then used to generate
PKGR.

Since this behavior typically happens BEFORE the BB_UNIHASH is
calculated for do_package, we need a way to defer the expansion
until after we have the unihash value.

Writing out the pkgdata files w/o AUTOPR and PKGV (AUTOINC) expanded
is the easiest way to deal with this.  The majority of items are still
expanded as expected.

In the next task, typically do_packagedata, we will then use the
UNIHASH from the do_package to get the PR (AUTOPR) as well as
generate the AUTOINC replacement value (now PRSERV_PV_AUTOINC).

Then, as required, the standard load routine, read_subpackage_metadata,
will then be used to read the values stored from do_package, and
do_packagedata which provide everything required for the expanded
PKGV and PKGR.

Note, we use read_subpackage_metadata, as PKGV and PKGR may be
set per sub-package with the way things are defined.  Due to this
they are not defined in a ${PN} file, but in the per package
files.

Signed-off-by: Mark Hatle <mark.hatle@kernel.crashing.org>
---
 meta/classes/package.bbclass     | 69 +++++++++++++++++++++++++-------
 meta/classes/packagedata.bbclass | 19 ++++++---
 meta/conf/bitbake.conf           |  1 +
 3 files changed, 69 insertions(+), 20 deletions(-)

diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
index d44c359e94..5570d4954c 100644
--- a/meta/classes/package.bbclass
+++ b/meta/classes/package.bbclass
@@ -7,7 +7,7 @@
 #
 # There are the following default steps but PACKAGEFUNCS can be extended:
 #
-# a) package_get_auto_pr - get PRAUTO from remote PR service
+# a) package_convert_autoinc - convert AUTOINC in PKGV to ${PRSERV_PV_AUTOINC}
 #
 # b) perform_packagecopy - Copy D into PKGD
 #
@@ -648,11 +648,21 @@ def get_package_additional_metadata (pkg_type, d):
         metadata_fields = [field.strip() for field in oe.data.typed_value(key, d)]
         return "\n".join(metadata_fields).strip()
 
+# Remove the excluded variables from a copy of d
+# This ensures that any users will list these variables when expanded
+def exclude_pkgdata_vars(d):
+    localdata = d.createCopy()
+    for exclude_var in (d.getVar('PKGDATA_VARS_EXCLUDE') or "").split():
+        localdata.delVar(exclude_var)
+    return localdata
+
 def runtime_mapping_rename (varname, pkg, d):
     #bb.note("%s before: %s" % (varname, d.getVar(varname)))
 
     new_depends = {}
-    deps = bb.utils.explode_dep_versions2(d.getVar(varname) or "")
+    localdata = exclude_pkgdata_vars(d)
+
+    deps = bb.utils.explode_dep_versions2(localdata.getVar(varname) or "")
     for depend, depversions in deps.items():
         new_depend = get_package_mapping(depend, pkg, d, depversions)
         if depend != new_depend:
@@ -667,6 +677,13 @@ def runtime_mapping_rename (varname, pkg, d):
 # Package functions suitable for inclusion in PACKAGEFUNCS
 #
 
+python package_convert_autoinc() {
+    pkgv = d.getVar("PKGV")
+
+    if 'AUTOINC' in pkgv:
+        d.setVar("PKGV", pkgv.replace("AUTOINC", "${PRSERV_PV_AUTOINC}"))
+}
+
 LOCALEBASEPN ??= "${PN}"
 
 python package_do_split_locales() {
@@ -1436,8 +1453,10 @@ python package_fixsymlinks () {
             if found == False:
                 bb.note("%s contains dangling symlink to %s" % (pkg, l))
 
+    localdata = exclude_pkgdata_vars(d)
+
     for pkg in newrdepends:
-        rdepends = bb.utils.explode_dep_versions2(d.getVar('RDEPENDS_' + pkg) or "")
+        rdepends = bb.utils.explode_dep_versions2(localdata.getVar('RDEPENDS_' + pkg) or "")
         for p in newrdepends[pkg]:
             if p not in rdepends:
                 rdepends[p] = []
@@ -1460,6 +1479,12 @@ PKGDESTWORK = "${WORKDIR}/pkgdata"
 
 PKGDATA_VARS = "PN PE PV PR PKGE PKGV PKGR LICENSE DESCRIPTION SUMMARY RDEPENDS RPROVIDES RRECOMMENDS RSUGGESTS RREPLACES RCONFLICTS SECTION PKG ALLOW_EMPTY FILES CONFFILES FILES_INFO PACKAGE_ADD_METADATA pkg_postinst pkg_postrm pkg_preinst pkg_prerm"
 
+# When we write or use PKGDATA_VARS items, components of this may need
+# to be excluded from expansion.
+# Really we only care about PRAUTO, but EXTENDPRAUTO evaluates this, so
+# exclude it instead
+PKGDATA_VARS_EXCLUDE = "EXTENDPRAUTO PRSERV_PV_AUTOINC"
+
 python emit_pkgdata() {
     from glob import glob
     import json
@@ -1498,7 +1523,7 @@ fi
                     scriptlet = "set -e\n" + "\n".join(scriptlet_split[0:])
             d.setVar('%s_%s' % (scriptlet_name, pkg), scriptlet)
 
-    def write_if_exists(f, pkg, var):
+    def write_if_exists(d, f, pkg, var):
         def encode(str):
             import codecs
             c = codecs.getencoder("unicode_escape")
@@ -1572,16 +1597,18 @@ fi
 
         subdata_file = pkgdatadir + "/runtime/%s" % pkg
         with open(subdata_file, 'w') as sf:
+            localdata = exclude_pkgdata_vars(d)
+
             for var in (d.getVar('PKGDATA_VARS') or "").split():
-                val = write_if_exists(sf, pkg, var)
+                write_if_exists(localdata, sf, pkg, var)
 
-            write_if_exists(sf, pkg, 'FILERPROVIDESFLIST')
-            for dfile in (d.getVar('FILERPROVIDESFLIST_' + pkg) or "").split():
-                write_if_exists(sf, pkg, 'FILERPROVIDES_' + dfile)
+            write_if_exists(localdata, sf, pkg, 'FILERPROVIDESFLIST')
+            for dfile in (localdata.getVar('FILERPROVIDESFLIST_' + pkg) or "").split():
+                write_if_exists(localdata, sf, pkg, 'FILERPROVIDES_' + dfile)
 
-            write_if_exists(sf, pkg, 'FILERDEPENDSFLIST')
-            for dfile in (d.getVar('FILERDEPENDSFLIST_' + pkg) or "").split():
-                write_if_exists(sf, pkg, 'FILERDEPENDS_' + dfile)
+            write_if_exists(localdata, sf, pkg, 'FILERDEPENDSFLIST')
+            for dfile in (localdata.getVar('FILERDEPENDSFLIST_' + pkg) or "").split():
+                write_if_exists(localdata, sf, pkg, 'FILERDEPENDS_' + dfile)
 
             sf.write('%s_%s: %d\n' % ('PKGSIZE', pkg, total_size))
 
@@ -2076,9 +2103,11 @@ def read_libdep_files(d):
 python read_shlibdeps () {
     pkglibdeps = read_libdep_files(d)
 
+    localdata = exclude_pkgdata_vars(d)
+
     packages = d.getVar('PACKAGES').split()
     for pkg in packages:
-        rdepends = bb.utils.explode_dep_versions2(d.getVar('RDEPENDS_' + pkg) or "")
+        rdepends = bb.utils.explode_dep_versions2(localdata.getVar('RDEPENDS_' + pkg) or "")
         for dep in sorted(pkglibdeps[pkg]):
             # Add the dep if it's not already there, or if no comparison is set
             if dep not in rdepends:
@@ -2108,9 +2137,10 @@ python package_depchains() {
     prefixes  = (d.getVar('DEPCHAIN_PRE') or '').split()
 
     def pkg_adddeprrecs(pkg, base, suffix, getname, depends, d):
+        localdata = exclude_pkgdata_vars(d)
 
         #bb.note('depends for %s is %s' % (base, depends))
-        rreclist = bb.utils.explode_dep_versions2(d.getVar('RRECOMMENDS_' + pkg) or "")
+        rreclist = bb.utils.explode_dep_versions2(localdata.getVar('RRECOMMENDS_' + pkg) or "")
 
         for depend in sorted(depends):
             if depend.find('-native') != -1 or depend.find('-cross') != -1 or depend.startswith('virtual/'):
@@ -2129,9 +2159,10 @@ python package_depchains() {
         d.setVar('RRECOMMENDS_%s' % pkg, bb.utils.join_deps(rreclist, commasep=False))
 
     def pkg_addrrecs(pkg, base, suffix, getname, rdepends, d):
+        localdata = exclude_pkgdata_vars(d)
 
         #bb.note('rdepends for %s is %s' % (base, rdepends))
-        rreclist = bb.utils.explode_dep_versions2(d.getVar('RRECOMMENDS_' + pkg) or "")
+        rreclist = bb.utils.explode_dep_versions2(localdata.getVar('RRECOMMENDS_' + pkg) or "")
 
         for depend in sorted(rdepends):
             if depend.find('virtual-locale-') != -1:
@@ -2285,7 +2316,7 @@ python do_package () {
         package_qa_handle_error("var-undefined", msg, d)
         return
 
-    bb.build.exec_func("package_get_auto_pr", d)
+    bb.build.exec_func("package_convert_autoinc", d)
 
     ###########################################################################
     # Optimisations
@@ -2359,6 +2390,14 @@ addtask do_package_setscene
 python do_packagedata () {
     src = d.expand("${PKGDESTWORK}")
     dest = d.expand("${WORKDIR}/pkgdata-pdata-input")
+
+    bb.build.exec_func("package_get_auto_pr", d)
+    # Store this for later retrieval
+    data_file = src + d.expand("/${PN}_prservice")
+    with open(data_file, 'w') as fd:
+        fd.write('PRAUTO: %s\n' % d.getVar('PRAUTO'))
+        fd.write('PRSERV_PV_AUTOINC: %s\n' % d.getVar("PRSERV_PV_AUTOINC"))
+
     oe.path.copyhardlinktree(src, dest)
 }
 
diff --git a/meta/classes/packagedata.bbclass b/meta/classes/packagedata.bbclass
index 5475d5c0da..63274b4365 100644
--- a/meta/classes/packagedata.bbclass
+++ b/meta/classes/packagedata.bbclass
@@ -9,7 +9,10 @@ python read_subpackage_metadata () {
     }
 
     data = oe.packagedata.read_pkgdata(vars["PN"], d)
+    for key in data.keys():
+        d.setVar(key, data[key])
 
+    data = oe.packagedata.read_pkgdata("%s_prservice" % d.getVar('PN'), d)
     for key in data.keys():
         d.setVar(key, data[key])
 
@@ -36,7 +39,6 @@ python read_subpackage_metadata () {
 package_get_auto_pr[vardepsexclude] = "BB_TASKDEPDATA"
 python package_get_auto_pr() {
     import oe.prservice
-    import re
 
     def get_do_package_hash(pn):
         if d.getVar("BB_RUNTASK") != "do_package":
@@ -44,7 +46,7 @@ python package_get_auto_pr() {
             for dep in taskdepdata:
                 if taskdepdata[dep][1] == "do_package" and taskdepdata[dep][0] == pn:
                     return taskdepdata[dep][6]
-        return d.getVar("BB_TASKHASH")
+        return None
 
     # Support per recipe PRSERV_HOST
     pn = d.getVar('PN')
@@ -56,8 +58,7 @@ python package_get_auto_pr() {
 
     # PR Server not active, handle AUTOINC
     if not d.getVar('PRSERV_HOST'):
-        if 'AUTOINC' in pkgv:
-            d.setVar("PKGV", pkgv.replace("AUTOINC", "0"))
+        d.setVar("PRSERV_PV_AUTOINC", "0")
         return
 
     auto_pr = None
@@ -66,6 +67,14 @@ python package_get_auto_pr() {
     pkgarch = d.getVar("PACKAGE_ARCH")
     checksum = get_do_package_hash(pn)
 
+    # If do_package isn't in the dependencies, we can't get the checksum...
+    if not checksum:
+        bb.warn('Task %s requested do_package unihash, but it was not available.' % d.getVar('BB_RUNTASK'))
+        #taskdepdata = d.getVar("BB_TASKDEPDATA", False)
+        #for dep in taskdepdata:
+        #    bb.warn('%s:%s = %s' % (taskdepdata[dep][0], taskdepdata[dep][1], taskdepdata[dep][6]))
+        return
+
     if d.getVar('PRSERV_LOCKDOWN'):
         auto_pr = d.getVar('PRAUTO_' + version + '_' + pkgarch) or d.getVar('PRAUTO_' + version) or None
         if auto_pr is None:
@@ -82,7 +91,7 @@ python package_get_auto_pr() {
                 srcpv = bb.fetch2.get_srcrev(d)
                 base_ver = "AUTOINC-%s" % version[:version.find(srcpv)]
                 value = conn.getPR(base_ver, pkgarch, srcpv)
-                d.setVar("PKGV", pkgv.replace("AUTOINC", str(value)))
+                d.setVar("PRSERV_PV_AUTOINC", str(value))
 
             auto_pr = conn.getPR(version, pkgarch, checksum)
     except Exception as e:
diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index 353caacef9..65b4432c63 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -208,6 +208,7 @@ PF = "${PN}-${EXTENDPE}${PV}-${PR}"
 EXTENDPE = "${@['','${PE}_'][int(d.getVar('PE') or 0) > 0]}"
 P = "${PN}-${PV}"
 
+PRSERV_PV_AUTOINC = "AUTOINC"
 PRAUTO = ""
 EXTENDPRAUTO = "${@['.${PRAUTO}', ''][not d.getVar('PRAUTO')]}"
 PRAUTOINX = "${PF}"
-- 
2.17.1

Re: [OE-core] [master][PATCH v2 0/6] Allow PR Service and hash equiv togther

[Richard Purdie]

On Wed, 2020-08-26 at 06:27 -0500, Mark Hatle wrote:
> v2:
> 
> Most comments have been addressed to create a v2 version.  I've refactored
> the commits to make a few things more clear, basically moving code around
> and fixing minor issues BEFORE the big patch.
> 
> Before there were two patches that together implemented the PR Serv/Hash
> work.  This has been combined into a single patch and the oe_nohash stuff
> has simply been removed as no longer applicable.
> 
> The only comment that was NOT addressed in this was the suggestion to
> sed the pkgdata files in do_packagedata.  I'm hesitent to do this as
> sed with ${...} in them has proven to be fragile for me in the past.  If
> we decide that is necessary, I'd suggest we start with this set and then
> make that change after this proves to work (or not).

Sorry to keep on about this but I am worried about code readability and
trying not to let the code get too unreadable (we have to try).

Your shell expansion issue is a very valid concern. How about setting
the variables we expand late to "@XXXX@" and then using that as the
expression to sed later?

I'm also wondering if we should just inject the @XXXX@ change (instead
of the delVar) into d directly at the start of do_package itself, where
the existing     bb.build.exec_func("package_get_auto_pr", d) call is?

That would then remove all the confusing localdata changes?

Cheers,

Richard

Re: [OE-core] [master][PATCH v2 0/6] Allow PR Service and hash equiv togther

[Mark Hatle]

On 8/26/20 6:48 AM, Richard Purdie wrote:
> On Wed, 2020-08-26 at 06:27 -0500, Mark Hatle wrote:
>> v2:
>>
>> Most comments have been addressed to create a v2 version.  I've refactored
>> the commits to make a few things more clear, basically moving code around
>> and fixing minor issues BEFORE the big patch.
>>
>> Before there were two patches that together implemented the PR Serv/Hash
>> work.  This has been combined into a single patch and the oe_nohash stuff
>> has simply been removed as no longer applicable.
>>
>> The only comment that was NOT addressed in this was the suggestion to
>> sed the pkgdata files in do_packagedata.  I'm hesitent to do this as
>> sed with ${...} in them has proven to be fragile for me in the past.  If
>> we decide that is necessary, I'd suggest we start with this set and then
>> make that change after this proves to work (or not).
> 
> Sorry to keep on about this but I am worried about code readability and
> trying not to let the code get too unreadable (we have to try).
> 
> Your shell expansion issue is a very valid concern. How about setting
> the variables we expand late to "@XXXX@" and then using that as the
> expression to sed later?

This was actually the first thing I tried, and it was a complete failure.  The
problem is that all of the variables that we play with (not just PKGV/PKGR, as
this impacts the RRECOMMENDS, RDEPENDS, etc..) all need this replacement done.

Then they reference PKGR, which references EXTENDPRAUTO, which is a python
variable which references PRAUTO which is what we actually care about.

So really what we should be doing is putting in 'PRAUTO' (@PRAUTO@) as the
replacement value and then replacing that later -- but we can't just do that
since if it's blank then EXTENDPRAUTO works differently then if it's set.

So we would need to wrap @EXTENDPRAUTO@, which "works".. but then we have to be
able to expand it on the reading of the data as well.  Which means modifications
of every routine that reads the variables from the pkgdata store.  But the
various places where the variables are read in don't have access to the list of
variables to expand -- so we might have to assume @...@ is used, which may work
but I'm still worried about how complicated it gets.

With the current usage, the only place I've seen issues (buildhistory) is where
they read the pkgdata directly and it never ends up in a datastore.  Assuming it
goes into a datastore then it's automatically expanded.  (Other then
buildhistory, I think all of the reader routines are located in
lib/oe/packagedata.py -- so we should be able to scan for this stuff in the
general case.)

(I also don't want to move EXTENDPRAUTO into say do_package, because I believe
the implementation of this is going to need to change in the future to support
the use-case where a standard distribution is produced, a user consumes it --
modifies a package and provides a variant of a few patches and just wants to
extend the EXTENDPRAUTO with additional release information.  I.e. initial
version of EXTENDPRAUTO is .1, where the user's customization makes it .1.1...)

> I'm also wondering if we should just inject the @XXXX@ change (instead
> of the delVar) into d directly at the start of do_package itself, where
> the existing     bb.build.exec_func("package_get_auto_pr", d) call is?
> 
> That would then remove all the confusing localdata changes?

Part of the reason I moved the change to a function as I thought it made it
easier to read and understand the why.

--Mark

> Cheers,
> 
> Richard
> 

Re: [OE-core] [master][PATCH v2 0/6] Allow PR Service and hash equiv togther

[Richard Purdie]

On Wed, 2020-08-26 at 07:14 -0500, Mark Hatle wrote:
> This was actually the first thing I tried, and it was a complete
> failure.  The problem is that all of the variables that we play with
> (not just PKGV/PKGR, as this impacts the RRECOMMENDS, RDEPENDS,
> etc..) all need this replacement done.
> 
> Then they reference PKGR, which references EXTENDPRAUTO, which is a
> python variable which references PRAUTO which is what we actually
> care about.
> 
> So really what we should be doing is putting in 'PRAUTO' (@PRAUTO@)
> as the replacement value and then replacing that later -- but we
> can't just do that since if it's blank then EXTENDPRAUTO works
> differently then if it's set.
> 
> So we would need to wrap @EXTENDPRAUTO@, which "works".. but then we
> have to be able to expand it on the reading of the data as
> well.  Which means modifications of every routine that reads the
> variables from the pkgdata store.  But the various places where the
> variables are read in don't have access to the list of variables to
> expand -- so we might have to assume @...@ is used, which may work
> but I'm still worried about how complicated it gets.

I take the point about needing to use EXTENDPRAUTO, that makes sense.

So if in do_package we set EXTENDPRAUTO = "@EXTENDPRAUTO@", then in
do_packagedata we set PRAUTO then do a sed @EXTENDPRAUTO@ to
d.getVar("EXTENDPRAUTO"), we should then have the right data from that
point on?

I know its not a clean abstraction to two variables like you have now
but I think if we document what is happening, that doesn't matter. Its
not as if we're going to extend this to other variables regularly.

> With the current usage, the only place I've seen issues
> (buildhistory) is where
> they read the pkgdata directly and it never ends up in a
> datastore.  Assuming it
> goes into a datastore then it's automatically expanded.  (Other then
> buildhistory, I think all of the reader routines are located in
> lib/oe/packagedata.py -- so we should be able to scan for this stuff
> in the
> general case.)
> 
> (I also don't want to move EXTENDPRAUTO into say do_package, because
> I believe
> the implementation of this is going to need to change in the future
> to support
> the use-case where a standard distribution is produced, a user
> consumes it --
> modifies a package and provides a variant of a few patches and just
> wants to
> extend the EXTENDPRAUTO with additional release information.  I.e.
> initial
> version of EXTENDPRAUTO is .1, where the user's customization makes
> it .1.1...)

Agreed, but I think we can preserve it as above.

> > I'm also wondering if we should just inject the @XXXX@ change
> > (instead
> > of the delVar) into d directly at the start of do_package itself,
> > where
> > the existing     bb.build.exec_func("package_get_auto_pr", d) call
> > is?
> > 
> > That would then remove all the confusing localdata changes?
> 
> Part of the reason I moved the change to a function as I thought it
> made it easier to read and understand the why.

Yes, having the function helps but it could be better again if we don't
need multiple datastores...

Cheers,

Richard