Bug 615848

Summary: Review Request: oflb-brett-fonts - A handwriting font
Product: [Fedora] Fedora Reporter: Parag Nemade <pnemade>
Component: Package ReviewAssignee: Ankur Sinha (FranciscoD) <sanjay.ankur>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, fonts-bugs, i18n-bugs, jonstanley, nicolas.mailhot, notting, paul, sanjay.ankur
Target Milestone: ---Flags: sanjay.ankur: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-07-22 05:33:38 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 477333    

Comment 1 Paul Flo Williams 2010-07-19 10:07:05 UTC
http://fedoraproject.org/wiki/Packaging:FontsPolicy#Naming

"When foundryname, projectname or fontfamilyname contain the font or fonts affix, this affix should be dropped from them"

The name of this package should be oflb-brett-fonts.

Comment 2 Ankur Sinha (FranciscoD) 2010-07-19 10:21:19 UTC
Thanks Paul.

Parag: I'll do the review once the package name has been corrected. 

Ankur

Comment 4 Ankur Sinha (FranciscoD) 2010-07-19 11:05:26 UTC
+ OK
- NA
? ISSUE

? Package meets naming and packaging guidelines
+ Spec file matches base package name.
+ Spec has consistant macro usage.
+ Meets Packaging Guidelines.
+ License
+ License field in spec matches
+ License file included in package
+ Spec in American English
+ Spec is legible.
- Sources match upstream md5sum:

- Package needs ExcludeArch
+ BuildRequires correct
- Spec handles locales/find_lang
- Package is relocatable and has a reason to be.
+ Package has %defattr and permissions on files is good.
? Package has a correct %clean section.
? Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ Package is code or permissible content.
- Doc subpackage needed/used.
+ Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- Spec has needed ldconfig in post and postun
- .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.

- Package is a GUI app and has a .desktop file

+ Package compiles and builds on at least one arch.
+ Package has no duplicate files in %files.
+ Package doesn't own any directories other packages own.
+ Package owns all the directories it creates.
? No rpmlint output.

SHOULD Items:

+ Should build in mock.
- Should build on all supported archs
- Should function as described.
- Should have sane scriptlets.
- Should have subpackages require base package with fully versioned depend.
+ Should have dist tag
- Should package latest version
- check for outstanding bugs on package. (For core merge reviews)

Issues:

1. The ttf name is BrettFont1.1.ttf

Any particular reason why you're not using 1.1 as version and the time stamp instead?

2. rpmlint output:

oflb-brett-fonts.spec: W: no-cleaning-of-buildroot %install
oflb-brett-fonts.spec: W: no-cleaning-of-buildroot %clean
oflb-brett-fonts.spec: W: no-buildroot-tag
oflb-brett-fonts.spec: W: no-%clean-section
oflb-brett-fonts.src: W: invalid-url URL: http://openfontlibrary.org/media/files/brettalton/205 HTTP Error 404: Not Found
oflb-brett-fonts.src: W: no-cleaning-of-buildroot %install
oflb-brett-fonts.src: W: no-cleaning-of-buildroot %clean
oflb-brett-fonts.src: W: no-buildroot-tag
oflb-brett-fonts.src: W: no-%clean-section
oflb-brett-fonts.noarch: W: invalid-url URL: http://openfontlibrary.org/media/files/brettalton/205 HTTP Error 404: Not Found
oflb-brett-fonts.src: W: invalid-url URL: http://openfontlibrary.org/media/files/brettalton/205 HTTP Error 404: Not Found
oflb-brett-fonts.src: W: no-cleaning-of-buildroot %install
oflb-brett-fonts.src: W: no-cleaning-of-buildroot %clean
oflb-brett-fonts.src: W: no-buildroot-tag
oflb-brett-fonts.src: W: no-%clean-section
3 packages and 1 specfiles checked; 0 errors, 15 warnings.

Ignorable warnings. 

Please check up the clean section and build root. I also think the version would be better as 1.1 rather than the time stamp for this font. 

I'll approve it once we have the version clarified. 

Ankur

Comment 5 Parag Nemade 2010-07-19 13:01:14 UTC
The reason I kept version same as old is that, font name says its 1.1, inside font information says 1.0 and as upstream is dead, I have no way to contact upstream to clarify version info.

I will say let's import this and if needed later on we will change it to 1.0 or 1.1 version. In any way if we are removing timestamp as version, we are going to use epoch.

Comment 6 Paul Flo Williams 2010-07-19 14:31:33 UTC
Welcome to the wacky world of font metadata. To inject a bit of font archaeology:

The metadata of the original release was _really_ strange, with some fields in the name table claiming that this font was Times New Roman version 3.00, and some claiming that this was BrettFont version 1.00. For the release as part of FC8, Jon Stanley used 1.0-2 as the nvr.

In May 2008, a new version was uploaded to OFLB, with the most obvious tell-tales of its hacky origin removed. Unfortunately, the metadata still claimed this as BrettFont 1.0. In the face of this confusion, Jon started using the date of release as the version.

I agree with Parag; there isn't likely to be a future release (unless Brett removes the final few "Monotype" strings from the metadata), so continuing to treat the internal version as unreliable and going for dates makes sense.

For the record, I'm examining:

sha1sum *.ttf
9fd5585ec2b2c9d9850a5a8e9acd94b0c2d2e9f1  brettalton_-_Brett_Font.ttf
424d6ef6eaa4a71c719b51ae39ef268f6f011418  BrettFont1.1.ttf

Comment 7 Ankur Sinha (FranciscoD) 2010-07-20 18:02:12 UTC
hey Paul, 

thanks for the history lesson :)

Guess it's approved then 

XXX APPROVED XXX

Comment 8 Parag Nemade 2010-07-21 03:35:15 UTC
Thanks for the review!
Thanks Paul for your feedback.

New Package CVS Request
=======================
Package Name: oflb-brett-fonts
Short Description: A handwriting font 
Owners: pnemade
Branches: 
InitialCC: i18n-team fonts-sig

Comment 9 Kevin Fenzi 2010-07-21 05:17:00 UTC
CVS done (by process-cvs-requests.py).

Comment 10 Parag Nemade 2010-07-22 05:33:38 UTC
Thanks all. Built in rawhide.