| Message ID | 20240515115620.420558-5-richard.purdie@linuxfoundation.org |
|---|---|
| State | Accepted, archived |
| Commit | b84eec5c4cbf4b39d6712800dd0d2fe5337721cb |
| Headers | show |
| Series | [01/10] recipes: Start WORKDIR -> UNPACKDIR transition | expand |
> -----Original Message----- > From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Richard Purdie > Sent: den 15 maj 2024 13:56 > To: openembedded-core@lists.openembedded.org > Subject: [OE-core] [PATCH 05/10] base: Switch UNPACKDIR to a subdir of WORKDIR > > Change do_unpack to unpack files to a subdirectory of WORKDIR instead of WORKDIR > itself. There are several good reasons for this but it is mainly about being able > to isolate the output of the unpack task and tell the files apart from other things > which are created in workdir (logs, sysroots, temp dirs and more). > > This means that when the do_unpack task reruns, we can clean UNPACKDIR and know > we have a standard point to start builds from. > > It also makes code in tools like devtool and recipetool easier. > > To reduce the impact to users, if a subdirectory under UNPACKDIR matches > the first subdirectory under WORKDIR of S, that directory is moved into position > inside WORKDIR. This preserves the behaviour of S = "${WORKDIR}/git", > S = "${WORKDIR}/${BPN}" and other commonly used source directory setups. > > The directory is moved since sadly many autotools based projects can't cope with > symlinks in their paths. Would it be an option to create a symbolic link by default, and instead let the autotools bbclass replace it with a moved directory? If it is in fact only autotools based projects that have this problem. > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> > --- > meta/classes-global/base.bbclass | 21 ++++++++++++++++++--- > meta/conf/bitbake.conf | 2 +- > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/meta/classes-global/base.bbclass b/meta/classes- > global/base.bbclass > index 066f3848f7c..cdce0538273 100644 > --- a/meta/classes-global/base.bbclass > +++ b/meta/classes-global/base.bbclass > @@ -153,20 +153,35 @@ python base_do_fetch() { > } > > addtask unpack after do_fetch > -do_unpack[dirs] = "${UNPACKDIR}" > - > -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')}" > +do_unpack[cleandirs] = "${UNPACKDIR}" > > python base_do_unpack() { > + import shutil > + > src_uri = (d.getVar('SRC_URI') or "").split() > if not src_uri: > return > > + sourcedir = d.getVar('S') > + basedir = None > + workdir = d.getVar('WORKDIR') > + unpackdir = d.getVar('UNPACKDIR') > + if sourcedir.startswith(workdir) and not sourcedir.startswith(unpackdir): > + basedir = sourcedir.replace(workdir, '').strip("/").split('/')[0] > + bb.utils.remove(sourcedir, True) This remove() seems wrong and should not be needed. There are two cases here: 1) either ${S} == ${WORKDIR}, in which case the above will remove ${WORKDIR}, which is sure to lead to problems, or 2) ${S} == ${WORKDIR}/foo[/...], in which case the removal of workdir + '/' + basedir below will also remove ${S} as basedir == "foo". > + if basedir: > + bb.utils.remove(workdir + '/' + basedir, True) > + > try: > fetcher = bb.fetch2.Fetch(src_uri, d) > fetcher.unpack(d.getVar('UNPACKDIR')) > except bb.fetch2.BBFetchException as e: > bb.fatal("Bitbake Fetcher Error: " + repr(e)) > + > + if basedir and os.path.exists(unpackdir + '/' + basedir): > + # Compatibility magic to ensure ${WORKDIR}/git and ${WORKDIR}/${BP} > + # as often used in S work as expected. > + shutil.move(unpackdir + '/' + basedir, workdir + '/' + basedir) > } > > SSTATETASKS += "do_deploy_source_date_epoch" > diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf > index b2c500d8739..75c850760f6 100644 > --- a/meta/conf/bitbake.conf > +++ b/meta/conf/bitbake.conf > @@ -405,7 +405,7 @@ STAMP = > "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" > STAMPCLEAN = "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/*-*" > BASE_WORKDIR ?= "${TMPDIR}/work" > WORKDIR = "${BASE_WORKDIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" > -UNPACKDIR ??= "${WORKDIR}" > +UNPACKDIR ??= "${WORKDIR}/sources-unpack" "sources-unpack" does not exactly fall off the tongue. Would "unpack" or "unpacked" be alternatives? > T = "${WORKDIR}/temp" > D = "${WORKDIR}/image" > S = "${WORKDIR}/${BP}" > -- > 2.40.1 //Peter
On Wed, 2024-05-15 at 17:42 +0000, Peter Kjellerstedt wrote: > > -----Original Message----- > > From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Richard Purdie > > Sent: den 15 maj 2024 13:56 > > To: openembedded-core@lists.openembedded.org > > Subject: [OE-core] [PATCH 05/10] base: Switch UNPACKDIR to a subdir of WORKDIR > > > > Change do_unpack to unpack files to a subdirectory of WORKDIR instead of WORKDIR > > itself. There are several good reasons for this but it is mainly about being able > > to isolate the output of the unpack task and tell the files apart from other things > > which are created in workdir (logs, sysroots, temp dirs and more). > > > > This means that when the do_unpack task reruns, we can clean UNPACKDIR and know > > we have a standard point to start builds from. > > > > It also makes code in tools like devtool and recipetool easier. > > > > To reduce the impact to users, if a subdirectory under UNPACKDIR matches > > the first subdirectory under WORKDIR of S, that directory is moved into position > > inside WORKDIR. This preserves the behaviour of S = "${WORKDIR}/git", > > S = "${WORKDIR}/${BPN}" and other commonly used source directory setups. > > > > The directory is moved since sadly many autotools based projects can't cope with > > symlinks in their paths. > > Would it be an option to create a symbolic link by default, and instead > let the autotools bbclass replace it with a moved directory? If it is > in fact only autotools based projects that have this problem. Maybe. I've reached the point with this where I really didn't want to try and explore that particular set of problems and I'd rather things were consistent. I'm not going to rule out trying to clean up and improve this somehow in the future but for now, it works, it solves the original major problem (cleanup of obsolete sources) and we have most of the major pieces like devtool working. It also doesn't look as different to users as moving S would from a documentation perspective. I'm a bit torn on what the end result for S should look like. I'm happy enough with the changes so far and I think the best thing to do is merge these, then consider if we do want to go further or not. > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> > > --- > > meta/classes-global/base.bbclass | 21 ++++++++++++++++++--- > > meta/conf/bitbake.conf | 2 +- > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/meta/classes-global/base.bbclass b/meta/classes- > > global/base.bbclass > > index 066f3848f7c..cdce0538273 100644 > > --- a/meta/classes-global/base.bbclass > > +++ b/meta/classes-global/base.bbclass > > @@ -153,20 +153,35 @@ python base_do_fetch() { > > } > > > > addtask unpack after do_fetch > > -do_unpack[dirs] = "${UNPACKDIR}" > > - > > -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')}" > > +do_unpack[cleandirs] = "${UNPACKDIR}" > > > > python base_do_unpack() { > > + import shutil > > + > > src_uri = (d.getVar('SRC_URI') or "").split() > > if not src_uri: > > return > > > > + sourcedir = d.getVar('S') > > + basedir = None > > + workdir = d.getVar('WORKDIR') > > + unpackdir = d.getVar('UNPACKDIR') > > + if sourcedir.startswith(workdir) and not sourcedir.startswith(unpackdir): > > + basedir = sourcedir.replace(workdir, '').strip("/").split('/')[0] > > + bb.utils.remove(sourcedir, True) > > This remove() seems wrong and should not be needed. There are two > cases here: > > 1) either ${S} == ${WORKDIR}, in which case the above will remove > ${WORKDIR}, which is sure to lead to problems, or Which is why S = WORKDIR has to become a hard error. We have to drop support for it. > 2) ${S} == ${WORKDIR}/foo[/...], in which case the removal of > workdir + '/' + basedir below will also remove ${S} as > basedir == "foo". You're right. This code did go through several iterations as I found all the weird corner cases this needs to support. Ultimately, I suspect it should probably unconditionally remove ${S}. I'd need yet another test run to try and identify if that causes any new weird issues though. > > > + if basedir: > > + bb.utils.remove(workdir + '/' + basedir, True) > > + > > try: > > fetcher = bb.fetch2.Fetch(src_uri, d) > > fetcher.unpack(d.getVar('UNPACKDIR')) > > except bb.fetch2.BBFetchException as e: > > bb.fatal("Bitbake Fetcher Error: " + repr(e)) > > + > > + if basedir and os.path.exists(unpackdir + '/' + basedir): > > + # Compatibility magic to ensure ${WORKDIR}/git and ${WORKDIR}/${BP} > > + # as often used in S work as expected. > > + shutil.move(unpackdir + '/' + basedir, workdir + '/' + basedir) > > } > > > > SSTATETASKS += "do_deploy_source_date_epoch" > > diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf > > index b2c500d8739..75c850760f6 100644 > > --- a/meta/conf/bitbake.conf > > +++ b/meta/conf/bitbake.conf > > @@ -405,7 +405,7 @@ STAMP = > > "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" > > STAMPCLEAN = "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/*-*" > > BASE_WORKDIR ?= "${TMPDIR}/work" > > WORKDIR = "${BASE_WORKDIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" > > -UNPACKDIR ??= "${WORKDIR}" > > +UNPACKDIR ??= "${WORKDIR}/sources-unpack" > > "sources-unpack" does not exactly fall off the tongue. > Would "unpack" or "unpacked" be alternatives? I don't feel they're as clear... Cheers, Richard
On Wed, 2024-05-15 at 12:56 +0100, Richard Purdie wrote: > diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf > index b2c500d8739..75c850760f6 100644 > --- a/meta/conf/bitbake.conf > +++ b/meta/conf/bitbake.conf > @@ -405,7 +405,7 @@ STAMP = > "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" > STAMPCLEAN = "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/*-*" > BASE_WORKDIR ?= "${TMPDIR}/work" > WORKDIR = "${BASE_WORKDIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" > -UNPACKDIR ??= "${WORKDIR}" > +UNPACKDIR ??= "${WORKDIR}/sources-unpack" > T = "${WORKDIR}/temp" > D = "${WORKDIR}/image" > S = "${WORKDIR}/${BP}" Why not use UNPACKDIR ??= "${WORKDIR}/sources" like it's done in the individual recipes? And shouldn't we do S = ?? "${UNPACKDIR}/${BP}" also? // Martin
On Thu, 2024-05-16 at 09:18 +0200, Martin Hundebøll wrote: > On Wed, 2024-05-15 at 12:56 +0100, Richard Purdie wrote: > > diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf > > index b2c500d8739..75c850760f6 100644 > > --- a/meta/conf/bitbake.conf > > +++ b/meta/conf/bitbake.conf > > @@ -405,7 +405,7 @@ STAMP = > > "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" > > STAMPCLEAN = "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/*-*" > > BASE_WORKDIR ?= "${TMPDIR}/work" > > WORKDIR = "${BASE_WORKDIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" > > -UNPACKDIR ??= "${WORKDIR}" > > +UNPACKDIR ??= "${WORKDIR}/sources-unpack" > > T = "${WORKDIR}/temp" > > D = "${WORKDIR}/image" > > S = "${WORKDIR}/${BP}" > > Why not use > > UNPACKDIR ??= "${WORKDIR}/sources" > > like it's done in the individual recipes? I think it is helpful for users to be able to tell the difference between a recipe where S is a subdirectory of this and when there is no subdirectory. I therefore left them visually different. > And shouldn't we do > > S = ?? "${UNPACKDIR}/${BP}" > > also? See my other emails. I'm torn on changing this. I've been hoping we could avoid extra directory levels, the extra pain of changing S everywhere and there is also some benefit to keeping extra unpacked files clearly separate from the other sources. We may end up doing it, I don't know. Cheers, Richard
On Wed, 2024-05-15 at 17:42 +0000, Peter Kjellerstedt wrote: > > > diff --git a/meta/classes-global/base.bbclass b/meta/classes- > > global/base.bbclass > > index 066f3848f7c..cdce0538273 100644 > > --- a/meta/classes-global/base.bbclass > > +++ b/meta/classes-global/base.bbclass > > @@ -153,20 +153,35 @@ python base_do_fetch() { > > } > > > > addtask unpack after do_fetch > > -do_unpack[dirs] = "${UNPACKDIR}" > > - > > -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')}" > > +do_unpack[cleandirs] = "${UNPACKDIR}" > > > > python base_do_unpack() { > > + import shutil > > + > > src_uri = (d.getVar('SRC_URI') or "").split() > > if not src_uri: > > return > > > > + sourcedir = d.getVar('S') > > + basedir = None > > + workdir = d.getVar('WORKDIR') > > + unpackdir = d.getVar('UNPACKDIR') > > + if sourcedir.startswith(workdir) and not > > sourcedir.startswith(unpackdir): > > + basedir = sourcedir.replace(workdir, > > '').strip("/").split('/')[0] > > + bb.utils.remove(sourcedir, True) > > This remove() seems wrong and should not be needed. There are two > cases here: > > 1) either ${S} == ${WORKDIR}, in which case the above will remove > ${WORKDIR}, which is sure to lead to problems, or > 2) ${S} == ${WORKDIR}/foo[/...], in which case the removal of > workdir + '/' + basedir below will also remove ${S} as > basedir == "foo". > > I did look into that remove() further and tried to make the removal unconditional. The challenge is that if S = UNPACKDIR, it will rmdir the directory and the code assumes UNPACKDIR to be created from the cleandirs. We don't want an empty S created though for the S != UNPACKDIR case. Making it conditional on sourcedir != unpackdir which was the original condition for that code block but that does still cause QA warnings for linux-yocto. I'm now convinced linux-yocto needs more work so I'm tempted to disable that QA check there for now, letting us improve base.bbclass. Cheers, Richard, running out of patience with hours long test cycles
diff --git a/meta/classes-global/base.bbclass b/meta/classes-global/base.bbclass index 066f3848f7c..cdce0538273 100644 --- a/meta/classes-global/base.bbclass +++ b/meta/classes-global/base.bbclass @@ -153,20 +153,35 @@ python base_do_fetch() { } addtask unpack after do_fetch -do_unpack[dirs] = "${UNPACKDIR}" - -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')}" +do_unpack[cleandirs] = "${UNPACKDIR}" python base_do_unpack() { + import shutil + src_uri = (d.getVar('SRC_URI') or "").split() if not src_uri: return + sourcedir = d.getVar('S') + basedir = None + workdir = d.getVar('WORKDIR') + unpackdir = d.getVar('UNPACKDIR') + if sourcedir.startswith(workdir) and not sourcedir.startswith(unpackdir): + basedir = sourcedir.replace(workdir, '').strip("/").split('/')[0] + bb.utils.remove(sourcedir, True) + if basedir: + bb.utils.remove(workdir + '/' + basedir, True) + try: fetcher = bb.fetch2.Fetch(src_uri, d) fetcher.unpack(d.getVar('UNPACKDIR')) except bb.fetch2.BBFetchException as e: bb.fatal("Bitbake Fetcher Error: " + repr(e)) + + if basedir and os.path.exists(unpackdir + '/' + basedir): + # Compatibility magic to ensure ${WORKDIR}/git and ${WORKDIR}/${BP} + # as often used in S work as expected. + shutil.move(unpackdir + '/' + basedir, workdir + '/' + basedir) } SSTATETASKS += "do_deploy_source_date_epoch" diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf index b2c500d8739..75c850760f6 100644 --- a/meta/conf/bitbake.conf +++ b/meta/conf/bitbake.conf @@ -405,7 +405,7 @@ STAMP = "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" STAMPCLEAN = "${STAMPS_DIR}/${MULTIMACH_TARGET_SYS}/${PN}/*-*" BASE_WORKDIR ?= "${TMPDIR}/work" WORKDIR = "${BASE_WORKDIR}/${MULTIMACH_TARGET_SYS}/${PN}/${PV}" -UNPACKDIR ??= "${WORKDIR}" +UNPACKDIR ??= "${WORKDIR}/sources-unpack" T = "${WORKDIR}/temp" D = "${WORKDIR}/image" S = "${WORKDIR}/${BP}"
Change do_unpack to unpack files to a subdirectory of WORKDIR instead of WORKDIR itself. There are several good reasons for this but it is mainly about being able to isolate the output of the unpack task and tell the files apart from other things which are created in workdir (logs, sysroots, temp dirs and more). This means that when the do_unpack task reruns, we can clean UNPACKDIR and know we have a standard point to start builds from. It also makes code in tools like devtool and recipetool easier. To reduce the impact to users, if a subdirectory under UNPACKDIR matches the first subdirectory under WORKDIR of S, that directory is moved into position inside WORKDIR. This preserves the behaviour of S = "${WORKDIR}/git", S = "${WORKDIR}/${BPN}" and other commonly used source directory setups. The directory is moved since sadly many autotools based projects can't cope with symlinks in their paths. Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> --- meta/classes-global/base.bbclass | 21 ++++++++++++++++++--- meta/conf/bitbake.conf | 2 +- 2 files changed, 19 insertions(+), 4 deletions(-)