On Tue, Nov 15, 2016 at 09:07:20PM +0100, Michał Górny wrote:
> On Tue, 15 Nov 2016 13:42:00 -0600
> William Hubbs <[email protected]> wrote:
> 
> > # Copyright 1999-2016 Gentoo Foundation
> > # Distributed under the terms of the GNU General Public License v2
> > # $Id$
> > 
> > # @ECLASS: tmpfiles.eclass
> > # @MAINTAINER:
> > # Gentoo systemd project <[email protected]>
> > # William Hubbs <[email protected]>
> > # @AUTHOR:
> > # Mike Gilbert <[email protected]>
> > # William Hubbs <[email protected]>
> > # @BLURB: Functions related to tmpfiles.d files
> > # @DESCRIPTION:
> > # Provides common functionality related to installation an processing
> 
> Typo: 'and'
> 
> > # of tmpfiles snippets.
> 
> Honestly, I would like to see here either a more complete description
> (with example!) on what purpose does this eclass serve and how it's
> supposed to be used.
> 
> It would probably make sense to explain that it can be used to
> *reliably* create temporary directories, i.e. without a need for
> an explicit fallback in ebuild. After all, that's the main goal of
> the whole effort.
 
 Sure, I'll add more info here.

> It may also be worth it to note that full support requires OpenRC or
> systemd. People not using either of the two will only get directories
> created in postinst, and can't use volatile filesystems.

The only thing another init system would have to do is run opentmpfiles
or systemd-tmpfiles at the appropriate point in bootup, so I can mention
that.

> 
> > if [[ -z $tmpfiles_eclass ]]; then
> 
> 1. Don't indent the whole eclass.
> 
> 2. Use capitals for global variables.
> 
> 3. Use ${} with braces.
> 
> >     tmpfiles_eclass=1
> > 
> >     case "${EAPI}" in
> >             5|6) ;;
> >             *) die "API is undefined for EAPI ${EAPI}" ;;
> >     esac
> > 
> >     DEPEND="virtual/tmpfiles"
> 
> You don't need build-time dependency on it. It's not used before pkg_*.
> 
> >     RDEPEND="virtual/tmpfiles"
> 
> 1. I'm wondering if there's a case for making this optional
> (I'm pretty sure we have USE-conditional installs of tmpfiles, however
> I'm not sure if we care for 100% exact deps).
 
hmm, use-conditional installs of tmpfiles... should we look into fixing
those (wrt our practice of always installing small files), or should we
revisit that whole practice (+1000 for revisiting it).

RDEPEND's can't depend on use flags, so I'm not sure how we would make
this optional.

> 2. I think it would be reasonable to match virtual versions to
> 'versions' of tmpfiles.d support, i.e. to be able to e.g. >=230 when
> the file needs new feature that was introduced in systemd-230. But
> maybe it'd good enough to have additional dependency in ebuild then.
> 

I'm not sure about adding an additional dependency to the ebuild. This
could be handled by bumping the virtual if necessary.

> ...
> >     # @FUNCTION: tmpfiles_create
> 
> Maybe tmpfiles_process?
> 
> >     # @USAGE: tmpfiles_create
> 
> I think you actually want to pass filenames here.
> 
> >     # @DESCRIPTION:
> >     # Call a tmpfiles implementation to process newly installed tmpfiles.d
> >     # snippets. Does nothing if no tmpfiles implementation is available.
> 
> I think it should error out when no implementation is available. We
> want to be able to reliably create temporary directories.
> 
> >     tmpfiles_create() {
> >             debug-print-function "${FUNCNAME}" "$@"
> > 
> >             [[ ${EBUILD_PHASE} == postinst ]] || die "${FUNCNAME}: Only 
> > valid in pkg_postinst"
> >             [[ ${#} -gt 0 ]] || die "${FUNCNAME}: Must specify at least one 
> > filename"
> >             [[ ${ROOT} == / ]] || return 0
> > 
> >             if type systemd-tmpfiles &> /dev/null; then
> >                     systemd-tmpfiles --create "$@"
> >             elif type opentmptmpfiles &> /dev/null; then
> >                     opentmpfiles --create "$@"
> >             fi
> >     }
> > 
> > fi
> 
> I'm wondering if we could go for some cleanup in prerm. But that's
> probably a far-away future goal.
> 
> -- 
> Best regards,
> Michał Górny
> <http://dev.gentoo.org/~mgorny/>


Attachment: signature.asc
Description: Digital signature

Reply via email to