Re: [ptxdist] [PATCH v2 01/23] xorg-font-ttf-android: isolate from xorg-fonts

["Baeuerle, Florian" <Florian.Baeuerle@allegion.com>]

Hi Michael,

Am Dienstag, den 04.09.2018, 11:34 +0200 schrieb Michael Olbrich:
> On Tue, Aug 07, 2018 at 02:40:47PM +0200, Florian Bäuerle wrote:
> > As all ttfs font packages, xorg-font-ttf-android used to install fonts
> > into xorg-fonts' package and defers the mkfontdir- and mkfontscale steps
> > to xorg-fonts.targetinstall. This was a nice trick to spare the
> > generation of fonts.{dir,scale} in every font package, but it breaks the
> > collection feature for ttf-fonts.
> > 
> > Signed-off-by: Florian Bäuerle <florian.baeuerle@allegion.com>
> > ---
> >  rules/xorg-font-ttf-android.make | 50 ++++++++++++++++++++------------
> >  1 file changed, 32 insertions(+), 18 deletions(-)
> > 
> > diff --git a/rules/xorg-font-ttf-android.make b/rules/xorg-font-ttf-android.make
> > index 443f855aa..adad187c5 100644
> > --- a/rules/xorg-font-ttf-android.make
> > +++ b/rules/xorg-font-ttf-android.make
> > @@ -1,6 +1,7 @@
> >  # -*-makefile-*-
> >  #
> >  # Copyright (C) 2015 by Michael Olbrich <m.olbrich@pengutronix.de>
> > +#           (C) 2018 by Florian Bäuerle <florian.baeuerle@allegion.com>
> >  #
> >  # See CREDITS for details about who has contributed to this project.
> >  #
> > @@ -28,18 +29,8 @@ XORG_FONT_TTF_ANDROID_LICENSE		:= Apache-2.0
> >  XORG_FONT_TTF_ANDROID_LICENSE_FILES	:= \
> >  	file://NOTICE;md5=9645f39e9db895a4aa6e02cb57294595
> >  
> > -ifdef PTXCONF_XORG_FONT_TTF_ANDROID
> > -$(STATEDIR)/xorg-fonts.targetinstall.post: $(STATEDIR)/xorg-font-ttf-android.targetinstall
> > -endif
> > -
> > -# ----------------------------------------------------------------------------
> > -# Prepare
> > -# ----------------------------------------------------------------------------
> > -
> > -#
> > -# autoconf
> > -#
> > -XORG_FONT_TTF_ANDROID_CONF_TOOL	:= NO
> 
> Don't remove this.
> 
> > +XORG_FONT_TTF_ANDROID_FONTDIR		:= $(XORG_FONTDIR)/truetype/android
> 
> ok
> 
> > +XORG_FONT_TTF_ANDROID_FONTPKGDIR	:= $(XORG_FONT_TTF_ANDROID_PKGDIR)$(XORG_FONT_TTF_ANDROID_FONTDIR)
> 
> remove
> 
> >  
> >  # ----------------------------------------------------------------------------
> >  # Compile
> > @@ -55,6 +46,18 @@ $(STATEDIR)/xorg-font-ttf-android.compile:
> >  
> >  $(STATEDIR)/xorg-font-ttf-android.install:
> >  	@$(call targetinfo)
> > +
> > +	@rm -rf $(XORG_FONT_TTF_ANDROID_PKGDIR)
> > +	@mkdir -p $(XORG_FONT_TTF_ANDROID_FONTPKGDIR)
> > +	@find $(XORG_FONT_TTF_ANDROID_DIR) \
> > +		-name "*.ttf" \
> > +		| \
> > +		while read file; do \
> > +		install -m 644 $${file} $(XORG_FONT_TTF_ANDROID_FONTPKGDIR); \
> > +	done
> > +	@mkfontscale $(XORG_FONT_TTF_ANDROID_FONTPKGDIR)
> > +	@mkfontdir $(XORG_FONT_TTF_ANDROID_FONTPKGDIR)
> 
> This adds a lot of boiler plate for all packages. I'd like to see this here:
> 
> 	@$(call world/install-fonts, XORG_FONT_TTF_ANDROID, *.ttf)

I would have also preferred that, *but* if you look at xorg-font-ttf-noto, the
original package author did not want to just install every font present in the
archive. I did not question that decision and instead concluded, that such a
macro cannot easily do the job for every font package.

So should I

a) reconsider the original author's decision
or
b) add the boilerplate to the noto package and use the macro everywhere else

I know the current approach sucks, so much that I edited all the makefiles with
sed.

> 
> And in rules/post/ptxd_make_world_fonts.make
> 
> define world/install-fonts
> rm -rf $($(1)_PKGDIR)
> ...
> @mkfontdir $($(1)_PKGDIR)/$($(1)_FONTDIR)
> endef
> 
> 
> 
> > +
> >  	@$(call touch)
> >  
> >  # ----------------------------------------------------------------------------
> > @@ -64,14 +67,25 @@ $(STATEDIR)/xorg-font-ttf-android.install:
> >  $(STATEDIR)/xorg-font-ttf-android.targetinstall:
> >  	@$(call targetinfo)
> >  
> > -	@mkdir -p $(XORG_FONTS_DIR_INSTALL)/truetype
> > +	@$(call install_init, xorg-font-ttf-android)
> > +	@$(call install_fixup, xorg-font-ttf-android,PRIORITY,optional)
> > +	@$(call install_fixup, xorg-font-ttf-android,SECTION,base)
> > +	@$(call install_fixup, xorg-font-ttf-android,AUTHOR,"Florian Bäuerle <florian.baeuerle@allegion.com>")
> > +	@$(call install_fixup, xorg-font-ttf-android,DESCRIPTION,missing)
> >  
> > -	@find $(XORG_FONT_TTF_ANDROID_DIR) \
> > -		-name "*.ttf" \
> > -		| \
> > -		while read file; do \
> > -		install -m 644 $${file} $(XORG_FONTS_DIR_INSTALL)/truetype; \
> > +	@$(call install_tree, xorg-font-ttf-android, 0, 0, -, $(XORG_FONT_TTF_ANDROID_FONTDIR))
> > +
> > +ifdef PTXCONF_XORG_FONTS_QT4_LINKS
> > +	@cd $(XORG_FONT_TTF_ANDROID_PKGDIR) && \
> > +	find . -type f -name "*.ttf" | while read file; do \
> > +		name=$$(basename $$file); \
> > +		$(call install_link, xorg-font-ttf-android, \
> > +			../../..$(XORG_FONT_TTF_ANDROID_FONTDIR)/$$name, \
> > +			/usr/lib/fonts/$$name); \
> >  	done
> > +endif
> 
> Create the symlinks in _PKGDIR in world/install-fonts and use '/' in
> install_tree above. You'll need to us a shell 'if' instead of 'ifdef' in
> world/install-fonts:
> 
> if [ "$(PTXCONF_XORG_FONTS_QT4_LINKS)" = "y" ]; then \
> ...
> 
> 
> The current code and your copies in all the different rules ignore some
> errors. I want to clean this up at some point and that will be easier if I
> only have to modify one or two copies and not all font rules.
> 
> These comments apply to all other patches as well.
> 

Thanks for the review!


- Florian

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de