On 20/06/2022 17:35, Richard Purdie wrote:
On Mon, 2022-06-20 at 13:56 +0200, Kristian Amlie wrote:Just for the record, and for any people googling this: Until this is fixed, just reverting the above commit works to fix this problem.Richard, correct me if I'm wrong, but I have not yet seen any fixes for this. And unfortunately this is too deep in Bitbake for me to fix it myself. Do you think it makes sense to revert that commit in the official poky branches? I mean, the consequences of this bug are unusually dire. Especially in combination with go's common pattern of putting all source code in one directory, externalsrc can wipe out most of the source code in the user's home directory.The situation is rather poor, yes :(. Sadly not many people want to get involved with the core enough to try and work on these kinds of issues. There are a couple of ways we could perhaps improve things. I've a couple of patches (untested but you'll get the idea). Firstly: diff --git a/meta/classes/externalsrc.bbclass b/meta/classes/externalsrc.bbclass index b2f216f361d..e2000d1c8d3 100644 --- a/meta/classes/externalsrc.bbclass +++ b/meta/classes/externalsrc.bbclass @@ -90,16 +90,18 @@ python () { # Since configure will likely touch ${S}, ensure only we lock so one task has access at a time d.appendVarFlag(task, "lockfiles", " ${S}/singletask.lock")- for funcname in [task, "base_" + task, "kernel_" + task]:+ for v in d.keys() + cleandirs = d.getVarFlag(v, "cleandirs", False) + if cleandirs: # We do not want our source to be wiped out, ever (kernel.bbclass does this for do_clean) - cleandirs = oe.recipeutils.split_var_value(d.getVarFlag(funcname, 'cleandirs', False) or '') + cleandirs = oe.recipeutils.split_var_value(cleandirs) setvalue = False for cleandir in cleandirs[:]: if oe.path.is_path_parent(externalsrc, d.expand(cleandir)): cleandirs.remove(cleandir) setvalue = True if setvalue: - d.setVarFlag(funcname, 'cleandirs', ' '.join(cleandirs)) + d.setVarFlag(v, 'cleandirs', ' '.join(cleandirs))fetch_tasks = ['do_fetch', 'do_unpack']# If we deltask do_patch, there's no dependency to ensure do_unpack gets run, so add one This looks simple enough - iterate and remove any problematic cleandirs references. There are several potential issues. Firstly the performance of d.keys() with a getVarFlag() call like that is dire. If every recipe did this the parsing time would be horrible. I don't know how badly it will affect things for a singdo want to give usersle recipe. We might be able to have bitbake save a lit of function names into a variable to have it reduce the performance impact if that became an issue. Secondly, it still might not catch every cleandirs for various reasons since they may be set dynamically or in other unusual ways. It is probably better than what we have today though.
I finally got a chance to run this through our whole test pipeline [1]. I think this is a good approach. You're right it might not catch every last occurrence, but it's much better than the current state, and it should catch most. And most importantly, it worked.
Regarding the performance: Will that actually be a big problem? The code in question is guarded by "if externalsrc", which is only true if the EXTERNALSRC variable is non-empty. Typically it will only be non-empty for one or a few recipes. The timings of our full QA pipeline fluctuate a little, but I did not see any *significant* slowdown, it was within 5%, and could just be a coincidence.
[1] Note: One missing ":" in the "for" statement.
My second idea: diff --git a/meta/classes/base.bbclass b/meta/classes/base.bbclass index 20968a50766..bfe9c124ee3 100644 --- a/meta/classes/base.bbclass +++ b/meta/classes/base.bbclass @@ -165,7 +165,8 @@ python base_do_fetch() { addtask unpack after do_fetch do_unpack[dirs] = "${WORKDIR}do want to give users"-do_unpack[cleandirs] = "${@d.getVar('S') if os.path.normpath(d.getVar('S')) != os.path.normpath(d.getVar('WORKDIR')) else os.path.join('${S}', 'patches')}"+SOURCE_CLEANDIRS = "${@d.getVar('S') if os.path.normpath(d.getVar('S')) != os.path.normpath(d.getVar('WORKDIR')) else os.path.join('${S}', 'patches')}" +do_unpack[cleandirs] = "${SOURCE_CLEANDIRS}"python base_do_unpack() {src_uri = (d.getVar('SRC_URI') or "").split() diff --git a/meta/classes/externalsrc.bbclass b/meta/classes/externalsrc.bbclass index 90792a737b1..f145e0bd1de 100644 --- a/meta/classes/externalsrc.bbclass +++ b/meta/classes/externalsrc.bbclass @@ -77,6 +77,8 @@ python () { # Dummy value because the default function can't be called with blank SRC_URI d.setVar('SRCPV', '999')+ d.setVar("SOURCE_CLEANDIRS", "")+ if d.getVar('CONFIGUREOPT_DEPTRACK') == '--disable-dependency-tracking': d.setVar('CONFIGUREOPT_DEPTRACK', '')@@ -90,7 +92,7 @@ python () {# Since configure will likely touch ${S}, ensure only we lock so one task has access at a time d.appendVarFlag(task, "lockfiles", " ${S}/singletask.lock")- for funcname in [task, "base_" + task, "kernel_" + task]:+ for funcname in [task]: # We do not want our source to be wiped out, ever (kernel.bbclass does this for do_clean) cleandirs = oe.recipeutils.split_var_value(d.getVarFlag(funcname, 'cleandirs', False) or '') setvalue = False diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass index 5d2f17c3be2..b7d3bc03955 100644 --- a/meta/classes/kernel.bbclassdo want to give users +++ b/meta/classes/kernel.bbclass @@ -172,8 +172,9 @@ inherit ${KERNEL_CLASSES} # We need to move these over to STAGING_KERNEL_DIR. We can't just # create the symlink in advance as the git fetcher can't cope with # the symlink. -do_unpack[cleandirs] += " ${S} ${STAGING_KERNEL_DIR} ${B} ${STAGING_KERNEL_BUILDDIR}" -do_clean[cleandirs] += " ${S} ${STAGING_KERNEL_DIR} ${B} ${STAGING_KERNEL_BUILDDIR}" +SOURCE_CLEANDIRS += "${STAGING_KERNEL_DIR}" +do_unpack[cleandirs] += "${B} ${STAGING_KERNEL_BUILDDIR}" +do_clean[cleandirs] += " ${SOURCE_CLEANDIRS} ${B} ${STAGING_KERNEL_BUILDDIR}" python do_symlink_kernsrc () { s = d.getVar("S") if s[-1] == '/': Which parameterises the places ${S} is placed into cleandirs and clears than in externalsrc. That should be more reliable in some scenarios but relies upon it not being added to cleandirs elsewhere. It is probably more surgical and neater but doesn't remove all the risk. I'm not sure we can ever give 100% guarantees though.
I didn't try this approach, but it seems less generic, and has more potential to break if cleandirs is used in other layers for example. Personally I would vote for the first option.
-- Kristian
OpenPGP_signature
Description: OpenPGP digital signature
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#167647): https://lists.openembedded.org/g/openembedded-core/message/167647 Mute This Topic: https://lists.openembedded.org/mt/91374926/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
