Re: [OE-core] [PATCH] kernel: Remove unnecessary cd and rm
On Tue, May 21, 2013 at 02:26:55PM -0700, Darren Hart wrote: > Please include a a comment. The burden is on the submitter to prove > the cd and the rm are unnecessary, not the reviewer. > > > On 05/21/2013 08:39 AM, Martin Jansa wrote: > > --- > > meta/classes/kernel.bbclass | 7 ++- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass > > index 3320747..47a96c5 100644 > > --- a/meta/classes/kernel.bbclass > > +++ b/meta/classes/kernel.bbclass > > @@ -378,13 +378,10 @@ kernel_do_deploy() { > > ln -sf ${MODULE_TARBALL_BASE_NAME}.bin > > ${MODULE_TARBALL_SYMLINK_NAME} > > fi > > > > - cd ${DEPLOYDIR} > > - rm -f ${KERNEL_IMAGE_SYMLINK_NAME}.bin > > - ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${KERNEL_IMAGE_SYMLINK_NAME}.bin > > - ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${KERNEL_IMAGETYPE} > > + ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin > > ${DEPLOYDIR}/${KERNEL_IMAGE_SYMLINK_NAME}.bin > > + ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${DEPLOYDIR}/${KERNEL_IMAGETYPE} > > So we are now in a different directory (do we know which one?) but the > source of the ln commands doesn't change. If we are in the same > directory, then why add DEPLOYDIR to the destination? yes we know which one - ${B} as defined in deploy.bbclass: do_deploy[dirs] = "${DEPLOYDIR} ${B}", and the same is defined again (maybe unnecessary) in kernel.bbclass DEPLOYDIR is defined in deploy.bbclass as well DEPLOYDIR = "${WORKDIR}/deploy-${PN}" DEPLOYDIR is used as target for every other file deployed by do_deploy, this was just unnecessary switching CWD to create relative symlink and I have no idea why rm -f was used together with ln -sf. I'll send v2, because ln -sf ${MODULE_TARBALL_BASE_NAME}.bin ${MODULE_TARBALL_SYMLINK_NAME} is wrong. See related http://git.openembedded.org/openembedded-core/commit/meta/classes/kernel.bbclass?id=fd5d80ab1a405cb4ef94f2cde25f8c251da490f0 -- Martin 'JaMa' Jansa jabber: martin.ja...@gmail.com signature.asc Description: Digital signature ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH] kernel: Remove unnecessary cd and rm
On 05/21/2013 02:26 PM, Darren Hart wrote: > Please include a a comment. The burden is on the submitter to prove > the cd and the rm are unnecessary, not the reviewer. > > > On 05/21/2013 08:39 AM, Martin Jansa wrote: >> --- >> meta/classes/kernel.bbclass | 7 ++- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass >> index 3320747..47a96c5 100644 >> --- a/meta/classes/kernel.bbclass >> +++ b/meta/classes/kernel.bbclass >> @@ -378,13 +378,10 @@ kernel_do_deploy() { >> ln -sf ${MODULE_TARBALL_BASE_NAME}.bin >> ${MODULE_TARBALL_SYMLINK_NAME} >> fi >> >> -cd ${DEPLOYDIR} >> -rm -f ${KERNEL_IMAGE_SYMLINK_NAME}.bin >> -ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${KERNEL_IMAGE_SYMLINK_NAME}.bin >> -ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${KERNEL_IMAGETYPE} >> +ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin >> ${DEPLOYDIR}/${KERNEL_IMAGE_SYMLINK_NAME}.bin >> +ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${DEPLOYDIR}/${KERNEL_IMAGETYPE} > > So we are now in a different directory (do we know which one?) but the > source of the ln commands doesn't change. If we are in the same > directory, then why add DEPLOYDIR to the destination? I suspect the answer is absolute path, but this is a good example of where a proper comment is useful. -- Darren Hart Intel Open Source Technology Center Yocto Project - Technical Lead - Linux Kernel ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH] kernel: Remove unnecessary cd and rm
Please include a a comment. The burden is on the submitter to prove the cd and the rm are unnecessary, not the reviewer. On 05/21/2013 08:39 AM, Martin Jansa wrote: > --- > meta/classes/kernel.bbclass | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass > index 3320747..47a96c5 100644 > --- a/meta/classes/kernel.bbclass > +++ b/meta/classes/kernel.bbclass > @@ -378,13 +378,10 @@ kernel_do_deploy() { > ln -sf ${MODULE_TARBALL_BASE_NAME}.bin > ${MODULE_TARBALL_SYMLINK_NAME} > fi > > - cd ${DEPLOYDIR} > - rm -f ${KERNEL_IMAGE_SYMLINK_NAME}.bin > - ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${KERNEL_IMAGE_SYMLINK_NAME}.bin > - ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${KERNEL_IMAGETYPE} > + ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin > ${DEPLOYDIR}/${KERNEL_IMAGE_SYMLINK_NAME}.bin > + ln -sf ${KERNEL_IMAGE_BASE_NAME}.bin ${DEPLOYDIR}/${KERNEL_IMAGETYPE} So we are now in a different directory (do we know which one?) but the source of the ln commands doesn't change. If we are in the same directory, then why add DEPLOYDIR to the destination? -- Darren > > cp ${COREBASE}/meta/files/deploydir_readme.txt > ${DEPLOYDIR}/README_-_DO_NOT_DELETE_FILES_IN_THIS_DIRECTORY.txt > - cd - > } > do_deploy[dirs] = "${DEPLOYDIR} ${B}" > do_deploy[prefuncs] += "package_get_auto_pr" > -- Darren Hart Intel Open Source Technology Center Yocto Project - Technical Lead - Linux Kernel ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core