Bug 239884 - Review Request: liberation-fonts - Fonts to replace commonly used Microsoft Windows Fonts
Review Request: liberation-fonts - Fonts to replace commonly used Microsoft ...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nicolas Mailhot
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FC7Target
  Show dependency treegraph
 
Reported: 2007-05-11 17:46 EDT by Matthias Clasen
Modified: 2010-01-12 19:52 EST (History)
12 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-17 23:32:26 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
nicolas.mailhot: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Matthias Clasen 2007-05-11 17:46:29 EDT
Spec URL: http://people.redhat.com/mclasen/liberation-fonts.spec
SRPM URL: http://people.redhat.com/mclasen/liberation-fonts-0.1-5.fc7.src.rpm
Description: 

The Liberation Fonts are intended to be replacements for the three
most commonly used fonts on Microsoft systems: Times New Roman,
Arial, and Courier New.
Comment 1 Matthias Clasen 2007-05-11 17:48:46 EDT
Just FYI, the font config configuration that we are planning for the liberation
fonts is to add them to the lists of metrically equivalent fonts in 
/etc/fonts/conf.d/30-aliases-fedora.conf
Comment 2 Nicolas Mailhot 2007-05-12 05:19:12 EDT
Formal review:

☑ MUST: rpmlint must be run on every package. OK

$ rpmlint /srv/rpm/liberation-fonts-0.1-5.fc7.nim.*
W: liberation-fonts incoherent-version-in-changelog 0.1-5 0.1-5.fc7.nim

- Just a dist/changelog clash, ignored

☑ MUST: The package must be named according to the Package Naming Guidelines. OK
☑ MUST: The spec file name must match the base package %{name}… OK
☑ MUST: The package must meet the Packaging Guidelines. OK
☑ MUST: The package must be licensed with an open-source compatible license… OK

☒ MUST: The License field in the package spec file must match the actual
license. NOK

- As the license is not vanilla GPL v2, GPL field is misleading

☒ MUST: If … text of the license(s) for the package must be included in %doc. NOK

1. Don't do that:
install -m 0644 License.txt %{buildroot}%{_datadir}/fonts/liberation
2. The license text does not seem UTF-8 encoded

☑ MUST: The spec file must be written in American English. OK
☑ MUST: The spec file for the package MUST be legible. OK
☒ MUST: The sources used to build the package must match the upstream source… NOK

1. Please add a source URL to the package
2. Make sure the archive name and content match the signed archive on the RH page
3. For fonts tar.bz2 is probably a better idea than tar.gz

☑ MUST: The package must successfully compile and build into binary rpms… OK
☑ MUST: If the package does not successfully compile, build or work on an… N/A
☑ MUST: All build dependencies must be listed in BuildRequires… OK
☑ MUST: The spec file MUST handle locales properly… N/A
☑ MUST: Every binary RPM package which stores shared library files… N/A
☑ MUST: If the package is designed to be relocatable… N/A
☑ A package must own all directories that it creates… OK
☑ MUST: A package must not contain any duplicate files in the %files listing. OK
☑ MUST: Permissions on files must be set properly… OK

- though a %defattr(0644,root,root,0755) would be nicer

☑ MUST: Each package must have a %clean section… OK

- please move the clean section to its usual place after %install

☑ MUST: Each package must consistently use macros… OK

- please define and use fontdir and fontconfdir as in other font packages
(dejavu-lgc…)

☑ MUST: The package must contain code, or permissable content… OK
☑ MUST: Large documentation files should go in a -doc subpackage… N/A
☑ MUST: If a package includes something as %doc… OK
☑ MUST: Header files must be in a -devel package. N/A
☑ MUST: Static libraries must be in a -static package. N/A
☑ MUST: Packages containing pkgconfig(.pc)… N/A
☑ MUST: If a package contains library files with a suffix… N/A
☑ MUST: In the vast majority of cases, devel packages… N/A
☑ MUST: Packages containing GUI applications… N/A
☑ MUST: Packages must not own files or directories already owned by other
packages… OK
☑ MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}… OK
☑ MUST: All filenames in rpm packages must be valid UTF-8. OK

☒ SHOULD: If the source package does not include license text(s) as a separate
file from upstream… NOK

1. The license text is partial: it describes the exception but not the main
license. A GPL text should be joined to the package

☑ SHOULD: The description and summary sections in the package spec file should
contain translations… N/A
☑ SHOULD: The reviewer should test that the package builds in mock. OK
☐ SHOULD: The package should compile and build into binary rpms on all supported
architectures. I pass, but that's a noarch package
☑ SHOULD: The reviewer should test that the package functions as described… OK
☒ SHOULD: If scriptlets are used, those scriptlets must be sane. NOK
Please use :

%post
if [ -x %{_bindir}/fc-cache ]; then 
  %{_bindir}/fc-cache %{_datadir}/fonts
fi

%postun
if [ "$1" = "0" ]; then
  if [ -x %{_bindir}/fc-cache ]; then 
    %{_bindir}/fc-cache %{_datadir}/fonts
  fi
fi

like in other font packages (or request them fixed if %{_bindir} is NOK)

☒ SHOULD: Usually, subpackages other than devel… N/A
☒ SHOULD: The placement of pkgconfig(.pc) files depends on their usecase… N/A
☒ SHOULD: If the package has file dependencies outside of /etc, /bin… N/A
Comment 3 Nicolas Mailhot 2007-05-12 05:41:23 EDT
Freeform review additions:

- please also use -m 0755 at directory install time
- it would be nice if the metadata declaration order followed the official
Fedora template
- please add a FAQ or at least the contact in charge of liberation fonts as the
referenced site has limited info and no contact information. In particular
everyone involved in FLOSS fonts would like to know the rationale behind GPL
choice when the painfully achieved consensus was to go OFL for all projects
- please drop a fontconfig configuration file in /etc/fonts/conf.d/ containing
at least the "assign generic names" bit of the dejavu-lgc one. After distussion
on IRC with behdad the right prio is probably between dejavu-lgc and other fonts
(595 unless dejavu-lgc moves to 58)
- relying on /etc/fonts/conf.d/30-aliases-fedora.conf means Conflicting with
fontconfig packages that do not include liberation info (major PITA). Consider
working with behdad to split this file in font-specific ones
(/etc/fonts/conf.d/30-001-fedora-helvetica-alias-liberation.conf,
/etc/fonts/conf.d/30-002-fedora-helvetica-alias-nimbus.conf etc) so next time a
new font package can just drop his own file there instead of relying on a
fontconfig update
Comment 4 Nicolas Mailhot 2007-05-12 05:53:11 EDT
→ needs more work
Comment 5 Matthias Clasen 2007-05-12 18:43:40 EDT
2. The license text does not seem UTF-8 encoded

This seems a bit picky, considering we are talking about a single copyright sign.

1. Please add a source URL to the package
2. Make sure the archive name and content match the signed archive on the RH page
3. For fonts tar.bz2 is probably a better idea than tar.gz

Can't really do that, since the upstream tarball is missing the License.txt
file, afaics. I'll see what I can do. gz vs bz2 is an upstream choice and 
pretty irrelevant to this review.


1. The license text is partial: it describes the exception but not the main
license. A GPL text should be joined to the package


I'll pass this on.  


 If scriptlets are used, those scriptlets must be sane. NOK

The current scriptlets are copied verbatim from the guidelines.
If you are not satisfied with them, lobby for a change of the
guidelines.

- please move the clean section to its usual place after %install
- it would be nice if the metadata declaration order followed the official
Fedora template

Don't include irrelevant nitpicky details here, please. 



- please add a FAQ or at least the contact in charge of liberation fonts as the
referenced site has limited info and no contact information. In particular
everyone involved in FLOSS fonts would like to know the rationale behind GPL
choice when the painfully achieved consensus was to go OFL for all projects


Not a topic for the package review. You already brought this up
on the mailing lists, which is a much better forum than this bug.



- please drop a fontconfig configuration file in /etc/fonts/conf.d/ containing
at least the "assign generic names" bit of the dejavu-lgc one. After distussion
on IRC with behdad the right prio is probably between dejavu-lgc and other fonts
(595 unless dejavu-lgc moves to 58)


Thats possible


- relying on /etc/fonts/conf.d/30-aliases-fedora.conf means Conflicting with
fontconfig packages that do not include liberation info (major PITA). 

Why do you think so ? That does not follow at all.


Consider working with behdad to split this file in font-specific ones
(/etc/fonts/conf.d/30-001-fedora-helvetica-alias-liberation.conf,
/etc/fonts/conf.d/30-002-fedora-helvetica-alias-nimbus.conf etc) so next time a
new font package can just drop his  own file there instead of relying on a
fontconfig update


Pleaes file a separate fontconfig bug if you think you have a working scheme
that is sufficiently better than what we have now.



Comment 6 Matthias Clasen 2007-05-12 23:38:25 EDT
- please drop a fontconfig configuration file in /etc/fonts/conf.d/ containing
at least the "assign generic names" bit of the dejavu-lgc one. After distussion
on IRC with behdad the right prio is probably between dejavu-lgc and other fonts
(595 unless dejavu-lgc moves to 58)

595 is not going to work though, since it sorts before 59-
Comment 7 Nicolas Mailhot 2007-05-13 04:01:14 EDT
(In reply to comment #5)
> 2. The license text does not seem UTF-8 encoded
> 
> This seems a bit picky, considering we are talking about a single copyright sign.

I reserve the right to be picky for highly visible packages where RH is the
upstream :)

> 1. Please add a source URL to the package
> 2. Make sure the archive name and content match the signed archive on the RH page
>
> Can't really do that, since the upstream tarball is missing the License.txt
> file, afaics. I'll see what I can do.

Please do.

> 3. For fonts tar.bz2 is probably a better idea than tar.gz
> gz vs bz2 is an upstream choice and pretty irrelevant to this review.

RH is the upstream there
 
> 1. The license text is partial: it describes the exception but not the main
> license. A GPL text should be joined to the package
> 
> 
> I'll pass this on.  
> 
> 
>  If scriptlets are used, those scriptlets must be sane. NOK
> 
> The current scriptlets are copied verbatim from the guidelines.
> If you are not satisfied with them, lobby for a change of the
> guidelines.

OK. It sucks guidelines differ from what our main font packages do in practice

> - please move the clean section to its usual place after %install
> - it would be nice if the metadata declaration order followed the official
> Fedora template
> 
> Don't include irrelevant nitpicky details here, please. 

I wouldn't mind it that much if people were not likely to take this spac as a
template later. But that's up to you.

> - please add a FAQ or at least the contact in charge of liberation fonts as the
> referenced site has limited info and no contact information. In particular
> everyone involved in FLOSS fonts would like to know the rationale behind GPL
> choice when the painfully achieved consensus was to go OFL for all projects
> 
> Not a topic for the package review. You already brought this up
> on the mailing lists, which is a much better forum than this bug.

That's your choice. Right now you're the only identified contact so prepare for
direct questions if you release the package as-is.

> - relying on /etc/fonts/conf.d/30-aliases-fedora.conf means Conflicting with
> fontconfig packages that do not include liberation info (major PITA). 
> 
> Why do you think so ? That does not follow at all.

The main feature of the packages is fonts that alias ms core fonts. If we allow
install on fontconfig-enabled system that do not have this aliasing, users won't
get the expected feature (and I'm sure our marketing people will make enough
noise users will expect it). Given we do not make font packages depend on
fontconfig that's a Conflict

> Consider working with behdad to split this file in font-specific ones
> (/etc/fonts/conf.d/30-001-fedora-helvetica-alias-liberation.conf,
> /etc/fonts/conf.d/30-002-fedora-helvetica-alias-nimbus.conf etc) so next time a
> new font package can just drop his  own file there instead of relying on a
> fontconfig update
>
> Pleaes file a separate fontconfig bug if you think you have a working scheme
> that is sufficiently better than what we have now.

already done (bug #239913) but liberation fonts is the first package that
clearly needs this

(In reply to comment #6)
> - please drop a fontconfig configuration file in /etc/fonts/conf.d/ containing
> at least the "assign generic names" bit of the dejavu-lgc one. After distussion
> on IRC with behdad the right prio is probably between dejavu-lgc and other fonts
> (595 unless dejavu-lgc moves to 58)
> 
> 595 is not going to work though, since it sorts before 59-

Yes that's a hack and people tend to get their hacks wrong. 605 then (and moving
lgc would be much cleaner)

Comment 8 Matthias Clasen 2007-05-14 14:57:42 EDT
New spec and srpm up at 

Spec URL: http://people.redhat.com/mclasen/liberation-fonts.spec
SRPM URL: http://people.redhat.com/mclasen/liberation-fonts-0.1-6.fc7.src.rpm

I ended up using just 59-liberation-fonts.conf, which sorts correctly wrt to
59-dejavu, and avoid any wierd 3digit games. We should just move dejavu to a
lower number at some point.

The new upstream tarball should hopefully be in place later today or tomorrow.
Comment 9 Matthias Clasen 2007-05-14 21:34:19 EDT
Upstream tarball is in place now:
https://www.redhat.com/f/fonts/liberation-fonts-ttf-2.tar.gz

I've updated the packages to point to that url:

Spec URL: http://people.redhat.com/mclasen/liberation-fonts.spec
SRPM URL: http://people.redhat.com/mclasen/liberation-fonts-0.1-7.fc7.src.rpm

Comment 10 Nicolas Mailhot 2007-05-15 04:39:03 EDT
n liberation-fonts-0.1-7.fc7.src.rpm:

☒ MUST: The License field in the package spec file must match the actual
license. NOK

I still feel declaring GPL when it's GPL v2 + additional terms is not right. I'd
like the confirmation of someone closer to FESCO on this before acking

☑ MUST: If … text of the license(s) for the package must be included in %doc OK
☑ MUST: The sources used to build the package must match the upstream source… OK
☑ SHOULD: If the source package does not include license text(s) as a separate
file from upstream… OK
☑ SHOULD: If scriptlets are used, those scriptlets must be sane. OK

To sum up, I still see two blockers:

1. the license field bit

2. fontconfig coordination. We can't push a font package hipped as core fonts
replacement if the associated required fontconfig plumbing is missing. I'm
pretty sure we have a "package works as advertised" MUST rule somewhere.
Long-term fix is the 30-aliases file split + a rule file provided by this
package. Short-term solutions lack appeal. Though since liberation fonts should
end up at the top of their respective aliases list dumping a rule file at 29 or
31 may work. I'll settle with seeing a fontconfig package with updated
30-aliases queued for F7.

Also I don't like the abandonware feeling of a package with no identified
contact on its web site or in %doc. Though the packager and whoever @rh goes to
the next text summit is going to take the blame, and I suppose I can let it
pass. It's still a bad example to give, especially for internally-produced stuff.
Comment 11 Matthias Clasen 2007-05-15 09:49:30 EDT
You are not uptodate...

[mclasen@localhost ~]$ koji latest-pkg f7-final fontconfig
Build                                     Tag                   Built by
----------------------------------------  --------------------  ----------------
fontconfig-2.4.2-3.fc7                    f7-final              mclasen


[mclasen@localhost ~]$ rpm -q --changelog fontconfig | head -2
* Fri May 11 2007 Matthias Clasen <mclasen@redhat.com> - 2.4.2-3
- Add Liberation fonts to 30-aliases-fedora.conf


I have changed the license field to say "GPL + font exception" now.
Check it out at the same place, release -8.

> Also I don't like the abandonware feeling of a package with no identified
> contact on its web site or in %doc. 

Thankfully package review is not about the dislikes and feelings of the 
reviewer, but about packaging standards.
Comment 12 Nicolas Mailhot 2007-05-15 10:07:02 EDT
Ok, approved
Comment 13 Matthias Clasen 2007-05-15 10:25:07 EDT
New Package CVS Request
=======================
Package Name: liberation-fonts 
Short Description: Fonts to replace commonly used Microsoft Windows Fonts
Owners: mclasen@redhat.com
Branches: 
InitialCC: 
Comment 14 Aurelien Bompard 2007-05-15 13:43:16 EDT
Could we have branches for FC6 too ?
Comment 15 Matthias Clasen 2007-05-16 13:29:56 EDT
Package Change Request
======================
Package Name: liberation-fonts
New Branches: FC-6

Note You need to log in before you can comment on or make changes to this bug.