Re: [PATCH] glibc-locale: Rewrite do_install using install utility instead of cp

[Peter Kjellerstedt <peter.kjellerstedt@axis.com>]

> -----Original Message-----
> From: Khem Raj <raj.khem@gmail.com>
> Sent: den 8 februari 2019 16:21
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> Cc: openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH] glibc-locale: Rewrite do_install using
> install utility instead of cp
> 
> On Fri, Feb 8, 2019 at 12:44 AM Peter Kjellerstedt
> <peter.kjellerstedt@axis.com> wrote:
> >
> > > -----Original Message-----
> > > From: openembedded-core-bounces@lists.openembedded.org
> <openembedded-
> > > core-bounces@lists.openembedded.org> On Behalf Of Khem Raj
> > > Sent: den 8 februari 2019 01:56
> > > To: openembedded-core@lists.openembedded.org
> > > Subject: [OE-core] [PATCH] glibc-locale: Rewrite do_install using
> > > install utility instead of cp
> > >
> > > This has been a constant source of trouble for build failures due
> to
> > > host-user-contaminated QA errors of sort
> > >
> > > ERROR: QA Issue: glibc-locale: /glibc-binary-localedata-ca-
> > > es+valencia/usr/lib/locale/ca_ES@valencia/LC_MONETARY is owned by
> uid
> > > 3004, which is the same as the user running bitbake. This may be
> due to
> > > host contamination [host-user-contaminated]
> > >
> > > So far we have tried to mould cp command into not carrying the
> build
> > > user permissions into install area but it is never entirely fixed
> since
> > > the issue keeps popping up in various scenes
> > >
> > > This patch replaces use of cp with install utility and specifies
> install
> > > mode for files explcitly
> > >
> > > Signed-off-by: Khem Raj <raj.khem@gmail.com>
> > > ---
> > >  meta/recipes-core/glibc/glibc-locale.inc | 41 ++++++++++++--------
> ----
> > >  1 file changed, 20 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/meta/recipes-core/glibc/glibc-locale.inc
> b/meta/recipes-core/glibc/glibc-locale.inc
> > > index 6384f9cbf1..9cce61bf0b 100644
> > > --- a/meta/recipes-core/glibc/glibc-locale.inc
> > > +++ b/meta/recipes-core/glibc/glibc-locale.inc
> > > @@ -71,28 +71,27 @@ FILES_localedef = "${bindir}/localedef"
> > >
> > >  LOCALETREESRC = "${COMPONENTS_DIR}/${PACKAGE_ARCH}/glibc-stash-
> locale"
> > >
> > > -do_install () {
> > > -     mkdir -p ${D}${bindir} ${D}${datadir}
> > > -     if [ -n "$(ls ${LOCALETREESRC}/${bindir})" ]; then
> > > -             cp -R --no-dereference --preserve=mode,links
> ${LOCALETREESRC}/${bindir}/* ${D}${bindir}
> > > -     fi
> > > -     if [ -n "$(ls ${LOCALETREESRC}/${localedir})" ]; then
> > > -             mkdir -p ${D}${localedir}
> > > -             cp -R --no-dereference --preserve=mode,links
> ${LOCALETREESRC}/${localedir}/* ${D}${localedir}
> > > -     fi
> > > -     if [ ${@d.getVar('PACKAGE_NO_GCONV')} -eq 0 ]; then
> > > -             mkdir -p ${D}${libdir}
> > > -             if [ -e ${LOCALETREESRC}/${libdir}/gconv ]; then
> > > -                     cp -R --no-dereference --preserve=mode,links
> ${LOCALETREESRC}/${libdir}/gconv ${D}${libdir}
> > > -             fi
> > > -             if [ -e ${LOCALETREESRC}/${datadir}/i18n ]; then
> > > -                     cp -R --no-dereference --preserve=mode,links
> ${LOCALETREESRC}/${datadir}/i18n ${D}${datadir}
> > > -             fi
> > > -     fi
> > > -     if [ -e ${LOCALETREESRC}/${datadir}/locale ]; then
> > > -             cp -R --no-dereference --preserve=mode,links
> ${LOCALETREESRC}/${datadir}/locale ${D}${datadir}
> > > +copy_locale_files() {
> > > +     local dir=$1 mode=$2
> > > +
> > > +     [ -e "${LOCALETREESRC}$dir" ] || return 0
> > > +
> > > +     for d in . $(find "${LOCALETREESRC}$dir" -type d -printf '%P
> '); do
> > > +             install -d ${D}$dir/$d
> > > +             find "${LOCALETREESRC}$dir/$d" -maxdepth 1 -type f \
> > > +             -exec install -m $mode -t "${D}$dir/$d" {} \;
> >
> > I know I am a sucker for details, but please align the broken find
> line so
> > that it is more obvious that the second line is a continuation, i.e.:
> >
> >                 find "${LOCALETREESRC}$dir/$d" -maxdepth 1 -type f \
> >                      -exec install -m $mode -t "${D}$dir/$d" {} \;
> >
> 
> You are doing good here, I really appreciate the feedback. Do you
> think aligning it
> under beginning of quote is more readable or aligning where find cmd
> begins is better ?
> I will make this as a followup patch and send it with next lot of
> minor changes that I have in
> queue.

The patch, as it was integrated to master, is fine.

> > > +     done
> > > +}
> > > +
> > > +do_install() {
> > > +     copy_locale_files ${bindir} 0755
> > > +     copy_locale_files ${localedir} 0644
> > > +     if [ ${PACKAGE_NO_GCONV} -eq 0 ]; then
> > > +             copy_locale_files ${libdir}/gconv 0755
> > > +             copy_locale_files ${datadir}/i18n 0644
> > >       fi
> > > -     cp -R --no-dereference --preserve=mode,links
> ${LOCALETREESRC}/SUPPORTED ${WORKDIR}
> > > +     copy_locale_files ${datadir}/locale 0644
> > > +     install -m 0644 ${LOCALETREESRC}/SUPPORTED
> ${WORKDIR}/SUPPORTED
> > >  }
> > >
> > >  inherit libc-package
> > > --
> > > 2.20.1
> >
> > //Peter
> >

//Peter