| Message ID | 20240227-clean-tasks-notes-v2-1-35fb627e9ca0@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Discourage using do_cleansstate and do_cleanall | expand |
Hi Luca, On 2/27/24 11:59, Luca Ceresoli via lists.yoctoproject.org wrote: > [You don't often get email from luca.ceresoli=bootlin.com@lists.yoctoproject.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > do_cleanall can produce failures when used in legitimate cases, wuch as s/wuch/such/ > with recipe variants (foo and foo-native) or a shared DL_DIR. This is why > it is forbidden when writing tests that will run on the autobuilders > (https://docs.yoctoproject.org/test-manual/intro.html?highlight=cleanall#considerations-when-writing-tests). > > Reword the documentation to clearly discourage, provide a safe alternative > (bitbake -f fecth), and the rationale with an example. s/fecth/fetch/ I'm pretty sure it should be `bitbake -f -c fetch` as well, from bitbake --help: """ -f, --force Force the specified targets/task to run (invalidating any existing stamp file). -c CMD, --cmd=CMD Specify the task to execute. The exact options available depend on the metadata. Some examples might be 'compile' or 'populate_sysroot' or 'listtasks' may give a list of the tasks available. """ One of the issues with using -f is that it taints the build as far as I remember and you'll get a warning for all subsequent builds until you clean stuff? > > Reported-by: Sam Liddicott > Link: https://bootlin.com/blog/yocto-sharing-the-sstate-cache-and-download-directories/#comment-2650335 > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > documentation/ref-manual/tasks.rst | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/documentation/ref-manual/tasks.rst b/documentation/ref-manual/tasks.rst > index 0db960b22f80..ebaa03dc7613 100644 > --- a/documentation/ref-manual/tasks.rst > +++ b/documentation/ref-manual/tasks.rst > @@ -470,9 +470,28 @@ You can run this task using BitBake as follows:: > > $ bitbake -c cleanall recipe > > -Typically, you would not normally use the :ref:`ref-tasks-cleanall` task. Do so only > -if you want to start fresh with the :ref:`ref-tasks-fetch` > -task. > +You should never use the :ref:`ref-tasks-cleanall` task in a normal > +scenario. If you want to start fresh with the :ref:`ref-tasks-fetch` task, Maybe we could suggest here that if one needs to do this, the recipe needs to be fixed so that this isn't needed. > +use instead:: > + > + $ bitbake -f fetch recipe > + I think we're missing a -c before fetch > +.. note:: > + > + The reason to prefer ``bitbake -f fetch`` is that the missing -c before fetch I believe > + :ref:`ref-tasks-cleanall` task would break in some cases, such as:: > + > + $ bitbake -c fetch recipe > + $ bitbake -c cleanall recipe-native > + $ bitbake -c unpack recipe > + > + because after step 1 there is a stamp file for the > + :ref:`ref-tasks-fetch` task of ``recipe``, and it won't be removed at > + step 2 because step 2 uses a different work dirctory. So the unpack task s/dirctory/directory/ I completely glossed over cleanall being for recipe-native in the example and was wondering why it would be in a different work directory. It'd be nice to specify this is the case because the first and third steps are for the target recipe and the second for the native recipe. They are in different work directories, and their stamp files are there. However, the source code in tarball/git form - if identical between target and native recipes - are stored in a directory common to the target and native recipes (DL_DIR). > + at step 3 will try to extract the downloaded archive and fail as it has > + been deleted in step 2. > + It's been a few months since I've played with Yocto but I think the example above will work just fine? -c unpack will still go through the task dependencies if they aren't done already? I don't really see a usecase for bitbake to NOT do the dependencies first? Might be a very incorrect recollection though :) > + This can be even more tricky if using a shared download directory. > It'd be nice to give a link to the documentation about shared download directory (if theres' one) or at the very least the variable name. I think we could also be a bit more explicit about the risks here. If a download directory (DL_DIR) is shared between different bitbake running at the same time (e.g. multiple bitbakes on the same machine, or spread over the network), removing stamps and source tarballs/git repo from one bitbake would likely break the other bitbakes trying to get the source from the common place that now doesn't exist anymore. Cheers, Quentin
Hi Quentin, thanks for you reviews. On Tue, 27 Feb 2024 12:24:03 +0100 Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > Hi Luca, > > On 2/27/24 11:59, Luca Ceresoli via lists.yoctoproject.org wrote: > > [You don't often get email from luca.ceresoli=bootlin.com@lists.yoctoproject.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > > > do_cleanall can produce failures when used in legitimate cases, wuch as > > s/wuch/such/ Thanks for noticing all of my typos... will fix. > One of the issues with using -f is that it taints the build as far as I > remember and you'll get a warning for all subsequent builds until you > clean stuff? Yes. But I don't see any other way out other than removing the entire tmp dir... the warning is maybe annoying but harmless anyway. > > > > > Reported-by: Sam Liddicott > > Link: https://bootlin.com/blog/yocto-sharing-the-sstate-cache-and-download-directories/#comment-2650335 > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > documentation/ref-manual/tasks.rst | 25 ++++++++++++++++++++++--- > > 1 file changed, 22 insertions(+), 3 deletions(-) > > > > diff --git a/documentation/ref-manual/tasks.rst b/documentation/ref-manual/tasks.rst > > index 0db960b22f80..ebaa03dc7613 100644 > > --- a/documentation/ref-manual/tasks.rst > > +++ b/documentation/ref-manual/tasks.rst > > @@ -470,9 +470,28 @@ You can run this task using BitBake as follows:: > > > > $ bitbake -c cleanall recipe > > > > -Typically, you would not normally use the :ref:`ref-tasks-cleanall` task. Do so only > > -if you want to start fresh with the :ref:`ref-tasks-fetch` > > -task. > > +You should never use the :ref:`ref-tasks-cleanall` task in a normal > > +scenario. If you want to start fresh with the :ref:`ref-tasks-fetch` task, > > Maybe we could suggest here that if one needs to do this, the recipe > needs to be fixed so that this isn't needed. Not sure about this. As I understand it, do_cleanall should just not be needed in normal cases, full stop. I personally use it when giving trainings because I want to show that some recipes are actually downloading and building, and not reusing the whole sstate cache from my previous training. However I think this goes out of what should be in a manual, and I cannot see other valid uses for do_cleanall. > > +use instead:: > > + > > + $ bitbake -f fetch recipe > > + > > I think we're missing a -c before fetch Right! Here and at other places. > I completely glossed over cleanall being for recipe-native in the > example and was wondering why it would be in a different work directory. > It'd be nice to specify this is the case because the first and third > steps are for the target recipe and the second for the native recipe. > They are in different work directories, and their stamp files are there. > However, the source code in tarball/git form - if identical between > target and native recipes - are stored in a directory common to the > target and native recipes (DL_DIR). > > > + at step 3 will try to extract the downloaded archive and fail as it has > > + been deleted in step 2. > > + > > It's been a few months since I've played with Yocto but I think the > example above will work just fine? It definitely fails. You can try it easily: bitbake -c fetch zstd bitbake -c cleanall zstd-native bitbake -c unpack zstd > -c unpack will still go through the task dependencies if they aren't > done already? I don't really see a usecase for bitbake to NOT do the > dependencies first? The point is: "if they aren't done already". Here zstd has a stamp for do_fetch after step 1 so at step 3 it is considered "done already". > > + This can be even more tricky if using a shared download directory. > > > > It'd be nice to give a link to the documentation about shared download > directory (if theres' one) or at the very least the variable name. OK, adding "(see :term:`DL_DIR`)" at the end of the sentence. > I think we could also be a bit more explicit about the risks here. > > If a download directory (DL_DIR) is shared between different bitbake > running at the same time (e.g. multiple bitbakes on the same machine, or > spread over the network), removing stamps and source tarballs/git repo > from one bitbake would likely break the other bitbakes trying to get the > source from the common place that now doesn't exist anymore. That's the example that was in v1 of this patch. However I now added an example of the error happening without a shared DL_DIR, which is much more relevant because you don't even need a shared dir. I then removed the initial example as I don't think we need a verbose list of examples here. i think we need to say: don't do this because it can fail even in the basic case. If anything should be done about the last sentence would rather be to remove it entirely. Luca
Hi Luca, On 2/27/24 13:00, Luca Ceresoli wrote: [...] >> One of the issues with using -f is that it taints the build as far as I >> remember and you'll get a warning for all subsequent builds until you >> clean stuff? > > Yes. But I don't see any other way out other than removing the entire > tmp dir... the warning is maybe annoying but harmless anyway. > I worked for a company whose build system was blurting out lines and lines of warnings and it took me some time to teach people to not ignore those warnings because sometimes those warnings were actually important, so that's where I'm coming from :) >> >>> >>> Reported-by: Sam Liddicott >>> Link: https://bootlin.com/blog/yocto-sharing-the-sstate-cache-and-download-directories/#comment-2650335 >>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> >>> --- >>> documentation/ref-manual/tasks.rst | 25 ++++++++++++++++++++++--- >>> 1 file changed, 22 insertions(+), 3 deletions(-) >>> >>> diff --git a/documentation/ref-manual/tasks.rst b/documentation/ref-manual/tasks.rst >>> index 0db960b22f80..ebaa03dc7613 100644 >>> --- a/documentation/ref-manual/tasks.rst >>> +++ b/documentation/ref-manual/tasks.rst >>> @@ -470,9 +470,28 @@ You can run this task using BitBake as follows:: >>> >>> $ bitbake -c cleanall recipe >>> >>> -Typically, you would not normally use the :ref:`ref-tasks-cleanall` task. Do so only >>> -if you want to start fresh with the :ref:`ref-tasks-fetch` >>> -task. >>> +You should never use the :ref:`ref-tasks-cleanall` task in a normal >>> +scenario. If you want to start fresh with the :ref:`ref-tasks-fetch` task, >> >> Maybe we could suggest here that if one needs to do this, the recipe >> needs to be fixed so that this isn't needed. > > Not sure about this. As I understand it, do_cleanall should just not be > needed in normal cases, full stop. I personally use it when giving > trainings because I want to show that some recipes are actually > downloading and building, and not reusing the whole sstate cache from > my previous training. However I think this goes out of what should be in > a manual, and I cannot see other valid uses for do_cleanall. > I've seen people "fix" pipelines by just cleanall stuff when it was not working and it magically worked then and that was good enough for them. In a way, that isn't really our problem if people want to fix it this way, except if it impacts our upstream recipes, but I understand we need to draw the line somewhere and maybe that's where we want it to be :) [...] >>> + at step 3 will try to extract the downloaded archive and fail as it has >>> + been deleted in step 2. >>> + >> >> It's been a few months since I've played with Yocto but I think the >> example above will work just fine? > > It definitely fails. You can try it easily: > > bitbake -c fetch zstd > bitbake -c cleanall zstd-native > bitbake -c unpack zstd > >> -c unpack will still go through the task dependencies if they aren't >> done already? I don't really see a usecase for bitbake to NOT do the >> dependencies first? > > The point is: "if they aren't done already". Here zstd has a > stamp for do_fetch after step 1 so at step 3 it is considered "done > already". > Fair point, thanks for taking the time to explain. >>> + This can be even more tricky if using a shared download directory. >>> [...] >> I think we could also be a bit more explicit about the risks here. >> >> If a download directory (DL_DIR) is shared between different bitbake >> running at the same time (e.g. multiple bitbakes on the same machine, or >> spread over the network), removing stamps and source tarballs/git repo >> from one bitbake would likely break the other bitbakes trying to get the >> source from the common place that now doesn't exist anymore. > > That's the example that was in v1 of this patch. However I now added an > example of the error happening without a shared DL_DIR, which is much > more relevant because you don't even need a shared dir. I then removed > the initial example as I don't think we need a verbose list of examples > here. i think we need to say: don't do this because it can fail > even in the basic case. > > If anything should be done about the last sentence would rather be to > remove it entirely. > I could suggest the following rewording: """ Note that this also applies to bitbake from concurrent processes when a shared download directory (:term:`DL_DIR`) is setup. """ In any case, the original is fine with me with the few typo fixed :) Cheers, Quentin
Hi Quentin, On Tue, 27 Feb 2024 13:23:52 +0100 Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > Hi Luca, > > On 2/27/24 13:00, Luca Ceresoli wrote: > [...] > >> One of the issues with using -f is that it taints the build as far as I > >> remember and you'll get a warning for all subsequent builds until you > >> clean stuff? > > > > Yes. But I don't see any other way out other than removing the entire > > tmp dir... the warning is maybe annoying but harmless anyway. > > > > I worked for a company whose build system was blurting out lines and > lines of warnings and it took me some time to teach people to not ignore > those warnings because sometimes those warnings were actually important, > so that's where I'm coming from :) While I feel your pain, I think no documentation can prevent people from actively shooting on their feet. So we can add this note for who is willing to read it, I guess... [...] > >>> + This can be even more tricky if using a shared download directory. > >>> > > [...] > > >> I think we could also be a bit more explicit about the risks here. > >> > >> If a download directory (DL_DIR) is shared between different bitbake > >> running at the same time (e.g. multiple bitbakes on the same machine, or > >> spread over the network), removing stamps and source tarballs/git repo > >> from one bitbake would likely break the other bitbakes trying to get the > >> source from the common place that now doesn't exist anymore. > > > > That's the example that was in v1 of this patch. However I now added an > > example of the error happening without a shared DL_DIR, which is much > > more relevant because you don't even need a shared dir. I then removed > > the initial example as I don't think we need a verbose list of examples > > here. i think we need to say: don't do this because it can fail > > even in the basic case. > > > > If anything should be done about the last sentence would rather be to > > remove it entirely. > > > > I could suggest the following rewording: > > """ > Note that this also applies to bitbake from concurrent processes when a > shared download directory (:term:`DL_DIR`) is setup. > """ Sounds better. Took your version as-is for v3, thanks! > In any case, the original is fine with me with the few typo fixed :) Cool. v3 incoming. Luca
diff --git a/documentation/ref-manual/tasks.rst b/documentation/ref-manual/tasks.rst index 0db960b22f80..ebaa03dc7613 100644 --- a/documentation/ref-manual/tasks.rst +++ b/documentation/ref-manual/tasks.rst @@ -470,9 +470,28 @@ You can run this task using BitBake as follows:: $ bitbake -c cleanall recipe -Typically, you would not normally use the :ref:`ref-tasks-cleanall` task. Do so only -if you want to start fresh with the :ref:`ref-tasks-fetch` -task. +You should never use the :ref:`ref-tasks-cleanall` task in a normal +scenario. If you want to start fresh with the :ref:`ref-tasks-fetch` task, +use instead:: + + $ bitbake -f fetch recipe + +.. note:: + + The reason to prefer ``bitbake -f fetch`` is that the + :ref:`ref-tasks-cleanall` task would break in some cases, such as:: + + $ bitbake -c fetch recipe + $ bitbake -c cleanall recipe-native + $ bitbake -c unpack recipe + + because after step 1 there is a stamp file for the + :ref:`ref-tasks-fetch` task of ``recipe``, and it won't be removed at + step 2 because step 2 uses a different work dirctory. So the unpack task + at step 3 will try to extract the downloaded archive and fail as it has + been deleted in step 2. + + This can be even more tricky if using a shared download directory. .. _ref-tasks-cleansstate:
do_cleanall can produce failures when used in legitimate cases, wuch as with recipe variants (foo and foo-native) or a shared DL_DIR. This is why it is forbidden when writing tests that will run on the autobuilders (https://docs.yoctoproject.org/test-manual/intro.html?highlight=cleanall#considerations-when-writing-tests). Reword the documentation to clearly discourage, provide a safe alternative (bitbake -f fecth), and the rationale with an example. Reported-by: Sam Liddicott Link: https://bootlin.com/blog/yocto-sharing-the-sstate-cache-and-download-directories/#comment-2650335 Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- documentation/ref-manual/tasks.rst | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)