Re: [PATCH v2] rootfs: clean package log files that are not owned by packages

[Henning Schild <henning.schild@siemens.com>]

Am Mon, 4 Oct 2021 15:07:04 +0000
schrieb <Venkata.Pyla@toshiba-tsip.com>:

> >-----Original Message-----
> >From: Henning Schild <henning.schild@siemens.com>
> >Sent: 04 October 2021 19:19
> >To: pyla venkata(ＴＳＩＰ) <Venkata.Pyla@toshiba-tsip.com>
> >Cc: isar-users@googlegroups.com; jan.kiszka@siemens.com
> >Subject: Re: [PATCH v2] rootfs: clean package log files that are not
> >owned by packages
> >
> >Am Mon, 4 Oct 2021 12:51:43 +0000
> >schrieb <Venkata.Pyla@toshiba-tsip.com>:
> >  
> >> >-----Original Message-----
> >> >From: Henning Schild <henning.schild@siemens.com>
> >> >Sent: 04 October 2021 17:35
> >> >To: pyla venkata(ＴＳＩＰ) <Venkata.Pyla@toshiba-tsip.com>
> >> >Cc: isar-users@googlegroups.com; jan.kiszka@siemens.com
> >> >Subject: Re: [PATCH v2] rootfs: clean package log files that are
> >> >not owned by packages
> >> >
> >> >Am Fri, 1 Oct 2021 20:07:48 +0530
> >> >schrieb <venkata.pyla@toshiba-tsip.com>:
> >> >  
> >> >> From: venkata pyla <venkata.pyla@toshiba-tsip.com>
> >> >>
> >> >>  /var/log/* files that are created during build stage and not
> >> >> owned by any package are not neccessary to be present in rootfs
> >> >> image, as these log files adds additional size to rootfs image,
> >> >> and also it create problems for reproducible build
> >> >> functionality.
> >> >>
> >> >>  so this ROOTFS feature 'clean-log-files' should help to clean
> >> >> the log files when it is enalbed, disable it if we need the log
> >> >> files for debugging purpose.
> >> >>
> >> >>  ROOTFS_FEATURE += clean-log-files  
> >> >
> >> >The two leading spaces of the commit message look a little weird.
> >> >Like you are not using git-format-patch or git-send-email
> >> >correctly. Or like you are using a funny editor for writing your
> >> >commit messages.  
> >>
> >> I will correct it, thanks.
> >>  
> >> >
> >> >I think that adds value but i do not see why we should model it
> >> >as a feature. It should probably be the "new normal" and we
> >> >introduce an option once there is a need for a switch.  
> >>
> >> I added it as feature because if we delete the logs file by default
> >> from the image, if any user want those log files for debugging
> >> purpose then there will be provision to disable this feature and
> >> get the log files as earlier
> >>  
> >> >  
> >> >> Signed-off-by: venkata pyla <venkata.pyla@toshiba-tsip.com>
> >> >> ---
> >> >>  meta/classes/rootfs.bbclass | 10 ++++++++++
> >> >>  1 file changed, 10 insertions(+)
> >> >>
> >> >> diff --git a/meta/classes/rootfs.bbclass
> >> >> b/meta/classes/rootfs.bbclass index f9151c5..ff0ecad 100644
> >> >> --- a/meta/classes/rootfs.bbclass
> >> >> +++ b/meta/classes/rootfs.bbclass
> >> >> @@ -12,6 +12,7 @@ ROOTFS_PACKAGES ?= ""
> >> >>  # 'clean-package-cache' - delete package cache from rootfs  #
> >> >> 'generate-manifest' - generate a package manifest of the rootfs
> >> >> into ${ROOTFS_MANIFEST_DEPLOY_DIR} # 'export-dpkg-status' -
> >> >> exports /var/lib/dpkg/status file to
> >> >> ${ROOTFS_DPKGSTATUS_DEPLOY_DIR} +# 'clean-log-files' - delete
> >> >> log files that are not owned by packages ROOTFS_FEATURES ?= ""
> >> >>  ROOTFS_APT_ARGS="install --yes -o
> >> >> Debug::pkgProblemResolver=yes" @@ -213,6 +214,15 @@
> >> >> rootfs_postprocess_clean_package_cache() { sudo rm -rf
> >> >> "${ROOTFSDIR}/var/lib/apt/lists/"* }
> >> >>
> >> >> +ROOTFS_POSTPROCESS_COMMAND +=
> >> >> "${@bb.utils.contains('ROOTFS_FEATURES', 'clean-log-files',
> >> >> 'rootfs_postprocess_clean_log_files', '', d)}"
> >> >> +rootfs_postprocess_clean_log_files() {
> >> >> +    # Delete log files that are not owned by packages
> >> >> +    sudo -E chroot '${ROOTFSDIR}' \
> >> >> +        /usr/bin/find /var/log/ \
> >> >> +        -exec sh -c '! dpkg -S {} > /dev/null 2>&1' ';' \
> >> >> +        -exec rm -rf {} ';'  
> >> >
> >> >I think we should restrict that to "-type f" and switch over to
> >> >"rm -f" (not -r)
> >> >  
> >>
> >> If we delete only files then there may be some residue left over as
> >> empty folders.  
> >
> >Which is exactly what i was aiming for! An empty folder belonging to
> >a package should be kept ... i think your code keeps it as well. But
> >empty folders do not hurt too much ... and them not being there
> >might have an impact on the applications expecting them to exist ...
> >they might not log or might not even start in the worst case.
> >
> >I think a proper debian package will contain required empty folders,
> >i am not sure we should remove empty folders that are seemingly not
> >needed.  
> 
> The dpkg -S should search even if it is empty folder and owned by
> packages, Also if the applications are expecting them even if it is
> not part of package list contents, I think it should be an
> application bug. Anyway keeping them doesn’t give any difference, so
> If no objections I will modify it.
> 
> I will resend the patch with the below changes, kindly let me know if
> there are more review comments.
> * correct the commit message by removing extra leading space
> characters
> * Use "type -f" to delete only files in the log folder

I am also not sure that is needed. I first thought a debian package was
not allowed to contain an empty folder ... like git or a gentoo
package. I do not feel strong about the "type f" and the "rm -f".

Henning

> * Set this feature to on as default in image.bbclass
> 
> Thanks.
> 
> >
> >Henning
> >  
> >> >Henning
> >> >  
> >> >> +}
> >> >> +
> >> >>  ROOTFS_POSTPROCESS_COMMAND +=
> >> >> "${@bb.utils.contains('ROOTFS_FEATURES', 'generate-manifest',
> >> >> 'rootfs_generate_manifest', '', d)}" rootfs_generate_manifest
> >> >> () { mkdir -p ${ROOTFS_MANIFEST_DEPLOY_DIR}  
> >>