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

[<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
* 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}
>>