Re: generic fonts makefile fragment

[Roland Illig <roland.illig%gmx.de@localhost>]

Am 05.03.2019 um 22:55 schrieb coypu%sdf.org@localhost:
> Index: fonts.mk
> ===================================================================
> RCS file: fonts.mk
> diff -N fonts.mk
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ fonts.mk	5 Mar 2019 21:52:17 -0000

When I saw these lines, I had no idea where you wanted to place the
file. To avoid this confusion, diffs are typically taken from the pkgsrc
root, which in this case would change the file name to mk/fonts.mk,
thereby making its location obvious.

> @@ -0,0 +1,22 @@
> +# $NetBSD$
> +#
> +# Wrapper for installing fonts> +#

For an infrastructure file that is meant for providing an API, the above
documentation is very thin.

Why is this called a wrapper, what does it wrap?

What effect does including this file have? Something like "installs all
TTF and OTF files from WRKSRC" would be more specific and thus more helpful.

> +NO_CONFIGURE=   yes
> +NO_BUILD=       yes

Overriding these variables unconditionally is not the style usually used
by the pkgsrc infrastructure files. These two variable assignments
should use the ?= operator instead of the = operator, so that packages
can override these variables by defining the variable either before or
after including fonts.mk.

In your example package, fonts.mk is included at the very end, which
would make it impossible to override these variables.

> +TTF_FONTS_DIR=  ${PREFIX}/share/fonts/X11/TTF
> +OTF_FONTS_DIR=  ${PREFIX}/share/fonts/X11/OTF

These two variables form part of the public API of the file, therefore
they should be documented at the top of the file, in the section called
"System-provided variables". See mk/fetch/bsd.fetch-vars.mk for a
copy-and-paste template.

> +INSTALLATION_DIRS= ${TTF_FONTS_DIR} ${OTF_FONTS_DIR}

To make fonts.mk a "mixin" feature, this line should use the += operator
instead of the = operator.

> +do-install:

To make fonts.mk a "mixin" feature, this line should rather be split
into two:

    do-install: install-fonts

    install-fonts: .PHONY

This allows a package to define its own do-install commands and have the
fonts added in addition to these.

> +	for font in `find ${WRKSRC} -name '*.ttf'`; \
> +	do \
> +		${INSTALL_DATA} $${font} ${DESTDIR}${TTF_FONTS_DIR}; \
> +	done; \
> +	for font in `find ${WRKSRC} -name '*.otf'`; \
> +	do \
> +		${INSTALL_DATA} $${font} ${DESTDIR}${OTF_FONTS_DIR}; \
> +	done;

Did you ever have to deal with font files whose name contains spaces or
other special characters? It would be good if that infrastructure part
could handle these files. To catch most of these special characters (or
even all?), the following patterns may help:

    find ${WRKSRC} -name '*.ttf' \
        -exec ${INSTALL_DATA} "{}" ${DESTDIR}${TTF_FONTS_DIR} ";"
    find ${WRKSRC} -name '*.otf' \
        -exec ${INSTALL_DATA} "{}" ${DESTDIR}${OTF_FONTS_DIR} ";"

This -exec pattern is already used more than 300 times in pkgsrc. I
might even teach pkglint to suggest this pattern instead of the above
for loops since this pattern handles exit codes properly.

Did you think about prefixing the installation commands with ${RUN}?
Most of the infrastructure files do this. In the installation phase
though, these commands that directly and obviously install files may
also omit the ${RUN}. Anyway, this should be a concious choice.

It might be helpful to show this message before installing the fonts:

    ${STEP_MSG} "Installing all fonts from WRKSRC"

It would be really cool to have a regression test in regress/fonts,
demonstrating that this part works and which characters may or may not
appear in the font filenames. See
https://www.netbsd.org/docs/pkgsrc/pkgsrc.html#regression for
documentation on regression testing. If you need further help with this,
just ask.

Best,
Roland