Patchwork kernel: Remove unnecessary cd and rm

login
register
mail settings
Submitter Martin Jansa
Date May 21, 2013, 3:39 p.m.
Message ID <1369150787-21878-1-git-send-email-Martin.Jansa@gmail.com>
Download mbox | patch
Permalink /patch/50265/
State Accepted, archived
Headers show

Comments

Martin Jansa - May 21, 2013, 3:39 p.m.
---
 meta/classes/kernel.bbclass | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Darren Hart - May 21, 2013, 9:26 p.m.
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 - May 21, 2013, 9:41 p.m.
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.
Martin Jansa - May 21, 2013, 10:16 p.m.
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

Patch

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}
 
 	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"