| Message ID | CAHUKmYaFA8_eHrSCqknXqDfPfkwySdjqxWKvo6thuAAmD4guNw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] kernel.bbclass: make do_symlink_kernelsrc reentrant | expand |
On Thu, 2023-12-21 at 22:49 +0100, Etienne Cordonnier via lists.openembedded.org wrote: > From: Etienne Cordonnier <ecordonnier@snap.com> > > The function do_symlink_kernsrc is not reentrant in the case where S is defined > to a non-default value. This causes build-failures e.g. when building linux-yocto, then updating > poky to a commit which modifies kernel.bbclass, and then building linux-yocto again. > > Bugzilla: https://bugzilla.yoctoproject.org/show_bug.cgi?id=15325 > > Tested with a recipe "my-custom-linux" which unpacks sources to a custom ${S} directory > and ran symlink_kernsrc several times: > $ bitbake -f -c symlink_kernsrc my-custom-linux Sorry for the delay in getting back to review this patch. I'm extremely worried about adding complexity into this function, particularly as that complexity is now adding in significant overhead. Firstly, can I ask why you're using a non-default directory for ${S}? Is this as a way of doing a kind of external source usage? Having spent time looking at what this code is doing, in normal usage, S gets set to STAGING_KERNEL_DIR, the source is unpacked there and none of this code triggers. For EXTERNALSRC, a symlink is put in place. The code should remove a symlink if present and create it with the current setup. In that sense, this patch isn't doing the right thing. I'm very tempted to say the "using a non default S value" just error out. Whilst I understand how we got to this level of complexity, I think it will just end up causing us pain in the long run and I'd rather just not support it. Cheers, Richard
Hi Richard, I also apologize for replying late. I was busy with other things and haven't found time to rework this patch. > Firstly, can I ask why you're using a non-default directory for ${S}? > Is this as a way of doing a kind of external source usage? My use-case is that the BSP I am using includes the source of the kernel as source alongside the BSP yocto layer (not in an extra git repository), and then points S to the directory containing the sources of the kernel without using the externalsrc class (as far as I know the externalsrc class isn't meant for this use-case, since there are several recipes provided as source in this way, and externalsrc only has one path pointing to the external source). I agree with you that the complexity of this function is very high, so not supporting a non-default S would be a better solution in this case. I'll try to rework the patch and test it with externalsrc as well. Étienne On Fri, Feb 9, 2024 at 6:36 PM Richard Purdie < richard.purdie@linuxfoundation.org> wrote: > On Thu, 2023-12-21 at 22:49 +0100, Etienne Cordonnier via > lists.openembedded.org wrote: > > From: Etienne Cordonnier <ecordonnier@snap.com> > > > > The function do_symlink_kernsrc is not reentrant in the case where S is > defined > > to a non-default value. This causes build-failures e.g. when building > linux-yocto, then updating > > poky to a commit which modifies kernel.bbclass, and then building > linux-yocto again. > > > > Bugzilla: > https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.yoctoproject.org_show-5Fbug.cgi-3Fid-3D15325&d=DwIFaQ&c=ncDTmphkJTvjIDPh0hpF_4vCHvabgGkICC2epckfdiw&r=AhkbNonVuMIGRfPx_Qj9TsRih1DULJTKUkSGa66m67E&m=e0iYk69Iih3UnIjABITtc6yS38bhv_6P4NMuSpVmmoSHk1-sSuvH3y702O0nTeZj&s=OtiPnSuoZjUXFgz9pxbGOOyjQmKPEq-OhEbhtsAmvZE&e= > > > > Tested with a recipe "my-custom-linux" which unpacks sources to a custom > ${S} directory > > and ran symlink_kernsrc several times: > > $ bitbake -f -c symlink_kernsrc my-custom-linux > > Sorry for the delay in getting back to review this patch. I'm extremely > worried about adding complexity into this function, particularly as > that complexity is now adding in significant overhead. > > Firstly, can I ask why you're using a non-default directory for ${S}? > Is this as a way of doing a kind of external source usage? > > Having spent time looking at what this code is doing, in normal usage, > S gets set to STAGING_KERNEL_DIR, the source is unpacked there and none > of this code triggers. > > For EXTERNALSRC, a symlink is put in place. The code should remove a > symlink if present and create it with the current setup. In that sense, > this patch isn't doing the right thing. > > I'm very tempted to say the "using a non default S value" just error > out. > > Whilst I understand how we got to this level of complexity, I think it > will just end up causing us pain in the long run and I'd rather just > not support it. > > Cheers, > > Richard >
On 27/05/2024 11:45:58+0200, Etienne Cordonnier via lists.openembedded.org wrote: > Hi Richard, > > I also apologize for replying late. I was busy with other things and > haven't found time to rework this patch. > > > Firstly, can I ask why you're using a non-default directory for ${S}? > > Is this as a way of doing a kind of external source usage? > > My use-case is that the BSP I am using includes the source of the kernel as > source alongside the BSP yocto layer (not in an extra git repository), and > then points S to the directory containing the sources of the kernel without > using the externalsrc class (as far as I know the externalsrc class isn't > meant for this use-case, since there are several recipes provided as source > in this way, and externalsrc only has one path pointing to the external > source). > You should rather educate your vendor to provide a proper git repository for the kernel. > I agree with you that the complexity of this function is very high, so not > supporting a non-default S would be a better solution in this case. I'll > try to rework the patch and test it with externalsrc as well. > > Étienne > > On Fri, Feb 9, 2024 at 6:36 PM Richard Purdie < > richard.purdie@linuxfoundation.org> wrote: > > > On Thu, 2023-12-21 at 22:49 +0100, Etienne Cordonnier via > > lists.openembedded.org wrote: > > > From: Etienne Cordonnier <ecordonnier@snap.com> > > > > > > The function do_symlink_kernsrc is not reentrant in the case where S is > > defined > > > to a non-default value. This causes build-failures e.g. when building > > linux-yocto, then updating > > > poky to a commit which modifies kernel.bbclass, and then building > > linux-yocto again. > > > > > > Bugzilla: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.yoctoproject.org_show-5Fbug.cgi-3Fid-3D15325&d=DwIFaQ&c=ncDTmphkJTvjIDPh0hpF_4vCHvabgGkICC2epckfdiw&r=AhkbNonVuMIGRfPx_Qj9TsRih1DULJTKUkSGa66m67E&m=e0iYk69Iih3UnIjABITtc6yS38bhv_6P4NMuSpVmmoSHk1-sSuvH3y702O0nTeZj&s=OtiPnSuoZjUXFgz9pxbGOOyjQmKPEq-OhEbhtsAmvZE&e= > > > > > > Tested with a recipe "my-custom-linux" which unpacks sources to a custom > > ${S} directory > > > and ran symlink_kernsrc several times: > > > $ bitbake -f -c symlink_kernsrc my-custom-linux > > > > Sorry for the delay in getting back to review this patch. I'm extremely > > worried about adding complexity into this function, particularly as > > that complexity is now adding in significant overhead. > > > > Firstly, can I ask why you're using a non-default directory for ${S}? > > Is this as a way of doing a kind of external source usage? > > > > Having spent time looking at what this code is doing, in normal usage, > > S gets set to STAGING_KERNEL_DIR, the source is unpacked there and none > > of this code triggers. > > > > For EXTERNALSRC, a symlink is put in place. The code should remove a > > symlink if present and create it with the current setup. In that sense, > > this patch isn't doing the right thing. > > > > I'm very tempted to say the "using a non default S value" just error > > out. > > > > Whilst I understand how we got to this level of complexity, I think it > > will just end up causing us pain in the long run and I'd rather just > > not support it. > > > > Cheers, > > > > Richard > > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#199898): https://lists.openembedded.org/g/openembedded-core/message/199898 > Mute This Topic: https://lists.openembedded.org/mt/103308574/3617179 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alexandre.belloni@bootlin.com] > -=-=-=-=-=-=-=-=-=-=-=- >
diff --git a/meta/classes-recipe/kernel.bbclass b/meta/classes-recipe/kernel.bbclass index 9ff37f5c38..45b63f1fa1 100644 --- a/meta/classes-recipe/kernel.bbclass +++ b/meta/classes-recipe/kernel.bbclass @@ -189,11 +189,17 @@ python do_symlink_kernsrc () { # drop trailing slash, so that os.symlink(kernsrc, s) doesn't use s as # directory name and fail s = s[:-1] - if d.getVar("EXTERNALSRC"): + if d.getVar("EXTERNALSRC") and not os.path.islink(s): # With EXTERNALSRC S will not be wiped so we can symlink to it os.symlink(s, kernsrc) else: import shutil + # perform idempotent/reentrant copy + s_copy = s + ".orig" + if not os.path.isdir(s_copy): + shutil.copytree(s, s_copy, ignore_dangling_symlinks=True) + bb.utils.remove(s, recurse=True) + shutil.copytree(s_copy, s, ignore_dangling_symlinks=True) shutil.move(s, kernsrc) os.symlink(kernsrc, s)