Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[konsolebox]

On Wed, Nov 30, 2016 at 8:35 PM, Mike Gilbert  wrote:
> On Wed, Nov 30, 2016 at 7:11 AM, konsolebox  wrote:
 I also prefer some things this way:

 - Indent the contents of the first `if` block for consistency's sake,
 and less confusion.
>>>
>>> I disagree; indenting the entire eclass is silly and does not really
>>> improve readability. Also, this is a very common pattern found in
>>> other eclasses.
>>
>> I prefer following consistency before anything else.  And it's just
>> uncommon and odd, but it's not silly.  Imagine if you use another `if`
>> block on the second level where more functions are defined.  Would you
>> also not indent that?
>
> That first "if" is a bit of a special case; it's only there to prevent
> the code from being executed more than once in global scope. This
> provides a minor speed boost when the eclass gets sourced more than
> once, and makes sure that any global variables are only set once.

OT: inherit() should have already been able to have a non-hacky
solution of making sure that eclass files are loaded only once.  Bash
4.2 has already supported global declaration of variables with -g (in
case the flag variable would need to be declared inside a function),
and associative arrays with -A.  Even older versions of Bash (<4.0)
can make use of multiple variables with a common prefix to imitate
associative arrays if compatibility is needed.  It could just check
BASH_VERSINFO if it has to downgrade to a less-efficient method.

Besides no longer having to rely on a special `if` block, it probably
would also make loading of ebuilds faster since script codes that are
referenced multiple times wouldn't have to be re-parsed again and
again by bash.  It's scary to imagine how it happens in all ebuilds
that's read in portage, but maybe repeated loading of eclass files
doesn't always happen, who knows.  (I think it's still significant
even if the dependencies are already cached.)

> I think of it as being similar to pre-processor statement you would
> find in a C header file; one does not generally indent all the code
> within a C header file, because it just introduces a big chunk of
> whitespace that does not improve code readability.

I did as well, but I'll still prefer to have it indented, because it
avoids trying to know what the last lone `fi` is for, if you haven't
seen the first `if` condition yet.  If they are indented, your mind is
automatically set to know that they are within a conditional block,
and no surprise happens.  Maybe that's not the case if people are
already used to seeing those setup, like in eclass files, but I'm not,
so it does help improve readability in my case.

To be honest I find it painful to see anything inside a block to be
not indented, no matter what the reason is.  You can say that they
look like preprocessors in C, and preprocessors don't make codes
indent, but preprocessors are not (functional) codes, and they are
naturally recognized to have their own "indentation space" or
patterns, unlike the shell's `if`.

But anyway, I think I find it understandable now, in the context of
eclass files.

-- 
konsolebox

Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[Mike Gilbert]

On Wed, Nov 30, 2016 at 7:11 AM, konsolebox  wrote:
>>> I also prefer some things this way:
>>>
>>> - Indent the contents of the first `if` block for consistency's sake,
>>> and less confusion.
>>
>> I disagree; indenting the entire eclass is silly and does not really
>> improve readability. Also, this is a very common pattern found in
>> other eclasses.
>
> I prefer following consistency before anything else.  And it's just
> uncommon and odd, but it's not silly.  Imagine if you use another `if`
> block on the second level where more functions are defined.  Would you
> also not indent that?

That first "if" is a bit of a special case; it's only there to prevent
the code from being executed more than once in global scope. This
provides a minor speed boost when the eclass gets sourced more than
once, and makes sure that any global variables are only set once.

I think of it as being similar to pre-processor statement you would
find in a C header file; one does not generally indent all the code
within a C header file, because it just introduces a big chunk of
whitespace that does not improve code readability.

Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[konsolebox]

On 11/30/16, Mike Gilbert  wrote:
> On Wed, Nov 30, 2016 at 3:38 AM, konsolebox  wrote:
>> - `[[ ${ROOT} == / ]] || return 0` seems to present a harmless false
>> condition, and it doesn't show an error message.  I would be helpful
>> to have a comment added above it to give details why.
>
> We only want to process tmpfiles for the currently running system.
>
> If ROOT is not /, it indicates we are installing the package for use
> on a different system or in a container. In either case, the tmpfiles
> would be processed upon boot when that system/container is started.
>
> I find this fairly obvious, but if William wants to document it that's
> fine.

I'm not familiar with that, and so would other scripters who would
look at the eclass.

>> I also prefer some things this way:
>>
>> - Indent the contents of the first `if` block for consistency's sake,
>> and less confusion.
>
> I disagree; indenting the entire eclass is silly and does not really
> improve readability. Also, this is a very common pattern found in
> other eclasses.

I prefer following consistency before anything else.  And it's just
uncommon and odd, but it's not silly.  Imagine if you use another `if`
block on the second level where more functions are defined.  Would you
also not indent that?

>> - Patterns in the `case` block doesn't have to be indented. This makes
>> the contents of the `case` block aligned with the contents of the
>> other blocks (`if`, `while`, etc.), and it makes the use of indents at
>> minimum when the block is used recursively.
>
> Sorry, I have no idea what you're trying to say here. Recursive blocks?

One expensive in the use of indents:

case $x in
a)
# first inner level
case $y in
1)
# second inner level
case ...
;;
2)
;;
esac
;;
b)
;;
esac

if [ "$x" = a ]; then
# first inner level
if [ "$y" = 1 ]; then
# second inner level
elif [ "$y" = 2 ]; then
:
else
:
fi
elif [ "$x" = b ]; then
:
else
:
fi

vs.

case $x in
a)
case $y in
1)
case ...
;;
2)
;;
esac
;;
b)
;;
esac

> This really feels like you're making a personal style suggestion here,
> and I personally see nothing wrong with it as-is.

Yes the second part is more personal.

-- 
konsolebox

Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[Mike Gilbert]

On Wed, Nov 30, 2016 at 3:38 AM, konsolebox  wrote:
> There are some things I noticed in the tmpfiles_process() function:
>
> - `type` currently also checks for functions, alias, and builtins,
> besides executable files.  If that's not intended, the `-P` option
> should be added.

I cannot conceive of any way a function or alias would be defined with
these names unless an ebuild author did it intentionally or a user did
it via /etc/portage/bashrc. In either case, I see no need to prevent
that.

> - `[[ ${ROOT} == / ]] || return 0` seems to present a harmless false
> condition, and it doesn't show an error message.  I would be helpful
> to have a comment added above it to give details why.

We only want to process tmpfiles for the currently running system.

If ROOT is not /, it indicates we are installing the package for use
on a different system or in a container. In either case, the tmpfiles
would be processed upon boot when that system/container is started.

I find this fairly obvious, but if William wants to document it that's fine.

> I also prefer some things this way:
>
> - Indent the contents of the first `if` block for consistency's sake,
> and less confusion.

I disagree; indenting the entire eclass is silly and does not really
improve readability. Also, this is a very common pattern found in
other eclasses.

> - Patterns in the `case` block doesn't have to be indented. This makes
> the contents of the `case` block aligned with the contents of the
> other blocks (`if`, `while`, etc.), and it makes the use of indents at
> minimum when the block is used recursively.

Sorry, I have no idea what you're trying to say here. Recursive blocks?

This really feels like you're making a personal style suggestion here,
and I personally see nothing wrong with it as-is.

Re: [gentoo-dev] new eclass: tmpfiles.eclass round 4

[konsolebox]

There are some things I noticed in the tmpfiles_process() function:

- `type` currently also checks for functions, alias, and builtins,
besides executable files.  If that's not intended, the `-P` option
should be added.
- What happens if neither systemd-tmpfiles nor opentmpfiles is found?
- Shouldn't the function return a nonzero if an error occurs?  It
would help scripts abort.
- `[[ ${ROOT} == / ]] || return 0` seems to present a harmless false
condition, and it doesn't show an error message.  I would be helpful
to have a comment added above it to give details why.

I also prefer some things this way:

- Indent the contents of the first `if` block for consistency's sake,
and less confusion.
- Patterns in the `case` block doesn't have to be indented. This makes
the contents of the `case` block aligned with the contents of the
other blocks (`if`, `while`, etc.), and it makes the use of indents at
minimum when the block is used recursively.
- The subject word in the case block does not have to be quoted.
- Always keep blocks isolated from adjacent lines.

-- 
konsolebox


tmpfiles.eclass
Description: Binary data

[gentoo-dev] new eclass: tmpfiles.eclass round 4

[William Hubbs]

# Copyright 1999-2016 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Id$

# @ECLASS: tmpfiles.eclass
# @MAINTAINER:
# Gentoo systemd project 
# William Hubbs 
# @AUTHOR:
# Mike Gilbert 
# William Hubbs 
# @BLURB: Functions related to tmpfiles.d files
# @DESCRIPTION:
# This eclass provides functionality related to installing and
# creating volatile and temporary files based on configuration files$and
# locations defined at this URL:
#
# https://www.freedesktop.org/software/systemd/man/tmpfiles.d.html
#
# The dotmpfiles and newtmpfiles functions are used to install
# configuration files into /usr/lib/tmpfiles.d, then in pkg_postinst, the
# tmpfiles_process function can be called to process the newly
# installed tmpfiles.d entries.
#
# @EXAMPLE:
# Typical usage of this eclass:
#
# @CODE
#   EAPI=6
#   inherit tmpfiles
#
#   ...
#
#   src_install() {
#   ...
#   dotmpfiles "${FILESDIR}"/file1.conf "${FILESDIR}"/file2.conf
#   newtmpfiles "${FILESDIR}"/file3.conf-${PV} file3.conf
#   ...
#   }
#
#   pkg_postinst() {
#   ...
#   tmpfiles_process file1.conf file2.conf file3.conf
#   ...
#   }
#
# @CODE

if [[ -z ${TMPFILES_ECLASS} ]]; then
TMPFILES_ECLASS=1

case "${EAPI}" in
6) ;;
*) die "API is undefined for EAPI ${EAPI}" ;;
esac

RDEPEND="virtual/tmpfiles"

# @FUNCTION: dotmpfiles
# @USAGE: dotmpfiles  ...
# @DESCRIPTION:
# Install one or more tmpfiles.d files into /usr/lib/tmpfiles.d.
dotmpfiles() {
debug-print-function "${FUNCNAME}" "$@"

local f
for f; do
if [[ ${f} != *.conf ]]
die "tmpfiles.d files must end with .conf"
fi
done

(
insinto /usr/lib/tmpfiles.d
doins "$@"
)
}

# @FUNCTION: newtmpfiles
# @USAGE: newtmpfiles  .conf
# @DESCRIPTION:
# Install a tmpfiles.d file in /usr/lib/tmpfiles.d under a new name.
newtmpfiles() {
debug-print-function "${FUNCNAME}" "$@"

if [[ $2 != *.conf ]]; then
die "tmpfiles.d files must end with .conf"
fi

(
insinto /usr/lib/tmpfiles.d
newins "$@"
)
}

# @FUNCTION: tmpfiles_process
# @USAGE: tmpfiles_process   ...
# @DESCRIPTION:
# Call a tmpfiles.d implementation to create new volatile and temporary
# files and directories.
tmpfiles_process() {
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

local rc=0
if type systemd-tmpfiles &> /dev/null; then
systemd-tmpfiles --create "$@"
rc=$?
elif type opentmpfiles &> /dev/null; then
opentmpfiles --create "$@"
rc=$?
fi
if [[ $rc -ne 0 ]]; then
ewarn "The tmpfiles processor failed with exit code $rc."
fi
}

fi


signature.asc
Description: Digital signature