Re: [PATCH] added mounting debugfs recipe

[Denys Dmytriyenko <denys@ti.com>]

Jingting,

I know there's already a thread going on, I just wanted to follow up on some 
of the previous comments from Chase to explain and/or correct, since it's 
still new to you and you may need some more details. Please see inline.


On Thu, May 02, 2013 at 07:54:40PM +0000, Maupin, Chase wrote:
> > diff --git a/meta-arago-distro/recipes-
> > core/packagegroups/packagegroup-arago-base-tisdk-server-extra.bb
> > b/meta-arago-distro/recipes-core/packagegroups/packagegroup-
> > arago-base-tisdk-server-extra.bb
> > index 46db7c2..ba618b2 100755
> > --- a/meta-arago-distro/recipes-core/packagegroups/packagegroup-
> > arago-base-tisdk-server-extra.bb
> > +++ b/meta-arago-distro/recipes-core/packagegroups/packagegroup-
> > arago-base-tisdk-server-extra.bb
> > @@ -66,7 +66,7 @@ RDEPENDS_${PN} = "\
> >      ptpd \
> >      vsftpd \
> >      syslog-ng \
> > -	syslog-ng-plugins \
> > +    syslog-ng-plugins \
> 
> This looks like a mistake.  Either in the other patch or this one.

In other words - this fix should be rolled into the previous patch (that 
is still under review), and not piggy-backing on the new patch, that has 
nothing to do with syslog.


> >      dtc \
> >      strongswan \
> >      strongswan-plugins \
> > @@ -95,4 +95,5 @@ RDEPENDS_${PN} = "\
> >      ti-netapi \
> >      ti-ipc \
> >      ebtables \
> > +    initscript-debugfs \
> 
> Usually this should be split into apatch that adds the recipe and one that 
> adds it to the packagegroup.

While this one is not really so critical, it's still better to have the first 
patch adding new functionality and the second patch to enable that 
functionality in the system - #1 adds initscript-debugfs.bb and #2 enables it 
in the packagegroup.


> > diff --git a/meta-arago-extras/recipes-bsp/initscript-
> > debugfs/initscript-debugfs.bb b/meta-arago-extras/recipes-
> > bsp/initscript-debugfs/initscript-debugfs.bb
> > new file mode 100755
> > index 0000000..c9cb7d0
> > --- /dev/null
> > +++ b/meta-arago-extras/recipes-bsp/initscript-
> > debugfs/initscript-debugfs.bb
> > @@ -0,0 +1,17 @@
> > +DESCRIPTION = "Initscripts for debugfs"
> > +LICENSE = "BSD"
> > +
> > +LIC_FILES_CHKSUM =
> > "file://debugfs.sh;md5=ecbae24c97c0fa80c5977bf22eec1f32"
> 
> This file does not have a real license header in it.  If you are going to 
> point here you should put the license header in it or use the license file 
> from the core layer.  And I wouldn't do the whole script because then you 
> have to update this for every script change.

IF and only if you have a license info at the top of your source file (.sh or 
.c or .h), then you would point to that file AND also specify beginline=# and 
endline=# in there to limit the checksum to those lines only. Like Chase said, 
that way you don't need to update checksum with every fix you make to the 
code.


> > +
> > +PR ="r1"
> 
> Why not start at r0?

I personally don't have preference here. If you put r0, then another person 
would start argue that you don't need to set it to r0, as that's the default 
and you can drop the line altogether. I prefer to have it from the beginning, 
so you won't forget to update it on the next change.


> > +
> > +SRC_URI = "file://debugfs.sh"
> > +
> > +S = "${WORKDIR}"
> > +
> > +do_install () {
> > +	install -d ${D}${sysconfdir}/init.d/
> > +	install -d ${D}${sysconfdir}/rcS.d/
> > +	install -c -m 755 ${WORKDIR}/debugfs.sh
> > ${D}${sysconfdir}/init.d/debugfs.sh
> > +	ln -sf ../init.d/debugfs.sh
> > ${D}${sysconfdir}/rcS.d/S09debugfs
> > +}
> 
> Use the update-rc.d class instead.  Life will be better this way.

Indeed.

-- 
Denys